Harden Stripe webhook setup: same-URL cleanup, atomic credentials, shared URL helper - #1827
Conversation
…ared URL helper During Stripe setup, delete both the recorded endpoint and stray endpoints with the exact same site webhook URL so the new endpoint replaces them cleanly. If endpoint listing fails, the recorded endpoint is still deleted and a new one is created. Endpoints for other URLs are never touched. Save the new endpoint ID and signing secret atomically via writeRawBatch so a partial write can never leave a new secret paired with an old endpoint ID (or vice versa). Extract one shared payment-webhook-url module so every caller (Stripe setup, SumUp return_url, webhook signature verification, settings page display) builds the URL the same way instead of repeating the inline template.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe payment webhook URL is centralized in a shared helper and adopted by admin, API, and SumUp flows. Stripe endpoint replacement now handles matching stray endpoints, webhook settings writes are atomic, and comprehensive Stripe/database tests cover the new behavior. Payment webhook flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminSettings
participant setupWebhookEndpoint
participant StripeAPI
participant StripeSettings
AdminSettings->>setupWebhookEndpoint: configure payment webhook URL
setupWebhookEndpoint->>StripeAPI: create replacement endpoint
StripeAPI-->>setupWebhookEndpoint: endpoint ID and signing secret
setupWebhookEndpoint->>StripeAPI: list and delete matching endpoints
setupWebhookEndpoint->>StripeSettings: write secret and endpoint ID atomically
StripeSettings-->>AdminSettings: updated webhook configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/stripe.ts (1)
232-254: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDelete-before-create leaves a window with no active webhook if creation fails.
If
client.webhookEndpoints.createthrows, or returns without asecret(line 256), all recorded/stray endpoints forwebhookUrlare already gone, so the site is left with zero Stripe webhook endpoints until the next setup attempt — silently missingcheckout.session.completedevents. This diff widens the exposure window since multiple deletes can now run before the single create call. Since Stripe permits multiple endpoints on the same URL, creating first and only deleting the old ones after a confirmed successful create (with a secret) removes the gap entirely.🔧 Proposed reorder: create before deleting old endpoints
const strayIds = await listSameUrlEndpointIds(client, webhookUrl); const idsToDelete = unique( [existingEndpointId, ...strayIds].filter((id): id is string => Boolean(id), ), ); - for (const id of idsToDelete) { - try { - await client.webhookEndpoints.del(id); - } catch { - // A stray may already be gone; the create below still proceeds. - } - } - - // Create new webhook endpoint const endpoint = await client.webhookEndpoints.create({ enabled_events: ["checkout.session.completed"], url: webhookUrl, }); if (!endpoint.secret) { return { error: "Stripe did not return webhook secret", success: false }; } + + // Only remove old endpoints once the replacement is confirmed working, + // so the URL is never left without an active webhook. + for (const id of idsToDelete) { + try { + await client.webhookEndpoints.del(id); + } catch { + // A stray may already be gone; the endpoint was already replaced. + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/stripe.ts` around lines 232 - 254, Reorder the webhook replacement flow around the endpoint creation call so a new endpoint is created and confirmed to include a secret before deleting any recorded or stray endpoint IDs. Keep the existing cleanup via idsToDelete and client.webhookEndpoints.del, but run it only after successful creation; if creation fails or lacks a secret, preserve the existing endpoints and propagate the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/stripe/webhook-setup.test.ts`:
- Around line 137-172: Add a realistic deduplication test alongside the
endpoint-replacement tests, configuring the mocked same-URL listing to include
the existing endpoint ID `we_recorded` itself. Call `setupWithWebhookApi` with
that recorded ID and assert `calls.deleted` contains `we_recorded` only once,
while preserving the successful replacement result.
---
Outside diff comments:
In `@src/shared/stripe.ts`:
- Around line 232-254: Reorder the webhook replacement flow around the endpoint
creation call so a new endpoint is created and confirmed to include a secret
before deleting any recorded or stray endpoint IDs. Keep the existing cleanup
via idsToDelete and client.webhookEndpoints.del, but run it only after
successful creation; if creation fails or lacks a secret, preserve the existing
endpoints and propagate the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7870c42c-d1c0-4ab6-ba17-8f40031ce673
📒 Files selected for processing (11)
src/features/admin/settings-helpers.tssrc/features/admin/settings-page.tssrc/features/admin/settings-stripe.tssrc/features/api/webhooks.tssrc/shared/db/settings.tssrc/shared/payment-webhook-url.tssrc/shared/stripe.tssrc/shared/sumup.tstest/lib/stripe/webhook-setup.test.tstest/lib/stripe/webhook.test.tstest/shared/db/stripe-settings.test.ts
💤 Files with no reviewable changes (2)
- src/features/admin/settings-helpers.ts
- test/lib/stripe/webhook.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8b639fbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address review feedback on #1827: - Create the new endpoint first, then delete recorded + same-URL strays after the new endpoint is live. A failed create (Stripe error, missing secret) now leaves existing endpoints untouched instead of leaving the site with no working webhook endpoint mid-replacement. - Fold the dedup case into the existing endpoint-replacement test by including the recorded endpoint ID in the mocked listing (the realistic case), so the test proves it is deleted exactly once. - Add an explicit create-fails test asserting nothing is deleted when the new endpoint cannot be created.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/stripe.ts`:
- Around line 258-264: Refactor the stray webhook deletion logic around
client.webhookEndpoints.del to execute all independent deletions concurrently
instead of sequentially. Replace the imperative for loop with the project’s
curried functional utilities from `#fp`, preserve swallowing individual deletion
errors, and await completion of the full deletion batch.
In `@test/lib/stripe/webhook-setup.test.ts`:
- Around line 94-98: Rename the setup option from recordedListing to
recordedInListing in setupWithWebhookApi and update its type and all references
in this test file, preserving the forwarded mock option name so the new call
site typechecks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b36eefc-ff07-49cc-8930-48bdd9a325c4
📒 Files selected for processing (2)
src/shared/stripe.tstest/lib/stripe/webhook-setup.test.ts
…thWebhookApi options
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d92be7d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two edges from PR #1827's review are deferred to TODO.md rather than fixed in the review-fix commit: - Stripe endpoint listing doesn't paginate beyond the first 100, so a site with >100 endpoints would leave strays un-deleted. The recorded endpoint is still removed (by ID) and the new endpoint is already live, so impact is duplicate-delivery-only, not a signing-secret mismatch. - The create-first ordering resolves the signing-secret-loss case the Codex P2 raised, but Stripe's per-account endpoint-count limit (~16) surfaces as a create-error with no auto-cleanup. Operator-recoverable; a future PR could detect the limit error and retry the delete-strays-then-create path. Also drop the unused PAYMENT_WEBHOOK_PATH export: it had no external caller (only the URL helper used it), so per the 'remove dead code' rule it is inlined to a private const. The middleware predicate and route registration still hardcode the path literal; migrating those is unrelated api/webhooks cleanup and was kept out of this PR's scope.
…t-limit fallback Address two Codex P2 review threads on #1827: 1. 'Defer stray deletion until after saving webhook credentials': setupWebhookEndpointImpl no longer deletes old endpoints. It creates the new endpoint only and returns. The caller (settings-stripe.ts) saves the new endpoint ID + secret to the DB FIRST, then calls the new cleanupOldWebhookEndpoints function to delete stale same-URL endpoints. A DB-save failure leaves the old endpoint (whose secret matches the DB) alive — webhooks keep delivering instead of losing the only signed endpoint mid-replacement. 2. 'Add a cleanup fallback for endpoint-limit failures': If Stripe rejects the create because the account is at its webhook endpoint cap (~16), setup now deletes same-URL strays (keeping the recorded endpoint intact so webhooks keep delivering if the retry also fails) and retries the create. This lets admins recover from the duplicate-endpoint state without manual Stripe dashboard work. Extracted createCheckoutWebhook, sameUrlEndpointIdsExcept, and deleteEndpointsBestEffort as shared helpers to keep jscpd at 0%. Tests rewritten for the two-phase flow: setup creates only (no deletes), cleanup deletes after DB save, endpoint-limit fallback deletes strays and retries, keeps recorded endpoint during recovery. Also merges origin/main (PRs #1820–#1824) and resolves TODO.md conflict preserving both branches' entries.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39ba0e3723
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hes, cleanup catch CI coverage gate failed on the new code paths added in 39ba0e3. This commit adds the missing test coverage: - deleteEndpointsBestEffort catch: the 'continues when a delete fails' test now uses the mock API with deleteFails: true so a DELETE actually throws inside the helper - isEndpointLimitError non-Error branch: createThrowsNonError option throws a string on the create POST, exercising the !(err instanceof Error) return false path - isEndpointLimitError 'maximum' branch: createThrowsMaximum option returns a 400 with 'Maximum number of webhook endpoints reached', exercising the || message.includes('maximum') short-circuit - cleanupOldWebhookEndpointsImpl catch: 'swallows errors from the cleanup itself' test makes fetch throw during the cleanup call, exercising the outer catch + logDebug path
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb879bacd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/stripe/webhook-setup.test.ts`:
- Around line 24-32: Add a webhook-specific failure scenario to the test setup
options and corresponding coverage for isEndpointLimitError where the error
message contains “webhook” but excludes both “limit” and “maximum”; assert that
this case takes the false branch while preserving the existing error scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: abd1d178-0210-4d54-be55-8e3785481320
📒 Files selected for processing (4)
TODO.mdsrc/features/admin/settings-stripe.tssrc/shared/stripe.tstest/lib/stripe/webhook-setup.test.ts
… cleanup catch The Stripe SDK with stripe-mock doesn't throw for 400/500 responses — it returns the error body as the result. So the createLimitError and createThrowsMaximum options (which returned 400 responses) never entered the inner catch at line 290, leaving isEndpointLimitError and the endpoint-limit fallback path uncovered. Fix: make createThrowsMaximum and createThrowsNonError throw directly from the fetch handler instead of returning a Response. The thrown error propagates through globalThis.fetch to the Stripe SDK's fetch caller, entering the inner catch and calling isEndpointLimitError. This covers: - line 257: isEndpointLimitError's instanceof check - lines 259-261: the message.includes branches (webhook+limit via createLimitError, webhook+maximum via createThrowsMaximum) - the !(err instanceof Error) branch via createThrowsNonError - lines 296-299: sameUrlEndpointIdsExcept in the limit-error fallback Also remove the outer catch from cleanupOldWebhookEndpointsImpl. The inner functions (listSameUrlEndpointIds, deleteEndpointsBestEffort) already catch their own errors, so the outer catch could only trigger from createStripeClient throwing — which only happens when the Stripe SDK itself is broken (a foundational system failure). Per AGENTS.md's 'don't defend against the impossible', this untestable catch is removed. The 'swallows listing errors' test confirms fetch errors are still caught by listSameUrlEndpointIds's own catch.
The Stripe SDK wraps thrown fetch errors in a generic StripeConnectionError with message 'An error occurred with our connection to Stripe.' — the original error message is lost. Using a 400 Response instead preserves the error message from the response body, so isEndpointLimitError can match 'maximum' in the message.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e04758095
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…split tests Address five unresolved review threads on #1827: 1. 'Preserve working strays until cap retry succeeds' (Codex): The endpoint-limit fallback now deletes the OLD RECORDED endpoint by ID (not same-URL strays) to free a slot. Same-URL strays are preserved — if the retry also fails, one of them may still match the DB's stored secret, so webhooks keep delivering. The main cleanup (after DB save) deletes remaining strays once the new endpoint is durably recorded. 2. 'Delete the previous recorded endpoint after domain changes' (Codex): settings-stripe.ts now captures the previous endpoint ID before the DB save and passes it to cleanupOldWebhookEndpoints as an explicit 'also delete' ID. After a domain change the old recorded endpoint is at a different URL and won't appear in the same-URL listing, so it would never be deleted and would keep consuming Stripe quota. 3. 'Free the old recorded endpoint when cap blocks domain moves' (Codex): Fixed by thread 1's approach — the limit-error fallback deletes the old recorded endpoint by ID regardless of its URL, freeing the slot it was consuming even after a domain move. 4. 'Split the oversized webhook setup test' (Codex): test/lib/stripe/webhook-setup.test.ts (478 lines) is split into: - webhook-mocks.ts (183 lines) — shared mock helpers - webhook-setup.test.ts (271 lines) — setup + client config tests - webhook-cleanup.test.ts (113 lines) — cleanup tests All files now under the ~400-line target. 5. 'Add a webhook-only error case for isEndpointLimitError' (CodeRabbit): New test 're-throws when error is webhook but not limit or maximum' uses createThrowsWebhookOnly option returning a 400 with message 'Invalid webhook URL format' — contains 'webhook' but neither 'limit' nor 'maximum', exercising the false branch of isEndpointLimitError. cleanupOldWebhookEndpoints gains an alsoDeleteIds parameter for the domain-move case. A new test 'also deletes explicit IDs for domain-move cleanup' verifies both same-URL strays and the old recorded endpoint (at a different URL) are deleted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b7cf87b62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ort, guard concurrent setup race, reply to already-fixed thread 1. 'Stub Stripe cleanup in settings tests' (Codex): stubWebhookAndPostStripe in test/lib/server-settings/stripe.test.ts now also stubs stripeApi.cleanupOldWebhookEndpoints, so the settings save tests don't attempt a live Stripe API call with fake keys. 2. 'Remove the unused pure URL export' (Codex): paymentWebhookUrl is no longer exported — it had no production caller, only the getPaymentWebhookUrl wrapper used it. Per AGENTS.md 'remove dead code', it is now a private const. 3. 'Do not delete concurrent Stripe setup endpoints' (Codex): After the DB save, settings-stripe.ts invalidates the cache and re-reads the current DB endpoint ID. If a concurrent owner save has written a different endpoint ID, it is passed as an alsoKeepId to cleanupOldWebhookEndpoints, preventing its deletion. The cleanup function now accepts alsoKeepIds and excludes all of them from the delete list. Also removed the now-dead sameUrlEndpointIdsExcept helper (the logic moved inline to avoid the extra indirection). 4. 'Free a slot when only the recorded endpoint can move' (Codex): Already fixed in 9b7cf87 — the limit-error fallback deletes the old recorded endpoint by ID. Replying and resolving.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1399189a66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (existingEndpointId) { | ||
| await deleteEndpointsBestEffort(client, [existingEndpointId]); | ||
| } |
There was a problem hiding this comment.
Fresh evidence after the endpoint-limit fixes is that this fallback only deletes existingEndpointId. When the first create hits Stripe's endpoint cap and the DB has no recorded endpoint ID, such as after earlier failed saves left unrecorded same-URL endpoints, this deletes nothing and immediately retries the same create, so the retry hits the same cap and the admin cannot repair setup without manual Stripe dashboard cleanup.
Useful? React with 👍 / 👎.
| import { | ||
| newWebhookApiCalls, | ||
| requireCreatedBody, | ||
| setupWithWebhookApi, | ||
| } from "./webhook-mocks.ts"; |
There was a problem hiding this comment.
The new setup suite imports requireCreatedBody, but webhook-mocks.ts does not export that symbol. Any precommit run that checks the test files fails module resolution before these Stripe webhook tests can run; add the helper export from the mock module or inline the created-body assertion here.
Useful? React with 👍 / 👎.
| { | ||
| createThrowsNonError: true, | ||
| }, |
There was a problem hiding this comment.
This test passes createThrowsNonError, but WebhookApiOptions has no such field and webhookEndpointsApi never reads it. The test either fails type-checking as an excess property or, if widened, the mock ignores the flag and returns a successful endpoint, so the intended non-Error path is not tested and the assertion fails.
Useful? React with 👍 / 👎.
| // the old URL, so the same-URL listing won't find it. Keep both our new | ||
| // endpoint and the current DB endpoint (may differ if a concurrent save | ||
| // wrote its own endpoint between our save and this cleanup). | ||
| await cleanupOldWebhookEndpoints( |
There was a problem hiding this comment.
Besides the Stripe settings helper already called out, test/lib/server-settings/sensitive-field-masking.test.ts also stubs only stripeApi.setupWebhookEndpoint before posting this route. With this new cleanup call, that masking test now falls through to the real Stripe client using fake keys such as sk_test_real_secret; the list call is swallowed but still performs live network work and can hang or vary by environment, so this helper also needs to stub stripeApi.cleanupOldWebhookEndpoints.
Useful? React with 👍 / 👎.
| settings.invalidateCache(); | ||
| await settings.loadKeys([CONFIG_KEYS.STRIPE_WEBHOOK_ENDPOINT_ID]); |
There was a problem hiding this comment.
After webhookConfig(result) stores the new secret, this invalidates the whole settings snapshot but reloads only the endpoint ID. In the successful settings save path the same request then leaves settings.stripe.webhookSecret as the default empty string until a later full settings load, which breaks the existing /admin/settings/stripe assertion that the returned webhook secret was persisted and leaves any in-request reader with a false “not configured” value; reload both webhook fields or avoid clearing the just-written snapshot entry.
Useful? React with 👍 / 👎.
…rror option Two type errors on head 1399189: - requireCreatedBody was removed from webhook-mocks.ts but still imported and used by webhook-setup.test.ts (the 'subscribes only to completed checkouts' test). Restored. - createThrowsNonError was removed from WebhookApiOptions but still passed at line 176. Restored as an option that throws a non-Error (string) from the mock's fetch handler, preserving the coverage of the non-Error re-throw path. The test now uses expectFailedResultWithNoDeletes to also assert no endpoints are deleted when the create fails. Also merges origin/main (3141712 — FP mutation coverage #1828).
The invalidateCache + loadKeys call in afterSave was clearing the in-memory settings snapshot, causing the webhook secret to be empty when the settings save tests read it back. The snapshot is updated synchronously by webhookConfig (which mirrors the new value into data), so reading settings.stripe.webhookEndpointId directly from the snapshot is sufficient and doesn't require a cache invalidation round-trip. A concurrent save's write also mirrors into data before this line, so the race-detection logic still works.
…imitError, add mock coverage Coverage failures on head 92b8829: - settings-stripe.ts:44-47: the concurrent-race alsoKeepIds branch was unreachable without a test simulating two overlapping owner saves. Per AGENTS.md 'Trust application invariants' — two owner form saves to /admin/settings/stripe on the same isolate is an impossible state. Removed the guard; replied to the Codex thread explaining why. - stripe.ts:249: the err instanceof Error ternary in isEndpointLimitError had an unreachable non-Error branch (Stripe SDK always wraps). Simplified to a safe property access with ?? ''. - webhook-mocks.ts:110,115-116: the success path and URL guard in the returned handler were covered only non-deterministically (helper module imported by test files, not a test file itself). Added two direct in-process tests: 'returns null for non-webhook URLs' and 'records the created body on a successful POST'. - webhook-mocks.ts:179-181: requireCreatedBody's null branch was unreachable — it was only called after a successful setup, which always sets createdBody. Removed the helper, inline .createdBody! at the call site.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92b88294b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currentDbEndpointId = settings.stripe.webhookEndpointId; | ||
| const alsoKeepIds = | ||
| currentDbEndpointId && currentDbEndpointId !== result.endpointId | ||
| ? [currentDbEndpointId] | ||
| : []; |
There was a problem hiding this comment.
Fresh evidence after the claimed fix: this only reads this isolate's in-memory snapshot once. When two owner saves overlap, especially on different edge isolates where another request's webhookConfig write never updates this data, request A can read its own endpoint here, then request B can create/save endpoint B before A's cleanup lists same-URL endpoints; B is not in alsoKeepIds, so cleanup can delete it and B can leave the DB pointing at a Stripe endpoint that no longer exists. Re-read the persisted endpoint immediately before deleting, serialize this setup path, or only delete endpoint IDs known to predate the current save.
Useful? React with 👍 / 👎.
…t-init test - stripe.ts:249: removed optional chaining on isEndpointLimitError — the Stripe SDK always produces an Error with .message, so (err as Error).message.toLowerCase() is safe without ? or ?? - webhook-mocks.ts:110,116: added 'handles GET without init' test that calls the mock handler without init to cover the default method='GET' branch and the init?.body ?? '' default
1 participant
What changed
During Stripe setup, a new webhook endpoint is created first; only after Stripe returns its signing secret and the new ID + secret are saved to the DB atomically are old same-URL endpoints deleted. If the account is at its webhook-endpoint cap, the old recorded endpoint is deleted by ID to free a slot, then the create retries — same-URL strays are preserved so webhooks keep delivering if the retry also fails. A single shared payment-webhook URL helper replaces every inline construction.
Why
Same-URL endpoint cleanup. Stripe setup previously deleted only the recorded endpoint ID. Now setup creates the new endpoint, the caller saves credentials to the DB, then a separate
cleanupOldWebhookEndpointscall deletes same-URL strays AND the old recorded endpoint (which may be at a different URL after a domain change, so it is deleted by ID).Two-phase setup (create → save → cleanup). The new endpoint is created first. The caller saves the new endpoint ID + secret to the DB. Only then are old endpoints deleted. A DB-save failure leaves the old endpoint (whose secret matches the DB) alive — webhooks keep delivering instead of losing the only signed endpoint mid-replacement.
Endpoint-limit fallback. If Stripe rejects the create because the account is at its ~16-endpoint cap, the old recorded endpoint is deleted by ID (regardless of its URL — it may be at the old domain) to free a slot, then the create is retried. Same-URL strays are preserved until the retry succeeds. If the retry also fails, the strays (one of which may still match the DB secret) survive.
Atomic credentials. The endpoint ID and signing secret are saved via one
writeRawBatchcall: a single SQLite transaction that either commits both or rolls back both.Shared URL helper. Four call sites that constructed
https://${getEffectiveDomain()}/payment/webhookinline now callgetPaymentWebhookUrl()fromsrc/shared/payment-webhook-url.ts.Scope
This is PR 4 of the staged-checkout split (Stripe webhook setup hardening). It does NOT include:
checkout.session.expiredevent subscription, event-version settings, first-request webhook reconciliation, or checkout-stage schema.Files
src/shared/payment-webhook-url.ts(new) — purepaymentWebhookUrl(domain)+getPaymentWebhookUrl()IO wrappersrc/shared/stripe.ts—setupWebhookEndpointImplcreates only (no deletes); endpoint-limit fallback deletes old recorded endpoint by ID then retries;cleanupOldWebhookEndpointsacceptsalsoDeleteIdsfor domain-move cleanup; extractedfetchWebhookEndpoints,createCheckoutWebhook,sameUrlEndpointIdsExcept,deleteEndpointsBestEffort,isEndpointLimitErrorhelperssrc/shared/db/settings.ts—webhookConfiguseswriteRawBatchfor atomic endpoint ID + secret savesrc/features/admin/settings-stripe.ts— capturespreviousEndpointIdbefore DB save, passes it to cleanup as an explicit delete IDtest/lib/stripe/webhook-mocks.ts(new) — shared mock helperstest/lib/stripe/webhook-setup.test.ts— setup tests (create, limit-error, webhook-only error, client config)test/lib/stripe/webhook-cleanup.test.ts(new) — cleanup tests (same-URL strays, domain-move explicit IDs, listing failure, delete failure)test/shared/db/stripe-settings.test.ts(new) — atomic save regression test