security(tracker): sanitize provider probe error bodies before surfacing in DoctorReport#122
security(tracker): sanitize provider probe error bodies before surfacing in DoctorReport#122
Conversation
Agent-Logs-Url: https://github.com/2389-research/tracker/sessions/91f5dd9e-cc86-473d-a43d-f4ecb14cf439 Co-authored-by: clintecker <26985+clintecker@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces the risk of leaking sensitive credential material by sanitizing LLM provider probe error strings before they’re surfaced in DoctorReport check details/messages.
Changes:
- Sanitize non-auth provider probe error messages via
sanitizeProviderError(...)before trimming and returning them. - Add regex-based redaction rules for Bearer tokens, API-key-like strings, and request-id patterns.
- Add unit tests covering direct sanitization and the probe-path behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tracker_doctor.go |
Applies sanitization to provider probe errors and introduces regex-based redaction rules. |
tracker_doctor_test.go |
Adds tests validating redaction and that probeProvider returns sanitized messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| re *regexp.Regexp | ||
| repl string | ||
| }{ | ||
| {regexp.MustCompile(`(?i)\bBearer\s+[A-Za-z0-9._~+/=-]+\b`), "Bearer [REDACTED]"}, |
There was a problem hiding this comment.
The Bearer-token sanitizer regex ends with a word-boundary (\b). If the token ends with '=' (common for base64/JWT padding), '\b' will not match and the token will remain unredacted. Consider replacing the trailing '\b' with a lookahead like (?=\s|$) (or otherwise anchoring on whitespace/end) so tokens ending in non-word characters are still sanitized, and add a unit test covering a Bearer token that ends with '='.
| {regexp.MustCompile(`(?i)\bBearer\s+[A-Za-z0-9._~+/=-]+\b`), "Bearer [REDACTED]"}, | |
| {regexp.MustCompile(`(?i)\bBearer\s+[A-Za-z0-9._~+/=-]+(?=\s|$)`), "Bearer [REDACTED]"}, |
| }, "test-key") | ||
|
|
||
| if ok { | ||
| t.Fatal("expected auth probe to fail") |
There was a problem hiding this comment.
This test failure message says "expected auth probe to fail", but the test is specifically asserting the non-auth error path is sanitized. Updating the message to reflect the actual expectation will make failures easier to interpret.
| t.Fatal("expected auth probe to fail") | |
| t.Fatal("expected non-auth probe to fail") |
| {regexp.MustCompile(`(?i)\bBearer\s+[A-Za-z0-9._~+/=-]+\b`), "Bearer [REDACTED]"}, | ||
| {regexp.MustCompile(`\bsk-ant-[A-Za-z0-9_-]+\b`), "[REDACTED_API_KEY]"}, |
There was a problem hiding this comment.
PR description says Bearer tokens are redacted to "******", but the implementation replaces them with "Bearer [REDACTED]". Either align the description with the actual redaction format or adjust the replacement string so downstream users know what to expect.
|
Superseded by merged #113, which added |
DoctorReportprovider probe failures were exposing raw provider error text (trimmed but unsanitized) inCheckDetail.Message. Some provider errors can echo sensitive request context (tokens, key fragments, request identifiers), creating downstream leak risk for logs/webhooks.What changed
sanitizeProviderError(msg string)intracker_doctor.go.probeProviderfor non-auth failures beforetrimErrMsg(..., 80)is used to build the public message."invalid or expired API key").Sanitization coverage
sk-ant-*,sk-*,AIza*).request-id=...,req_...).Targeted test additions