From 030201fe66d7fa6b9b2ffa44e4853ffa4c337987 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Thu, 16 Jul 2026 23:16:02 +0200 Subject: [PATCH] fix(billing): resolve Stripe plan via the priceId map in admin sync and reconcile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveStripePlan in billing.admin.service.js and billing.reconcile.service.js read only price/plan.metadata.planId and fell back to 'free' — the exact defect config.stripe.prices, never propagated here. Real Stripe Price objects carry no metadata.planId, so: - syncOrgFromStripe (a DB write) could silently downgrade a paying org to free on the very tool ops runs to resolve a billing divergence. - runReconciliation (log-only) emitted a false planMismatch alert for every paid org, drowning real divergences. Extracted the webhook's resolvePlan/buildPriceIdToPlanMap into a shared modules/billing/lib/billing.planResolver.js so all three call sites resolve via ONE implementation. The shared resolver returns null (never 'free') when unresolved; the admin path now ABORTS the write (409) instead of guessing, and the reconcile path skips the plan comparison instead of alerting. Replaced the unrealistic metadata.planId-only test fixtures with priceId-based ones matching real Stripe payloads, and added regression coverage: a paid subscription with no metadata.planId resolves correctly in both admin and reconcile, and admin sync never writes 'free' for it. Closes #3964 Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup --- ERRORS.md | 1 + .../controllers/billing.admin.controller.js | 6 +- modules/billing/lib/billing.planResolver.js | 91 ++++++++++ .../billing/services/billing.admin.service.js | 48 ++++-- .../services/billing.reconcile.service.js | 36 ++-- .../services/billing.webhook.service.js | 60 +------ .../tests/billing.admin.service.unit.tests.js | 52 +++++- ...billing.admin.toolkit.integration.tests.js | 19 ++ .../tests/billing.planResolver.unit.tests.js | 162 ++++++++++++++++++ .../billing.reconcile.service.unit.tests.js | 59 ++++++- ...billing.webhook.subscription.unit.tests.js | 7 +- 11 files changed, 455 insertions(+), 86 deletions(-) create mode 100644 modules/billing/lib/billing.planResolver.js create mode 100644 modules/billing/tests/billing.planResolver.unit.tests.js diff --git a/ERRORS.md b/ERRORS.md index 6b8583d01..664f91777 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -31,3 +31,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-06-04] repository: top-level `const Foo = mongoose.model('Foo')` in a repository file -> this is evaluated at import time; safe in an HTTP server (loadModels() runs first) but silently crashes standalone scripts (crons, migrations) with `MissingSchemaError` when import order differs; tests miss it because jest mocks intercept the module entirely; fix = lazy getter `const Foo = () => mongoose.model('Foo')` (call sites: `Foo().find(...)`) or dynamic import after `loadModels()` in the entrypoint; see pierreb-devkit/Node#3789 - [2026-06-15] deps/audit: leaving `npm audit` advisories unaddressed on the assumption they need a major bump -> run `npm audit fix` (never `--force`) first; the runtime-tree DoS/ReDoS items (`qs`, `path-to-regexp`, `brace-expansion`) all fixed via in-range bumps, no residual. These are DoS-class but NOT attacker-reachable in this stack: Express route patterns are static (no user-controlled `path-to-regexp` input) and `qs`/`brace-expansion` only parse server-side query strings under fixed code paths — still bump them to keep the tree clean and avoid scanner noise. - [2026-07-16] security: `users.repository.js findByIdAndUpdatePopulated()` does `.populate()` with no `.select()`, so `organizations.controller.js switchOrganization` serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; `users.account.controller.js me()` separately forwarded `providerData` (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through `UserService.removeSensitive()` (whitelist, `modules/users/utils/sanitizeUser.js`) at the response boundary, never serialize `req.user`/a populated doc directly; a local-signup fixture's `providerData` defaults to `{}` so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see pierreb-devkit/Node#3963 +- [2026-07-16] billing/stripe: the #3742 priceId-map fix (`resolvePlan`/`buildPriceIdToPlanMap`) was applied only to the webhook handler; `billing.admin.service.js` `resolveStripePlan` (admin force-sync, a DB WRITE path) and `billing.reconcile.service.js` `resolveStripePlan` (LOG-ONLY divergence check) kept their own copies reading only `metadata.planId` -> a paid org's Stripe subscription (which never carries `metadata.planId`) resolved to `'free'` on admin sync (silently downgrading a paying org) and produced a false `planMismatch` alert on every reconcile run; fix = one shared resolver (`modules/billing/lib/billing.planResolver.js`) used by all three call sites, and a null-unresolved sentinel (never guess `'free'`) so the admin write path ABORTS instead of downgrading and the reconcile log path skips the comparison instead of alerting; see pierreb-devkit/Node#3964 diff --git a/modules/billing/controllers/billing.admin.controller.js b/modules/billing/controllers/billing.admin.controller.js index 916c73207..1ffa6eb3a 100644 --- a/modules/billing/controllers/billing.admin.controller.js +++ b/modules/billing/controllers/billing.admin.controller.js @@ -139,7 +139,11 @@ const adminSyncFromStripe = async (req, res) => { return responses.success(res, 'subscription synced from Stripe')(result); } catch (err) { const status = err.status ?? 500; - const title = status === 404 ? 'Not Found' : status === 422 ? 'Unprocessable Entity' : status === 502 ? 'Bad Gateway' : 'Internal Server Error'; + const title = status === 404 ? 'Not Found' + : status === 422 ? 'Unprocessable Entity' + : status === 409 ? 'Conflict' + : status === 502 ? 'Bad Gateway' + : 'Internal Server Error'; return responses.error(res, status, title, 'Failed to sync from Stripe')(err); } }; diff --git a/modules/billing/lib/billing.planResolver.js b/modules/billing/lib/billing.planResolver.js new file mode 100644 index 000000000..f3c402510 --- /dev/null +++ b/modules/billing/lib/billing.planResolver.js @@ -0,0 +1,91 @@ +/** + * Module dependencies + */ +import config from '../../../config/index.js'; +import logger from '../../../lib/services/logger.js'; + +/** + * Valid plan names from config (immutable set for O(1) lookups). + */ +const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', 'enterprise']); + +/** + * Build a reverse-map from Stripe price ID → plan name, sourced from `config.stripe.prices` + * at module load. Shape: `{ starter: { monthly: 'price_xxx', annual: 'price_yyy' }, pro: {...} }`. + * + * Why: `price.metadata.planId` is empty on real Stripe payloads — `planId` lives on the + * Product, not the Price exposed by webhook/subscription objects. The reverse-map gives a + * robust priceId→plan lookup without an extra Stripe API call. + * + * Shared by the webhook handler, the admin force-sync tool, and the reconcile cron so all + * three resolve identically (#3964 / #1250 — the admin + reconcile copies had drifted into + * a metadata-only resolver that silently fell back to 'free' for every real subscription). + * + * @returns {Record} priceId → planId map (built once at module init) + */ +export const buildPriceIdToPlanMap = () => { + const map = {}; + const stripePrices = config.stripe?.prices || {}; + for (const [planId, intervals] of Object.entries(stripePrices)) { + if (!validPlans.has(planId)) continue; + if (intervals?.monthly) map[intervals.monthly] = planId; + if (intervals?.annual) map[intervals.annual] = planId; + } + return map; +}; + +const priceIdToPlan = buildPriceIdToPlanMap(); + +/** + * @description Look up the plan for a raw Stripe price ID (no subscription object needed). + * Used when only a bare price ID is available (e.g. reading `previous_attributes` on a + * webhook diff, where the previous line item is a partial object, not a full subscription). + * @param {string|undefined} priceId - Stripe price ID (price_xxx). + * @returns {string|undefined} plan name, or undefined when the price ID is not mapped. + */ +export const lookupPlanByPriceId = (priceId) => (priceId ? priceIdToPlan[priceId] : undefined); + +/** + * @description Resolve the plan name from a Stripe subscription object. + * Strategy (most-specific first): + * 1. config priceId map (price_xxx → planId) — robust, no metadata dependency. + * 2. price.metadata.planId / plan.metadata.planId legacy fallback (test fixtures, manual + * Stripe edits) — validated against the known plan enum. + * 3. `null` when nothing resolves — deliberately NOT 'free'. Silently downgrading an + * unresolvable paid subscription to 'free' is the exact defect this resolver replaces; + * the caller decides the safe behavior for its context (log-only vs a DB write). + * @param {Object} subscription - Stripe subscription object. + * @param {Object} [opts] + * @param {string} [opts.logPrefix] - Log tag for the caller module (e.g. '[billing.admin]'). + * @returns {string|null} plan name, or null when unresolved. + */ +export const resolvePlanFromSubscription = (subscription, { logPrefix = '[billing]' } = {}) => { + const item = subscription?.items?.data?.[0]; + const priceId = item?.price?.id; + if (priceId && priceIdToPlan[priceId]) { + return priceIdToPlan[priceId]; + } + + // Legacy fallback: price/plan metadata set explicitly (e.g. test fixtures or manual Stripe edits). + const rawMeta = item?.price?.metadata?.planId || item?.plan?.metadata?.planId; + if (rawMeta) { + if (validPlans.has(rawMeta)) return rawMeta; + logger.warn(`${logPrefix} resolvePlanFromSubscription: unrecognized planId in metadata`, { + raw: rawMeta, + validPlans: [...validPlans], + }); + return null; + } + + // Nothing resolved — warn so a misconfigured config.stripe.prices (or an unmapped plan + // such as a manually-sold enterprise price) is visible instead of silently downgrading. + if (priceId) { + logger.warn(`${logPrefix} resolvePlanFromSubscription: priceId not in priceIdToPlan map and no metadata`, { + priceId, + stripeSubscriptionId: subscription?.id, + }); + } + return null; +}; + +export default { buildPriceIdToPlanMap, resolvePlanFromSubscription, lookupPlanByPriceId }; diff --git a/modules/billing/services/billing.admin.service.js b/modules/billing/services/billing.admin.service.js index cda37ee2a..7c4cae34e 100644 --- a/modules/billing/services/billing.admin.service.js +++ b/modules/billing/services/billing.admin.service.js @@ -1,7 +1,6 @@ /** * Module dependencies */ -import config from '../../../config/index.js'; import getStripe from '../lib/stripe.js'; import logger from '../../../lib/services/logger.js'; import { bumpEventMarkers } from '../lib/billing.markerBump.js'; @@ -11,11 +10,7 @@ import OrganizationRepository from '../../organizations/repositories/organizatio import BillingExtraBalanceRepository from '../repositories/billing.extraBalance.repository.js'; import BillingWebhookService from './billing.webhook.service.js'; import { getDollarsToUnitRatio } from '../lib/billing.constants.js'; - -/** - * Valid plan names from config (immutable set for O(1) lookups). - */ -const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', 'enterprise']); +import { resolvePlanFromSubscription } from '../lib/billing.planResolver.js'; /** * @function getCustomerStatus @@ -93,6 +88,29 @@ const syncOrgFromStripe = async (orgId) => { const stripeSub = await stripe.subscriptions.retrieve(existing.stripeSubscriptionId); const newPlan = resolveStripePlan(stripeSub); + + // Safety guard (#3964): never silently downgrade a paying org to 'free'. If the Stripe + // subscription's price doesn't map via config.stripe.prices AND carries no valid + // metadata.planId, the plan is genuinely unresolvable (misconfigured price map, or an + // unmapped price such as a manually-sold enterprise plan) — abort the write entirely + // rather than guess. Ops must fix the config.stripe.prices mapping (or set the plan via + // bumpOrgPlan) before retrying sync. + if (!newPlan) { + logger.error('[billing.admin] syncOrgFromStripe — cannot resolve plan from Stripe subscription, aborting sync to avoid downgrading org', { + orgId, + stripeSubscriptionId: existing.stripeSubscriptionId, + stripePriceId: stripeSub.items?.data?.[0]?.price?.id ?? null, + currentDbPlan: existing.plan, + }); + throw Object.assign( + new Error( + `cannot resolve plan for Stripe subscription ${existing.stripeSubscriptionId} (orgId=${orgId}) — ` + + 'no priceId→plan mapping and no valid metadata.planId; aborting sync to avoid corrupting the org\'s plan', + ), + { status: 409 }, + ); + } + const newStatus = stripeSub.status; // Stripe API ≥ 2025-08-27 moved current_period_start to items.data[0]. // Read from items first, fall back to top-level for older API versions (mirrors webhook handler). @@ -373,17 +391,19 @@ const creditDisputeReinstated = async (chargeId, amountCents, reason, refundRequ /** * @function resolveStripePlan - * @description Resolve the plan name from a Stripe subscription object. - * Same heuristic as billing.webhook.service.js (price.metadata.planId first). + * @description Resolve the plan name from a Stripe subscription object, via the shared + * priceId→plan resolver (`../lib/billing.planResolver.js` — also used by the + * webhook handler and the reconcile cron; #3964/#1250). + * Unlike the webhook resolver, this does NOT fall back to 'free' when + * unresolved — `syncOrgFromStripe` is a direct DB write on the admin escape-hatch + * path, and silently downgrading an unresolvable paid subscription to 'free' is + * the exact defect this fixes. Returns `null` so the caller can abort instead. * @param {Object} subscription - Stripe subscription object. - * @returns {string} plan name. + * @returns {string|null} plan name, or null when unresolved (caller must abort the write). */ // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik -const resolveStripePlan = (subscription) => { - const item = subscription.items?.data?.[0]; - const raw = item?.price?.metadata?.planId || item?.plan?.metadata?.planId; - return validPlans.has(raw) ? raw : 'free'; -}; +const resolveStripePlan = (subscription) => + resolvePlanFromSubscription(subscription, { logPrefix: '[billing.admin]' }); /** * @function dispatchWebhookEvent diff --git a/modules/billing/services/billing.reconcile.service.js b/modules/billing/services/billing.reconcile.service.js index d85357a84..484e3a505 100644 --- a/modules/billing/services/billing.reconcile.service.js +++ b/modules/billing/services/billing.reconcile.service.js @@ -6,6 +6,7 @@ import getStripe from '../lib/stripe.js'; import logger from '../../../lib/services/logger.js'; import billingEvents from '../lib/events.js'; import { currentWeekKey, weekStartDate } from '../lib/billing.isoWeek.js'; +import { resolvePlanFromSubscription } from '../lib/billing.planResolver.js'; /** * Page size for the reconciliation cursor — fetch in batches to avoid long-running queries. @@ -17,23 +18,20 @@ const RECONCILE_PAGE_SIZE = 100; */ const RECONCILE_STATUSES = ['active', 'past_due', 'trialing', 'unpaid']; -/** - * Valid plan names from config. - */ -const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', 'enterprise']); - /** * @function resolveStripePlan - * @description Resolve the plan name from a Stripe subscription object. + * @description Resolve the plan name from a Stripe subscription object, via the shared + * priceId→plan resolver (`../lib/billing.planResolver.js` — also used by the + * webhook handler and the admin force-sync tool; #3964/#1250). Returns `null` + * when unresolved (LOG-ONLY caller — never guesses 'free', which previously + * produced a false `planMismatch` divergence alert for every paid org whose + * price lacked `metadata.planId`, i.e. every real Stripe subscription). * @param {Object} subscription - Stripe subscription object. - * @returns {string} plan name. + * @returns {string|null} plan name, or null when unresolved. */ // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik -const resolveStripePlan = (subscription) => { - const item = subscription.items?.data?.[0]; - const raw = item?.price?.metadata?.planId || item?.plan?.metadata?.planId; - return validPlans.has(raw) ? raw : 'free'; -}; +const resolveStripePlan = (subscription) => + resolvePlanFromSubscription(subscription, { logPrefix: '[billing.reconcile]' }); /** * @function runReconciliation @@ -193,7 +191,19 @@ const _reconcileOne = async (stripe, sub) => { const stripePlan = resolveStripePlan(stripeSub); const statusMismatch = sub.status !== stripeStatus; - const planMismatch = sub.plan !== stripePlan; + // stripePlan === null means the price is unresolvable (no priceId mapping, no valid + // metadata) — skip the plan comparison rather than risk a false divergence against a + // guessed 'free'. Log distinctly so a misconfigured config.stripe.prices stays visible. + if (stripePlan === null) { + logger.warn('[billing.reconcile] cannot resolve Stripe plan — skipping plan comparison for this subscription', { + organizationId: orgId, + subscriptionId: String(sub._id), + stripeSubscriptionId: subId, + stripePriceId: stripeSub.items?.data?.[0]?.price?.id ?? null, + dbPlan: sub.plan, + }); + } + const planMismatch = stripePlan !== null && sub.plan !== stripePlan; // Check meter↔extras divergence for this org (Opus C1 detection layer). // This runs regardless of status/plan match — a healthy subscription can still diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 66c966b7d..0eb8a85ca 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -16,6 +16,7 @@ import billingEvents from '../lib/events.js'; import { SENTINEL_PENDING } from '../lib/billing.constants.js'; import { retryWithBackoff } from '../lib/billing.retry.js'; import { isNonTransientStripeError } from '../lib/billing.stripe-errors.js'; +import { resolvePlanFromSubscription, lookupPlanByPriceId } from '../lib/billing.planResolver.js'; import AnalyticsService from '../../../lib/services/analytics.js'; /** @@ -58,61 +59,18 @@ const validatePlan = (plan) => { */ const planRanks = Object.fromEntries((config.billing?.plans || []).map((p, i) => [p, i])); -/** - * Build a reverse-map from Stripe price ID → plan name, sourced from `config.stripe.prices` - * at module load. Shape: `{ growth: { monthly: 'price_xxx', annual: 'price_yyy' }, pro: {...} }`. - * - * Why: `price.metadata.planId` is empty on real Stripe webhook payloads — `planId` lives on - * the Product, not the Price exposed by `customer.subscription.updated`. The reverse-map gives - * a robust priceId→plan lookup without an extra Stripe API call per webhook. `resolvePlan` - * keeps `price.metadata.planId` as a legacy fallback (test fixtures, manual Stripe edits). - * - * @returns {Record} priceId → planId map (built once at module init) - */ -const buildPriceIdToPlanMap = () => { - const map = {}; - const stripePrices = config.stripe?.prices || {}; - for (const [planId, intervals] of Object.entries(stripePrices)) { - if (!validPlans.has(planId)) continue; - if (intervals.monthly) map[intervals.monthly] = planId; - if (intervals.annual) map[intervals.annual] = planId; - } - return map; -}; -const priceIdToPlan = buildPriceIdToPlanMap(); - /** * @description Resolve the plan name from a Stripe subscription object. - * Strategy (most-specific first): - * 1. config price-ID map (price_xxx → planId) — robust, no metadata dependency. - * 2. price.metadata.planId legacy fallback (works only if metadata was explicitly set). - * 3. plan.metadata.planId further legacy fallback. - * 4. 'free' when nothing resolves. + * Delegates to the shared priceId→plan resolver (see `../lib/billing.planResolver.js`, + * also used by the admin force-sync tool and the reconcile cron — #3964/#1250). Strategy + * (most-specific first): config priceId map → price/plan.metadata.planId legacy fallback → + * 'free' when nothing resolves (webhook-specific last resort: an event still needs a plan + * written; unlike the admin/reconcile call sites, there is no "abort" option mid-webhook). * @param {Object} subscription - Stripe subscription object * @returns {string} plan name */ -const resolvePlan = (subscription) => { - const item = subscription.items?.data?.[0]; - const priceId = item?.price?.id; - if (priceId && priceIdToPlan[priceId]) { - return priceIdToPlan[priceId]; - } - // Legacy fallback: price metadata set explicitly (e.g. test fixtures or manual Stripe edits) - const rawMeta = item?.price?.metadata?.planId || item?.plan?.metadata?.planId; - const fromMeta = validatePlan(rawMeta); - if (fromMeta) return fromMeta; - // Last-resort fallback — warn only when metadata is also absent so misconfigured - // config.stripe.prices is visible (otherwise this silently downgrades paid orgs to 'free', - // which is the exact bug #1250 fixed). When metadata is present but invalid, validatePlan() - // above already emitted an "unrecognized planId" warning — no double-warn needed. - if (priceId && !rawMeta) { - logger.warn('[billing.webhook] resolvePlan: priceId not in priceIdToPlan map and no metadata — falling back to free', { - priceId, - stripeSubscriptionId: subscription?.id, - }); - } - return 'free'; -}; +const resolvePlan = (subscription) => + resolvePlanFromSubscription(subscription, { logPrefix: '[billing.webhook]' }) ?? 'free'; /** * @description Sync the organization plan field to match the subscription plan. @@ -480,7 +438,7 @@ const handleSubscriptionUpdated = async (subscription, event) => { let planChangeResetTriggered = false; if (previousItems) { const previousPriceId = previousItems[0]?.price?.id; - const previousRaw = (previousPriceId && priceIdToPlan[previousPriceId]) + const previousRaw = lookupPlanByPriceId(previousPriceId) || previousItems[0]?.price?.metadata?.planId || previousItems[0]?.plan?.metadata?.planId || null; diff --git a/modules/billing/tests/billing.admin.service.unit.tests.js b/modules/billing/tests/billing.admin.service.unit.tests.js index 6547d0543..db1944110 100644 --- a/modules/billing/tests/billing.admin.service.unit.tests.js +++ b/modules/billing/tests/billing.admin.service.unit.tests.js @@ -48,6 +48,15 @@ describe('BillingAdminService unit tests:', () => { meterMode: true, plans: ['free', 'starter', 'pro', 'enterprise'], }, + // Real config.stripe.prices shape (#3964) — priceId → planId map, sourced by + // billing.planResolver.js. Real Stripe Price objects carry no metadata.planId; + // this map is what lets resolveStripePlan resolve without it. + stripe: { + prices: { + starter: { monthly: 'price_starter_monthly', annual: 'price_starter_annual' }, + pro: { monthly: 'price_pro_monthly', annual: 'price_pro_annual' }, + }, + }, }; mockSubscriptionRepository = { @@ -109,11 +118,13 @@ describe('BillingAdminService unit tests:', () => { }), }, subscriptions: { + // Realistic Stripe payload (#3964): price.id present, metadata.planId ABSENT — this + // is the actual shape Stripe sends; resolveStripePlan must resolve via the priceId map. retrieve: jest.fn().mockResolvedValue({ id: stripeSubId, status: 'active', current_period_start: 1699000000, - items: { data: [{ price: { metadata: { planId: 'pro' } } }] }, + items: { data: [{ price: { id: 'price_pro_monthly', metadata: {} } }] }, }), cancel: jest.fn().mockResolvedValue({ id: stripeSubId, status: 'canceled' }), }, @@ -218,6 +229,41 @@ describe('BillingAdminService unit tests:', () => { expect(result).toHaveProperty('updated'); }); + // ─── #3964 regression: priceId-map resolution + no-silent-downgrade guard ─── + test('#3964: resolves plan via priceId map for a paid subscription with NO metadata.planId (real Stripe shape)', async () => { + // Realistic payload — metadata is genuinely empty on real Stripe Price objects. + mockStripeInstance.subscriptions.retrieve.mockResolvedValue({ + id: stripeSubId, + status: 'active', + current_period_start: 1699000000, + items: { data: [{ price: { id: 'price_starter_annual', metadata: {} } }] }, + }); + + await BillingAdminService.syncOrgFromStripe(orgId); + + const updateCall = mockSubscriptionRepository.update.mock.calls[0][0]; + expect(updateCall.plan).toBe('starter'); + expect(updateCall.plan).not.toBe('free'); + expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'starter'); + }); + + test('#3964: ABORTS the sync (no DB write) when the Stripe price is unresolvable — never downgrades to free', async () => { + // Neither the priceId map nor metadata resolves — e.g. a manually-sold enterprise + // price never added to config.stripe.prices, or a misconfigured price map. + mockStripeInstance.subscriptions.retrieve.mockResolvedValue({ + id: stripeSubId, + status: 'active', + current_period_start: 1699000000, + items: { data: [{ price: { id: 'price_unmapped_xyz', metadata: {} } }] }, + }); + + await expect(BillingAdminService.syncOrgFromStripe(orgId)).rejects.toMatchObject({ status: 409 }); + + // Critical: no partial/incorrect write — the org's plan must be left untouched. + expect(mockSubscriptionRepository.update).not.toHaveBeenCalled(); + expect(mockOrganizationRepository.setPlan).not.toHaveBeenCalled(); + }); + test('throws 404 when no subscription exists', async () => { mockSubscriptionRepository.findByOrganization.mockResolvedValue(null); @@ -251,7 +297,7 @@ describe('BillingAdminService unit tests:', () => { items: { data: [{ current_period_start: periodStart, - price: { metadata: { planId: 'pro' } }, + price: { id: 'price_pro_monthly', metadata: {} }, }], }, }); @@ -269,7 +315,7 @@ describe('BillingAdminService unit tests:', () => { status: 'active', current_period_start: periodStart, items: { - data: [{ price: { metadata: { planId: 'pro' } } }], // no current_period_start on item + data: [{ price: { id: 'price_pro_monthly', metadata: {} } }], // no current_period_start on item }, }); diff --git a/modules/billing/tests/billing.admin.toolkit.integration.tests.js b/modules/billing/tests/billing.admin.toolkit.integration.tests.js index e174c3c70..1c090c6b9 100644 --- a/modules/billing/tests/billing.admin.toolkit.integration.tests.js +++ b/modules/billing/tests/billing.admin.toolkit.integration.tests.js @@ -287,6 +287,25 @@ describe('Billing admin toolkit integration tests:', () => { expect(res.status).toHaveBeenCalledWith(502); }); + // #3964 — resolveStripePlan aborts (409) rather than silently downgrading to 'free' + // when the Stripe price is unresolvable; the controller must surface that status. + test('returns 409 when service throws 409 (unresolvable plan — sync aborted)', async () => { + const routes = await buildRoutes(); + const route = routes.get('/api/admin/billing/sync/:orgId'); + const res = makeRes(); + mockAdminService.syncOrgFromStripe.mockRejectedValue( + Object.assign(new Error('cannot resolve plan for Stripe subscription'), { status: 409 }), + ); + + await runHandlers( + [...route.all, ...route.post], + { headers: { 'x-role': 'admin' }, params: { orgId }, body: {} }, + res, + ); + + expect(res.status).toHaveBeenCalledWith(409); + }); + test('returns 500 on unexpected error', async () => { const routes = await buildRoutes(); const route = routes.get('/api/admin/billing/sync/:orgId'); diff --git a/modules/billing/tests/billing.planResolver.unit.tests.js b/modules/billing/tests/billing.planResolver.unit.tests.js new file mode 100644 index 000000000..3f04d6cfb --- /dev/null +++ b/modules/billing/tests/billing.planResolver.unit.tests.js @@ -0,0 +1,162 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals'; + +/** + * Unit tests for the shared plan resolver (`../lib/billing.planResolver.js`). + * Extracted from billing.webhook.service.js (#3964/#1250) so the webhook handler, the admin + * force-sync tool, and the reconcile cron all resolve a Stripe subscription's plan via ONE + * implementation instead of three drifted copies. + */ +describe('billing.planResolver unit tests:', () => { + let buildPriceIdToPlanMap; + let resolvePlanFromSubscription; + let lookupPlanByPriceId; + let mockLogger; + + const mockConfig = { + billing: { plans: ['free', 'starter', 'pro', 'enterprise'] }, + stripe: { + prices: { + starter: { monthly: 'price_starter_monthly', annual: 'price_starter_annual' }, + pro: { monthly: 'price_pro_monthly', annual: 'price_pro_annual' }, + // 'enterprise' deliberately has no Stripe price — sold manually, never mapped. + }, + }, + }; + + beforeEach(async () => { + jest.resetModules(); + mockLogger = { info: jest.fn(), error: jest.fn(), warn: jest.fn(), debug: jest.fn() }; + + jest.unstable_mockModule('../../../config/index.js', () => ({ default: mockConfig })); + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ default: mockLogger })); + + const mod = await import('../lib/billing.planResolver.js'); + ({ buildPriceIdToPlanMap, resolvePlanFromSubscription, lookupPlanByPriceId } = mod); + }); + + afterEach(() => jest.restoreAllMocks()); + + describe('buildPriceIdToPlanMap:', () => { + test('maps monthly + annual priceIds to their planId', () => { + const map = buildPriceIdToPlanMap(); + expect(map).toEqual({ + price_starter_monthly: 'starter', + price_starter_annual: 'starter', + price_pro_monthly: 'pro', + price_pro_annual: 'pro', + }); + }); + + test('skips plan entries not present in the valid-plans enum', async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + billing: { plans: ['free', 'starter', 'pro', 'enterprise'] }, + stripe: { + prices: { + starter: { monthly: 'price_starter_monthly' }, + // 'legacy_gold' is not in billing.plans — must be excluded from the map. + legacy_gold: { monthly: 'price_legacy_gold_monthly' }, + }, + }, + }, + })); + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ default: mockLogger })); + + const { buildPriceIdToPlanMap: freshBuild } = await import('../lib/billing.planResolver.js'); + const map = freshBuild(); + + expect(map).toEqual({ price_starter_monthly: 'starter' }); + expect(map.price_legacy_gold_monthly).toBeUndefined(); + }); + }); + + describe('resolvePlanFromSubscription — priceId map (primary):', () => { + test('resolves via priceId map when metadata.planId is absent (real Stripe payload)', () => { + const plan = resolvePlanFromSubscription({ + id: 'sub_1', + items: { data: [{ price: { id: 'price_pro_monthly', metadata: {} } }] }, + }); + expect(plan).toBe('pro'); + }); + + test('resolves annual priceId to the correct plan', () => { + const plan = resolvePlanFromSubscription({ + id: 'sub_2', + items: { data: [{ price: { id: 'price_starter_annual', metadata: {} } }] }, + }); + expect(plan).toBe('starter'); + }); + + test('priceId map takes priority over conflicting metadata.planId', () => { + const plan = resolvePlanFromSubscription({ + id: 'sub_3', + items: { data: [{ price: { id: 'price_starter_monthly', metadata: { planId: 'pro' } } }] }, + }); + expect(plan).toBe('starter'); + }); + }); + + describe('resolvePlanFromSubscription — metadata fallback (legacy):', () => { + test('falls back to price.metadata.planId when priceId is not mapped', () => { + const plan = resolvePlanFromSubscription({ + id: 'sub_4', + items: { data: [{ price: { id: 'price_fixture_unknown', metadata: { planId: 'pro' } } }] }, + }); + expect(plan).toBe('pro'); + }); + + test('falls back to plan.metadata.planId when price.metadata is absent', () => { + const plan = resolvePlanFromSubscription({ + id: 'sub_5', + items: { data: [{ price: {}, plan: { metadata: { planId: 'starter' } } }] }, + }); + expect(plan).toBe('starter'); + }); + }); + + describe('resolvePlanFromSubscription — unresolvable (null):', () => { + test('returns null and warns when priceId is unmapped and metadata is absent', () => { + const plan = resolvePlanFromSubscription( + { id: 'sub_6', items: { data: [{ price: { id: 'price_unmapped_xyz', metadata: {} } }] } }, + { logPrefix: '[billing.test]' }, + ); + expect(plan).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[billing.test] resolvePlanFromSubscription: priceId not in priceIdToPlan map and no metadata', + expect.objectContaining({ priceId: 'price_unmapped_xyz', stripeSubscriptionId: 'sub_6' }), + ); + }); + + test('returns null and warns when metadata.planId is present but not a valid plan', () => { + const plan = resolvePlanFromSubscription( + { id: 'sub_7', items: { data: [{ price: { metadata: { planId: 'prod_unknownXYZ' } } }] } }, + { logPrefix: '[billing.test]' }, + ); + expect(plan).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[billing.test] resolvePlanFromSubscription: unrecognized planId in metadata', + expect.objectContaining({ raw: 'prod_unknownXYZ' }), + ); + }); + + test('returns null for a subscription with no items (defensive)', () => { + expect(resolvePlanFromSubscription({ id: 'sub_8' })).toBeNull(); + expect(resolvePlanFromSubscription(null)).toBeNull(); + }); + }); + + describe('lookupPlanByPriceId:', () => { + test('returns the plan for a known priceId', () => { + expect(lookupPlanByPriceId('price_pro_monthly')).toBe('pro'); + }); + + test('returns undefined for an unmapped or missing priceId', () => { + expect(lookupPlanByPriceId('price_unknown')).toBeUndefined(); + expect(lookupPlanByPriceId(undefined)).toBeUndefined(); + }); + }); +}); diff --git a/modules/billing/tests/billing.reconcile.service.unit.tests.js b/modules/billing/tests/billing.reconcile.service.unit.tests.js index 19e7c6f88..d361f9651 100644 --- a/modules/billing/tests/billing.reconcile.service.unit.tests.js +++ b/modules/billing/tests/billing.reconcile.service.unit.tests.js @@ -36,11 +36,13 @@ describe('BillingReconcileService.runReconciliation unit tests:', () => { /** * Build a stub Stripe subscription. + * Realistic Stripe payload (#3964): price.id present, metadata.planId ABSENT — real Stripe + * Price objects carry no metadata.planId; resolveStripePlan must resolve via the priceId map. */ const makeStripeSub = (overrides = {}) => ({ id: stripeSubId, status: 'active', - items: { data: [{ price: { metadata: { planId: 'pro' } } }] }, + items: { data: [{ price: { id: 'price_pro_monthly', metadata: {} } }] }, ...overrides, }); @@ -55,6 +57,14 @@ describe('BillingReconcileService.runReconciliation unit tests:', () => { meterMode: true, plans: ['free', 'starter', 'pro', 'enterprise'], }, + // Real config.stripe.prices shape (#3964) — priceId → planId map used by the shared + // resolver (billing.planResolver.js), same as production config. + stripe: { + prices: { + starter: { monthly: 'price_starter_monthly', annual: 'price_starter_annual' }, + pro: { monthly: 'price_pro_monthly', annual: 'price_pro_annual' }, + }, + }, }; mockStripeInstance = { @@ -143,6 +153,53 @@ describe('BillingReconcileService.runReconciliation unit tests:', () => { expect(mockEvents.emit).not.toHaveBeenCalled(); }); + // ─── #3964 regression: priceId-map resolution replaces the metadata-only resolver ─── + test('#3964: resolves plan via priceId map for a paid sub with NO metadata.planId — no false planMismatch', async () => { + // DB says 'pro'; Stripe price resolves to 'pro' via the priceId map (no metadata at all). + // Before the fix this resolver defaulted to 'free', producing a false planMismatch alert + // for every paid org — exactly the noise this issue reports. + const sub = makeDbSub({ plan: 'pro' }); + mockSubscriptionRepository.findPageForReconciliation + .mockResolvedValueOnce([sub]) + .mockResolvedValue([]); + mockStripeInstance.subscriptions.retrieve.mockResolvedValue( + makeStripeSub({ items: { data: [{ price: { id: 'price_pro_monthly', metadata: {} } }] } }), + ); + + const result = await BillingReconcileService.runReconciliation(); + + expect(result).toMatchObject({ checked: 1, divergences: 0, errors: 0 }); + const planMismatchEmits = mockEvents.emit.mock.calls.filter( + ([, p]) => p?.planMismatch === true, + ); + expect(planMismatchEmits).toHaveLength(0); + }); + + test('#3964: unresolvable Stripe price — skips plan comparison instead of emitting a false divergence', async () => { + // Neither the priceId map nor metadata resolves. The old resolver would have defaulted + // to 'free' here and (falsely) diverged against any non-free DB plan. The fix must skip + // the plan comparison entirely rather than guess. + const sub = makeDbSub({ plan: 'pro', status: 'active' }); + mockSubscriptionRepository.findPageForReconciliation + .mockResolvedValueOnce([sub]) + .mockResolvedValue([]); + mockStripeInstance.subscriptions.retrieve.mockResolvedValue( + makeStripeSub({ status: 'active', items: { data: [{ price: { id: 'price_unmapped_xyz', metadata: {} } }] } }), + ); + + const result = await BillingReconcileService.runReconciliation(); + + expect(result).toMatchObject({ checked: 1, divergences: 0, errors: 0 }); + const planMismatchEmits = mockEvents.emit.mock.calls.filter( + ([, p]) => p?.planMismatch === true, + ); + expect(planMismatchEmits).toHaveLength(0); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[billing.reconcile] cannot resolve Stripe plan — skipping plan comparison for this subscription', + expect.objectContaining({ organizationId: orgId, dbPlan: 'pro' }), + ); + }); + test('detects status divergence — emits billing.reconciliation.divergence and logs', async () => { const sub = makeDbSub({ status: 'active' }); mockSubscriptionRepository.findPageForReconciliation diff --git a/modules/billing/tests/billing.webhook.subscription.unit.tests.js b/modules/billing/tests/billing.webhook.subscription.unit.tests.js index 4e9ebf407..c74a95dd4 100644 --- a/modules/billing/tests/billing.webhook.subscription.unit.tests.js +++ b/modules/billing/tests/billing.webhook.subscription.unit.tests.js @@ -664,7 +664,7 @@ describe('Billing webhook subscription unit tests:', () => { ); }); - test('V8.1 — validatePlan warns on unrecognized non-empty planId (falls back to free)', async () => { + test('V8.1 — resolvePlanFromSubscription warns on unrecognized non-empty planId (falls back to free)', async () => { const existing = { _id: subId, organization: orgId, @@ -684,9 +684,10 @@ describe('Billing webhook subscription unit tests:', () => { BillingWebhookService.handleInvoicePaymentSucceeded({ subscription: 'sub_456' }, makeEvent()), ).resolves.not.toThrow(); - // validatePlan should have logged a warning for the unrecognized plan + // Shared resolver (billing.planResolver.js) should have logged a warning for the + // unrecognized plan (#3964 — now shared with the admin + reconcile call sites). expect(mockLogger.warn).toHaveBeenCalledWith( - '[billing.webhook] validatePlan: unrecognized planId', + '[billing.webhook] resolvePlanFromSubscription: unrecognized planId in metadata', expect.objectContaining({ raw: 'prod_unknownXYZ' }), ); });