-
Notifications
You must be signed in to change notification settings - Fork 241
Add option to skip signature verification #595
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
base: master
Are you sure you want to change the base?
Add option to skip signature verification #595
Conversation
examples/echo_bot/server.go
Outdated
cb, err := webhook.ParseRequestWithOption(channelSecret, req, &webhook.ParseOption{ | ||
SkipSignatureValidation: func() bool { return true }, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, webhook signature verification "should" be done. Providing examples that skip it might lead to uninformed users using this, so please do not offer it as an example.
func ParseRequest(channelSecret string, r *http.Request) (*CallbackRequest, error) { | ||
return ParseRequestWithOption(channelSecret, r, nil) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do these like the Java SDK?
- Write comments explaining why users need this option in
ParseRequestWithOption
/when it should be used. - Indicate to
ParseRequest
users that there is a version where the option can be utilized. - Write sufficient tests.
4fe9ebf
to
fa77c70
Compare
type ParseOption struct { | ||
// SkipSignatureValidation is a function that determines whether to skip | ||
// webhook signature verification. | ||
// | ||
// If the function returns true, the signature verification step is skipped. | ||
// This can be useful in scenarios such as when you're in the process of updating | ||
// the channel secret and need to temporarily bypass verification to avoid disruptions. | ||
SkipSignatureValidation func() bool | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding another option later, would it always require a major version upgrade? I think it's better to ensure the code works seamlessly with users' existing builds, even when adding another option.
Changes
Motivation
The signature returned with webhooks is calculated using a single channel secret. If the bot owner changes their channel secret, the signature for webhooks starts being calculated using the new channel secret. To avoid signature verification failures, the bot owner must update the channel secret on their server, which is used for signature verification. However, if there is a timing mismatch in the update—and such a mismatch is almost unavoidable—verification will fail during that period.
In such cases, having an option to skip signature verification for webhooks would be a convenient way to avoid these issues.
Related PRs