From c86b3de611f5b987623a916a3f9a76f16a6f8120 Mon Sep 17 00:00:00 2001 From: ced4568 Date: Fri, 28 Aug 2026 09:46:51 -0500 Subject: [PATCH] fix(payment): correlate PaymentIntent/Charge-referencing webhooks to the right transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real Stripe TEST traffic (Phase 9.1) exposed a genuine defect the deterministic TestProvider never could: payment_intent.payment_failed and charge.refunded events reference Stripe's PaymentIntent/Charge object, never the Checkout Session, but payment_transactions. provider_reference only ever stored the Session id. A real failure or refund webhook would have silently failed to match any transaction (UNKNOWN_TRANSACTION). - payment-provider.js: session creation now also sets payment_intent_data.metadata (the same metadata already carrying our transaction_id), so it propagates onto the PaymentIntent Stripe creates. normalizeEvent() (now async) reads transaction_id back from the PaymentIntent's own metadata directly for payment_intent.payment_failed, and via one extra paymentIntents.retrieve call for charge.refunded (Charges don't inherit PaymentIntent metadata automatically). - commerce.js: the transaction lookup now matches provider_reference OR transaction_id — one additional OR clause, no schema/migration change. - webhook-routes.js: added the now-required await on verifyAndParseWebhook (TestProvider's synchronous return is unaffected). - docker-compose.dev.yml: wires PAYMENT_PROVIDER/STRIPE_SECRET_KEY/STRIPE_WEBHOOK_SECRET through to the api container (previously entirely absent — Phase 9 never needed it against the deterministic provider), empty/unset by default matching EMAIL_PROVIDER's own convention. Verified fixed against a real Stripe TEST refund (Phase 9.1); e2e-phase9.js 20/20 and the Winter Break commerce rehearsal 13/13 rerun clean after the fix. --- platform/api/src/commerce.js | 8 ++++++- platform/api/src/payment-provider.js | 36 ++++++++++++++++++++++++---- platform/api/src/webhook-routes.js | 2 +- platform/docker-compose.dev.yml | 8 +++++++ 4 files changed, 48 insertions(+), 6 deletions(-) diff --git a/platform/api/src/commerce.js b/platform/api/src/commerce.js index 53d4416..5c112bc 100644 --- a/platform/api/src/commerce.js +++ b/platform/api/src/commerce.js @@ -85,7 +85,13 @@ async function processWebhookEvent(pool, studentsAuth, provider, normalizedEvent return { result: 'ignored' }; } - const { rows: txRows } = await pool.query(`SELECT * FROM payment_transactions WHERE provider_reference = $1`, [providerReference]); + // Phase 9.1 real-Stripe-TEST fix — providerReference is usually the Checkout Session id, but + // for payment_intent.payment_failed/charge.refunded StripeProvider.normalizeEvent() instead + // returns OUR OWN transaction_id (there is no other reliable Stripe-side reference for those + // event types back to the Session-keyed row) — match either column, never a schema change. + const { rows: txRows } = await pool.query( + `SELECT * FROM payment_transactions WHERE provider_reference = $1 OR transaction_id::text = $1`, [providerReference] + ); const transaction = txRows[0]; if (!transaction) { await pool.query(`UPDATE payment_webhook_events SET result = 'error', processed_at = now() WHERE webhook_event_id = $1`, [eventId]); diff --git a/platform/api/src/payment-provider.js b/platform/api/src/payment-provider.js index d0acb53..d396afb 100644 --- a/platform/api/src/payment-provider.js +++ b/platform/api/src/payment-provider.js @@ -42,13 +42,22 @@ class StripeProvider { success_url: successUrl, cancel_url: cancelUrl, metadata, + // Phase 9.1 real-Stripe-TEST fix — a later payment_intent.payment_failed or + // charge.refunded event references the PaymentIntent/Charge, never the Checkout Session + // (a genuinely different Stripe object id, only discovered by testing against real + // Stripe — the deterministic TestProvider always reused one caller-supplied reference for + // every event type, which never exposed this). The Session's own `metadata` above does + // NOT propagate to its PaymentIntent automatically; `payment_intent_data.metadata` does — + // this is what lets normalizeEvent() below correlate those later events back to OUR OWN + // transaction_id, not Stripe's session id. + payment_intent_data: { metadata }, }); return { checkoutUrl: session.url, providerReference: session.id }; } // Real signature verification via Stripe's own SDK helper — never a hand-rolled HMAC compare. // Throws on a bad/forged signature; the webhook route treats that as a clean 400, never a 500. - verifyAndParseWebhook(rawBody, signatureHeader) { + async verifyAndParseWebhook(rawBody, signatureHeader) { if (!this.client || !this.webhookSecret) throw Object.assign(new Error('Stripe webhook verification is not configured.'), { code: 'PROVIDER_NOT_CONFIGURED' }); const event = this.client.webhooks.constructEvent(rawBody, signatureHeader, this.webhookSecret); return this.normalizeEvent(event); @@ -58,14 +67,33 @@ class StripeProvider { // types this integration actually acts on are mapped to a real status; everything else comes // back with normalizedStatus: null, which webhook-routes.js records (for audit completeness) // but never acts on. - normalizeEvent(event) { + // + // providerReference is usually the Checkout Session id (matches payment_transactions. + // provider_reference, set at checkout creation) — but payment_intent.payment_failed and + // charge.refunded reference the PaymentIntent/Charge instead, which was never stored anywhere + // as a lookup key. Both are correlated back via OUR OWN transaction_id, echoed back through + // payment_intent_data.metadata at session creation (see createCheckoutSession above); + // commerce.js's transaction lookup matches on either provider_reference OR transaction_id. + async normalizeEvent(event) { const obj = event.data.object; let providerReference = obj.id; let normalizedStatus = null; if (event.type === 'checkout.session.completed') { normalizedStatus = 'paid'; providerReference = obj.id; } else if (event.type === 'checkout.session.expired') { normalizedStatus = 'cancelled'; providerReference = obj.id; } - else if (event.type === 'payment_intent.payment_failed') { normalizedStatus = 'failed'; providerReference = obj.metadata?.checkout_session_id || obj.id; } - else if (event.type === 'charge.refunded') { normalizedStatus = obj.amount_refunded < obj.amount ? 'partially_refunded' : 'refunded'; providerReference = obj.payment_intent || obj.id; } + else if (event.type === 'payment_intent.payment_failed') { + // obj IS the PaymentIntent — its own metadata already carries our transaction_id. + normalizedStatus = 'failed'; providerReference = obj.metadata?.transaction_id || obj.id; + } else if (event.type === 'charge.refunded') { + normalizedStatus = obj.amount_refunded < obj.amount ? 'partially_refunded' : 'refunded'; + // obj is a Charge — Charges do not inherit their PaymentIntent's metadata, so retrieve + // the PaymentIntent (one extra call, only on the rare refund path) to read ours back. + let piMetadataRef = null; + if (obj.payment_intent) { + try { const pi = await this.client.paymentIntents.retrieve(obj.payment_intent); piMetadataRef = pi.metadata?.transaction_id || null; } + catch { /* fall through to the raw PaymentIntent id below */ } + } + providerReference = piMetadataRef || obj.payment_intent || obj.id; + } return { eventId: event.id, eventType: event.type, providerReference, normalizedStatus }; } } diff --git a/platform/api/src/webhook-routes.js b/platform/api/src/webhook-routes.js index c16bdac..db3272d 100644 --- a/platform/api/src/webhook-routes.js +++ b/platform/api/src/webhook-routes.js @@ -20,7 +20,7 @@ module.exports = function registerWebhookRoutes(app, deps) { const signature = req.get('Stripe-Signature') || req.get('X-Test-Payment-Signature'); let normalizedEvent; try { - normalizedEvent = provider.verifyAndParseWebhook(req.rawBody, signature); + normalizedEvent = await provider.verifyAndParseWebhook(req.rawBody, signature); } catch (e) { // A forged/invalid signature is a clean, quiet 400 — never a 500, never a hint about // WHY verification failed (timing/oracle hygiene, matching this codebase's existing diff --git a/platform/docker-compose.dev.yml b/platform/docker-compose.dev.yml index 1a31e6b..3a00b11 100644 --- a/platform/docker-compose.dev.yml +++ b/platform/docker-compose.dev.yml @@ -95,6 +95,14 @@ services: # e2e-phase5c.js sets this to "1" via .env for one test to prove production gating for # real, then unsets it again — see that test's own comment. COOKIE_SECURE: ${COOKIE_SECURE:-} + # Phase 9.1 — real-Stripe-TEST-mode local validation. Empty/unset by default (fail-safe: + # payment-provider.js defaults to the deterministic TestProvider when PAYMENT_PROVIDER is + # unset — the same "dev by default, real provider opt-in" pattern EMAIL_PROVIDER already + # uses above). Set all three in platform/.env only for a deliberate real-Stripe-TEST + # session; STRIPE_SECRET_KEY must be a TEST key (sk_test_...) — never a live one. + PAYMENT_PROVIDER: ${PAYMENT_PROVIDER:-} + STRIPE_SECRET_KEY: ${STRIPE_SECRET_KEY:-} + STRIPE_WEBHOOK_SECRET: ${STRIPE_WEBHOOK_SECRET:-} # TRUST_PROXY: "1" # set once fronted by a trusted reverse proxy depends_on: postgres: