Skip to content

Commit 0d42b52

Browse files
fix(billing): address review feedback on JSDoc, error messages, and test coverage
- Update isAllowedUrl JSDoc to document env-specific behavior - Make error messages env-aware (clarify production-only restrictions) - Fix getStripe JSDoc to use standard summary format - Add production regression test for hostname mismatch rejection
1 parent 55c024a commit 0d42b52

3 files changed

Lines changed: 34 additions & 11 deletions

File tree

modules/billing/lib/stripe.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import config from '../../../config/index.js';
1111
let stripeClient = null;
1212

1313
/**
14-
* @desc Get or create the Stripe client instance
14+
* Get or create the lazily-initialised Stripe client instance.
1515
* @returns {Object|null} Stripe client or null if not configured
1616
*/
1717
const getStripe = () => {

modules/billing/services/billing.service.js

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ import BillingPlansService from './billing.plans.service.js';
77
import SubscriptionRepository from '../repositories/billing.subscription.repository.js';
88

99
/**
10-
* @desc Validate that a URL uses https (or http in dev/test) and belongs to the app domain
10+
* Validate that a redirect URL is safe for the current environment.
11+
* In production the URL must use HTTPS and, when config.domain is set,
12+
* its hostname must match the application domain.
13+
* In development / test only basic URL parsing is enforced (HTTP allowed,
14+
* any hostname accepted) so that localhost workflows are not blocked.
1115
* @param {String} url - URL to validate
1216
* @returns {Boolean} true if valid
1317
*/
@@ -40,13 +44,13 @@ const createCheckout = async (organization, priceId, successUrl, cancelUrl) => {
4044
if (!stripe) throw new Error('Stripe is not configured');
4145

4246
if (!isAllowedUrl(successUrl) || !isAllowedUrl(cancelUrl)) {
43-
throw new Error('Invalid redirect URL: must use HTTPS and match the application domain');
47+
throw new Error('Invalid redirect URL: must be a valid URL (production requires HTTPS and a matching application domain)');
4448
}
4549

46-
// Validate priceId against known active Stripe prices
50+
// Validate priceId against known active Stripe prices and resolve the canonical plan id
4751
const plans = await BillingPlansService.getPlans();
48-
const allowedPriceIds = plans.flatMap((p) => [p.stripePriceMonthly, p.stripePriceAnnual].filter(Boolean));
49-
if (!allowedPriceIds.includes(priceId)) {
52+
const matchedPlan = plans.find((p) => p.stripePriceMonthly === priceId || p.stripePriceAnnual === priceId);
53+
if (!matchedPlan) {
5054
throw new Error('Invalid priceId: must be an active published price');
5155
}
5256

@@ -86,9 +90,6 @@ const createCheckout = async (organization, priceId, successUrl, cancelUrl) => {
8690
if (latest?.stripeCustomerId) subscription = latest;
8791
}
8892

89-
// Derive plan name from priceId for checkout metadata
90-
const matchedPlan = plans.find((p) => p.stripePriceMonthly === priceId || p.stripePriceAnnual === priceId);
91-
9293
const session = await stripe.checkout.sessions.create({
9394
customer: subscription.stripeCustomerId,
9495
mode: 'subscription',
@@ -97,7 +98,7 @@ const createCheckout = async (organization, priceId, successUrl, cancelUrl) => {
9798
cancel_url: cancelUrl,
9899
metadata: {
99100
organizationId: String(organization._id),
100-
plan: matchedPlan?.planId || 'free',
101+
plan: matchedPlan.planId,
101102
},
102103
});
103104

@@ -119,7 +120,7 @@ const createPortalSession = async (organization, returnUrl) => {
119120

120121
const params = { customer: subscription.stripeCustomerId };
121122
if (returnUrl) {
122-
if (!isAllowedUrl(returnUrl)) throw new Error('Invalid return URL: must use HTTPS and match the application domain');
123+
if (!isAllowedUrl(returnUrl)) throw new Error('Invalid return URL: must be a valid URL (production requires HTTPS and a matching application domain)');
123124
params.return_url = returnUrl;
124125
}
125126

modules/billing/tests/billing.checkout.unit.tests.js

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,28 @@ describe('Billing service unit tests:', () => {
122122
expect(url).toBe('https://checkout.stripe.com/session_123');
123123
});
124124

125+
test('should reject mismatched hostname in production when config.domain is set', async () => {
126+
const originalEnv = process.env.NODE_ENV;
127+
process.env.NODE_ENV = 'production';
128+
129+
jest.unstable_mockModule('../../../config/index.js', () => ({
130+
default: { stripe: { secretKey: 'sk_test_prodcheck' }, domain: 'https://myapp.com' },
131+
}));
132+
133+
mockSubscriptionRepository.findByOrganization.mockResolvedValue({
134+
stripeCustomerId: 'cus_existing',
135+
});
136+
137+
const mod = await import('../services/billing.service.js');
138+
BillingService = mod.default;
139+
140+
await expect(
141+
BillingService.createCheckout(mockOrganization, 'price_starter_m', 'https://evil.com/success', 'https://myapp.com/cancel'),
142+
).rejects.toThrow('Invalid redirect URL');
143+
144+
process.env.NODE_ENV = originalEnv;
145+
});
146+
125147
test('should throw when priceId is not in allowed plans', async () => {
126148
jest.unstable_mockModule('../../../config/index.js', () => ({
127149
default: { stripe: { secretKey: 'sk_test_price' } },

0 commit comments

Comments
 (0)