Log extra context for Webhook validation failure.#302
Merged
tmcgilchrist merged 1 commit intoocurrent:masterfrom Nov 29, 2021
Merged
Log extra context for Webhook validation failure.#302tmcgilchrist merged 1 commit intoocurrent:masterfrom
tmcgilchrist merged 1 commit intoocurrent:masterfrom
Conversation
talex5
reviewed
Nov 12, 2021
Contributor
talex5
left a comment
There was a problem hiding this comment.
Looks useful - good idea putting a hint in the error message!
Note that we already have a Prometheus counter for "number of warnings logged". We should probably instead focus on making that one more useful, by removing bogus warnings. The main problem is we report pending installation requests as warnings. They should probably be reported separately as a guage.
plugins/github/current_github.ml
Outdated
| | false -> | ||
| Log.warn (fun f -> f "Invalid X-Hub-Signature-256 received!"); | ||
| | (false, request_signature, calculated_signature)-> | ||
| Log.err (fun f -> f {| |
Contributor
There was a problem hiding this comment.
It's not really an error, because it could be from some spammer. Rejecting invalid signatures is correct behaviour, but it does seem like something worth warning about.
plugins/github/current_github.ml
Outdated
| let signature = "sha256=" ^ Hex.show @@ Hex.of_cstruct @@ | ||
| Mirage_crypto.Hash.SHA256.(hmac ~key:(Cstruct.of_string webhook_secret) (Cstruct.of_string body)) in | ||
| Eqaf.equal signature request_signature | ||
| (Eqaf.equal signature request_signature, signature, request_signature) |
Contributor
There was a problem hiding this comment.
Might be simpler to generate the error here and return a result type (Ok () or Error msg).
6263525 to
8a7fdeb
Compare
talex5
approved these changes
Nov 29, 2021
plugins/github/current_github.ml
Outdated
| Mirage_crypto.Hash.SHA256.(hmac ~key:(Cstruct.of_string webhook_secret) (Cstruct.of_string body)) in | ||
| Eqaf.equal signature request_signature | ||
| if Eqaf.equal signature request_signature then | ||
| `Ok () |
Contributor
There was a problem hiding this comment.
Suggested change
| `Ok () | |
| Ok () |
Why not use the built-in type?
plugins/github/current_github.ml
Outdated
| Log.info (fun f -> f "Got GitHub event %a" Fmt.(option ~none:(any "NONE") (quote string)) event); | ||
| Prometheus.Counter.inc_one (Metrics.webhook_events_total (Option.value event ~default:"NONE")); | ||
| let event_str = Option.value ~default:"NONE" event in | ||
| Log.info (fun f -> f "Got GitHub event %a" Fmt.(quote string) event_str); |
Contributor
There was a problem hiding this comment.
Suggested change
| Log.info (fun f -> f "Got GitHub event %a" Fmt.(quote string) event_str); | |
| Log.info (fun f -> f "Got GitHub event %S" event_str); |
I think this does the same thing.
Generates a prometheus metrics for failures that can be hooked into a monitoring solution.
8a7fdeb to
269f0c7
Compare
tmcgilchrist
added a commit
to tmcgilchrist/opam-repository
that referenced
this pull request
Apr 7, 2022
…, current_github, current_git, current_examples, current_docker and current (0.6) CHANGES: Core: - Implement labelling of clusters on the Graphviz diagram (@ewanmellor ocurrent/ocurrent#255) - Abort propagation on constant/equal changes (@art-w ocurrent/ocurrent#318) API: - GitHub: Record build status using CheckRun (@tmcgilchrist ocurrent/ocurrent#279) - GitHub: Add details_url to check_run. (@tmcgilchrist ocurrent/ocurrent#282) - GitHub: Add Current_github.Api.cmdliner_opt to allow writing pipelines which can optionally be run as GitHub apps. (@talex5 ocurrent/ocurrent#281) - GitHub: Provide markdown details for CheckRun. (@tmcgilchrist ocurrent/ocurrent#288) - GitHub: Fix wrong name used for repository (@tmcgilchrist ocurrent/ocurrent#289 ocurrent/ocurrent#290) - GitHub: Support Github rebuild via webooks. (@tmcgilchrist ocurrent/ocurrent#283) - GitHub: monitor GraphQL queries (@art-w ocurrent/ocurrent#298) - GitHub: Limit CheckRunStatus summary and text fields to 65535. (@tmcgilchrist ocurrent/ocurrent#300) - GitHub: Log extra context for Webhook validation failure. (@tmcgilchrist ocurrent/ocurrent#302) - GitLab: Initial GitLab plugin work. (@tmcgilchrist ocurrent/ocurrent#299) - Git: Make git reset less verbose (@kit-ty-kate ocurrent/ocurrent#293) Web UI: - Use Lwt.pause instead of Lwt_unix.yield (@MisterDA ocurrent/ocurrent#297) - Use `ansi` instead of `current_ansi` (@samoht ocurrent/ocurrent#321) - Show line numbers and allow jumping to specific lines in job logs (@punchagan ocurrent/ocurrent#309) Docker: - Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocurrent#304) - Stop using `Dockerfile.t` completely and use strings instead. (@MisterDA ocurrent/ocurrent#301 ocurrent/ocurrent#316) Other: - Update to cohttp 4.0.0 (ocurrent/ocurrent#274, @talex5) - Move `Current_incr` to its own repository (ocurrent/ocurrent#284, @talex5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Generates a prometheus metrics for failures that can be hooked into a
monitoring solution.