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

webhook: export ComputeSignature #602

Closed
wants to merge 1 commit into from
Closed

Conversation

enzadesai
Copy link

Fixes #497.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the contribution!

I left a few comments below, but otherwise it looks good to me.

@@ -15,7 +15,7 @@ import (
)

const (
// Signatures older than this will be rejected by ConstructEvent
// DefaultTolerance signatures older than this will be rejected by ConstructEvent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix here, but could you make this a sensible sentence? Like DefaultTolerance is the amount of drift in time that's allowed by ConstructEven as it's validating a signature.

// https://stripe.com/docs/webhooks#signatures
func computeSignature(t time.Time, payload []byte, secret string) []byte {
mac := hmac.New(sha256.New, []byte(secret))
func ComputeSignature(t time.Time, payload, secret []byte) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for trying to follow the advice I was giving in #497!

Contrary to what I said there though, I now see that unfortunately changing the signature to []byte ends up breaking compatibility on a lot of these already-exported functions. It's also still not perfectly consistent because header in some of the functions below is still a string.

Sorry for the reversal, but for those reasons, I think it actually might be better to leave secret here as a string, at least for the time being. Would you mind changing it back?

@akamensky
Copy link

Would this still be merged? I am trying to use hooks and surprise surprise -- I need to manually write code for signature verification (and what if the algo later will change?).

brandur-stripe pushed a commit that referenced this pull request Aug 6, 2018
Replaces #602 to export `webhook.ComputeSignature` for testing purposes
and resolve #497.

Fixes #497.
@brandur-stripe
Copy link
Contributor

This pull request seems to have stalled out, so I'm going to close it in favor of #650.

nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
* updated circleci config

* removed SFDX tests for prod env and add readme entry on how to fix that mess up

* adding non-nil refresh token

* added refresh token gen for prod

* reverted config

* added more documentation

* addressed PR feedback

* fixed typo

* Update README.md
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

Successfully merging this pull request may close these issues.

3 participants