From e2ccdc2ee3aaab90f57ae58684716cdb6040bc2e Mon Sep 17 00:00:00 2001 From: aamoghS Date: Thu, 24 Sep 2026 22:50:04 -0400 Subject: [PATCH] fix(stripe): only confirm intents minted for this user's membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit confirmMembershipAfterPayment refused an intent only when metadata.userId was present and named someone else. A succeeded intent with no userId — hosted Checkout, a payment link, a Dashboard charge — passed with no type or amount check, and readPlan(undefined) granted a full year. Anyone holding such a charge id could claim it, including a $1 link or a $15 semester checkout confirmed before the webhook arrived. It now applies the same gate as the webhook and reconcileMyPayments: metadata.userId must equal the caller, metadata.type must be a membership or the bootcamp add-on, and the amount must be within MAX_MEMBERSHIP_CHARGE_CENTS. Every intent createPaymentIntent mints has carried type since June; mock intents now carry it too. The insert also uses onConflictDoNothing: when the webhook records the same intent first, confirm skips the grant instead of surfacing the unique violation as a 500 on a successful payment. --- .../.internal-tests/stripe-payments.test.ts | 94 ++++++++++++++++++- packages/api/src/routers/stripe.ts | 71 +++++++++----- 2 files changed, 135 insertions(+), 30 deletions(-) diff --git a/packages/api/src/.internal-tests/stripe-payments.test.ts b/packages/api/src/.internal-tests/stripe-payments.test.ts index 656100db..68ef69bb 100644 --- a/packages/api/src/.internal-tests/stripe-payments.test.ts +++ b/packages/api/src/.internal-tests/stripe-payments.test.ts @@ -41,14 +41,20 @@ const mockUpdateSet = vi.fn(); */ /** Payment intents `reconcileMyPayments` should find. Set per test. */ const mockSearchResults = vi.fn<() => unknown[]>(() => []); +/** The intent `paymentIntents.retrieve` returns. Throws unless a test sets it. */ +const mockRetrieve = vi.fn<(id: string) => unknown>((id) => { + throw new Error(`No such payment_intent: ${id}`); +}); +/** What an insert … onConflictDoNothing().returning() yields; [] is a conflict. */ +const mockConflictReturning = vi.fn<() => unknown[]>(() => [ + { id: "payment_row" }, +]); vi.mock("stripe", () => ({ default: class { paymentIntents = { search: vi.fn(async () => ({ data: mockSearchResults() })), - retrieve: vi.fn(async (id: string) => { - throw new Error(`No such payment_intent: ${id}`); - }), + retrieve: vi.fn(async (id: string) => mockRetrieve(id)), create: vi.fn(async () => ({ id: "pi_stub", client_secret: "pi_stub_secret", @@ -86,7 +92,7 @@ vi.mock("@query/db", () => { return Object.assign(Promise.resolve(val), { returning: vi.fn().mockResolvedValue([{ id: "payment_row" }]), onConflictDoNothing: vi.fn().mockImplementation(() => ({ - returning: vi.fn().mockResolvedValue([{ id: "payment_row" }]), + returning: vi.fn(async () => mockConflictReturning()), })), }); }, @@ -152,6 +158,11 @@ describe("Membership payments", () => { beforeEach(() => { vi.clearAllMocks(); cache.clear(); + // clearAllMocks keeps implementations; these two are set per test. + mockRetrieve.mockImplementation((id) => { + throw new Error(`No such payment_intent: ${id}`); + }); + mockConflictReturning.mockImplementation(() => [{ id: "payment_row" }]); // Both are set per-test; clearing here keeps one test's mode from leaking // into the next. delete process.env.STRIPE_SECRET_KEY; @@ -339,6 +350,81 @@ describe("Membership payments", () => { }); }); + /** + * A live intent is only honoured if this server minted it for the caller, + * for a membership or the add-on, within the membership ceiling. An intent + * with no userId — hosted Checkout, a payment link, a Dashboard charge — + * used to pass, and an absent plan granted a full year. + */ + describe("confirming a live payment", () => { + const intent = (overrides: Record = {}) => ({ + id: "pi_live_1", + status: "succeeded", + amount: MEMBERSHIP_CENTS, + currency: "usd", + metadata: { userId: USER, type: "membership", plan: "annual" }, + ...overrides, + }); + + const membershipWritten = () => + mockInsert.mock.calls.some((c) => c[0]?.[0]?.firstName); + + beforeEach(() => { + process.env.STRIPE_SECRET_KEY = "sk_test_abc"; + }); + + const refused = async (pi: unknown) => { + mockRetrieve.mockImplementation(() => pi); + const err: any = await caller() + .stripe.confirmMembershipAfterPayment({ paymentIntentId: "pi_live_1" }) + .catch((e: unknown) => e); + expect(err.code).toBe("FORBIDDEN"); + expect(mockInsert).not.toHaveBeenCalled(); + }; + + it("grants the caller's own membership intent", async () => { + mockRetrieve.mockImplementation(() => intent()); + + await caller().stripe.confirmMembershipAfterPayment({ + paymentIntentId: "pi_live_1", + }); + + expect(membershipWritten()).toBe(true); + }); + + it("refuses an intent that carries no userId", async () => { + await refused(intent({ metadata: {} })); + }); + + it("refuses another user's intent", async () => { + await refused( + intent({ metadata: { userId: "someone_else", type: "membership" } }), + ); + }); + + it("refuses an intent that is not a membership", async () => { + await refused(intent({ metadata: { userId: USER, type: "donation" } })); + }); + + it("refuses an amount over the membership ceiling", async () => { + await refused(intent({ amount: 1_000_000 })); + }); + + // The webhook records the same intent under the same session id. Losing + // that race is not an error: the webhook has already granted. + it("does not grant twice when the webhook recorded it first", async () => { + mockRetrieve.mockImplementation(() => intent()); + mockConflictReturning.mockImplementation(() => []); + + await expect( + caller().stripe.confirmMembershipAfterPayment({ + paymentIntentId: "pi_live_1", + }), + ).resolves.toMatchObject({ success: true }); + expect(membershipWritten()).toBe(false); + }); + }); + describe("input and account preconditions", () => { it("rejects a return URL that is not a URL", async () => { process.env.STRIPE_MOCK_MODE = "true"; diff --git a/packages/api/src/routers/stripe.ts b/packages/api/src/routers/stripe.ts index d31607f0..ad8c201b 100644 --- a/packages/api/src/routers/stripe.ts +++ b/packages/api/src/routers/stripe.ts @@ -445,7 +445,7 @@ export const stripeRouter = createTRPCRouter({ userId: ctx.userId!, bootcamp: mockAddOn ? "true" : "false", plan: mockPlan, - ...(mockAddOn ? { type: BOOTCAMP_ADDON_PAYMENT_TYPE } : {}), + type: mockAddOn ? BOOTCAMP_ADDON_PAYMENT_TYPE : "membership", }, }; } else { @@ -470,12 +470,22 @@ export const stripeRouter = createTRPCRouter({ }); } - // Ensure the intent was for this user (guard against replay attacks) - if (pi.metadata?.userId && pi.metadata.userId !== ctx.userId) { + // Only an intent this server minted for this caller, for a membership or + // the add-on, within the membership ceiling — the same gate the webhook + // and reconcileMyPayments apply. An intent with no userId (hosted + // Checkout, a payment link, a Dashboard charge) used to pass, and + // readPlan(undefined) granted a full year for whatever it cost. + const paymentType = pi.metadata?.type; + if ( + pi.metadata?.userId !== ctx.userId || + (paymentType !== "membership" && + paymentType !== BOOTCAMP_ADDON_PAYMENT_TYPE) || + pi.amount > MAX_MEMBERSHIP_CHARGE_CENTS + ) { logSecurityEvent({ type: "validation_error", identifier: ctx.userId ?? "unknown", - details: `PaymentIntent userId mismatch: ${pi.metadata.userId} vs ${ctx.userId}`, + details: `PaymentIntent refused: userId=${pi.metadata?.userId ?? "none"} type=${paymentType ?? "none"} amount=${pi.amount}`, }); throw new TRPCError({ code: "FORBIDDEN", @@ -506,27 +516,35 @@ export const stripeRouter = createTRPCRouter({ // rather than only logging it. try { if (!existing) { - await ctx.db!.transaction(async (tx) => { - await tx.insert(stripePayments).values({ - stripeSessionId: `pi_${pi.id}`, - stripeCustomerId: - typeof pi.customer === "string" - ? pi.customer - : (pi.customer?.id ?? ""), - stripePaymentIntentId: pi.id, - customerEmail: ( - pi.receipt_email ?? - user?.email ?? - "" - ).toLowerCase(), - customerName: user?.name ?? "Member", - amountTotal: pi.amount, - currency: pi.currency, - paymentStatus: "paid", - linkedUserId: ctx.userId!, - linkedAt: new Date(), - metadata: JSON.stringify(pi.metadata ?? {}), - }); + const granted = await ctx.db!.transaction(async (tx) => { + // The webhook records the same intent under the same session id. If + // it lands between the read above and this insert, it has already + // granted: skip rather than surface the unique violation as a 500. + const [inserted] = await tx + .insert(stripePayments) + .values({ + stripeSessionId: `pi_${pi.id}`, + stripeCustomerId: + typeof pi.customer === "string" + ? pi.customer + : (pi.customer?.id ?? ""), + stripePaymentIntentId: pi.id, + customerEmail: ( + pi.receipt_email ?? + user?.email ?? + "" + ).toLowerCase(), + customerName: user?.name ?? "Member", + amountTotal: pi.amount, + currency: pi.currency, + paymentStatus: "paid", + linkedUserId: ctx.userId!, + linkedAt: new Date(), + metadata: JSON.stringify(pi.metadata ?? {}), + }) + .onConflictDoNothing() + .returning({ id: stripePayments.id }); + if (!inserted) return false; await createOrUpdateMembership(tx as unknown as DrizzleDB, { userId: ctx.userId!, @@ -536,9 +554,10 @@ export const stripeRouter = createTRPCRouter({ addOnOnly, plan, }); + return true; }); - membershipGrants.inc({ source: "confirm", plan }); + if (granted) membershipGrants.inc({ source: "confirm", plan }); } else if (!existing.linkedUserId) { // Payment exists but wasn't linked — link it now await ctx