Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
@willnorris willnorris force-pushed the will/payload-signature branch from 6c7300d to 5920108 Compare March 31, 2023 23:40
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
@gmlewis gmlewis deleted the will/payload-signature branch April 1, 2023 17:16
# for free to join this conversation on GitHub. Already have an account? # 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