fix(billing): address review feedback on security and correctness

- Validate successUrl/cancelUrl against app domain to prevent open redirects
- Fix toObject() spread in update that could break with populated org ref
- Add optional return_url to portal session creation
- Add Zod CheckoutRequest/PortalRequest schemas with model.isValid middleware
- Fix CASL resolveSubject to use route-derived billing subjects
- Update unit tests to cover URL validation and portal returnUrl

Author: PierreBrisorgueil

lib/middlewares/policy.js

@@ -125,7 +125,10 @@ const resolveSubject = (req) => {
   // For user admin routes, the loaded user is stored in req.model by userByID param middleware
   if (req.model) return { subjectType: 'UserAdmin', document: normalizeForCasl(req.model) };
   if (req.membershipDoc) return { subjectType: 'Membership', document: normalizeForCasl(req.membershipDoc) };
-  if (req.organization) return { subjectType: 'Organization', document: normalizeForCasl(req.organization) };
+  // Billing routes use req.organization for context but authorize via route-derived subjects
+  if (req.organization && !req.route?.path?.startsWith('/api/billing')) {
+    return { subjectType: 'Organization', document: normalizeForCasl(req.organization) };
+  }
   return null;
 };
 

modules/billing/controllers/billing.controller.js

@@ -28,7 +28,8 @@ const checkout = async (req, res) => {
  */
 const portal = async (req, res) => {
   try {
-    const url = await BillingService.createPortalSession(req.organization);
+    const { returnUrl } = req.body;
+    const url = await BillingService.createPortalSession(req.organization, returnUrl);
     responses.success(res, 'portal session created')({ url });
   } catch (err) {
     responses.error(res, 422, 'Unprocessable Entity', 'Failed to create portal session')(err);

modules/billing/models/billing.subscription.schema.js

@@ -41,7 +41,29 @@ const SubscriptionCore = z.object({
 
 const SubscriptionUpdate = SubscriptionCore.partial();
 
+/**
+ * Checkout request body schema
+ */
+const CheckoutRequest = z
+  .object({
+    priceId: z.string().trim().min(1, 'priceId is required'),
+    successUrl: z.string().url('successUrl must be a valid URL'),
+    cancelUrl: z.string().url('cancelUrl must be a valid URL'),
+  })
+  .strict();
+
+/**
+ * Portal request body schema
+ */
+const PortalRequest = z
+  .object({
+    returnUrl: z.string().url('returnUrl must be a valid URL').optional(),
+  })
+  .strict();
+
 export default {
   Subscription,
   SubscriptionUpdate,
+  CheckoutRequest,
+  PortalRequest,
 };

modules/billing/routes/billing.routes.js

@@ -3,8 +3,10 @@
  */
 import passport from 'passport';
 
+import model from '../../../lib/middlewares/model.js';
 import policy from '../../../lib/middlewares/policy.js';
 import organization from '../../organizations/middleware/organizations.middleware.js';
+import billingSchema from '../models/billing.subscription.schema.js';
 import billingPlans from '../controllers/billing.plans.controller.js';
 import billing from '../controllers/billing.controller.js';
 
@@ -21,13 +23,13 @@ export default (app) => {
   app
     .route('/api/billing/checkout')
     .all(passport.authenticate('jwt', { session: false }), organization.resolveOrganization, policy.isAllowed)
-    .post(billing.checkout);
+    .post(model.isValid(billingSchema.CheckoutRequest), billing.checkout);
 
   // portal
   app
     .route('/api/billing/portal')
     .all(passport.authenticate('jwt', { session: false }), organization.resolveOrganization, policy.isAllowed)
-    .post(billing.portal);
+    .post(model.isValid(billingSchema.PortalRequest), billing.portal);
 
   // subscription
   app

modules/billing/services/billing.service.js

@@ -22,6 +22,26 @@ const getStripe = () => {
   return stripeClient;
 };
 
+/**
+ * @desc Validate that a URL uses https (or http in dev/test) and belongs to the app domain
+ * @param {String} url - URL to validate
+ * @returns {Boolean} true if valid
+ */
+const isAllowedUrl = (url) => {
+  try {
+    const parsed = new URL(url);
+    const allowHttp = process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test';
+    if (!allowHttp && parsed.protocol !== 'https:') return false;
+    if (config.domain) {
+      const configHost = new URL(config.domain.startsWith('http') ? config.domain : `https://${config.domain}`).hostname;
+      if (parsed.hostname !== configHost) return false;
+    }
+    return true;
+  } catch {
+    return false;
+  }
+};
+
 /**
  * @desc Create a Stripe Checkout Session for the given organization
  * @param {Object} organization - The organization document
@@ -34,6 +54,10 @@ const createCheckout = async (organization, priceId, successUrl, cancelUrl) => {
   const stripe = getStripe();
   if (!stripe) throw new Error('Stripe is not configured');
 
+  if (!isAllowedUrl(successUrl) || !isAllowedUrl(cancelUrl)) {
+    throw new Error('Invalid redirect URL: must use HTTPS and match the application domain');
+  }
+
   // Find or create subscription record with Stripe customer
   let subscription = await SubscriptionRepository.findByOrganization(organization._id);
 
@@ -45,7 +69,7 @@ const createCheckout = async (organization, priceId, successUrl, cancelUrl) => {
 
     if (subscription) {
       subscription = await SubscriptionRepository.update({
-        ...subscription.toObject(),
+        _id: subscription._id,
         stripeCustomerId: customer.id,
       });
     } else {
@@ -70,18 +94,23 @@ const createCheckout = async (organization, priceId, successUrl, cancelUrl) => {
 /**
  * @desc Create a Stripe Customer Portal session for the given organization
  * @param {Object} organization - The organization document
+ * @param {String} returnUrl - Optional URL to redirect back to after portal
  * @returns {Promise<String>} Portal session URL
  */
-const createPortalSession = async (organization) => {
+const createPortalSession = async (organization, returnUrl) => {
   const stripe = getStripe();
   if (!stripe) throw new Error('Stripe is not configured');
 
   const subscription = await SubscriptionRepository.findByOrganization(organization._id);
   if (!subscription?.stripeCustomerId) throw new Error('No Stripe customer found for this organization');
 
-  const session = await stripe.billingPortal.sessions.create({
-    customer: subscription.stripeCustomerId,
-  });
+  const params = { customer: subscription.stripeCustomerId };
+  if (returnUrl) {
+    if (!isAllowedUrl(returnUrl)) throw new Error('Invalid return URL: must use HTTPS and match the application domain');
+    params.return_url = returnUrl;
+  }
+
+  const session = await stripe.billingPortal.sessions.create(params);
 
   return session.url;
 };

modules/billing/tests/billing.service.unit.tests.js

@@ -79,6 +79,32 @@ describe('Billing service unit tests:', () => {
       );
     });
 
+    test('should throw when redirect URL is invalid', async () => {
+      jest.unstable_mockModule('../../../config/index.js', () => ({
+        default: { stripe: { secretKey: 'sk_test_url' } },
+      }));
+
+      const mod = await import('../services/billing.service.js');
+      BillingService = mod.default;
+
+      await expect(BillingService.createCheckout(mockOrganization, 'price_123', 'not-a-url', 'http://cancel')).rejects.toThrow(
+        'Invalid redirect URL',
+      );
+    });
+
+    test('should reject URL with wrong domain when config.domain is set', async () => {
+      jest.unstable_mockModule('../../../config/index.js', () => ({
+        default: { stripe: { secretKey: 'sk_test_domain' }, domain: 'https://myapp.com' },
+      }));
+
+      const mod = await import('../services/billing.service.js');
+      BillingService = mod.default;
+
+      await expect(
+        BillingService.createCheckout(mockOrganization, 'price_123', 'http://evil.com/success', 'http://myapp.com/cancel'),
+      ).rejects.toThrow('Invalid redirect URL');
+    });
+
     test('should create customer and subscription when none exists', async () => {
       jest.unstable_mockModule('../../../config/index.js', () => ({
         default: { stripe: { secretKey: 'sk_test_123' } },
@@ -111,7 +137,6 @@ describe('Billing service unit tests:', () => {
       const existingSub = {
         _id: 'sub_existing',
         stripeCustomerId: null,
-        toObject: jest.fn().mockReturnValue({ _id: 'sub_existing', stripeCustomerId: null }),
       };
       mockSubscriptionRepository.findByOrganization.mockResolvedValue(existingSub);
       mockSubscriptionRepository.update.mockResolvedValue({ stripeCustomerId: 'cus_new123' });
@@ -196,7 +221,7 @@ describe('Billing service unit tests:', () => {
       );
     });
 
-    test('should return portal session URL when customer exists', async () => {
+    test('should return portal session URL without returnUrl', async () => {
       jest.unstable_mockModule('../../../config/index.js', () => ({
         default: { stripe: { secretKey: 'sk_test_portal3' } },
       }));
@@ -213,6 +238,38 @@ describe('Billing service unit tests:', () => {
         customer: 'cus_portal',
       });
     });
+
+    test('should include return_url when returnUrl is provided', async () => {
+      jest.unstable_mockModule('../../../config/index.js', () => ({
+        default: { stripe: { secretKey: 'sk_test_portal4' } },
+      }));
+
+      mockSubscriptionRepository.findByOrganization.mockResolvedValue({ stripeCustomerId: 'cus_portal' });
+
+      const mod = await import('../services/billing.service.js');
+      BillingService = mod.default;
+
+      const url = await BillingService.createPortalSession(mockOrganization, 'http://app/settings');
+
+      expect(url).toBe('https://billing.stripe.com/portal_123');
+      expect(mockStripeInstance.billingPortal.sessions.create).toHaveBeenCalledWith({
+        customer: 'cus_portal',
+        return_url: 'http://app/settings',
+      });
+    });
+
+    test('should throw when returnUrl is invalid', async () => {
+      jest.unstable_mockModule('../../../config/index.js', () => ({
+        default: { stripe: { secretKey: 'sk_test_portal5' } },
+      }));
+
+      mockSubscriptionRepository.findByOrganization.mockResolvedValue({ stripeCustomerId: 'cus_portal' });
+
+      const mod = await import('../services/billing.service.js');
+      BillingService = mod.default;
+
+      await expect(BillingService.createPortalSession(mockOrganization, 'not-a-url')).rejects.toThrow('Invalid return URL');
+    });
   });
 
   describe('getSubscription', () => {