Skip to content

Restore and harden credential masking for /targets#1697

Closed
ygndotgg wants to merge 4 commits into
parseablehq:mainfrom
ygndotgg:fix-alerts-secrets
Closed

Restore and harden credential masking for /targets#1697
ygndotgg wants to merge 4 commits into
parseablehq:mainfrom
ygndotgg:fix-alerts-secrets

Conversation

@ygndotgg

@ygndotgg ygndotgg commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

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 /targets API.

Root Cause:
During the v2.8.0 refactor (PR #1398), target masking was deliberately disabled (removed masking from targets fow now) and never restored. The Target::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:

  1. It leaked custom HTTP headers (like Authorization: Bearer ...) in cleartext.
  2. It used naive byte-slicing (&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).

  • Add should_redact_header() to aggressively neutralize sensitive headers regardless of casing.
  • Add mask_url() using safe rfind('/') path truncation, optimized for standard webhook architectures.
  • Apply consistent masking across Slack, Webhook, and AlertManager types.
  • Add strict unit tests enforcing that secrets never hit the serializer.
  • Purge all commented-out insecure code paths from targets.rs to prevent accidental reintroduction.

This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Summary by CodeRabbit

  • Bug Fixes
    • Updated /targets API responses to always return masked target details, preventing exposure of sensitive values.
    • Improved URL redaction by masking full endpoints (paths, queries, and fragments) consistently across target types.
    • Strengthened webhook/endpoint masking by redacting all header values and ensuring AlertManager auth secrets are never revealed.
    • Added unit coverage to confirm redaction markers are present and secrets remain hidden.
  • Behavior Changes
    • Enhanced log-context query handling to accept either window-based or explicit start/end times, with stricter validation and auto-scaling timestamp formatting.
  • Documentation / Scripts
    • Updated ingest scripts to require/propgate a configurable stream name via the CLI.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

The PR addresses three independent functional areas: re-enables Target::mask() redaction with improved helpers for all /targets API response paths, adds configurable stream names to ingest setup scripts with Fluent Bit propagation, and refactors the log context API to support explicit timestamp bounds with mutual-exclusivity validation and anchor-in-bounds checks.

Changes

Target Secrets Masking

Layer / File(s) Summary
Masking helpers and Target::mask() implementation
src/alerts/target.rs
Adds private mask_url() helper that truncates URLs to /******** and clears query/fragment. Reworks Target::mask() so Slack, Other, and AlertManager endpoints all use mask_url(); Other targets also redact sensitive header values (Authorization, Cookie, and header names containing token/secret/key) to ********. Updates AlertManager JSON to use the masked endpoint. Includes #[cfg(test)] unit test verifying secrets are hidden.
HTTP handlers apply mask() on all response paths
src/handlers/http/targets.rs
post, list, get, update, and delete each call .mask() before returning JSON, replacing previously unmasked web::Json(target) serialization.

Ingest Script Configurable Stream Name

Layer / File(s) Summary
PowerShell script stream parameter and Setup-FluentBit wiring
scripts/ingest.ps1
Adds Param3 (stream) and Param4 (tenant) to parameter block. Updates Setup-FluentBit signature to accept required StreamName (alongside IngestorHost and ApiKey), validates presence of all three, and propagates $StreamName into generated Fluent Bit config's X-P-Stream header. Updates help text and examples to document new argument order.
Shell script stream parameter and setup_fluent_bit wiring
scripts/ingest.sh
Updates setup_fluent_bit to extract STREAM_NAME from second argument and API_KEY from third, validates all three required fields are present, and propagates $STREAM_NAME into generated Fluent Bit config's X-P-Stream header instead of hardcoded node-metrics. Updates help, usage, and dispatch logic for correct argument positioning.

Log Context API Bounds Validation and Refactoring

Layer / File(s) Summary
Request schema updates
src/handlers/http/query_context.rs
Changes LogContextRequest fields context_window, context_start_time, and context_end_time from required to Option<String> to support flexible request patterns.
Bounds resolution and validation logic
src/handlers/http/query_context.rs
Introduces resolve_log_context_bounds() enforcing mutual exclusivity: errors when contextWindow and contextStartTime/contextEndTime are both provided or when the start/end pair is incomplete; returns bounds via window-mode or explicit-mode. Adds log_context_explicit_bounds() to parse and validate timestamp ordering. Adds validate_log_context_anchor_in_bounds() to ensure anchor timestamp falls within resolved [start, end) window. Updates query_context handler to resolve then validate.
Timestamp parsing and formatting improvements
src/handlers/http/query_context.rs
Refactors parse_log_context_timestamp() to delegate to new parse_log_context_time_field() helper providing field-specific error messages. Changes format_log_context_api_time() formatting from SecondsFormat::Secs to SecondsFormat::AutoSi.
Log context tests updated for new resolver behavior
src/handlers/http/query_context.rs
Unit tests verify resolve_log_context_bounds() mutual-exclusivity, log_context_explicit_bounds() parsing, and validate_log_context_anchor_in_bounds() enforcement. SQL-building tests now derive bounds via log_context_explicit_bounds() instead of removed log_context_bounds().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • parseablehq/parseable#1365: Modifies the same src/alerts/target.rs and src/handlers/http/targets.rs files with the original Target::mask() implementation that this PR re-enables and improves.
  • parseablehq/parseable#1489: Creates the ingest scripts (scripts/ingest.ps1 and scripts/ingest.sh) that this PR extends with configurable stream names and Fluent Bit propagation.
  • parseablehq/parseable#1695: Updates the log context API request types and bounds validation; this PR's refactoring of resolve_log_context_bounds(), explicit-bounds parsing, and anchor validation directly builds on that PR's foundation.

Suggested labels

for next release

Suggested reviewers

  • nikhilsinhaparseable
  • parmesant

Poem

🐇 Three tasks, three pathways, rabbit's delight—
Secrets masked in Target, headers tucked tight!
Streams now named in scripts, Fluent Bit knows the way,
Context bounds validated—no anchor astray. ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Changes to query_context.rs (context window validation and timestamp parsing) and ingest scripts (stream name parameter) appear unrelated to the credential masking objective from issue #1693. Remove or justify changes to query_context.rs and ingest scripts, or split them into separate PRs focused on their specific objectives.
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main objective of the PR: restoring and hardening credential masking for the /targets API.
Description check ✅ Passed The PR description comprehensively addresses all required sections: it fixes issue #1693, explains the root cause and chosen solution, describes key changes, and acknowledges the checklist items.
Linked Issues check ✅ Passed The PR fully addresses issue #1693 by restoring and improving credential masking across all /targets handlers, eliminating the cleartext leakage of webhook tokens, basic-auth passwords, and sensitive headers.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

AlertManager returns wrong type field value.

Both branches return "type":"webhook" but the TargetType::AlertManager variant 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

📥 Commits

Reviewing files that changed from the base of the PR and between c849704 and 391c497.

📒 Files selected for processing (2)
  • src/alerts/target.rs
  • src/handlers/http/targets.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 22, 2026
@ygndotgg ygndotgg changed the title This patch re-enables masking with a few backend changes Restore and harden credential masking for /targets Jun 22, 2026
- 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.
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 23, 2026
nikhilsinhaparseable and others added 3 commits June 23, 2026 11:28
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants