From a391af1667a25521ca2307daebf56a7600b3bd61 Mon Sep 17 00:00:00 2001 From: Henry Date: Fri, 11 Sep 2026 04:30:37 +0800 Subject: [PATCH] fix(agent): stop provider fallback from leaking secrets and raw continuation text (#3393) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../agent/runtime/dynamic/conductor.go | 43 +- .../agent/runtime/dynamic/conductor_test.go | 12 +- .../runtime/dynamic/continuation_safety.go | 6 +- .../dynamic/continuation_security_test.go | 153 +++++- .../agent/runtime/routingerr/sanitize.go | 367 ++++++++++++-- .../agent/runtime/routingerr/sanitize_test.go | 477 +++++++++++++++++- .../system-design/provider-error-recovery.md | 51 ++ 7 files changed, 1047 insertions(+), 62 deletions(-) diff --git a/apps/backend/internal/agent/runtime/dynamic/conductor.go b/apps/backend/internal/agent/runtime/dynamic/conductor.go index 229391294a..538159d8f5 100644 --- a/apps/backend/internal/agent/runtime/dynamic/conductor.go +++ b/apps/backend/internal/agent/runtime/dynamic/conductor.go @@ -432,15 +432,20 @@ const continuationFieldLimit = 4000 // predecessor left off, not where it began. ToolSummary and FailureReason can // carry raw tool output or provider-controlled error text respectively, so // both are sanitized before crossing to a different provider or reaching -// durable storage. +// durable storage. TaskDescription, PlanSummary, and RepositorySummary are +// user/agent-authored carrier text rather than diagnostics: a credential +// pasted into any of them must not cross to a different provider either, but +// the full Sanitize rule set would mangle legitimate long identifiers (commit +// SHAs, UUIDs) these fields routinely contain, so they use the narrower +// credential-only tier instead. func BuildBoundedContinuation(input ContinuationInput) Continuation { return Continuation{ - TaskDescription: bounded(input.TaskDescription), + TaskDescription: bounded(routingerr.SanitizeCredentials(input.TaskDescription)), WorkflowStep: bounded(input.WorkflowStep), Conversation: boundedConversation(input.UserMessages, input.Conversation), ToolSummary: sanitizedHead(input.ToolSummary), - RepositorySummary: bounded(input.RepositorySummary), - PlanSummary: bounded(input.PlanSummary), + RepositorySummary: bounded(routingerr.SanitizeCredentials(input.RepositorySummary)), + PlanSummary: bounded(routingerr.SanitizeCredentials(input.PlanSummary)), FailureReason: bounded(routingerr.Sanitize(input.FailureReason)), } } @@ -451,8 +456,8 @@ func BuildBoundedContinuation(input ContinuationInput) Continuation { // every user message; splitting the limit guarantees both survive. const conversationUserBudget = continuationFieldLimit / 2 -// maxRedactionInputBytes bounds how much of a raw field Redact scans -// directly. A task's message history is unpaginated, so a large session +// maxRedactionInputBytes bounds how much of a raw field SanitizeFullUnbounded +// scans directly. A task's message history is unpaginated, so a large session // (repeated tool output, long command logs) would otherwise force all // redaction rules across an unbounded input to emit a few kept bytes. 64x // continuationFieldLimit comfortably covers ordinary sessions while @@ -470,21 +475,21 @@ const redactionLookbackBytes = 64 * 1024 // sanitizedTail returns up to budget bytes of the newest content in raw, // with credentials redacted. For input at or under maxRedactionInputBytes, -// Redact sees the complete input before any cut, so an anchored rule (e.g. -// "Authorization:") always sees its full literal prefix and value together -// and cannot be bisected by a budget window. For larger input, the window's -// leading edge is snapped back to the nearest preceding line boundary -// (within redactionLookbackBytes) before Redact runs, so an anchored rule -// starting on an earlier line is not split by the window itself. The scan -// stays bounded by maxRedactionInputBytes+redactionLookbackBytes regardless -// of how large raw is — unlike a full-input fallback, its cost cannot grow -// with session size. +// SanitizeFullUnbounded sees the complete input before any cut, so an +// anchored rule (e.g. "Authorization:") always sees its full literal prefix +// and value together and cannot be bisected by a budget window. For larger +// input, the window's leading edge is snapped back to the nearest preceding +// line boundary (within redactionLookbackBytes) before SanitizeFullUnbounded +// runs, so an anchored rule starting on an earlier line is not split by the +// window itself. The scan stays bounded by +// maxRedactionInputBytes+redactionLookbackBytes regardless of how large raw +// is — unlike a full-input fallback, its cost cannot grow with session size. func sanitizedTail(raw string, budget int) string { if len(raw) <= maxRedactionInputBytes { - return boundedTailN(routingerr.Redact(raw), budget) + return boundedTailN(routingerr.SanitizeFullUnbounded(raw), budget) } start := windowStartWithLookback(raw, len(raw)-maxRedactionInputBytes) - return boundedTailN(routingerr.Redact(raw[start:]), budget) + return boundedTailN(routingerr.SanitizeFullUnbounded(raw[start:]), budget) } // sanitizedHead mirrors sanitizedTail for ToolSummary, whose final cut @@ -493,10 +498,10 @@ func sanitizedTail(raw string, budget int) string { // ending later is not split by the window itself. func sanitizedHead(raw string) string { if len(raw) <= maxRedactionInputBytes { - return bounded(routingerr.Redact(raw)) + return bounded(routingerr.SanitizeFullUnbounded(raw)) } end := windowEndWithLookahead(raw, maxRedactionInputBytes) - return bounded(routingerr.Redact(raw[:end])) + return bounded(routingerr.SanitizeFullUnbounded(raw[:end])) } // windowStartWithLookback extends the tail window's leading edge back by a diff --git a/apps/backend/internal/agent/runtime/dynamic/conductor_test.go b/apps/backend/internal/agent/runtime/dynamic/conductor_test.go index 0fefe5d6ad..432bf37691 100644 --- a/apps/backend/internal/agent/runtime/dynamic/conductor_test.go +++ b/apps/backend/internal/agent/runtime/dynamic/conductor_test.go @@ -151,9 +151,12 @@ func TestConductorSanitizesAndPersistsPrebuiltContinuation(t *testing.T) { } const secret = "sk-prebuiltABCD1234567890" prebuilt := &Continuation{ - Conversation: "Authorization: " + secret, - ToolSummary: "tool output token=" + secret, - FailureReason: "provider failed with token=" + secret, + TaskDescription: "task token=" + secret, + Conversation: "Authorization: " + secret, + ToolSummary: "tool output token=" + secret, + RepositorySummary: "origin token=" + secret, + PlanSummary: "plan token=" + secret, + FailureReason: "provider failed with token=" + secret, } downstream := &recordingConductorTestDownstream{} store := &conductorContinuationStore{} @@ -184,7 +187,8 @@ func TestConductorSanitizesAndPersistsPrebuiltContinuation(t *testing.T) { t.Fatalf("saved continuation count = %d, want 1", len(store.saved)) } saved := store.saved[0].Continuation - if strings.Contains(saved.Conversation+saved.ToolSummary+saved.FailureReason, secret) { + if strings.Contains(saved.TaskDescription+saved.Conversation+saved.ToolSummary+ + saved.RepositorySummary+saved.PlanSummary+saved.FailureReason, secret) { t.Fatalf("persisted continuation retained the raw secret: %#v", saved) } } diff --git a/apps/backend/internal/agent/runtime/dynamic/continuation_safety.go b/apps/backend/internal/agent/runtime/dynamic/continuation_safety.go index 0d9deaee92..aff4eddad1 100644 --- a/apps/backend/internal/agent/runtime/dynamic/continuation_safety.go +++ b/apps/backend/internal/agent/runtime/dynamic/continuation_safety.go @@ -90,12 +90,12 @@ func skipBisectedRunForward(raw string, pos int) int { // written by an older process before the redaction rules were added. func sanitizeContinuation(continuation Continuation) Continuation { return Continuation{ - TaskDescription: bounded(continuation.TaskDescription), + TaskDescription: bounded(routingerr.SanitizeCredentials(continuation.TaskDescription)), WorkflowStep: bounded(continuation.WorkflowStep), Conversation: sanitizedTail(continuation.Conversation, continuationFieldLimit), ToolSummary: sanitizedHead(continuation.ToolSummary), - RepositorySummary: bounded(continuation.RepositorySummary), - PlanSummary: bounded(continuation.PlanSummary), + RepositorySummary: bounded(routingerr.SanitizeCredentials(continuation.RepositorySummary)), + PlanSummary: bounded(routingerr.SanitizeCredentials(continuation.PlanSummary)), FailureReason: bounded(routingerr.Sanitize(continuation.FailureReason)), } } diff --git a/apps/backend/internal/agent/runtime/dynamic/continuation_security_test.go b/apps/backend/internal/agent/runtime/dynamic/continuation_security_test.go index 2ace99f2ef..8354f73a52 100644 --- a/apps/backend/internal/agent/runtime/dynamic/continuation_security_test.go +++ b/apps/backend/internal/agent/runtime/dynamic/continuation_security_test.go @@ -197,6 +197,46 @@ func rawValueSuffix(value, s string) string { return "" } +// longKeyedSecretValue is longer than the fixed 512-byte look-behind slack +// sanitizedTail used before it was replaced with a sanitize-then-cut +// approach, and alternates characters with '.' so no substring of it is 32 +// bytes of contiguous alnum — long enough to defeat Sanitize's generic +// catch-all rule. If a tail-cut boundary ever separates this value from its +// "token=" key marker, nothing else would redact the leftover fragment. +var longKeyedSecretValue = strings.Repeat("Zx.", 220) + +// longSecretRotations are every phase-alignment of a 9-byte window over +// longKeyedSecretValue's 3-byte repeating unit, so a leaked fragment is +// caught regardless of which byte offset within the value the tail cut +// happened to preserve. +var longSecretRotations = []string{"Zx.Zx.Zx.", "x.Zx.Zx.Z", ".Zx.Zx.Zx"} + +// TestContinuationRedactsLongSecretStraddlingTailCut is the R3-A regression: +// sanitizedTail used to cut raw input to a fixed budget+512-byte window +// before sanitizing it, so a credential value longer than that slack could +// have its "token=" key marker fall outside the window while a tail +// fragment of the value remained inside it — a fragment with no visible key +// and no long-enough alnum run, so no rule ever redacted it. Swept across +// alignments since the exact point where the boundary falls inside the +// value depends on the surrounding padding length. +func TestContinuationRedactsLongSecretStraddlingTailCut(t *testing.T) { + secret := "token=" + longKeyedSecretValue + for tailLen := 1700; tailLen <= 2550; tailLen += 25 { + tail := strings.Repeat("z", tailLen) + userMessage := strings.Repeat("ab ", 800) + secret + " " + tail + + continuation := BuildBoundedContinuation(ContinuationInput{ + UserMessages: []string{userMessage}, + }) + + for _, rotation := range longSecretRotations { + if strings.Contains(continuation.Conversation, rotation) { + t.Fatalf("tailLen=%d: Conversation retained a fragment of the long secret: %q", tailLen, continuation.Conversation) + } + } + } +} + // TestContinuationRedactsAnchoredCredentialAtWindowCut proves that // sanitizedTail's tail cut never bisects an anchored rule's literal prefix // ("Authorization:", "--api-key", "Bearer") away from its value: for input @@ -555,8 +595,8 @@ func TestSanitizedTailRedactsAnchorSeparatedFromValueAtWindowCut(t *testing.T) { // Precondition: the rule really does redact this credential when // the anchor and value are both inside the window, so a // vacuously empty result cannot make the test pass. - if strings.Contains(routingerr.Redact(tc.anchor+tc.value), tc.value) { - t.Fatalf("precondition failed: routingerr.Redact does not redact %q", tc.anchor+tc.value) + if strings.Contains(routingerr.Sanitize(tc.anchor+tc.value), tc.value) { + t.Fatalf("precondition failed: routingerr.Sanitize does not redact %q", tc.anchor+tc.value) } // The filler collapses to "***" under the generic 32+ char rule, @@ -602,7 +642,7 @@ func TestSanitizedTailDropsValueAfterAnchorBeyondLookback(t *testing.T) { prefix := strings.Repeat("p", redactionLookbackBytes+1024) separator := strings.Repeat("\n", redactionLookbackBytes+4096) filler := strings.Repeat("a", 260000) - if strings.Contains(routingerr.Redact(anchor+separator+credential), credential) { + if strings.Contains(routingerr.Sanitize(anchor+separator+credential), credential) { t.Fatal("precondition failed: intact anchored credential was not redacted") } @@ -625,3 +665,110 @@ func TestSanitizedTailDropsValueAfterAnchorBeyondLookback(t *testing.T) { }) } } + +// TestContinuationRedactsUserAuthoredFieldSecrets is the deferred-finding +// regression: TaskDescription, PlanSummary, and RepositorySummary are +// user/agent-authored carrier text that crosses to a different provider via +// ContinuationPrompt, so a credential pasted into any of them must not +// survive into the persisted continuation JSON or the rendered prompt any +// more than one in ToolSummary, FailureReason, or Conversation does. +func TestContinuationRedactsUserAuthoredFieldSecrets(t *testing.T) { + fields := map[string]func(Continuation) string{ + "TaskDescription": func(c Continuation) string { return c.TaskDescription }, + "PlanSummary": func(c Continuation) string { return c.PlanSummary }, + "RepositorySummary": func(c Continuation) string { return c.RepositorySummary }, + } + for name, get := range fields { + t.Run(name, func(t *testing.T) { + input := ContinuationInput{} + carrier := "token=" + secretShapedToken + switch name { + case "TaskDescription": + input.TaskDescription = carrier + case "PlanSummary": + input.PlanSummary = carrier + case "RepositorySummary": + input.RepositorySummary = carrier + default: + t.Fatalf("unhandled field %q: add a case wiring it into ContinuationInput", name) + } + + continuation := BuildBoundedContinuation(input) + if strings.Contains(get(continuation), secretShapedToken) { + t.Fatalf("%s retained the raw secret: %q", name, get(continuation)) + } + + payload, err := json.Marshal(continuation) + if err != nil { + t.Fatalf("marshal continuation: %v", err) + } + if strings.Contains(string(payload), secretShapedToken) { + t.Fatalf("continuation_json retained the raw secret via %s: %s", name, payload) + } + + prompt := ContinuationPrompt("do the task", continuation) + if strings.Contains(prompt, secretShapedToken) { + t.Fatalf("rendered prompt retained the raw secret via %s: %q", name, prompt) + } + }) + } +} + +// TestContinuationRedactsURLCredentialsInUserAuthoredFields is the R1-F2 +// regression: the credential-only tier used for TaskDescription, PlanSummary, +// and RepositorySummary left URL userinfo (user:pass@host) untouched, so a +// credential embedded in a repository remote URL survived verbatim into all +// three fields and the rendered successor prompt. +func TestContinuationRedactsURLCredentialsInUserAuthoredFields(t *testing.T) { + const secretPassword = "s3cr3tpassw0rd" + raw := "origin https://alice:" + secretPassword + "@github.com/acme/repo.git (fetch)" + + continuation := BuildBoundedContinuation(ContinuationInput{ + RepositorySummary: raw, + TaskDescription: raw, + PlanSummary: raw, + }) + + if strings.Contains(continuation.RepositorySummary, secretPassword) { + t.Fatalf("RepositorySummary retained the raw URL credential: %q", continuation.RepositorySummary) + } + if strings.Contains(continuation.TaskDescription, secretPassword) { + t.Fatalf("TaskDescription retained the raw URL credential: %q", continuation.TaskDescription) + } + if strings.Contains(continuation.PlanSummary, secretPassword) { + t.Fatalf("PlanSummary retained the raw URL credential: %q", continuation.PlanSummary) + } + + prompt := ContinuationPrompt("do the task", continuation) + if strings.Contains(prompt, secretPassword) { + t.Fatalf("rendered prompt retained the raw URL credential: %q", prompt) + } +} + +// TestContinuationPreservesNonCredentialContentInUserAuthoredFields is the +// collateral-damage guard: the credential-only tier used for +// TaskDescription, PlanSummary, and RepositorySummary must not run the full +// Sanitize rule set, which would collapse a legitimate 40-char commit SHA (or +// any other 32+ char identifier) to "***". +func TestContinuationPreservesNonCredentialContentInUserAuthoredFields(t *testing.T) { + commitSHA := "bda95f25f3ef161764b0c710205e08fb13f3ef80" + if len(commitSHA) != 40 { + t.Fatalf("test fixture commitSHA is %d chars, want 40", len(commitSHA)) + } + + continuation := BuildBoundedContinuation(ContinuationInput{ + TaskDescription: "fixes regression introduced in " + commitSHA, + PlanSummary: "verified against " + commitSHA, + RepositorySummary: "HEAD at " + commitSHA, + }) + + if !strings.Contains(continuation.TaskDescription, commitSHA) { + t.Fatalf("TaskDescription mangled a non-credential commit SHA: %q", continuation.TaskDescription) + } + if !strings.Contains(continuation.PlanSummary, commitSHA) { + t.Fatalf("PlanSummary mangled a non-credential commit SHA: %q", continuation.PlanSummary) + } + if !strings.Contains(continuation.RepositorySummary, commitSHA) { + t.Fatalf("RepositorySummary mangled a non-credential commit SHA: %q", continuation.RepositorySummary) + } +} diff --git a/apps/backend/internal/agent/runtime/routingerr/sanitize.go b/apps/backend/internal/agent/runtime/routingerr/sanitize.go index 67790e2450..b54a489f15 100644 --- a/apps/backend/internal/agent/runtime/routingerr/sanitize.go +++ b/apps/backend/internal/agent/runtime/routingerr/sanitize.go @@ -11,36 +11,311 @@ const MaxRawExcerptBytes = 4096 const redactionMask = "***" -type redaction struct { - pattern *regexp.Regexp - replace string +// redaction transforms a string, applied in sequence by applyRedactions. +type redaction func(string) string + +// literalRedaction wraps a fixed regexp substitution as a redaction. +func literalRedaction(pattern, replace string) redaction { + re := regexp.MustCompile(pattern) + return func(s string) string { return re.ReplaceAllString(s, replace) } } -var redactions = []redaction{ +// credentialRedactions matches only credential-shaped patterns: API keys, +// personal access tokens, auth headers, and password/secret/token +// assignments. It excludes the URL rewrite, the opaque session-id rule, the +// 32-plus-char catch-all, and home-path normalization, all of which are safe +// to apply to provider diagnostics but would mangle unrelated content (commit +// SHAs, UUIDs, base64 samples, URLs with paths) in user-authored text. +var credentialRedactions = []redaction{ + literalRedaction(`sk-[A-Za-z0-9_-]{12,}`, "sk-"+redactionMask), + literalRedaction(`github_pat_[A-Za-z0-9_]{50,}`, "github_pat_"+redactionMask), + literalRedaction(`ghp_[A-Za-z0-9]{30,}`, "ghp_"+redactionMask), + // Kandev personal access tokens, including the ?token= form used by + // headerless WS clients. + literalRedaction(`kandev_pat_[A-Za-z0-9_]+`, "kandev_pat_"+redactionMask), + literalRedaction(`(?i)Bearer\s+[A-Za-z0-9._\-+/=]{20,}`, "Bearer "+redactionMask), + literalRedaction(`(?i)Authorization:\s*[^\r\n]+`, "Authorization: "+redactionMask), + literalRedaction(`--api-key[= ]\S+`, "--api-key "+redactionMask), + redactAssignments, + // URL userinfo (user:pass@host) carries a live credential even though the + // rest of the URL does not. Unlike the full Sanitize tier's URL rewrite, + // this masks only the userinfo and keeps the path and query intact. The + // scheme is not restricted to http(s): any scheme://user:pass@ form + // (postgres, mysql, redis, ...) carries the same live credential. The + // userinfo class includes '@' so the match backtracks to the LAST '@' + // before the next '/' or whitespace, matching how a real credential + // embedding a raw '@' actually terminates. + literalRedaction(`(?i)([a-z][a-z0-9+.-]*://)[^\s/?#]+@`, "$1"+redactionMask+"@"), +} + +var redactions = append(append([]redaction{ // Provider diagnostics may include account/workspace links and opaque // session identifiers. These are not useful recovery details and must not // cross the lifecycle or message boundaries. // Keep only the scheme and host. This preserves the existing safe-endpoint // contract used by MCP diagnostics while dropping paths, query strings, and // fragments that can carry account or workspace identifiers. - {regexp.MustCompile(`(https?://)(?:[^@\s/]+@)?([^/\s?#]+)[^\s]*`), "$1$2"}, - {regexp.MustCompile(`\b(?:wrk|ses|run)_[A-Za-z0-9_-]+\b`), "[redacted-id]"}, - {regexp.MustCompile(`sk-[A-Za-z0-9_-]{12,}`), "sk-" + redactionMask}, - {regexp.MustCompile(`github_pat_[A-Za-z0-9_]{50,}`), "github_pat_" + redactionMask}, - {regexp.MustCompile(`ghp_[A-Za-z0-9]{30,}`), "ghp_" + redactionMask}, - // Kandev personal access tokens, including the ?token= form used by - // headerless WS clients. The generic token/32-char rules below also catch - // these; this keeps the credential type visible in redacted logs. - {regexp.MustCompile(`kandev_pat_[A-Za-z0-9_]+`), "kandev_pat_" + redactionMask}, - {regexp.MustCompile(`(?i)Bearer\s+[A-Za-z0-9._\-+/=]{20,}`), "Bearer " + redactionMask}, - {regexp.MustCompile(`(?i)Authorization:\s*[^\r\n]+`), "Authorization: " + redactionMask}, - {regexp.MustCompile(`--api-key[= ]\S+`), "--api-key " + redactionMask}, - {regexp.MustCompile(`(?i)(password|secret|token)\s*[:=]\s*\S+`), "$1: " + redactionMask}, - {regexp.MustCompile(`[A-Za-z0-9+/=_-]{32,}`), redactionMask}, - {regexp.MustCompile(`/Users/[^/\s]+/`), "/Users//"}, - {regexp.MustCompile(`/home/[^/\s]+/`), "/home//"}, + literalRedaction(`(https?://)(?:[^@\s/]+@)?([^/\s?#]+)[^\s]*`, "$1$2"), + literalRedaction(`\b(?:wrk|ses|run)_[A-Za-z0-9_-]+\b`, "[redacted-id]"), +}, credentialRedactions...), []redaction{ + literalRedaction(`[A-Za-z0-9+/=_-]{32,}`, redactionMask), + literalRedaction(`/Users/[^/\s]+/`, "/Users//"), + literalRedaction(`/home/[^/\s]+/`, "/home//"), + redactLocalPaths, +}...) + +// credentialAssignmentKey matches a password/secret/token/api-key field name +// plus its `:`/`=` separator, but not the value: the value is consumed +// separately by scanValue, which needs to make quote- and delimiter-aware +// decisions a single regexp alternation cannot express reliably (see +// redactAssignments). The key's surrounding quote is optional but, when +// present, must match on both sides (`"key"` or `'key'`) — RE2 has no +// backreferences, so the three shapes (double-quoted, single-quoted, bare) +// are spelled out as separate alternatives instead of an independent +// `["']?` on each side, which could consume an opening quote with no +// corresponding closing quote and silently drop it from the output. +var credentialAssignmentKey = regexp.MustCompile(`(?i)(?:"([A-Za-z0-9_.-]*(?:password|secret|token|api[_-]?key)[A-Za-z0-9_.-]*)"|'([A-Za-z0-9_.-]*(?:password|secret|token|api[_-]?key)[A-Za-z0-9_.-]*)'|([A-Za-z0-9_.-]*(?:password|secret|token|api[_-]?key)[A-Za-z0-9_.-]*))\s*[:=]\s*`) + +// assignmentKeySpan returns the start/end offsets of whichever +// credentialAssignmentKey alternative (double-quoted, single-quoted, or +// bare) matched the key name in a FindAllStringSubmatchIndex result. +func assignmentKeySpan(loc []int) (int, int) { + for i := 1; i <= 3; i++ { + if start := loc[2*i]; start != -1 { + return start, loc[2*i+1] + } + } + return -1, -1 } +// isBareDecimalInteger reports whether s consists of one or more ASCII +// digits and nothing else. +func isBareDecimalInteger(s string) bool { + if s == "" { + return false + } + for i := 0; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { + return false + } + } + return true +} + +// isCountKey reports whether a credential-like assignment key is a known +// token-count field. Only these fields can preserve a bare decimal value. +func isCountKey(key string) bool { + switch strings.ToLower(key) { + case "tokens", "max_tokens", "input_tokens", "output_tokens", + "total_tokens", "prompt_tokens", "completion_tokens": + return true + default: + return false + } +} + +// redactAssignments replaces each password/secret/token/api-key assignment +// with its key name and a mask, consuming the value with scanValue so an +// embedded quote or leading structural delimiter cannot truncate the match +// early and leave part of the credential in cleartext. A bare decimal integer +// is preserved only for the explicit token-count keys, because the key pattern +// also matches non-credential fields that contain a keyword as a substring. +func redactAssignments(s string) string { + matches := credentialAssignmentKey.FindAllStringSubmatchIndex(s, -1) + if matches == nil { + return s + } + var b strings.Builder + last := 0 + for _, loc := range matches { + start, end := loc[0], loc[1] + if start < last { + // Already consumed as part of a preceding assignment's value. + continue + } + valueLen := scanValue(s[end:]) + keyStart, keyEnd := assignmentKeySpan(loc) + b.WriteString(s[last:start]) + if isCountKey(s[keyStart:keyEnd]) && isBareDecimalInteger(s[end:end+valueLen]) { + b.WriteString(s[start : end+valueLen]) + } else { + b.WriteString(s[keyStart:keyEnd]) + b.WriteString(": ") + b.WriteString(redactionMask) + } + last = end + valueLen + } + b.WriteString(s[last:]) + return b.String() +} + +// delimiterBytes identify closing structural characters that can follow a +// bare value. Commas and semicolons are handled only as part of a trailing +// structural suffix, because either can appear inside a credential value. +const delimiterBytes = "}])>" + +func isSpaceByte(c byte) bool { + switch c { + case ' ', '\t', '\n', '\r', '\f', '\v': + return true + default: + return false + } +} + +func isDelimiterByte(c byte) bool { + return strings.IndexByte(delimiterBytes, c) >= 0 +} + +// scanValue returns the byte length of the credential value starting at s[0]. +// A value starting with a quote is scanned by scanQuoted; anything else, +// including a value whose first character is itself a structural delimiter +// (`,`, `}`, ...), is scanned by scanBare so at least one character is always +// consumed. +func scanValue(s string) int { + if len(s) == 0 { + return 0 + } + if s[0] == '"' || s[0] == '\'' { + if n, ok := scanQuoted(s); ok { + return n + } + } + return scanBare(s) +} + +// scanBare consumes a bare value until whitespace. A closing delimiter at the +// end of the token is kept outside the value so JSON-like structure survives; +// a delimiter inside the token remains part of the value and cannot expose a +// suffix after redaction. +func scanBare(s string) int { + n := len(s) + i := 0 + for i < n { + c := s[i] + if isSpaceByte(c) { + break + } + i++ + } + if i == 0 { + return 0 + } + suffixStart := i + hasClosingDelimiter := false + for suffixStart > 0 { + c := s[suffixStart-1] + switch { + case isDelimiterByte(c): + hasClosingDelimiter = true + suffixStart-- + case c == ',' || c == ';': + suffixStart-- + default: + if hasClosingDelimiter && suffixStart > 0 { + return suffixStart + } + return i + } + } + return i +} + +// scanQuoted consumes a quoted value starting at s[0], honoring backslash +// escapes so an escaped quote does not end the string early. It returns +// ok=false if no closing quote is found before a newline or the end of s, so +// the caller falls back to bare scanning. +// +// A closing quote ends the value when it is followed by whitespace, the end +// of the string, or a trailing structural suffix. Otherwise the quote did not +// terminate the credential, and scanning continues to consume the value. +func scanQuoted(s string) (int, bool) { + n := len(s) + quote := s[0] + j := 1 + for j < n { + switch { + case s[j] == '\\' && j+1 < n: + j += 2 + case s[j] == quote: + j++ + return extendPastQuote(s, j), true + case s[j] == '\n' || s[j] == '\r': + return 0, false + default: + j++ + } + } + return 0, false +} + +// extendPastQuote is called once a closing quote is found at s[:j]. See +// scanQuoted for when the scan needs to continue past that quote. +func extendPastQuote(s string, j int) int { + n := len(s) + if j >= n || isSpaceByte(s[j]) { + return j + } + if isDelimiterByte(s[j]) { + if isTrailingStructuralSuffix(s[j:]) { + return j + } + return j + scanBare(s[j:]) + } + return j + scanContinuation(s[j:]) +} + +// isTrailingStructuralSuffix reports whether s starts with a closing +// delimiter and then contains only structural punctuation until whitespace or +// the end. Such a suffix belongs to the surrounding object, not the value. +func isTrailingStructuralSuffix(s string) bool { + if len(s) == 0 || !isDelimiterByte(s[0]) { + return false + } + hasClosingDelimiter := false + for i := 0; i < len(s) && !isSpaceByte(s[i]); i++ { + switch { + case isDelimiterByte(s[i]): + hasClosingDelimiter = true + case s[i] != ',' && s[i] != ';': + return false + } + } + return hasClosingDelimiter +} + +// scanContinuation scans the trailing content directly after a quote that +// turned out not to terminate the value. It behaves like scanBare (stopping +// at whitespace or a structural delimiter) but re-enters scanQuoted on +// another quote character, since that one might be the real terminator. +func scanContinuation(s string) int { + n := len(s) + i := 0 + for i < n { + c := s[i] + if isSpaceByte(c) { + return i + } + if c == '"' || c == '\'' { + if extra, ok := scanQuoted(s[i:]); ok { + return i + extra + } + i++ + continue + } + if isDelimiterByte(c) { + return i + } + i++ + } + return i +} + +// localUnixPathPattern and localWindowsPathPattern back redactLocalPaths, +// the last rule in the broad redactions tier: any absolute filesystem path +// mentioned in provider diagnostics can carry a workspace or account +// identifier, so it is collapsed to a fixed placeholder rather than only +// normalizing the well-known /Users and /home prefixes above. var ( localUnixPathPattern = regexp.MustCompile(`/(?:[^/\s"']+/)+[^\r\n\s"'<>]+`) localWindowsPathPattern = regexp.MustCompile(`(?i)[A-Z]:[\\/][^\r\n"']+`) @@ -91,22 +366,21 @@ func redactUnixPath(path string) string { } } -// Redact applies the credential and local-path rules without truncation. -// The function is idempotent: applying it twice equals applying it once. -func Redact(s string) string { - for _, r := range redactions { - s = r.pattern.ReplaceAllString(s, r.replace) +// applyRedactionsUnbounded runs rules over the entire input with no output +// cap, so every credential's key marker is guaranteed to stay attached to its +// full value regardless of length. The credential rules have no maximum +// matched length, so any caller that instead cut its input to a fixed window +// before redacting could split a marker from a value long enough to cross +// the window boundary, leaving the split-off remainder unredacted. +func applyRedactionsUnbounded(s string, rules []redaction) string { + for _, r := range rules { + s = r(s) } - s = redactLocalPaths(s) return s } -// Sanitize redacts likely credentials and local paths, then truncates -// to MaxRawExcerptBytes on a rune boundary so a multi-byte character (e.g. -// Vietnamese, CJK) is never split into invalid UTF-8. The function is -// idempotent: applying it twice equals applying it once. -func Sanitize(s string) string { - s = Redact(s) +func applyRedactions(s string, rules []redaction) string { + s = applyRedactionsUnbounded(s, rules) if len(s) > MaxRawExcerptBytes { cut := MaxRawExcerptBytes for cut > 0 && !utf8.RuneStart(s[cut]) { @@ -117,6 +391,35 @@ func Sanitize(s string) string { return s } +// Sanitize redacts likely credentials, normalizes home paths, and truncates +// to MaxRawExcerptBytes. The function is idempotent: applying it twice +// equals applying it once. Use this for provider stdout/stderr and other +// diagnostic text, where collapsing opaque identifiers and paths is +// acceptable collateral. +func Sanitize(s string) string { + return applyRedactions(s, redactions) +} + +// SanitizeCredentials redacts only credential-shaped patterns (API keys, +// tokens, auth headers, password/secret/token assignments) and truncates to +// MaxRawExcerptBytes. Unlike Sanitize, it leaves URLs, opaque IDs, and any +// other 32-plus-char run untouched, so it is safe to apply to user-authored +// text (a task description, a plan) that must not carry a live credential +// across a provider boundary but should otherwise survive intact. It is +// idempotent for the same reason Sanitize is. +func SanitizeCredentials(s string) string { + return applyRedactions(s, credentialRedactions) +} + +// SanitizeFullUnbounded applies the same rule set as Sanitize but without the +// MaxRawExcerptBytes output cap. Use this only when the caller will apply its +// own tail truncation afterward (see dynamic.sanitizedTail): sanitizing +// before cutting, rather than cutting a window and sanitizing that, is what +// guarantees a credential is never split from its key marker by the cut. +func SanitizeFullUnbounded(s string) string { + return applyRedactionsUnbounded(s, redactions) +} + type sanitizedError struct { cause error message string diff --git a/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go b/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go index b8e611085b..8f8b644e46 100644 --- a/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go +++ b/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go @@ -3,11 +3,37 @@ package routingerr import ( "errors" "fmt" + "math/rand" "strings" "testing" "unicode/utf8" ) +// fuzzIdempotenceIterations bounds the seeded idempotence fuzz tests (see +// TestSanitize_IdempotentFuzz / TestSanitizeCredentials_IdempotentFuzz). +const fuzzIdempotenceIterations = 20000 + +// credentialFuzzAlphabet is a small vocabulary of tokens combined randomly +// by randCredentialString. It is dense in the shapes redactAssignments and +// scanValue branch on — quotes, separators, structural delimiters, and +// keyword fragments — so a seeded run reliably explores the boundary cases a +// fixed golden list would only cover by accident. +var credentialFuzzAlphabet = []string{ + `"`, `'`, ":", "=", " ", "\t", "\n", ",", "}", ")", "]", ">", ";", `\`, + "password", "secret", "token", "api_key", "apikey", "api-key", + "max_tokens", "AWS_SECRET_ACCESS_KEY", + "a", "1", "2", "_", "-", ".", "@", "hunter2", "x", "kid-1234", +} + +func randCredentialString(rng *rand.Rand) string { + n := rng.Intn(12) + 1 + var b strings.Builder + for i := 0; i < n; i++ { + b.WriteString(credentialFuzzAlphabet[rng.Intn(len(credentialFuzzAlphabet))]) + } + return b.String() +} + func TestSanitize_RedactionsGolden(t *testing.T) { cases := []struct { name string @@ -57,6 +83,18 @@ func TestSanitize_RedactionsGolden(t *testing.T) { mustNotHave: []string{"hunter2-rocks"}, mustHave: []string{"password: ***"}, }, + { + name: "password value contains a quote", + in: "password=p@ss'word-tail", + mustNotHave: []string{"word-tail", "p@ss"}, + mustHave: []string{"password: ***"}, + }, + { + name: "secret value contains multiple quotes", + in: "secret=abc'def'ghi", + mustNotHave: []string{"abc", "def", "ghi"}, + mustHave: []string{"secret: ***"}, + }, { name: "user home path", in: "file at /Users/alice/work/repo/main.go failed", @@ -69,6 +107,156 @@ func TestSanitize_RedactionsGolden(t *testing.T) { mustNotHave: []string{"/home/bob/"}, mustHave: []string{"/home//"}, }, + { + name: "quoted value with unescaped trailing quote leaks tail", + in: `token: "a"LEAKED"`, + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"token: ***"}, + }, + { + name: "quoted value with embedded at-sign then trailing quote", + in: `password="p@ss"LEAKED"`, + mustNotHave: []string{"LEAKED", "p@ss"}, + mustHave: []string{"password: ***"}, + }, + { + name: "single-quoted value with trailing quote", + in: `secret='x'LEAKED'`, + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "JSON-escaped quote inside quoted value", + in: `{"password":"abc\"LEAKED_SUFFIX"}`, + mustNotHave: []string{"LEAKED_SUFFIX"}, + mustHave: []string{"{password: ***}"}, + }, + { + name: "bare value starting with comma delimiter", + in: "password=,LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"password: ***"}, + }, + { + name: "bare value starting with close brace", + in: "secret=}LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "bare value starting with semicolon", + in: "token=;LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"token: ***"}, + }, + { + name: "bare value starting with close paren", + in: "password=)LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"password: ***"}, + }, + { + name: "nested JSON quoted token value stops at closing brace", + in: `{"a": {"token": "hunter2"}, "b": 1}`, + mustNotHave: []string{"hunter2"}, + mustHave: []string{`{"a": {token: ***}, "b": 1}`}, + }, + { + name: "quoted value containing spaces", + in: `secret: "multi word secret"`, + mustNotHave: []string{"multi word secret"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "quoted JSON password value stops at closing brace", + in: `{"password": "hunter2"}`, + mustNotHave: []string{"hunter2"}, + mustHave: []string{"{password: ***}"}, + }, + { + name: "bare token value stops at trailing brace", + in: "token=hunter2}", + mustNotHave: []string{"hunter2"}, + mustHave: []string{"token: ***}"}, + }, + { + name: "bare value containing a comma is fully consumed", + in: "password=abc,def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"password: ***"}, + }, + { + name: "bare value containing a semicolon is fully consumed", + in: "token=abc;def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"token: ***"}, + }, + { + name: "bare value containing a closing paren is fully consumed", + in: "password=abc)def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"password: ***"}, + }, + { + name: "bare value containing a closing bracket is fully consumed", + in: "secret=abc]def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "bare value containing a closing angle bracket is fully consumed", + in: "token=abc>def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"token: ***"}, + }, + { + name: "numeric password is redacted", + in: "password=123456", + mustNotHave: []string{"123456"}, + mustHave: []string{"password: ***"}, + }, + { + name: "numeric token is redacted", + in: "token=123456", + mustNotHave: []string{"123456"}, + mustHave: []string{"token: ***"}, + }, + { + name: "numeric api key is redacted", + in: "api_key=123456", + mustNotHave: []string{"123456"}, + mustHave: []string{"api_key: ***"}, + }, + { + name: "numeric qualified secret key is redacted", + in: "AWS_SECRET_ACCESS_KEY=123456", + mustNotHave: []string{"123456"}, + mustHave: []string{"AWS_SECRET_ACCESS_KEY: ***"}, + }, + { + name: "max_tokens numeric value is not redacted", + in: "max_tokens: 8192 > 4096, which is the maximum allowed for this model", + mustNotHave: []string{"***"}, + mustHave: []string{"max_tokens: 8192 > 4096"}, + }, + { + name: "input_tokens and output_tokens numeric values are not redacted", + in: "input_tokens=12000 output_tokens=900 total cost 0.12", + mustNotHave: []string{"***"}, + mustHave: []string{"input_tokens=12000 output_tokens=900"}, + }, + { + name: "max_tokens numeric value in parens is not redacted", + in: "context window exceeded (max_tokens=4096)", + mustNotHave: []string{"***"}, + mustHave: []string{"(max_tokens=4096)"}, + }, + { + name: "tokens numeric value is not redacted", + in: "tokens: 500 remaining", + mustNotHave: []string{"***"}, + mustHave: []string{"tokens: 500 remaining"}, + }, { name: "temporary workspace path", in: "file at /tmp/kandev/task/repo/main.go failed", @@ -111,6 +299,24 @@ func TestSanitize_RedactionsGolden(t *testing.T) { } } +// TestSanitize_IdempotentFuzz complements TestSanitize_Idempotent's fixed +// golden list with a seeded fuzz over a credential-flavoured alphabet: a +// golden list only proves idempotence for the handful of inputs it +// enumerates, and a prior regression (an unbalanced optional quote consumed +// by the key match but never re-emitted) shrank output by one byte on the +// second pass for inputs the golden list did not happen to cover. +func TestSanitize_IdempotentFuzz(t *testing.T) { + rng := rand.New(rand.NewSource(1)) + for i := 0; i < fuzzIdempotenceIterations; i++ { + s := randCredentialString(rng) + first := Sanitize(s) + second := Sanitize(first) + if first != second { + t.Fatalf("Sanitize not idempotent for %q: first=%q second=%q", s, first, second) + } + } +} + func TestSanitize_Idempotent(t *testing.T) { inputs := []string{ "plain text", @@ -126,7 +332,7 @@ func TestSanitize_Idempotent(t *testing.T) { first := Sanitize(in) second := Sanitize(first) if first != second { - t.Fatalf("sanitize not idempotent for %q: first=%q second=%q", in, first, second) + t.Fatalf("Sanitize not idempotent for %q: first=%q second=%q", in, first, second) } } } @@ -156,6 +362,275 @@ func TestSanitize_TruncationBoundary(t *testing.T) { } } +func TestSanitizeCredentials_RedactsCredentialPatterns(t *testing.T) { + cases := []struct { + name string + in string + mustNotHave []string + mustHave []string + }{ + { + name: "anthropic-style key", + in: "use sk-abcdef1234567890QQQQ to call", + mustNotHave: []string{"sk-abcdef1234567890"}, + mustHave: []string{"sk-***"}, + }, + { + name: "github classic pat", + in: "token ghp_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA leaks", + mustNotHave: []string{"ghp_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"}, + mustHave: []string{"ghp_***"}, + }, + { + name: "github fine-grained pat", + in: "use github_pat_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMN here", + mustNotHave: []string{"github_pat_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMN"}, + mustHave: []string{"github_pat_***"}, + }, + { + name: "kandev pat", + in: "auth via kandev_pat_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 succeeded", + mustNotHave: []string{"kandev_pat_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"}, + mustHave: []string{"kandev_pat_***"}, + }, + { + name: "bearer token", + in: "header: Bearer abcdefghij1234567890XYZ", + mustNotHave: []string{"abcdefghij1234567890XYZ"}, + mustHave: []string{"Bearer ***"}, + }, + { + name: "authorization header", + in: "Authorization: token ZZZZZZZZZZZZ\nnext", + mustNotHave: []string{"ZZZZZZZZZZZZ"}, + mustHave: []string{"Authorization: ***"}, + }, + { + name: "api-key flag", + in: "--api-key=AAAAAAAAAAAAAAAA --other", + mustNotHave: []string{"AAAAAAAAAAAAAAAA"}, + mustHave: []string{"--api-key ***"}, + }, + { + name: "password=value", + in: "password=hunter2-rocks", + mustNotHave: []string{"hunter2-rocks"}, + mustHave: []string{"password: ***"}, + }, + { + name: "url userinfo credential", + in: "clone from https://alice:s3cr3tpassw0rd@example.test/api", + mustNotHave: []string{"s3cr3tpassw0rd", "alice:s3cr3tpassw0rd@"}, + mustHave: []string{"https://***@example.test/api"}, + }, + { + name: "api_key query param", + in: "callback https://example.test/callback?api_key=abc123def456", + mustNotHave: []string{"abc123def456"}, + mustHave: []string{"api_key: ***"}, + }, + { + name: "non-http scheme userinfo credential", + in: "repro via postgres://admin:hunter2@db.internal:5432/app", + mustNotHave: []string{"hunter2", "admin:hunter2@"}, + mustHave: []string{"postgres://***@db.internal:5432/app"}, + }, + { + name: "quoted JSON password field", + in: `{"password": "hunter2-rocks"}`, + mustNotHave: []string{"hunter2-rocks"}, + mustHave: []string{"password: ***"}, + }, + { + name: "secret=value", + in: "secret=hunter2-rocks", + mustNotHave: []string{"hunter2-rocks"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "token=value", + in: "token=hunter2-rocks", + mustNotHave: []string{"hunter2-rocks"}, + mustHave: []string{"token: ***"}, + }, + { + name: "password value contains a quote", + in: "password=p@ss'word-tail", + mustNotHave: []string{"word-tail", "p@ss"}, + mustHave: []string{"password: ***"}, + }, + { + name: "secret value contains multiple quotes", + in: "secret=abc'def'ghi", + mustNotHave: []string{"abc", "def", "ghi"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "qualified env var key AWS_SECRET_ACCESS_KEY", + in: "AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMIK7MDENGbPxRfiCYEXAMPLEKEY", + mustNotHave: []string{"wJalrXUtnFEMIK7MDENGbPxRfiCYEXAMPLEKEY"}, + mustHave: []string{"***"}, + }, + { + name: "qualified env var key SECRET_KEY", + in: "SECRET_KEY=django-insecure-abc123def456ghi789", + mustNotHave: []string{"django-insecure-abc123def456ghi789"}, + mustHave: []string{"***"}, + }, + { + name: "quoted value with unescaped trailing quote leaks tail", + in: `token: "a"LEAKED"`, + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"token: ***"}, + }, + { + name: "quoted value with embedded at-sign then trailing quote", + in: `password="p@ss"LEAKED"`, + mustNotHave: []string{"LEAKED", "p@ss"}, + mustHave: []string{"password: ***"}, + }, + { + name: "single-quoted value with trailing quote", + in: `secret='x'LEAKED'`, + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "JSON-escaped quote inside quoted value", + in: `{"password":"abc\"LEAKED_SUFFIX"}`, + mustNotHave: []string{"LEAKED_SUFFIX"}, + mustHave: []string{"{password: ***}"}, + }, + { + name: "bare value starting with comma delimiter", + in: "password=,LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"password: ***"}, + }, + { + name: "bare value starting with close brace", + in: "secret=}LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "bare value starting with semicolon", + in: "token=;LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"token: ***"}, + }, + { + name: "bare value starting with close paren", + in: "password=)LEAKED", + mustNotHave: []string{"LEAKED"}, + mustHave: []string{"password: ***"}, + }, + { + name: "url userinfo with multiple at-signs leaks tail past first", + in: "postgres://alice:prefix@LEAKED_SUFFIX@db.internal/app", + mustNotHave: []string{"LEAKED_SUFFIX", "alice:prefix"}, + mustHave: []string{"postgres://***@db.internal/app"}, + }, + { + name: "nested JSON quoted token value stops at closing brace", + in: `{"a": {"token": "hunter2"}, "b": 1}`, + mustNotHave: []string{"hunter2"}, + mustHave: []string{`{"a": {token: ***}, "b": 1}`}, + }, + { + name: "quoted value containing spaces", + in: `secret: "multi word secret"`, + mustNotHave: []string{"multi word secret"}, + mustHave: []string{"secret: ***"}, + }, + { + name: "quoted JSON password value stops at closing brace", + in: `{"password": "hunter2"}`, + mustNotHave: []string{"hunter2"}, + mustHave: []string{"{password: ***}"}, + }, + { + name: "bare token value stops at trailing brace", + in: "token=hunter2}", + mustNotHave: []string{"hunter2"}, + mustHave: []string{"token: ***}"}, + }, + { + name: "bare value containing a comma is fully consumed", + in: "password=abc,def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"password: ***"}, + }, + { + name: "bare value containing a semicolon is fully consumed", + in: "token=abc;def", + mustNotHave: []string{"abc", "def"}, + mustHave: []string{"token: ***"}, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := SanitizeCredentials(c.in) + for _, bad := range c.mustNotHave { + if strings.Contains(got, bad) { + t.Fatalf("expected %q to be redacted, got %q", bad, got) + } + } + for _, good := range c.mustHave { + if !strings.Contains(got, good) { + t.Fatalf("expected %q in output, got %q", good, got) + } + } + }) + } +} + +// TestSanitizeCredentials_PreservesNonCredentialContent is the collateral- +// damage guard: SanitizeCredentials must not run the 32+ char catch-all, the +// URL path/query rewrite, or the home-path normalization, so commit SHAs, +// UUIDs, and URLs with paths survive intact. +func TestSanitizeCredentials_PreservesNonCredentialContent(t *testing.T) { + cases := []struct { + name string + in string + }{ + {name: "40-char commit sha", in: "fixed in 94e7b02458b6c1a2d3e4f5061728394a5b6c7d8"}, + {name: "uuid", in: "task id 3f9a1c2e-4b5d-4e6f-8a9b-0c1d2e3f4a5b"}, + {name: "base64 sample", in: "payload: aGVsbG8gd29ybGQgdGhpcyBpcyBhIHRlc3Q="}, + {name: "url with path and query", in: "see https://example.com/org/repo/pull/123?tab=files for details"}, + {name: "url query email", in: "see https://app.example.test?email=alice@example.test for details"}, + {name: "url fragment email", in: "see https://app.example.test#alice@example.test for details"}, + {name: "user home path", in: "file at /Users/alice/work/repo/main.go failed"}, + {name: "max_tokens numeric value is not redacted", in: "max_tokens: 8192 > 4096, which is the maximum allowed for this model"}, + {name: "input_tokens and output_tokens numeric values are not redacted", in: "input_tokens=12000 output_tokens=900 total cost 0.12"}, + {name: "max_tokens numeric value in parens is not redacted", in: "context window exceeded (max_tokens=4096)"}, + {name: "tokens numeric value is not redacted", in: "tokens: 500 remaining"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := SanitizeCredentials(c.in) + if got != c.in { + t.Fatalf("expected content to survive unchanged, got %q from input %q", got, c.in) + } + }) + } +} + +// TestSanitizeCredentials_IdempotentFuzz is SanitizeCredentials' counterpart +// to TestSanitize_IdempotentFuzz; see its comment for why this replaced a +// golden list. +func TestSanitizeCredentials_IdempotentFuzz(t *testing.T) { + rng := rand.New(rand.NewSource(2)) + for i := 0; i < fuzzIdempotenceIterations; i++ { + s := randCredentialString(rng) + first := SanitizeCredentials(s) + second := SanitizeCredentials(first) + if first != second { + t.Fatalf("SanitizeCredentials not idempotent for %q: first=%q second=%q", s, first, second) + } + } +} + // TestSanitize_TruncatesOnRuneBoundary proves Sanitize never splits a // multi-byte rune when cutting to MaxRawExcerptBytes. Vietnamese (3-byte) and // CJK (3-byte) runes are repeated at every byte alignment relative to diff --git a/docs/specs/platform/system-design/provider-error-recovery.md b/docs/specs/platform/system-design/provider-error-recovery.md index 0250050847..8c2aee8af1 100644 --- a/docs/specs/platform/system-design/provider-error-recovery.md +++ b/docs/specs/platform/system-design/provider-error-recovery.md @@ -409,6 +409,57 @@ an earlier decision occurred. Raw streams, credentials, account identifiers, and unbounded error text are not stored in policy or route state. +### Continuation package sanitization tiers + +The dynamic-routing continuation package (`dynamic.BuildBoundedContinuation`, +persisted as `dynamic_route_states.continuation_json` and rendered into the +successor's prompt by `ContinuationPrompt`) mixes two kinds of carrier text, +and `routingerr` sanitizes them with two different rule sets: + +- **Provider diagnostics and the full `Conversation`** (`ToolSummary`, + `FailureReason`, and both the user and agent halves of `Conversation`) run + through `routingerr.Sanitize`, the full rule set. In addition to credential + patterns it collapses any 32-plus-character run, rewrites URLs down to + scheme and host, and normalizes home paths. `Conversation` is sanitized + before `sanitizedTail` applies its byte budget, so an identifier cannot + split a credential rule at the retained-tail boundary. +- **User/agent-authored carrier text** (`TaskDescription`, `PlanSummary`, and + `RepositorySummary`) runs through `routingerr.SanitizeCredentials`, a + narrower tier covering only credential-shaped patterns (`sk-`, `ghp_`, + `github_pat_`, `kandev_pat_`, `Bearer`, `Authorization:`, `--api-key`, + `password|secret|token|api_key` assignments, and URL userinfo). Each field is + bounded to `continuationFieldLimit` after redaction, with the head retained. + The narrow tier excludes + the 32-plus-char catch-all, the scheme-and-host URL rewrite, and home-path + normalization, because this text is already shown to the user unredacted + and commonly carries legitimate long identifiers — commit SHAs, UUIDs, file + hashes — that the full rule set would render as `***`. + +Both tiers close the credential shapes they explicitly pattern-match: a `sk-` +key, a GitHub/Kandev PAT, a bearer/auth header, a `--api-key` flag, a +`password`/`secret`/`token`/`api_key` assignment — including a qualified +env-var key where the keyword is a prefix or suffix rather than the whole +name (`AWS_SECRET_ACCESS_KEY=`, `SECRET_KEY=`), and including a value that +contains an embedded quote character — or a URL's `user:pass@` userinfo is +redacted before persistence or a cross-provider fallback, in either tier. A +quoted value not terminated on the same line (an embedded, unescaped newline +before the closing quote) is redacted only up to the line break — unchanged +from the plain-text matching this replaced. Neither tier is a general secret +scanner: a credential shaped like something not on that list (a +vendor-specific token prefix, for example) survives the narrow tier unless it +also matches a listed pattern. + +The key match is a substring match, not exact-name, so it also matches a key +merely containing a keyword without naming a credential (`max_tokens`, +`tokenizer`); an exact-name allowlist would drop the qualified env-var keys +(`AWS_SECRET_ACCESS_KEY=`) this match exists to catch. A bare decimal integer +value is left untouched only for the explicit count-key allowlist +(`tokens`, `max_tokens`, `input_tokens`, `output_tokens`, `total_tokens`, +`prompt_tokens`, and `completion_tokens`). Numeric values under credential keys, +such as `password: 123456` or `api_key=123456`, are redacted like other +credential values. A non-numeric value (`tokenizer: cl100k_base`) is also +redacted. + ## API surface - Dynamic profile CRUD accepts and returns the versioned per-class policy