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

verify payload signature if present #2732

Merged
merged 1 commit into from Apr 1, 2023
Merged

Conversation

willnorris
Copy link
Collaborator

Verify the payload signature if the request has a signature present in HTTP headers, or if a non-empty secretToken is passed to the ValidatePayload method, indicating that a signature is expected.

This modifies the behavior added in #1127, but not the spirit of what was requested in #1126, which is to support webhooks that don't have configured secrets.

Specifically, this no longer allows signature checking to be skipped entirely, even for webhooks with a configured secret, simply by passing an empty secretToken to ValidatePayload.

This also cleans up some documentation and unused fields in the accompanying test.

Fixes #2731

/cc @myitcv

@willnorris willnorris requested a review from gmlewis March 31, 2023 23:32
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #2732 (5920108) into master (388d921) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2732   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files         131      131           
  Lines       11505    11505           
=======================================
  Hits        11279    11279           
  Misses        154      154           
  Partials       72       72           
Impacted Files Coverage Δ
github/messages.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Verify the payload signature if the request has a signature present in
HTTP headers, or if a non-empty secretToken is passed to the
ValidatePayload method, indicating that a signature is expected.

This modifies the behavior added in #1127, but not the spirit of what
was requested in #1126, which is to support webhooks that don't have
configured secrets.

Specifically, this no longer allows signature checking to be skipped
entirely, even for webhooks with a configured secret, simply by passing
an empty secretToken to ValidatePayload.

Fixes #2731
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @willnorris !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Mar 31, 2023
@willnorris
Copy link
Collaborator Author

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

I don't think a second approval should be necessarily when the change is coming from another maintainer... they are basically the second :) At least, that's how I recall internal Google systems handling it. (But I also don't recall when that review policy got added to the process and what its purpose was.)

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 1, 2023

Oh, sorry. Just habit. Thank you, @willnorris !
Merging.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Apr 1, 2023
@gmlewis gmlewis merged commit b1c53f8 into master Apr 1, 2023
12 checks passed
@gmlewis gmlewis deleted the will/payload-signature branch April 1, 2023 17:16
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.

ValidatePayload should not accept nil or empty secretToken slice
2 participants