webhooks/stripe: Verify webhook signatures in incoming payloads. #19854
Conversation
Added signature verification in for incoming stripe webhooks Solves issue zulip#19774
There was a problem hiding this comment.
@madrix01 Thanks for working on this! I left a few comments, here are some additional points of feedback:
- The commits will need to be squashed.
- The backend webhook tests are failing because the test code doesn't pass the request the
HTTP_STRIPE_SIGNATUREheader. There are other webhook tests such as GitLab/GitHub that pass headers in tests, so you can probably take a look there to get an idea of how that works.
Thanks again! :)
| @@ -53,6 +55,14 @@ def api_stripe_webhook( | |||
| payload: Dict[str, Any] = REQ(argument_type="body"), | |||
| stream: str = REQ(default="test"), | |||
| ) -> HttpResponse: | |||
| sig_header = request.META["HTTP_STRIPE_SIGNATURE"] | |||
There was a problem hiding this comment.
Before we do any of this, we should check whether the stripe_webhook_endpoint_secret is even set, if the user didn't set it, then the construct_event call would fail.
There was a problem hiding this comment.
@eeshangarg if there is no stripe_webhook_endpoint_secret should I return a HTTP 400 or just continue sending webhook without signature verification ?
| except ValueError: | ||
| return HttpResponse(status=400) | ||
| except stripe.error.SignatureVerificationError: | ||
| return HttpResponse(status=400) |
There was a problem hiding this comment.
We should add automated tests for this. And the docs will need to be updated as well.
|
Hmm, the I think what we need to do is make this an optional configuration field for this integration, using the BotConfig table. Which effectively means finishing this project is blocked on some more complicated refactoring projects. I suppose we could offer an optional signature verification feature by having the user include the secret in the Stripe URL, but that isn't a happy security model even if it does something. Adding a link to #10907 as the other Stripe issue; @PIG208 I'm trying to recall what remains for completing that issue? What I'd try next is doing the same process for some other integrations (see https://github.com/zulip/zulip/issues?q=is%3Aissue+is%3Aopen+integrations). |
|
@timabbott Oh that's right, I definitely think we should not include the signature in the URL. Hmm.. |
|
@eeshangarg tests needs to be changed in |
|
@madrix01 As @timabbott pointed out in his comment above, this PR is blocked on some other work by @PIG208, so this one will, unfortunately, have to wait on some of that work to be done. In the meantime, there are other similar issues in integrations for verifying signatures that you could perhaps work on. Feel free to ping me for a review! Thanks for contributing! :) |
Solves issue #19774
Testing plan:
I have used stripe cli tool for testing by sending test payloads.
I implemented the code before it assigns the
topicandbodyfrom the payload. It would first check theHTTP_STRIPE_SIGNATUREfrom the request and check it with thestripe.Webhook.construct_event. If the signatures are not verified it would return status code400else it would return200status code