diff --git a/src/plugins/tool-result-secret-scrub.test.ts b/src/plugins/tool-result-secret-scrub.test.ts index ae601fb1e..c98df19a3 100644 --- a/src/plugins/tool-result-secret-scrub.test.ts +++ b/src/plugins/tool-result-secret-scrub.test.ts @@ -42,6 +42,63 @@ MIIEpAIBAAKCAQEA7 "See docs/authentication.md for how API keys are issued and rotated. No secrets in this paragraph."; expect(scrubSecretShapedContent(text)).toBe(text); }); + + test("redacts bare-KEY env assignments (CL-10756)", () => { + for (const key of ["OPENAI_KEY", "STRIPE_KEY", "GITHUB_KEY"]) { + const text = `./app/.env:7:${key}=proj-live-value-9z`; + const out = scrubSecretShapedContent(text); + expect(out).toContain(`${key}=${CREDENTIAL_REDACTION}`); + expect(out).not.toContain("proj-live-value-9z"); + } + }); + + test("redacts credential-bearing DATABASE_URL connection strings (CL-10756)", () => { + const text = + "./app/.env:2:DATABASE_URL=postgres://admin:s3cret-pw@db.internal:5432/appdb"; + const out = scrubSecretShapedContent(text); + expect(out).toContain(`DATABASE_URL=${CREDENTIAL_REDACTION}`); + expect(out).not.toContain("s3cret-pw"); + }); + + test("leaves non-secret KEY words and credential-less URLs alone (CL-10756)", () => { + expect(scrubSecretShapedContent("MONKEY=banana")).toBe("MONKEY=banana"); + expect(scrubSecretShapedContent("DATABASE_URL=sqlite:///local.db")).toBe( + "DATABASE_URL=sqlite:///local.db", + ); + }); + + test("leaves scoped npm URLs and email-query URLs readable", () => { + const registry = "REGISTRY_URL=https://registry.npmjs.org/@scope/pkg"; + const docs = "DOCS_URL=https://medium.com/@octocat/hello"; + const support = + "SUPPORT_URL=https://example.com/help?email=dev@example.com"; + expect(scrubSecretShapedContent(registry)).toBe(registry); + expect(scrubSecretShapedContent(docs)).toBe(docs); + expect(scrubSecretShapedContent(support)).toBe(support); + }); + + test("redacts user:pass@host connection URLs", () => { + const text = "DATABASE_URL=postgres://admin:foo@db.internal:5432/appdb"; + const out = scrubSecretShapedContent(text); + expect(out).toBe(`DATABASE_URL=${CREDENTIAL_REDACTION}`); + expect(out).not.toContain("admin:foo"); + }); + + test("redacts the whole env value when it contains its own key", () => { + const text = "OPENAI_KEY=proj-OPENAI_KEY-live-value-9z"; + const out = scrubSecretShapedContent(text); + expect(out).toBe(`OPENAI_KEY=${CREDENTIAL_REDACTION}`); + expect(out).not.toContain("proj-"); + expect(out).not.toContain("live-value-9z"); + }); + + test("redacts Password= DSNs that lack a scheme", () => { + const text = + "DATABASE_URL=Server=db.internal;Database=appdb;User Id=admin;Password=s3cret-pw;"; + const out = scrubSecretShapedContent(text); + expect(out).toBe(`DATABASE_URL=${CREDENTIAL_REDACTION}`); + expect(out).not.toContain("s3cret-pw"); + }); }); describe("scrubSecretShapedValue", () => { @@ -219,6 +276,24 @@ describe("toolResultSecretScrubPlugin", () => { expect(result.content).toContain(CREDENTIAL_REDACTION); expect(result.content).not.toContain("supersecretvalue"); }); + test("scrubs bare-KEY and DATABASE_URL through the shared middleware path (CL-10756)", async () => { + const plugin = toolResultSecretScrubPlugin(); + if (plugin.middleware === undefined) + throw new Error("expected middleware plugin"); + const handler = plugin.middleware( + next( + "secrets/.env:7:OPENAI_KEY=proj-live-value-9z\nsecrets/.env:2:DATABASE_URL=postgres://admin:s3cret-pw@db.internal:5432/appdb", + ), + ); + const result = await handler( + { id: "c2", name: "read_file", arguments: { path: "secrets/.env" } }, + new AbortController().signal, + ); + expect(result.content).toContain(`OPENAI_KEY=${CREDENTIAL_REDACTION}`); + expect(result.content).toContain(`DATABASE_URL=${CREDENTIAL_REDACTION}`); + expect(result.content).not.toContain("proj-live-value-9z"); + expect(result.content).not.toContain("s3cret-pw"); + }); test("scrubs error results without stringifying object content", async () => { const plugin = toolResultSecretScrubPlugin(); if (plugin.middleware === undefined) diff --git a/src/plugins/tool-result-secret-scrub.ts b/src/plugins/tool-result-secret-scrub.ts index 9fffe104e..fea0d5741 100644 --- a/src/plugins/tool-result-secret-scrub.ts +++ b/src/plugins/tool-result-secret-scrub.ts @@ -27,17 +27,51 @@ const JSON_CREDENTIAL_FIELD = // Grep/shell lines often look like path:line:KEY=value const ENV_ASSIGNMENT = /(?:^|:)([A-Z][A-Z0-9_]+)=([^\n]+)/gm; -// Connection-string keys (DATABASE_URL and friends) are deliberately NOT -// matched here. Widening this shape-classifier would redact every benign -// connection string in tool output — a false-positive blast radius on a scrub -// path, not a prompt path. The gap stays documented, not silently fixed. -function isSecretEnvKey(key: string): boolean { +// Connection-string env names (DATABASE_URL and friends). Redaction additionally +// requires a credential-bearing value (see carriesConnectionCredentials), so +// benign sqlite paths and bare hostnames in tool output stay readable. +function isConnectionStringEnvKey(key: string): boolean { return ( - key === "API_KEY" || - /(?:SECRET|TOKEN|PASSWORD|PRIVATE|CREDENTIAL|AUTH)/.test(key) + /(?:_|^)(?:URL|URI|DSN)$/.test(key) || key.includes("CONNECTION_STRING") ); } +// True when a connection-string value actually carries credentials: URI +// userinfo of the form scheme://user:pass@host (a colon in the authority +// before @, and before the next / or ?), or embedded password= material. +// ADO-style DSNs carry Password= without a scheme, so the password check +// does not require ://. An @ in the path or query (npm scopes, profile +// URLs, email query params) is not userinfo. +function carriesConnectionCredentials(value: string): boolean { + return /password=/i.test(value) || hasCredentialUserinfo(value); +} + +function hasCredentialUserinfo(value: string): boolean { + const schemeSep = value.indexOf("://"); + if (schemeSep < 0) return false; + const rest = value.slice(schemeSep + 3); + const path = rest.indexOf("/"); + const query = rest.indexOf("?"); + let authorityEnd = rest.length; + if (path >= 0) authorityEnd = Math.min(authorityEnd, path); + if (query >= 0) authorityEnd = Math.min(authorityEnd, query); + const authority = rest.slice(0, authorityEnd); + const at = authority.indexOf("@"); + if (at < 0) return false; + return authority.slice(0, at).includes(":"); +} + +function isSecretEnvKey(key: string, value: string): boolean { + if (/(?:SECRET|TOKEN|PASSWORD|PRIVATE|CREDENTIAL|AUTH)/.test(key)) { + return true; + } + // Bare-KEY class (OPENAI_KEY, STRIPE_KEY, API_KEY): ENV-assignment scope + // only. The (^|_) boundary keeps plain words ending in KEY (MONKEY) out. + if (/(?:^|_)KEY$/.test(key)) return true; + if (isConnectionStringEnvKey(key)) return carriesConnectionCredentials(value); + return false; +} + function replaceAll( text: string, pattern: RegExp, @@ -65,12 +99,15 @@ export function scrubSecretShapedContent(text: string): string { `${prefix}${CREDENTIAL_REDACTION}${suffix}`, ); - result = result.replace(ENV_ASSIGNMENT, (match, key: string) => { - if (!isSecretEnvKey(key)) return match; - const prefixEnd = match.lastIndexOf(key); - const prefix = match.slice(0, prefixEnd); - return `${prefix}${key}=${CREDENTIAL_REDACTION}`; - }); + result = result.replace( + ENV_ASSIGNMENT, + (match, key: string, value: string) => { + if (!isSecretEnvKey(key, value)) return match; + const assignment = `${key}=${value}`; + const prefix = match.slice(0, match.length - assignment.length); + return `${prefix}${key}=${CREDENTIAL_REDACTION}`; + }, + ); return result; }