fix(billing): bump event markers on direct-write paths (#3628)

* fix(billing): bump event markers on direct-write paths

V8 audit C3: markUnpaid, adminUpdatePlanOnly, and cancelSubscription
skipped bumpEventMarkers, allowing a stale Stripe redelivery to pass
updateIfEventNewer and silently overwrite the direct write.

- Add bumpEventMarkers helper (modules/billing/lib/billing.markerBump.js)
- Spread markers into markUnpaid $set ('dunning' source)
- Spread markers into adminUpdatePlanOnly $set ('admin-bump' source)
- Spread markers into cancelSubscription update payload ('admin-cancel' source)
- Add 3 V8-C3 unit tests asserting marker presence post-write
- Update 2 existing exact-match assertions to objectContaining

* docs(billing): fix stale JSDoc + test comment after marker-bump fix

- adminUpdatePlanOnly JSDoc now reflects that lastSubscriptionEvent* markers
  are bumped (per V8-C3 fix) — the old text said "Never touches … markers"
- markUnpaid V8-C3 test comment: was "invoice.payment_failed" but the guard
  and markers are subscription-family; corrected to "customer.subscription.updated"

Author: PierreBrisorgueil

modules/billing/lib/billing.markerBump.js

@@ -0,0 +1,18 @@
+/**
+ * Build the event-ordering marker fields to merge into a direct-write update,
+ * so a stale Stripe redelivery cannot overwrite the write via updateIfEventNewer.
+ * Use ONLY in direct-write paths (admin overrides, dunning sweep, cancellation) —
+ * normal webhook handlers go through updateIfEventNewer which sets these on its own.
+ *
+ * @param {'subscription'|'invoice'} family
+ * @param {string} source - prefix for the synthesised event id (e.g. 'admin-bump', 'dunning')
+ * @returns {Object} { last{Family}EventCreatedAt, last{Family}EventId }
+ */
+export const bumpEventMarkers = (family, source) => {
+  const ms = Date.now();
+  const fieldPrefix = family === 'invoice' ? 'lastInvoiceEvent' : 'lastSubscriptionEvent';
+  return {
+    [`${fieldPrefix}CreatedAt`]: Math.floor(ms / 1000),
+    [`${fieldPrefix}Id`]: `${source}-${ms}`,
+  };
+};

modules/billing/repositories/billing.subscription.repository.js

@@ -2,6 +2,7 @@
  * Module dependencies
  */
 import mongoose from 'mongoose';
+import { bumpEventMarkers } from '../lib/billing.markerBump.js';
 
 const Subscription = mongoose.model('Subscription');
 
@@ -206,7 +207,7 @@ const markUnpaid = (id, threshold) => {
   if (!(threshold instanceof Date)) throw new TypeError('threshold must be a Date instance');
   return Subscription.findOneAndUpdate(
     { _id: id, status: 'past_due', pastDueSince: { $lte: threshold } },
-    { $set: { status: 'unpaid', plan: 'free' } },
+    { $set: { status: 'unpaid', plan: 'free', ...bumpEventMarkers('subscription', 'dunning') } },
     { returnDocument: 'after', runValidators: true },
   ).exec();
 };
@@ -273,16 +274,19 @@ const updateIfEventNewer = (id, eventCreatedAt, eventId, fields, family = 'subsc
 
 /**
  * @function adminUpdatePlanOnly
- * @description Restricted admin update — sets ONLY `plan` and `adminUpdatedAt`. Never touches
- *              `status`, `stripeCustomerId`, `stripeSubscriptionId`, `currentPeriodStart`, or
- *              the per-family event-ordering markers. Webhook handlers retain exclusive write
- *              authority on those fields via `updateIfEventNewer`.
+ * @description Restricted admin update — sets ONLY `plan`, `adminUpdatedAt`, and the
+ *              subscription-family event-ordering markers. Never touches `status`,
+ *              `stripeCustomerId`, `stripeSubscriptionId`, or `currentPeriodStart`.
  *
  *              Why this restriction matters: a free `update()` call with `{ plan, status }` from
  *              an admin path would overwrite Stripe-driven status (active/past_due/etc.) with
  *              whatever the admin sent, breaking dunning + access control. Splitting the surface
  *              forces admin overrides to plan-only territory.
  *
+ *              The subscription-family markers (`lastSubscriptionEvent*`) are bumped so a
+ *              subsequent stale Stripe webhook redelivery (customer.subscription.*) cannot
+ *              overwrite the admin plan change via `updateIfEventNewer`.
+ *
  * @param {string} id - Subscription ObjectId (string).
  * @param {string} planId - The new plan identifier (must be in config.billing.plans enum).
  * @param {string} adminUserId - The admin user ObjectId (string) who triggered the bump (for audit).
@@ -302,6 +306,7 @@ const adminUpdatePlanOnly = (id, planId, adminUserId) => {
         plan: planId,
         adminUpdatedAt: new Date(),
         adminUpdatedBy: safeAdminUserId,
+        ...bumpEventMarkers('subscription', 'admin-bump'),
       },
     },
     { returnDocument: 'after', runValidators: true },

modules/billing/services/billing.admin.service.js

@@ -4,6 +4,7 @@
 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';
 import SubscriptionRepository from '../repositories/billing.subscription.repository.js';
 import ProcessedStripeEventRepository from '../repositories/billing.processedStripeEvent.repository.js';
 import OrganizationRepository from '../../organizations/repositories/organizations.repository.js';
@@ -273,6 +274,7 @@ const cancelSubscription = async (orgId) => {
     _id: existing._id,
     plan: 'free',
     status: 'canceled',
+    ...bumpEventMarkers('subscription', 'admin-cancel'),
   });
 
   await OrganizationRepository.setPlan(orgId, 'free');

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

@@ -435,6 +435,22 @@ describe('BillingAdminService unit tests:', () => {
         expect.any(Object),
       );
     });
+
+    // V8 audit C3 — cancelSubscription must bump markers so a stale
+    // customer.subscription.updated redelivery cannot restore 'active' after
+    // the admin cancellation wrote 'canceled' via direct SubscriptionRepository.update.
+    test('V8-C3: bumps lastSubscriptionEventCreatedAt + lastSubscriptionEventId so stale webhook is rejected', async () => {
+      const before = Math.floor(Date.now() / 1000);
+      await BillingAdminService.cancelSubscription(orgId);
+      const after = Math.floor(Date.now() / 1000);
+
+      const updateCall = mockSubscriptionRepository.update.mock.calls[0][0];
+      const { lastSubscriptionEventCreatedAt, lastSubscriptionEventId } = updateCall;
+
+      expect(lastSubscriptionEventCreatedAt).toBeGreaterThanOrEqual(before);
+      expect(lastSubscriptionEventCreatedAt).toBeLessThanOrEqual(after);
+      expect(lastSubscriptionEventId).toMatch(/^admin-cancel-\d+$/);
+    });
   });
 
   // ─────────────────────────────────────────────────────────────────────────────

modules/billing/tests/billing.cron.dunningSweep.unit.tests.js

@@ -105,7 +105,7 @@ describe('billing.dunningSweep cron — BillingSubscriptionRepository:', () => {
 
       expect(mockModel.findOneAndUpdate).toHaveBeenCalledWith(
         { _id: subId, status: 'past_due', pastDueSince: { $lte: threshold } },
-        { $set: { status: 'unpaid', plan: 'free' } },
+        { $set: expect.objectContaining({ status: 'unpaid', plan: 'free' }) },
         expect.objectContaining({ returnDocument: 'after' }),
       );
       expect(result).toEqual(updated);

modules/billing/tests/billing.subscription.repository.unit.tests.js

@@ -105,7 +105,7 @@ describe('BillingSubscriptionRepository unit tests:', () => {
 
       expect(mockModel.findOneAndUpdate).toHaveBeenCalledWith(
         { _id: subId, status: 'past_due', pastDueSince: { $lte: threshold } },
-        { $set: { status: 'unpaid', plan: 'free' } },
+        { $set: expect.objectContaining({ status: 'unpaid', plan: 'free' }) },
         { returnDocument: 'after', runValidators: true },
       );
       expect(result).toEqual(updated);
@@ -392,5 +392,48 @@ describe('BillingSubscriptionRepository unit tests:', () => {
       const callArgs = mockModel.findByIdAndUpdate.mock.calls[0];
       expect(callArgs[1].$set.adminUpdatedAt).toBeInstanceOf(Date);
     });
+
+    // V8 audit C3 — event-ordering markers must be bumped so a stale Stripe redelivery
+    // cannot overwrite the admin plan bump via updateIfEventNewer.
+    test('V8-C3: bumps lastSubscriptionEventCreatedAt + lastSubscriptionEventId so stale webhook is rejected', async () => {
+      const execMock = jest.fn().mockResolvedValue({ _id: subId, plan: 'pro' });
+      const populateMock = jest.fn().mockReturnValue({ exec: execMock });
+      mockModel.findByIdAndUpdate.mockReturnValue({ populate: populateMock });
+
+      const before = Math.floor(Date.now() / 1000);
+      await BillingSubscriptionRepository.adminUpdatePlanOnly(subId, 'pro', adminId);
+      const after = Math.floor(Date.now() / 1000);
+
+      const callArgs = mockModel.findByIdAndUpdate.mock.calls[0];
+      const { lastSubscriptionEventCreatedAt, lastSubscriptionEventId } = callArgs[1].$set;
+
+      expect(lastSubscriptionEventCreatedAt).toBeGreaterThanOrEqual(before);
+      expect(lastSubscriptionEventCreatedAt).toBeLessThanOrEqual(after);
+      expect(lastSubscriptionEventId).toMatch(/^admin-bump-\d+$/);
+    });
+  });
+
+  // ── markUnpaid — V8 audit C3 marker bump ─────────────────────────────────
+
+  describe('markUnpaid — V8-C3 event-ordering markers', () => {
+    const threshold = new Date(Date.now() - 14 * 24 * 60 * 60 * 1000);
+
+    // V8 audit C3 — markUnpaid must bump markers so a stale customer.subscription.updated
+    // redelivery cannot restore 'past_due' after the dunning sweep set 'unpaid'.
+    test('V8-C3: bumps lastSubscriptionEventCreatedAt + lastSubscriptionEventId so stale webhook is rejected', async () => {
+      const updated = { _id: subId, status: 'unpaid', plan: 'free' };
+      mockModel.findOneAndUpdate.mockReturnValue({ exec: jest.fn().mockResolvedValue(updated) });
+
+      const before = Math.floor(Date.now() / 1000);
+      await BillingSubscriptionRepository.markUnpaid(subId, threshold);
+      const after = Math.floor(Date.now() / 1000);
+
+      const callArgs = mockModel.findOneAndUpdate.mock.calls[0];
+      const { lastSubscriptionEventCreatedAt, lastSubscriptionEventId } = callArgs[1].$set;
+
+      expect(lastSubscriptionEventCreatedAt).toBeGreaterThanOrEqual(before);
+      expect(lastSubscriptionEventCreatedAt).toBeLessThanOrEqual(after);
+      expect(lastSubscriptionEventId).toMatch(/^dunning-\d+$/);
+    });
   });
 });