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.

Sign up for GitHub

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

Jump to bottom

Generate Signature #912

Closed
sandstrom opened this issue Apr 22, 2020 · 2 comments · Fixed by #915
Closed

Generate Signature #912

sandstrom opened this issue Apr 22, 2020 · 2 comments · Fixed by #915

Comments

@sandstrom
Copy link

sandstrom commented Apr 22, 2020 •
edited

We run a bunch of tests emulating possible stripe event payloads being sent to our webhook.

We'd also like to test the signature verification code. There doesn't seem to be an easy way to generate the signature: https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/webhook.rb

Would you be open to making such a method public?

# would like to do something like this in our tests

test 'should respond to ping' do
  post :process_event, PING_PAYLOAD, { :stripe_signature => build_sig(PING_PAYLOAD) }

  assert_response 200
end

I've added a spike (non-complete) PR just to illustrate my thinking: #913
@sandstrom sandstrom mentioned this issue Apr 22, 2020
Closed
@sandstrom
Copy link
Author

This is what @brandur-stripe responded with in the illustrative PR, for reference:

@sandstrom Looks largely good. Do you think you take a look at a couple things? (There are quite a few words here, but all these points are pretty simple.)

  1. Foremost, I think we should change the API slightly to take in a timestamp instead of a full header like compute_signature(time, payload, secret). Not only does this match what we have in Go, but it also allows the function to be called in a more granular way. Instead of the caller needing to have a preconstructed Stripe header, all they need is their payload, a signing secret, and the current time — the method takes care of building the header for them.
  2. Given it's becoming a public API, I think it makes sense to add a basic test in test/stripe/webhook_test.rb to make sure it works. It's already being tested roundaboutedly by other cases in there, but it'd be nice to have a direct one for compute_signature specifically in case we want to exercise just this one method.
  3. The test suite is failing in your PR. This is because although compute_signature is not public, we do cheat a bit by calling it from the test suite with send. We'll need to update the invocation to account for the new signature.

Anyway, thanks for sending us a PR! Looking good.

#913 (comment)

brandur-stripe pushed a commit that referenced this issue Apr 24, 2020
@brandur
Expose Stripe::Webhook.compute_signature
bb7fbaa 
Exposes the `.compute_signature` method, which may be useful when
testing webhook signing in test suites.

I change the API slightly so that a caller isn't forced to do as much
string mangling, and to match the one that we already have in stripe-go:

``` go
func ComputeSignature(t time.Time, payload []byte, secret string) []byte {
```

Add basic documentation and test case. I also change a few things around
so that we send `Time` objects around more often where applicable, and
don't change then to Unix integers until the last moment that we need
to.

The one other alternative API I considered is this one, which would
default the timestamp to the current time to allow the method to be
called with one fewer arg:

``` ruby
def self.compute_signature(payload, secret: timestamp: Time.now)
```

I decided against it in the end though because it does remove some
explicitness, and it's not a big deal to just pass in `Time.now`,
especially given that this is not expected to be a commonly used method.

Fixes #912.
@brandur brandur mentioned this issue Apr 24, 2020
Merged
brandur-stripe pushed a commit that referenced this issue Apr 24, 2020
@brandur
Expose Stripe::Webhook.compute_signature (#915)
e117c9f 
Exposes the `.compute_signature` method, which may be useful when
testing webhook signing in test suites.

I change the API slightly so that a caller isn't forced to do as much
string mangling, and to match the one that we already have in stripe-go:

``` go
func ComputeSignature(t time.Time, payload []byte, secret string) []byte {
```

Add basic documentation and test case. I also change a few things around
so that we send `Time` objects around more often where applicable, and
don't change then to Unix integers until the last moment that we need
to.

The one other alternative API I considered is this one, which would
default the timestamp to the current time to allow the method to be
called with one fewer arg:

``` ruby
def self.compute_signature(payload, secret: timestamp: Time.now)
```

I decided against it in the end though because it does remove some
explicitness, and it's not a big deal to just pass in `Time.now`,
especially given that this is not expected to be a commonly used method.

Fixes #912.
@brandur-stripe
Copy link
Contributor

Released as 5.19.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Expose Stripe::Webhook.compute_signature stripe/stripe-ruby
2 participants
@sandstrom @brandur-stripe