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

Expose Stripe::Webhook.compute_signature #915

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Apr 24, 2020

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:

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:

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.

r? @ob-stripe
cc @stripe/api-libraries

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

Thanks as usual OB.

@brandur-stripe brandur-stripe merged commit e117c9f into master Apr 24, 2020
@sandstrom
Copy link

@brandur-stripe Awesome, much appreciated!

@jarthod
Copy link

jarthod commented Apr 25, 2020

Nice, that simplifies a bit the testing code:
image

Out of curiosity though is there a way to avoid hardcoding the whole header construct?, something like:

timestamp = Time.now
signature = Stripe::Webhook::Signature.compute_signature(timestamp, event_json, secret)
scheme = Stripe::Webhook::Signature::EXPECTED_SCHEME
"t=#{timestamp.to_i},#{scheme}=#{signature}"

⬇️

Stripe::Webhook::Signature.compute_signature_header(Time.now, event_json, secret)

That could be nice ;)

@brandur-stripe
Copy link
Contributor

Ah interesting — yeah, something like that makes sense.

@ob-stripe Can I get your opinion on this one? The method proposed above would look like this:

      # Computes the value that would be added to a `Stripe-Signature` for a
      # given webhook payload.
      #
      # Note that this isn't needed to verify webhooks in any way, and is
      # mainly here to make writing test cases easier.
      def self.compute_signature_header(timestamp, payload, secret)
        raise ArgumentError, "timestamp should be an instance of Time" \
          unless timestamp.is_a?(Time)
        raise ArgumentError, "payload should be a string" \
          unless payload.is_a?(String)
        raise ArgumentError, "secret should be a string" \
          unless secret.is_a?(String)

        signature = compute_signature(timestamp, payload, secret)
        "t=#{timestamp.to_i},#{EXPECTED_SCHEME}=#{signature}"
      end

But I was thinking that we might want something a little more granular, where you have to generate a signature separately and inject it:

      # Generates a value that would be added to a `Stripe-Signature` for a
      # given webhook payload.
      #
      # Note that this isn't needed to verify webhooks in any way, and is
      # mainly here to make writing test cases easier.
      def self.generate_header(timestamp, signature, scheme: Stripe::Webhook::Signature::EXPECTED_SCHEME)
        raise ArgumentError, "timestamp should be an instance of Time" \
          unless timestamp.is_a?(Time)
        raise ArgumentError, "signature should be a string" \
          unless signature.is_a?(String)
        raise ArgumentError, "scheme should be a string" \
          unless scheme.is_a?(String)

        "t=#{timestamp.to_i},#{scheme}=#{signature}"
      end

Calling this is a little more work, but more flexible (like in case you already have a signature):

t = Time.now
payload = ...
signature = Stripe::Webhook::Signature.compute_signature(t, payload, SECRET)
header = Stripe::Webhook::Signature.generate_header(t, signature)

Thoughts?

@brandur-stripe
Copy link
Contributor

Oh, and I forgot to mention: the other downside is that this would be a method in the API that's really only relevant for tests. Not sure how you feel about that in general.

@ob-stripe ob-stripe deleted the brandur-expose-compute-signature branch April 27, 2020 19:13
@ob-stripe
Copy link
Contributor

I can definitely see how it would be useful, even if it's just meant to be used in tests. We kind of have something like that in our own test suite:

def generate_header(opts = {})
opts[:timestamp] ||= Time.now
opts[:payload] ||= EVENT_PAYLOAD
opts[:secret] ||= SECRET
opts[:scheme] ||= Stripe::Webhook::Signature::EXPECTED_SCHEME
opts[:signature] ||= Stripe::Webhook::Signature.compute_signature(
opts[:timestamp],
opts[:payload],
opts[:secret]
)
"t=#{opts[:timestamp].to_i},#{opts[:scheme]}=#{opts[:signature]}"
end

If we add this, I'd prefer the more granular version as well.

brandur-stripe pushed a commit that referenced this pull request Apr 27, 2020
Adds a new `generate_header` method for the webhooks module, following
up #915. This method doesn't help in any way with webhook verification,
but may be useful to users in their test suites as it allows them to
easily simulate the contents of a header that Stripe might have sent.

We briefly discussed an alternative design here, but this one seems like
the best fit:
#915 (comment)
@brandur-stripe
Copy link
Contributor

Thanks OB. Opened #916.

brandur-stripe pushed a commit that referenced this pull request Apr 27, 2020
Adds a new `generate_header` method for the webhooks module, following
up #915. This method doesn't help in any way with webhook verification,
but may be useful to users in their test suites as it allows them to
easily simulate the contents of a header that Stripe might have sent.

We briefly discussed an alternative design here, but this one seems like
the best fit:
#915 (comment)
gaffneyc added a commit to gaffneyc/stripe_event that referenced this pull request May 12, 2020
As of 5.19.0, the compute_signature method was made public and they
split the timestamp into a separate param (which must be a Time). This
version checks compute_signature based on the number of params it takes
since doing a version check felt odd. Now that it is public, the method
shouldn't change it's API.

1: stripe/stripe-ruby#915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate Signature
6 participants