Merge pull request #1476 from corbitsdev/cl-10756-secret-scrub-key-class

fix(plugins): widen secret-scrub to bare-KEY and credentialed URLs

Author: TheGreatAxios

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)

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;
 }