Restore and harden credential masking for /targets#1698
Conversation
Walkthrough
ChangesTarget Secrets Redaction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 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 |
![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 (1)
src/alerts/target.rs (1)
663-691: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider extending coverage to Slack/Other and embedded-userinfo cases.
The regression test only exercises the
AlertManagerbranch with separateauth. The higher-risk paths — Slack URLs (secret lives entirely in the path),Otherheader values, and endpoints with embeddeduser:password@userinfo — are not asserted. Adding a case that constructs an endpoint likehttps://user:s3cret@host/secretpath?token=abcand asserts the secret is absent would directly guard the leak noted inmask_url.🤖 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 `@src/alerts/target.rs` around lines 663 - 691, The test_masked_target_hides_secrets function only covers the AlertManager variant with separate auth credentials and does not test higher-risk secret exposure paths. Extend the test by adding additional assertions or test cases that verify secrets embedded directly in endpoint URLs (such as user:password@host patterns, path components, and query parameters) are properly masked across other TargetType variants like Slack and Other. Construct test endpoints with embedded secrets (e.g., https://user:s3cret@host/secretpath?token=abc) and assert these secrets are absent from the masked output, ensuring the mask method properly handles all the cases that mask_url is designed to protect against.
🤖 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.
Inline comments:
In `@src/alerts/target.rs`:
- Around line 198-208: The mask_url function has a security vulnerability where
it fails to strip userinfo (username and password) from URLs, allowing
credentials to leak in the masked output. Fix this by reliably removing userinfo
before serialization - test carefully whether setting both username and password
via the Url API methods fully removes the userinfo component, or alternatively
reconstruct the URL to include only the scheme, host, and port components.
Additionally, remove the redundant segments.clear() call since the subsequent
set_path call will overwrite the entire path anyway, and update the doc comment
to accurately describe the function's behavior as redacting userinfo and
removing path, query, and fragment components rather than just obliterating the
trailing path segment.
---
Nitpick comments:
In `@src/alerts/target.rs`:
- Around line 663-691: The test_masked_target_hides_secrets function only covers
the AlertManager variant with separate auth credentials and does not test
higher-risk secret exposure paths. Extend the test by adding additional
assertions or test cases that verify secrets embedded directly in endpoint URLs
(such as user:password@host patterns, path components, and query parameters) are
properly masked across other TargetType variants like Slack and Other. Construct
test endpoints with embedded secrets (e.g.,
https://user:s3cret@host/secretpath?token=abc) and assert these secrets are
absent from the masked output, ensuring the mask method properly handles all the
cases that mask_url is designed to protect against.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90ac2817-af22-4381-b9c6-dbe25b5613f8
📒 Files selected for processing (2)
src/alerts/target.rssrc/handlers/http/targets.rs
ygndotgg
force-pushed
the
fix-alerts-masking
branch
from
fd7d7f2 to
cc1dcdd
Compare
- Add mask_url() using url based parsing - Apply consistent masking across Slack, Webhook, and AlertManager types. - Add strict unit tests enforcing that secrets never hit the serializer.
ygndotgg
force-pushed
the
fix-alerts-masking
branch
from
cc1dcdd to
38b707f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/alerts/target.rs (1)
225-235: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winBasic-auth
usernameis still returned in cleartext.The endpoint and password are now redacted, but
auth.usernameis serialized as-is. The linked issue explicitly lists AlertManager "basic-auth username and password combinations" as exposed secrets, so anyGetAlert-scoped reader still recovers half of every basic-auth credential. Confirm whether exposing the username is intentional; if not, redact it (e.g.********) like the other fields.🛡️ Proposed redaction
if let Some(auth) = alert_manager.auth { let password = "********"; json!({ "name":self.name, "type":"webhook", "endpoint":endpoint, - "username":auth.username, + "username":"********", "password":password, "skipTlsCheck":alert_manager.skip_tls_check, "id":self.id })🤖 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 `@src/alerts/target.rs` around lines 225 - 235, The basic-auth username is being exposed in cleartext in the JSON response object while the password is properly redacted with "********". In the alert_manager JSON object creation, the "username" field is currently serialized with auth.username directly. Change this to use a redacted value (such as "********") matching the approach used for the password field, so both the username and password are masked in the response.
🧹 Nitpick comments (1)
src/alerts/target.rs (1)
648-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the new
OtherWebHookheader redaction.This test only exercises the
AlertManagerbranch. The most novel part of this change—redacting everyOtherWebHookheader value while preserving keys—has no regression test. A target with a header likeAuthorization: Bearer <token>should be asserted to no longer contain the token in masked output. Consider adding aTargetType::Othercase (and a Slack case) to lock in the redaction guarantee across all branches.🤖 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 `@src/alerts/target.rs` around lines 648 - 677, The test_masked_target_hides_secrets function currently only covers the AlertManager branch and does not test the new OtherWebHook header redaction feature. Extend this test to include additional test cases for TargetType::Other with headers containing sensitive values (such as "Authorization: Bearer <token>") and a TargetType::Slack case. For each new case, create a masked Target and verify that the sensitive header values do not appear in the serialized output while the redaction marker "********" is present, similar to the existing AlertManager assertions. This ensures the header redaction guarantee is maintained across all target type branches.
🤖 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.
Outside diff comments:
In `@src/alerts/target.rs`:
- Around line 225-235: The basic-auth username is being exposed in cleartext in
the JSON response object while the password is properly redacted with
"********". In the alert_manager JSON object creation, the "username" field is
currently serialized with auth.username directly. Change this to use a redacted
value (such as "********") matching the approach used for the password field, so
both the username and password are masked in the response.
---
Nitpick comments:
In `@src/alerts/target.rs`:
- Around line 648-677: The test_masked_target_hides_secrets function currently
only covers the AlertManager branch and does not test the new OtherWebHook
header redaction feature. Extend this test to include additional test cases for
TargetType::Other with headers containing sensitive values (such as
"Authorization: Bearer <token>") and a TargetType::Slack case. For each new
case, create a masked Target and verify that the sensitive header values do not
appear in the serialized output while the redaction marker "********" is
present, similar to the existing AlertManager assertions. This ensures the
header redaction guarantee is maintained across all target type branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0c4cef73-8d25-448d-b1c6-a9ea59f84b38
📒 Files selected for processing (2)
src/alerts/target.rssrc/handlers/http/targets.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/targets.rs
2 participants
Fixes #1693.
Description
Resolves a credential leakage vulnerability where notification target secrets (webhook tokens, basic-auth passwords, sensitive headers) were exposed in cleartext via the
/targetsAPI.Root Cause: Target masking was disabled in PR #1398 and never restored. The
Target::mask()calls were commented out in all handlers.Why not just uncomment: The legacy masking logic was flawed—it leaked custom HTTP headers in cleartext and used unsafe byte-slicing (
&endpoint[..20]) that risked partial secret leakage and panics on non-ASCII boundaries.Solution:
mask_url()method using theurlcrate API (no byte-slicing)target.mask()in all 5 HTTP handlersThis PR has:
Summary by CodeRabbit
{scheme}://********), avoiding accidental leakage of full URLs.********.username(when present) while redactingpassword; omitted credentials are returned asnull.********marker.