fix: add env parameter to Stripe metadata, check value before processing events #6234
Conversation
| string, | ||
| | StripeMetadataInvalidError | ||
| | StripeMetadataValidPaymentIdNotFoundError | ||
| | StripeMetadataIncorrectEnvError |
There was a problem hiding this comment.
hm would it make more sense to check for the env in a separate function instead of getMetadataPaymentId? then we can also do the check earlier and avoid processing the webhook (in stripe.controller.ts) if it's from the wrong env
There was a problem hiding this comment.
hmmm, we could but it would still be quite messy. The target of the call to getMetadataPaymentId is different based on different cases (see the switch in handleStripeEventUpdates), since the metadata to be checked exists in different places in each type of event. If in the future we want to extract other data from the metadata, we can expand the scope of this function to extract all relevant values. It's just that currently, we only need the payment id.
| @@ -43,6 +81,7 @@ export const getChargeIdFromNestedCharge = ( | |||
| const isStripeMetadata = ( | |||
| obj: Stripe.Metadata, | |||
| ): obj is StripePaymentMetadataDto => | |||
| // hasProp(obj, 'env') && // TODO: Make this required later | |||
There was a problem hiding this comment.
nit: can we create an issue tagged to the TODO so we don't forget about this later on?
| meta: { ...logMeta, metadata }, | ||
| }) | ||
| return err(new StripeMetadataValidPaymentIdNotFoundError()) | ||
| } | ||
| // Explicit check for metadata.env to ensure that legacy metadata which does | ||
| // not have the env value still gets processed. | ||
| // TODO: remove the existence check later. |
There was a problem hiding this comment.
nit: same for the TODO here! :)
There was a problem hiding this comment.
lgtm! there's a typo in your PR description btw, i think you meant payments.controller instead of payments.service.
Adding more context in this comment for posterity (feel free to add on + add it to the PR description also):
- Webhooks are sent to all connected platform accounts' endpoints when a Stripe account is connected to multiple platform accounts
- Stripe disables endpoints if an endpoint hasn’t responded with a 2xx HTTP status code for multiple days in a row (Stripe docs)
- test Events are sent to both live and test connect webhook endpoints (Stripe docs). Instead of using the
livemodeproperty, we're choosing to useenvinstead to better distinguish between our different environments.
|
You're right, updated the PR description! Also for the TODOs, will open a PR for it almost immediately. Just need to make sure we only merge it in a week after this PR goes out |
…ing events (#6234) * fix: add env parameter to Stripe metadata, check value before processing events * fix: better error codes for webhook handler returns * fix: remove unneeded Joi validation on previous payment endpoint * chore: remove unused variable * chore: clarify comment * test: add SSM_ENV_SITE_NAME to test env * test: update tests
Problem
Our Stripe webhooks are being sent everywhere! This risks our webhook handlers being disabled by Stripe for consistently rejecting webhooks that aren't meant for each environment.
Solution
An
envparameter is added to the Stripe metadata. This enables us to check which environment the webhook is meant for when it is received. If the webhook is not meant for the environment, we return a202 Accepted.Additional random stuff this PR includes:
payments.controller.mapRouteErrto be used in webhook handler as well.Sorry for the slight mess! 🙏
Breaking Changes
envvalue is not enforced, to enable backward-compatibility on webhooks that did not have the env parameter previously. Once we stop receiving such webhooks, we should enable these checks instripe.utils.ts.Deploy Notes
SSM_ENV_SITE_NAME: set this toprodfor production prior to deployment