Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only support signed webhooks #95

Closed
kylefox opened this issue Dec 4, 2017 · 6 comments
Closed

Only support signed webhooks #95

kylefox opened this issue Dec 4, 2017 · 6 comments

Comments

@kylefox
Copy link

kylefox commented Dec 4, 2017

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: #85 (comment))

rmm5t added a commit that referenced this issue Dec 4, 2017
Instead, favor Stripe's new wehook signatures

Ref #95 [skip ci]
@rmm5t
Copy link
Member

rmm5t commented Dec 4, 2017

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
Copy link
Member

rmm5t commented Dec 4, 2017

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
Copy link
Member

rmm5t commented Dec 5, 2017

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
Copy link
Member

rmm5t commented Dec 6, 2017

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
Copy link
Member

rmm5t commented Dec 11, 2017

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
Copy link
Contributor

invisiblefunnel commented Dec 11, 2017

@rmm5t sgtm. Exciting to be approaching 2.0 🎉

@rmm5t rmm5t closed this as completed in #97 Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants