Restore and harden credential masking for /targets#1697
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughThe PR addresses three independent functional areas: re-enables ChangesTarget Secrets Masking
Ingest Script Configurable Stream Name
Log Context API Bounds Validation and Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
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)
257-281:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlertManager returns wrong
typefield value.Both branches return
"type":"webhook"but theTargetType::AlertManagervariant is serialized as"alertManager"per its#[serde(rename = "alertManager")]attribute. Clients differentiating target types will break.🐛 Proposed fix
TargetType::AlertManager(alert_manager) => { let endpoint = Self::mask_url(&alert_manager.endpoint); if let Some(auth) = alert_manager.auth { let password = "********"; json!({ "name":self.name, - "type":"webhook", + "type":"alertManager", "endpoint":endpoint, "username":auth.username, "password":password, "skipTlsCheck":alert_manager.skip_tls_check, "id":self.id }) } else { json!({ "name":self.name, - "type":"webhook", + "type":"alertManager", "endpoint":endpoint, "username":Value::Null, "password":Value::Null, "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 257 - 281, The TargetType::AlertManager match arm incorrectly returns "type":"webhook" in both the if and else branches of the json! macro calls, but it should return "type":"alertManager" to match the serde rename attribute. Change the "type" field value from "webhook" to "alertManager" in both json! macro invocations within the AlertManager branch to ensure clients can correctly differentiate target types.
🤖 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 257-281: The TargetType::AlertManager match arm incorrectly
returns "type":"webhook" in both the if and else branches of the json! macro
calls, but it should return "type":"alertManager" to match the serde rename
attribute. Change the "type" field value from "webhook" to "alertManager" in
both json! macro invocations within the AlertManager branch to ensure clients
can correctly differentiate target types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 39e020dc-d533-409a-a23f-6c590291ff1a
📒 Files selected for processing (2)
src/alerts/target.rssrc/handlers/http/targets.rs
- Add should_redact_header() to aggressively neutralize sensitive headers.
- Add mask_url() using safe rfind('/') path truncation.
- Apply consistent masking across Slack, Webhook, and AlertManager types.
- Add strict unit tests enforcing that secrets never hit the serializer.
391c497 to
d32bf50
Compare
context api request to have support to send either of - 1. context window (1m, 5m, 10m etc) 2. context start and end time add validations - 1. p_timestamp of the reference log should be within the context window 2. window and start/end time - both should not be present in the request 3. start <= end time
- 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.
Description
The goal of this PR is to resolve a critical credential leakage vulnerability (Issue #1693) where notification target secrets (webhook tokens, basic-auth passwords, sensitive headers) were exposed in cleartext to any low-privilege user via the
/targetsAPI.Root Cause:
During the v2.8.0 refactor (PR #1398), target masking was deliberately disabled (
removed masking from targets fow now) and never restored. TheTarget::mask()calls were commented out in the handlers, causing the raw structs to be serialized directly to the HTTP response.Simply uncommenting the old code was rejected because the legacy masking logic was fundamentally flawed:
Authorization: Bearer ...) in cleartext.&endpoint[..20]) for URL truncation, which risked partial secret leakage and Rust panics on non-ASCII URL boundaries.Chosen Solution:
We completely rewrote the masking logic to guarantee zero leakage while maintaining stability, and re-enabled the calls across all handlers (including DELETE).
should_redact_header()to aggressively neutralize sensitive headers regardless of casing.mask_url()using saferfind('/')path truncation, optimized for standard webhook architectures.targets.rsto prevent accidental reintroduction.This PR has:
Summary by CodeRabbit
/targetsAPI responses to always return masked target details, preventing exposure of sensitive values.streamname via the CLI.