fix(payment-stripe): handle Stripe webhook signature errors - #16228
fix(payment-stripe): handle Stripe webhook signature errors#16228calebcgates wants to merge 1 commit into
Conversation
Wrap the constructEvent call in constructWebhookEvent with a try/catch that rethrows through buildError, matching the error-handling convention used by every other method in this provider. Stripe's constructEvent throws a StripeSignatureVerificationError on an invalid or missing signature; normalizing it here keeps webhook failures consistent with the rest of the module's error output.
🦋 Changeset detectedLatest commit: fcee74e The changes in this PR will be included in the next version bump. This PR includes changesets to release 79 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for the contribution! Initial automated review looks good. Small, well-scoped bug fix that wraps constructWebhookEvent's Stripe SDK call in a try/catch and rethrows via the existing buildError helper, matching the pattern used by every other method in the class. Changeset is present and correctly formatted as a patch. A unit test exercising the new error path is included and correct. No security, performance, or correctness issues found. Triggered by: manual workflow dispatch |
NicolasGorga
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution!
shahednasser
left a comment
There was a problem hiding this comment.
@calebcgates Can you fix the following test error:
Error: src/core/__tests__/stripe-base.spec.ts(86,39): error TS2352: Conversion of type '{ rawData: string; headers: { "stripe-signature": string; }; }' to type '{ data: Record<string, unknown>; rawData: string | Buffer<ArrayBufferLike>; headers: Record<string, unknown>; }' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
Summary
What — What changes are introduced in this PR?
StripeBase.constructWebhookEventcallsthis.stripe_.webhooks.constructEvent(...)and returns its result without atry/catch. This PR wraps that call in atry/catchthat rethrows through the existingbuildErrorhelper, so a failure during webhook construction produces the same normalized error as every other method in the provider. No behavior changes on the happy path.Why — Why are these changes relevant or necessary?
Stripe's
constructEventthrows aStripeSignatureVerificationErrorwhen the payload signature can't be verified (an invalid or spoofedstripe-signatureheader, or a missing/misconfiguredwebhookSecret).constructWebhookEventis the only method instripe-base.tsthat lets a Stripe SDK error escape unwrapped. Every other method (cancelPayment,capturePayment,refundPayment,retrievePayment,updatePayment, the account-holder and payment-method methods, and so on) already wraps its Stripe call intry/catchand rethrows viathis.buildError(...), producing a consistent"An error occurred in <method>: <detail>"message.Because
getWebhookActionAndData(which callsconstructWebhookEvent) runs in the payment webhook subscriber, a verification failure currently surfaces as a raw Stripe error in the webhook processing/retry path, inconsistent with how the rest of the module reports Stripe failures. Wrapping it keeps the error shape uniform and easier to identify in logs.How — How have these changes been implemented?
constructWebhookEvent(data: ProviderWebhookPayload["payload"]): Stripe.Event { const signature = data.headers["stripe-signature"] as string - return this.stripe_.webhooks.constructEvent( - data.rawData as string | Buffer, - signature, - this.options_.webhookSecret - ) + try { + return this.stripe_.webhooks.constructEvent( + data.rawData as string | Buffer, + signature, + this.options_.webhookSecret + ) + } catch (error) { + throw this.buildError("An error occurred in constructWebhookEvent", error) + } }The catch block mirrors the surrounding convention exactly: same catch-variable name, same terse message format, and the same untyped-
errorhandoff tobuildErrorthat the other methods use.Testing — How have these changes been tested, or how can the reviewer test the feature?
Added a unit test to
packages/modules/providers/payment-stripe/src/core/__tests__/stripe-base.spec.tsthat callsconstructWebhookEventwith an invalidstripe-signatureheader and asserts the thrown error is the normalized"An error occurred in constructWebhookEvent"message rather than the raw Stripe error.constructEventperforms signature verification locally (no network), so the test is deterministic.Examples
Checklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
Scoped intentionally to the single unhandled call site in
constructWebhookEvent; no other behavior is changed. Reference: Stripe'sconstructEventthrowsStripeSignatureVerificationErroron signature-verification failure (https://docs.stripe.com/webhooks/signature).On the issues-first guideline: I'm raising this directly as a PR because the change is single-method in scope and mirrors an existing convention already used by every other method in the file, so a written issue would be longer than the diff. Happy to open a tracking issue instead if you'd prefer.