fix(security): bind Stripe webhook secrets to their event family (confused-deputy) - #68
Open
keithfawcett wants to merge 2 commits into
Open
fix(security): bind Stripe webhook secrets to their event family (confused-deputy)#68keithfawcett wants to merge 2 commits into
keithfawcett wants to merge 2 commits into
Conversation
…fused-deputy)
/webhooks/stripe verified against any secret in one combined
STRIPE_WEBHOOK_SECRET and trusted openpartner_tenant_id metadata to establish
the tenant — so a holder of ANY configured secret could forge an event from
the OTHER destination (e.g. a Connect secret carrying a fabricated
checkout.session.completed / invoice.paid with attacker-set tenant metadata)
and mint subscriptions, commissions, or funding state for any tenant. Not
outsider-exploitable today (no merchant holds a verifying secret), but a
latent cross-tenant confused-deputy.
Split the signing secrets by Stripe "Event destination":
- STRIPE_WEBHOOK_SECRET_PLATFORM → platform-account events (checkout.*,
customer.*, invoice.*, charge.*, payment_intent.*)
- STRIPE_WEBHOOK_SECRET_CONNECT → connected-account events (account.updated,
transfer.*)
Verification records which family the matching secret belongs to; an event
whose type doesn't match its verifying family is rejected
(secret_family_mismatch) before any funding/tenant processing. Connect events
now resolve the tenant from the connected-account id (authoritative), not
attacker-influenceable object metadata.
Back-compat: the legacy combined STRIPE_WEBHOOK_SECRET still works (verifies
both families, enforcement OFF) with a startup deprecation warning, so
existing prod deploys are unchanged until they adopt the split vars. Docs +
.env.example updated with the migration.
Note: securely supporting merchant-pointed ("Rewardful") webhooks would need
a THIRD, per-merchant secret family scoped to attribution-only events — out
of scope here and still unsupported.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review of the branch found: 1. (CRITICAL) account.updated selected the partner to update from account.metadata.openpartner_partner_id — a Standard account can influence its own metadata, so partner A setting it to B's id would re-point B's stripeConnectAccountId and hijack B's payouts. The partner↔account link is always established server-side at /connect/start (accounts.create → persist the id), so resolve the target (and the tenant) by the connected-account id ONLY and drop the metadata path entirely. 2. (HIGH) transfer.updated/transfer.reversed were classified as connected-account (Connect) events, but we create Connect transfers with the PLATFORM key, so Stripe fires those on the platform account — they arrive on Destination A. As connect-family they'd be rejected secret_family_mismatch (breaking funding reversal handling) once the split secrets are adopted. Moved them to PLATFORM_EVENT_TYPES; only account.updated remains connect. Deploy doc updated (transfer.* under Destination A). Tests: forged-metadata account.updated leaves the victim's account untouched; transfer.reversed passes on the platform secret and is rejected on the connect secret. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (audit #11 — CRITICAL, latent)
/webhooks/stripeverified the signature against any secret in one combinedSTRIPE_WEBHOOK_SECRET, then trustedopenpartner_tenant_idobject metadata to establish the tenant. So a holder of any configured signing secret could craft a validly-signed event from the other destination — e.g. the Connect secret carrying a fabricatedcheckout.session.completed/invoice.paidwith attacker-set tenant metadata — and mint subscriptions, commissions, or funding state for an arbitrary tenant.account.updatedsimilarly trusted object metadata to pick the partner.Not outsider-exploitable today (no merchant holds a verifying secret, and the "point your Stripe webhook at us" flow is currently non-functional), but a real cross-tenant confused-deputy the moment any additional secret is introduced.
Fix
Bind each signing secret to its Stripe Event destination family:
STRIPE_WEBHOOK_SECRET_PLATFORM→ platform-account events (checkout.*,customer.*,invoice.*,charge.*,payment_intent.*)STRIPE_WEBHOOK_SECRET_CONNECT→ connected-account events (account.updated,transfer.*)Verification records which family matched; an event whose type belongs to the other family is rejected (
secret_family_mismatch) before any funding/tenant processing. Connect events now resolve the tenant from the connected-account id (authoritative), not object metadata — metadata may confirm a mapping, never establish one.Back-compat (safe to merge / deploy as-is)
The legacy combined
STRIPE_WEBHOOK_SECRETstill works — it verifies both families with either secret (enforcement OFF) and logs a startup deprecation warning. Existing prod is unchanged until it adopts the split vars..env.example+docs/deploy-production.mddocument the migration; once the split vars are set, cross-family events are rejected.Out of scope
Securely supporting merchant-pointed (Rewardful-style) webhooks would need a third, per-merchant secret family scoped to attribution-only events and pinned to that merchant's tenant. Not built here; that flow remains unsupported.
Tests
stripe-webhook-family.test.ts: platform-typed event on the Connect secret → rejected; connect-typed event on the platform secret → rejected; each on its correct secret → passes the family gate; unknown secret → 400. The existingstripe-webhook.test.ts(legacy single-secret path) passes unchanged, proving back-compat. Full API suite green (252); typecheck clean.🤖 Generated with Claude Code