fix(trogon-gateway): verify GitLab signing tokens#157
Merged
Conversation
Member
yordis
commented
May 13, 2026
yordis
commented
- Protect GitLab webhook ingestion with payload authenticity and integrity guarantees instead of relying on a static shared header.
- Reduce replay risk for GitLab webhook deliveries.
PR SummaryHigh Risk Overview Configuration and docs are updated to replace The existing incident.io signature verifier is refactored to use a new shared Reviewed by Cursor Bugbot for commit 1d2a062. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
WalkthroughThis PR replaces GitLab webhook authentication from simple token validation to HMAC-SHA256 signature verification. It introduces ChangesGitLab Webhook Signature Authentication Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 1d2a062 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-gateway/src/source/gitlab/signature.rs (2)
183-190: ⚡ Quick winKeep
WebhookIdandWebhookTimestamptyped through the verifier.
verify()constructs the value objects and then immediately unwraps them back to&strforverify_signature()andsigned_content(). Passing the domain types through those helpers keeps the construction-time guarantee intact and makes it harder for a future internal caller to bypass validation.As per coding guidelines, "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixinstead ofString). Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable."Also applies to: 214-220, 263-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-gateway/src/source/gitlab/signature.rs` around lines 183 - 190, The verifier currently constructs WebhookId and WebhookTimestamp then immediately converts them to &str for verify_signature() and signed_content(), losing the typed guarantees; update verify() (and the other occurrences around the blocks at lines noted) to pass the WebhookId and WebhookTimestamp value objects directly into verify_signature() and signed_content() (and modify those helpers' signatures if needed) so the typed domain objects are used end-to-end—ensure verify_signature(signing_token, webhook_id: &WebhookId, webhook_timestamp: &WebhookTimestamp, body, signature_header) and any callers (including the signed_content helper) accept the typed values rather than raw &str to preserve construction-time validation.
221-224: ⚡ Quick winHash the three segments directly instead of materializing
signed_content.This currently allocates and copies the full body on every request before feeding HMAC. Updating the MAC incrementally removes that extra allocation on the request path.
♻️ Proposed change
- let signed_content = signed_content(webhook_id, webhook_timestamp, body); let mut mac = HmacSha256::new_from_slice(signing_token.as_bytes()).expect("HMAC-SHA256 accepts any key length"); - mac.update(&signed_content); + mac.update(webhook_id.as_bytes()); + mac.update(b"."); + mac.update(webhook_timestamp.as_bytes()); + mac.update(b"."); + mac.update(body); let computed = mac.finalize().into_bytes();Also applies to: 263-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-gateway/src/source/gitlab/signature.rs` around lines 221 - 224, The code currently builds signed_content and then feeds it to the HMAC, causing an extra allocation; change to update the HMAC incrementally instead: after creating the HmacSha256 via HmacSha256::new_from_slice(signing_token.as_bytes()), call mac.update for the webhook_id bytes, then for the newline separator, then for the webhook_timestamp bytes, then for the newline separator, and finally for the raw body bytes, and then call mac.finalize().into_bytes() to produce computed; apply the same incremental-update pattern to the other identical block around the signed_content usage (the later block referenced at lines 263-270) and remove the signed_content helper/usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/source/gitlab/signature.rs`:
- Around line 183-190: The verifier currently constructs WebhookId and
WebhookTimestamp then immediately converts them to &str for verify_signature()
and signed_content(), losing the typed guarantees; update verify() (and the
other occurrences around the blocks at lines noted) to pass the WebhookId and
WebhookTimestamp value objects directly into verify_signature() and
signed_content() (and modify those helpers' signatures if needed) so the typed
domain objects are used end-to-end—ensure verify_signature(signing_token,
webhook_id: &WebhookId, webhook_timestamp: &WebhookTimestamp, body,
signature_header) and any callers (including the signed_content helper) accept
the typed values rather than raw &str to preserve construction-time validation.
- Around line 221-224: The code currently builds signed_content and then feeds
it to the HMAC, causing an extra allocation; change to update the HMAC
incrementally instead: after creating the HmacSha256 via
HmacSha256::new_from_slice(signing_token.as_bytes()), call mac.update for the
webhook_id bytes, then for the newline separator, then for the webhook_timestamp
bytes, then for the newline separator, and finally for the raw body bytes, and
then call mac.finalize().into_bytes() to produce computed; apply the same
incremental-update pattern to the other identical block around the
signed_content usage (the later block referenced at lines 263-270) and remove
the signed_content helper/usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f5d011b-632d-4a10-9189-9511660d8642
📒 Files selected for processing (12)
devops/docker/compose/services/trogon-gateway/README.mdrsworkspace/crates/trogon-gateway/README.mdrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/constants.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/config.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/constants.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/mod.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/server.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/signature.rsrsworkspace/crates/trogon-gateway/src/streams.rsservices/trogon-gateway/gateway.toml
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 07365a5. Configure here.
yordis
force-pushed
the
yordis/fix-gitlab-signature
branch
2 times, most recently
from
b6631ce to
d100570
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-gateway/src/config.rs (1)
2499-2499: ⚡ Quick winCentralize GitLab test token literals to reduce leak-scanner noise.
These repeated inline
whsec_...literals are likely to keep triggering secret scanners and create avoidable test-fixture churn. Please source them from one helper/constant and interpolate where needed.♻️ Example consolidation pattern
- let toml = r#" - [sources.gitlab.integrations.primary.webhook] - signing_token = "whsec_MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDE=" - "#; + let toml = format!( + r#" + [sources.gitlab.integrations.primary.webhook] + signing_token = "{}" + "#, + gitlab_signing_token() + );Also applies to: 2822-2822, 2838-2838, 3287-3287, 3427-3427, 3574-3574
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-gateway/src/config.rs` at line 2499, Centralize the repeated GitLab test token string by adding a single constant (e.g., TEST_GITLAB_SIGNING_TOKEN) in the same module and replace every inline "whsec_..." literal assigned to signing_token with that constant; update places referenced (the signing_token assignments at the shown sites) to interpolate or reference TEST_GITLAB_SIGNING_TOKEN instead of hardcoding the value so secret-scanner noise is reduced and maintenance is easier.rsworkspace/crates/trogon-gateway/src/source/standard_webhooks.rs (1)
24-65: ⚡ Quick winCapture the missing header name in
MissingHeadersfor parity withInvalidHeaderValue.
InvalidHeaderValuealready carries the offending header name, butMissingHeaderscollapses three distinct failure points (webhook_id,webhook_timestamp,webhook_signature) into one opaque variant. When a sender misconfigures their headers in production, you currently can't tell from the typed error which header was absent. Promoting the name into the variant is a small change that materially improves diagnostics and stays consistent with the rest of the error contract.♻️ Proposed change
#[derive(Debug)] pub enum SignatureError { - MissingHeaders, + MissingHeader { name: &'static str }, InvalidHeaderValue { name: &'static str, source: axum::http::header::ToStrError, }, ... } impl fmt::Display for SignatureError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::MissingHeaders => f.write_str("missing required signature headers"), + Self::MissingHeader { name } => write!(f, "missing required signature header {name}"), ... } } } impl std::error::Error for SignatureError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { ... - Self::MissingHeaders | Self::StaleTimestamp | Self::MissingV1Signature | Self::Mismatch => None, + Self::MissingHeader { .. } | Self::StaleTimestamp | Self::MissingV1Signature | Self::Mismatch => None, } } } fn header_str<'a>(headers: &'a HeaderMap, name: &'static str) -> Result<&'a str, SignatureError> { headers .get(name) - .ok_or(SignatureError::MissingHeaders)? + .ok_or(SignatureError::MissingHeader { name })? .to_str() .map_err(|source| SignatureError::InvalidHeaderValue { name, source }) }Also applies to: 209-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-gateway/src/source/standard_webhooks.rs` around lines 24 - 65, SignatureError::MissingHeaders currently has no context; change the enum variant to MissingHeaders { name: &'static str } (mirroring InvalidHeaderValue), update fmt::Display arm to print the header name (e.g., "missing required header {name}"), and adjust any constructors/places that produce SignatureError::MissingHeaders to pass the specific header names ("webhook_id", "webhook_timestamp", "webhook_signature"); no change to source() is needed since MissingHeaders has no inner error. Also update any references noted around the other occurrence (lines ~209-215) to construct the new variant accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Line 2499: Centralize the repeated GitLab test token string by adding a single
constant (e.g., TEST_GITLAB_SIGNING_TOKEN) in the same module and replace every
inline "whsec_..." literal assigned to signing_token with that constant; update
places referenced (the signing_token assignments at the shown sites) to
interpolate or reference TEST_GITLAB_SIGNING_TOKEN instead of hardcoding the
value so secret-scanner noise is reduced and maintenance is easier.
In `@rsworkspace/crates/trogon-gateway/src/source/standard_webhooks.rs`:
- Around line 24-65: SignatureError::MissingHeaders currently has no context;
change the enum variant to MissingHeaders { name: &'static str } (mirroring
InvalidHeaderValue), update fmt::Display arm to print the header name (e.g.,
"missing required header {name}"), and adjust any constructors/places that
produce SignatureError::MissingHeaders to pass the specific header names
("webhook_id", "webhook_timestamp", "webhook_signature"); no change to source()
is needed since MissingHeaders has no inner error. Also update any references
noted around the other occurrence (lines ~209-215) to construct the new variant
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e41ffbeb-1d3e-44b3-b9c6-37b8ff9447cb
📒 Files selected for processing (16)
devops/docker/compose/services/trogon-gateway/README.mdrsworkspace/crates/trogon-gateway/README.mdrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/constants.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/config.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/constants.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/gitlab_signing_token.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/mod.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/server.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/signature.rsrsworkspace/crates/trogon-gateway/src/source/incidentio/signature.rsrsworkspace/crates/trogon-gateway/src/source/mod.rsrsworkspace/crates/trogon-gateway/src/source/standard_webhooks.rsrsworkspace/crates/trogon-gateway/src/streams.rsservices/trogon-gateway/gateway.toml
✅ Files skipped from review due to trivial changes (4)
- devops/docker/compose/services/trogon-gateway/README.md
- rsworkspace/crates/trogon-gateway/src/constants.rs
- services/trogon-gateway/gateway.toml
- rsworkspace/crates/trogon-gateway/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-gateway/src/source/gitlab/constants.rs
- rsworkspace/crates/trogon-gateway/src/source/gitlab/server.rs
yordis
force-pushed
the
yordis/fix-gitlab-signature
branch
2 times, most recently
from
b1ddb06 to
6dfbe0b
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/fix-gitlab-signature
branch
from
6dfbe0b to
1d2a062
Compare
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
1 participant
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.