search

integrallis / stripe_event

Stripe webhook integration for Rails applications.
https://rubygems.org/gems/stripe_event
MIT License
840 stars 107 forks source link

Only support signed webhooks #95

Closed kylefox closed 6 years ago

kylefox commented 6 years ago

Stripe supports signed webhooks as a way to verify that the events have been sent by Stripe and not spoofed by a third-party. Ideally, StripeEvent would support only signed webhooks. Doing so would encourage best practices, plus would simplify the event-retrieving code because the events can be safely constructed from the POST data.

This eliminates the need for an additional API call to Stripe to fetch the same event data. Not only is that extra API call wasteful, but it becomes complex when handling both normal Stripe and Stripe Connect events.

This should work for all event types (including Connect events), and will verify the signature:

Stripe::Webhook.construct_event(
  request.body.read,
  request.headers['Stripe-Signature'],
  StripeEvent.signing_secret
)

StripeEvent could safely construct the event, which makes the event_retriever unnecessary. Or, it could be renamed to event_preprocessor and passed the constructed Stripe::Event. I think this would greatly simplify things, and would require developers to secure their endpoints — which is never a bad thing to encourage.

(Original discussion: https://github.com/integrallis/stripe_event/issues/85#issuecomment-348339304)

rmm5t commented 6 years ago

I'm in favor of this proposal. Some things to consider.

  1. Once implemented, this should probably bump the major version number (i.e. bump to v2.0.0). This will help convey backwards incompatibility issues to those who still rely on the old method of securing webhooks.
  2. We'll need to update/consolidate the README to solely reference the signing_secret mechanism. I already updated the README to document the deprecation of our original basic auth approach (see 5ea8a32). It is likely worth releasing an interim version (e.g. v1.9.1) with authentication_secret just deprecated.
  3. Deprecating and/or removing and/or renaming event_retriever might be a little more nuanced, because it's sometimes used as a mechanism for hooking into the processing before the associated subscriber is called. I'm cool with renaming it to event_preprocessor and passing it the constructed Stripe::Event.
rmm5t commented 6 years ago

See PR #96 for the initial deprecation of StripeEvent.authentication_secret=. I plan to merge and release v1.9.1 later today or tomorrow with that deprecation if no one has objections with the PR.

rmm5t commented 6 years ago

I now also have a work-in-progress branch for only supporting signed webhooks. https://github.com/rmm5t/stripe_event/tree/95-require-signing_secret

This branch removes authentication_secret support and replaces event_retriever with an optional event_filter. It will also error if no signing_secret is configured.

The branch is green, and should be in working order. I still need to update the README and sleep on it before submitting a pull request, but it's ready for some testing (if anyone is brave enough). I should have a formal pull-request ready tomorrow.

rmm5t commented 6 years ago

For those following this proposal, please review PR #97. The goal is to merge it in by end of week and then get a v2.0.0 release out soon after.

rmm5t commented 6 years ago

My plan is to merge #97 in later today and close this issue out, but I think we need to address Issue #98 before releasing a v2.0.0 release.

@invisiblefunnel Any objections to this plan of attack?

invisiblefunnel commented 6 years ago

@rmm5t sgtm. Exciting to be approaching 2.0 🎉