Redact secrets in denial alert URLs via the summarizer

[marcelogdeandrade]

Summary

The Slack denial summary could surface credentials passed as URL query-string parameters (e.g. api_key=) — both in the LLM-generated summary text and in the raw "Requests blocked:" URL list. CrabTrap already redacted sensitive headers, but not query-string credentials in this notification.

This folds redaction into the existing alert summarizer instead of adding a separate redaction module:

• The summarizer now makes a single LLM call that returns both the summary and the redacted form of each denied URL (secrets → [REDACTED]), in order.
• The Slack sender renders only those redacted URLs.
• If the LLM is unavailable or returns output that can't be trusted (parse failure / count mismatch), the URL list is omitted — the alert shows the blocked count and a generic summary rather than leaking raw URLs.
• Upstream proxying is unchanged — real credentials still reach the target API.

Notes

• Redaction is intentionally scoped to the Slack denial notification path (low-volume, leaves the system, affordable to run through an LLM). Audit logs, judge prompts, and proxy logs are out of scope here.
• No deterministic/static redaction layer is introduced; redaction is entirely the LLM pass, with a safe no-URL fallback.

Test plan

• go test ./internal/alerting/
• Trigger a denied request with a credential query param; confirm the Slack denial summary and blocked-requests list show [REDACTED], not the raw secret
• Confirm upstream requests still forward the real credential

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR folds URL-secret redaction into the existing denial alert summarizer: a single LLM call now returns both the human-readable summary and a redacted copy of each denied URL, and the Slack sender renders only those redacted URLs. When the LLM is unavailable or returns output with a mismatched URL count, the URL list is omitted entirely rather than leaking raw credentials.

• The count-mismatch guard and the parse-failure fallback together ensure raw credential-bearing URLs never reach Slack if the LLM can't be trusted.
• promptLineSanitizer correctly strips newlines from d.URL and d.Reason to prevent numbered-list injection, but d.Method and botID are inserted into the same prompt without the same treatment — a minor inconsistency in the defence-in-depth layer.
• summarizer_test.go is thorough: it covers the happy path, count mismatch, adapter failures, nil adapter, newline injection, and code-fence stripping.

Confidence Score: 5/5

Safe to merge — the core credential-leakage fix is correct and well-tested; two minor sanitization gaps exist in the prompt builder but do not affect the URL-redaction safety guarantee.

The primary goal — preventing raw credential-bearing URLs from appearing in Slack — is achieved correctly: the count-mismatch guard and the error fallback both zero out the URL list rather than fall back to raw URLs. The two unsanitized fields (d.Method in the numbered list and botID in the preamble) are attacker-influenced but constrained in practice (HTTP methods are framework-validated; bot IDs come from the auth system), so the practical exploitability is low and does not threaten the credential-leakage guarantee.

internal/alerting/summarizer.go — the prompt-building loop sanitizes d.URL and d.Reason but leaves d.Method and botID unsanitized.

Important Files Changed

Filename	Overview
internal/alerting/summarizer.go	Core change: extends Summarize to return JSON with redacted URLs via a single LLM call; adds promptLineSanitizer for URL and Reason fields; count-mismatch guard prevents leaking unredacted URLs. d.Method is not sanitized in the same numbered-list lines.
internal/alerting/alerting.go	Wires the new two-return Summarize interface; drops URL list on error to prevent credential leakage; switches Slack message to use BlockedCount for display — looks correct.
internal/alerting/sender.go	Adds BlockedCount to Message; conditionally renders the 'Requests blocked:' list only when redacted URLs are present — correct behaviour.
internal/alerting/summarizer_test.go	Good coverage: happy path, count mismatch, adapter failure, nil adapter, newline injection, and code-fence stripping all tested.
internal/admin/notification_integration_test.go	Integration test mock updated to match new Summarize signature; mock returns denials as-is, which is fine for plumbing tests that don't exercise redaction logic.

_{Reviews (4): Last reviewed commit: "fix(alerting): sanitize newlines in summ..." | Re-trigger Greptile}

[marcelogdeandrade]

Thanks @greptile — addressing the review in 1706999.

P1 — Prompt injection via newlines in URL/Reason (fixed). Good catch, especially since LLMReason is itself LLM-generated and could carry a literal newline. d.URL and d.Reason now have CR/LF/tab collapsed to spaces (sanitizePromptField) before being placed into the numbered list, so neither can inject a fake numbered entry and desync the index→URL mapping. Added a regression test (TestSummarize_SanitizesNewlinesInDenialFields).

Summary field lacks programmatic redaction verification (acknowledged, by design). The URL slots can be hard-checked because they're a fixed-length array we map by index; free-form summary text can't be verified the same way without a deterministic secret-detector — which is exactly the static-redaction layer we deliberately chose not to build here. The summary is redacted by the same LLM pass, and the newline sanitization above also reduces the injection surface for it. If the LLM fails or returns untrusted output we fall back to a count-only generic summary with no URLs, so the failure mode is safe. I'd treat hardening the summary further (or dropping URL detail from it entirely) as a follow-up if we want a stronger guarantee.

[greptile-apps]

The fixes look solid. Both points are addressed well:

P1 (prompt injection via newlines): The sanitizePromptField / promptLineSanitizer approach is clean and correct — collapsing \n/\r/\t to spaces before inserting into the numbered list prevents a crafted URL or LLM-generated Reason from injecting a fake entry and desyncing the index→URL mapping. The regression test covers the exact attack vector.

Summary field acknowledgment: The reasoning is sound. The URL slots get a structural integrity check (exact count match, index-aligned); free-form prose can't get the same treatment without a deterministic secret-detector. The failure mode (LLM down → no URLs, generic count summary) is appropriately conservative. If summary-level guarantee needs hardening later, dropping URLs from the summary field entirely or running a lightweight regex scrub as a backstop would be the natural next steps — but neither is needed to ship this safely.

One minor thing to double-check: TestSummarize_SanitizesNewlinesInDenialFields returns "urls":["https://example.com/"] for a denial whose original URL contained an injected newline. The test correctly asserts the prompt is clean but doesn't assert that the returned redacted URL doesn't contain the injected fragment. Since the mock returns a clean URL, this is fine — just worth noting the test is validating prompt sanitization specifically, not model output (which is the right scope for a unit test).

LGTM overall — nice clean fix.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.