Stop the secrets check flagging code that reads an environment variable (GST-1894) (#4157)

* Stop the secrets check flagging code that reads an environment variable (GST-1894)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Keep the new secrets test rows out of the line-start credential shape

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Give the fixture-label exemption only to files named like tests

A production file such as ab-test.ts or load-test.ts matched the test-file pattern, so a weak real password in it stopped being flagged. The pattern now matches only the documented forms: *.test.*, *.spec.*, *_test.* and test_*.py.

Review-Finding: p1/red-team#1 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Keep flagging a hardcoded fallback in backticks or on the next line

The env-read exemption only looked for a quoted fallback on the same line, so token = opts?.env?.API_TOKEN ?? followed by a backtick literal, or by a literal wrapped onto the next line, passed the secrets check. Backtick literals now count, and the next line is read when the expression continues there. The changelog now says plainly that an ALL-CAPS literal used as the variable name still passes.

Review-Finding: voices/outside-voice#1 reader=outside-voice
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Exempt an env read only when its line has no fallback, and stop reading the next line

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Say in the changelog that quoted lowercase env names now fail the secrets check

Review-Finding: p1/red-team#1 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

---------

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

changelog.d/stop-secrets-check-flagging-env-var-reads.md

@@ -0,0 +1 @@
+- **The preflight secrets check no longer fails a branch for code that reads an environment variable, or for a self-describing fixture label in a test file.** `env.kv` already let `process.env.X` through, but `token = opts?.env?.AGENT_API_TOKEN;`, `ctx.env.X;`, `import.meta.env.X;`, `process.env["X"];` and `System.getenv("X");` read as credential values and failed `gstack-pregate` (yc-software/qm, GST-1894). An `env` member of any object read by an ALL-CAPS name, and an indexed or call read whose argument is a quoted ALL-CAPS name, are now code, but only when the line ends the expression with `;`, `,` or `)` and holds no fallback or default (`??`, `||`, `or`, a ternary `:`, or a second argument). Any other line, including one whose fallback follows a comment, a blank line or a line break, gets no new exemption. In `*.test.*`, `*.spec.*`, `test_*.py` and `*_test.*` files only, a value made only of lowercase words with a credential word among them (`"personal-secret"`, `"staging-api-key"`) is a fixture label. A literal fallback, the same label outside a test file, and every known key prefix still fail. A quoted name that isn't ALL-CAPS in an indexed or call read now fails where it used to pass: a lowercase name such as `os.environ["db_password"]`, and a literal used as the name such as `os.environ["<secret>"]`, which passed because the check skipped the quote the match had taken.

lib/redact-patterns.ts

@@ -690,12 +690,43 @@ function urlPasswordIsPlaceholder(span: string): boolean {
 const POSTGRES_LOOPBACK_DEFAULT =
   /^postgres(?:ql)?:\/\/postgres:postgres@(?:localhost|127\.0\.0\.1)(?::\d{1,5})?(?![\w.:@%-])/;
 
-/** A value span that is only an environment-variable read expression (#2912). */
+/**
+ * A value span that is only an environment-variable read expression (#2912):
+ * `process.env.X`, an `env` member of any object chain read by an ALL-CAPS
+ * name (`opts?.env?.AGENT_API_TOKEN`, `import.meta.env.VITE_X`, GST-1894),
+ * or the opening of an indexed or call read (`process.env[`, `os.environ[`,
+ * `os.getenv(`, `System.getenv(`). `env` must be a whole segment, so
+ * `myenv.X` is a value.
+ */
 const ENV_READ_SPAN =
+  /^(?:os\.environ\[|os\.environ\.get\(|(?:[A-Za-z_$][\w$]*\.)?getenv\(|ENV\[|process\.env\.[A-Za-z_$][\w$]*[;,)]?|(?:[A-Za-z_$][\w$]*[!?]?\.)*env(?:[!?]?\.[A-Z_][A-Z0-9_]*[;,)]?|(?:\?\.)?\[))$/;
+/** The reads #2912 exempted before GST-1894; a line with a fallback keeps exactly this exemption. */
+const ENV_READ_SPAN_2912 =
   /^(?:os\.environ\[|os\.environ\.get\(|os\.getenv\(|getenv\(|ENV\[|process\.env\.[A-Za-z_$][\w$]*[;,)]?)$/;
+/** The quoted ALL-CAPS variable name an indexed or call read opens with (`"AGENT_API_TOKEN"]`, `'DB_PASSWORD',`). */
+const ENV_NAME_ARGUMENT = /^(["'])[A-Z_][A-Z0-9_]*\1/;
+/** A fallback or default after the read: `??`, `||`, `or` or a ternary `:`, anywhere on the line. */
+const ENV_READ_FALLBACK = /\?\?|\|\||(?<![\w$])or(?![\w$])|:/;
+/**
+ * The rest of the line is read from the span's end, so a quote the match took
+ * is still seen: a literal fallback (`os.getenv("X", "<secret>")`) or a
+ * literal used as the name (`os.environ["<secret>"]`) keeps the finding.
+ * GST-1894's wider reads (`opts?.env?.X`, `process.env["X"]`,
+ * `System.getenv("X")`) are exempt only when the line holds no fallback, no
+ * second argument, and ends the expression with `;`, `,` or `)`. Any other
+ * line reads exactly as #2912 did, so a fallback after a comment, a blank line
+ * or on the next line is never exempted by the wider reads. The next line is
+ * never read.
+ */
 function isBareEnvRead(span: string, match: RegExpExecArray): boolean {
-  if (!ENV_READ_SPAN.test(span)) return false;
-  return !carriesSecretLiteral(match.input.slice(match.index + match[0].length).split("\n", 1)[0]);
+  const opensArgument = /[[(]$/.test(span);
+  const line = match.input.slice(spanBounds(match).end).split("\n", 1)[0];
+  const rest = opensArgument ? line.replace(ENV_NAME_ARGUMENT, "") : line;
+  if (ENV_READ_FALLBACK.test(rest) || (opensArgument && /^[ \t]*,/.test(rest)))
+    return ENV_READ_SPAN_2912.test(span) && !carriesSecretLiteral(rest);
+  if (!ENV_READ_SPAN.test(span) || carriesSecretLiteral(rest)) return false;
+  const after = opensArgument ? rest.replace(/^[\])]/, "") : rest;
+  return ENV_READ_SPAN_2912.test(span) || /[;,)]$/.test(span) || /^[ \t]*[;,)]/.test(after);
 }
 
 /** True when `text` holds a quoted, non-placeholder, high-entropy literal. */
@@ -732,6 +763,20 @@ const SCHEMA_FIELD_VALUE = /^[a-z][a-z0-9]*(?:[_.-][a-z][a-z0-9]*)+$/;
 function isSchemaFieldIdentifier(span: string, match: RegExpExecArray): boolean {
   return SCHEMA_FIELD_NAME.test(match[0]) && SCHEMA_FIELD_VALUE.test(span);
 }
+/**
+ * GST-1894: in a test file, a value made only of lowercase words joined by `-`
+ * or `_`, one of them a credential word (`personal-secret`,
+ * `staging-api-key`), is a fixture label that says what it stands for.
+ * Decided by the file name (`*.test.ts`, `*.spec.tsx`, `test_*.py`,
+ * `*_test.go`). Outside a test file, without a path, or with a digit, an
+ * uppercase letter or no credential word, the value is checked as before.
+ */
+const TEST_SOURCE = /(?:^|[\\/])(?:[^\\/]+(?:\.test|\.spec|_test)\.(?:[cm]?[jt]sx?|py|go|rb|dart)|test_[^\\/]+\.py)$/;
+const FIXTURE_LABEL = /^[a-z]+(?:[-_][a-z]+)+$/;
+const FIXTURE_CREDENTIAL_WORD = /(?:^|[-_])(?:secret|token|password|passwd|key|credential)s?(?:[-_]|$)/;
+function isTestFixtureLabel(span: string, sourcePath: string | undefined): boolean {
+  return !!sourcePath && TEST_SOURCE.test(sourcePath) && FIXTURE_LABEL.test(span) && FIXTURE_CREDENTIAL_WORD.test(span);
+}
 /**
  * GST-1064: the first argument of a lookup call (`get`, `getenv`, `read`,
  * `load`, `loads`, `open`, `pop`, `fetch`) that is a lowercase file name
@@ -1482,6 +1527,7 @@ export const PATTERNS: RedactPattern[] = [
     // #2899: a function call assigned to the name is code, not a value (see
     // isCallExpression). #3048, #3630: so is a TS/JSX/Dart/Python type or expression (isSourceExpression).
     // #3208: so is a schema field identifier (isSchemaFieldIdentifier).
+    // GST-1894: so is a self-named fixture label in a test file (isTestFixtureLabel).
     validate: (span, match, opts) =>
       isCredentialShapedEnvName(match[0]) &&
       !isPlaceholderSpan(span) &&
@@ -1490,6 +1536,7 @@ export const PATTERNS: RedactPattern[] = [
       !isCallExpression(span, match) &&
       !isSourceExpression(span, match, opts?.sourcePath) &&
       !isSchemaFieldIdentifier(span, match) &&
+      !isTestFixtureLabel(span, opts?.sourcePath) &&
       shannonEntropy(span) >= 3.0,
   },
   {

test/pregate-secrets-source-context.test.ts

@@ -165,6 +165,17 @@ describe('pregate secrets: source context and diff integrity', () => {
     } finally { cleanup(fx); }
   });
 
+  test('a test file reading an env var in code and holding a self-named fixture label passes; a literal beside the read or in a .env file still fails (GST-1894)', () => {
+    const token = '8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ';
+    const qm = 'let token: string | undefined;\nfunction start(opts?: { env?: Record<string, string> }) {\n    token = opts?.env?.AGENT_API_TOKEN;\n}\nconst brain = {\n      clientSecret: "personal-secret",\n};\n';
+    expect(repo({ 'memory-registry-pg.test.ts': qm }).secrets().status).toBe('pass');
+    const r = repo({ 'memory-registry-pg.test.ts': qm.replace('AGENT_API_TOKEN;', `AGENT_API_TOKEN ?? "${token}";`), 'agent.env': `AGENT_API_TOKEN=${token}\n` }).secrets();
+    expect(r.status).toBe('fail');
+    const kv = r.lines.filter((l) => l.includes('env.kv'));
+    expect(kv).toHaveLength(2);
+    expect(kv).toEqual(expect.arrayContaining([expect.stringContaining('agent.env, added line 1'), expect.stringContaining('memory-registry-pg.test.ts, added line 3')]));
+  });
+
   test('the same assignment in a .env file still fails, naming the file', () => {
     const f = repo({ 'deploy.env': 'API_TOKEN=8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ\n' });
     const r = f.secrets();

test/redact-context-exemptions.test.ts

@@ -153,6 +153,106 @@ describe("env.kv: a name-shaped literal inside a call is a file or dict key, not
   );
 });
 
+describe("env.kv: reading an environment variable in code names a secret without holding one (GST-1894)", () => {
+  const FAKE_KEY = "Zx9Qw8Er7Ty6Ui5Op4As3Df2";
+  const GH = ["ghp", "_", "Zx9Qw8Er7Ty6Ui5Op4As3Df2Gh7Jk8Lm9Np0"].join("");
+  table(
+    "env.kv",
+    "MEDIUM",
+    [
+      String.raw`    token = opts?.env?.AGENT_API_TOKEN;`,
+      String.raw`token = ctx.env.AGENT_API_TOKEN;`,
+      String.raw`token = env?.AGENT_API_TOKEN;`,
+      String.raw`token = env.AGENT_API_TOKEN,`,
+      String.raw`token = import.meta.env.VITE_API_TOKEN;`,
+      String.raw`token = Bun.env.AGENT_API_TOKEN;`,
+      String.raw`token = this.env!.AGENT_API_TOKEN;`,
+      String.raw`token = process.env.AGENT_API_TOKEN;`,
+      String.raw`token = process.env["AGENT_API_TOKEN"];`,
+      String.raw`token = opts?.env?.["AGENT_API_TOKEN"];`,
+      String.raw`token = os.environ["OPENROUTER_API_KEY_V2"]`,
+      String.raw`token = os.getenv("OPENROUTER_API_KEY_V2")`,
+      String.raw`token = System.getenv("AGENT_API_TOKEN");`,
+    ],
+    [
+      String.raw`token = "${FAKE_KEY}"`,
+      `AGENT_API_TOKEN=${FAKE_KEY}`,
+      `export AGENT_API_TOKEN=${FAKE_KEY}`,
+      String.raw`token = opts?.env?.AGENT_API_TOKEN ?? "${FAKE_KEY}"`,
+      String.raw`token = env.AGENT_API_TOKEN||"${FAKE_KEY}"`,
+      String.raw`token = process.env["AGENT_API_TOKEN"] || "${FAKE_KEY}"`,
+      String.raw`token = os.getenv("AGENT_API_TOKEN", "${FAKE_KEY}")`,
+      String.raw`token = process.env["${FAKE_KEY}"]`,
+      String.raw`token = os.environ["${FAKE_KEY}"]`,
+      `token = env.${FAKE_KEY}`,
+      String.raw`token = myenv.AGENT_API_TOKEN;`,
+    ],
+  );
+
+  test("a known key prefix beside an env name still reports HIGH", () => {
+    for (const input of [`AGENT_API_TOKEN=${GH}`, String.raw`token = opts?.env?.AGENT_API_TOKEN ?? "${GH}"`]) {
+      expect(findings(input).some((f) => f.severity === "HIGH")).toBe(true);
+    }
+  });
+});
+
+describe("env.kv: the env-read exemption needs a finished expression with no fallback; otherwise the line reads as on main (GST-1894)", () => {
+  const FAKE_KEY = "Zx9Qw8Er7Ty6Ui5Op4As3Df2";
+  const kv = (text: string) => findings(text).some((f) => f.id === "env.kv");
+  test.each([
+    ["comment after ??", String.raw`token = opts?.env?.AGENT_API_TOKEN ?? // fallback` + "\n" + String.raw`  "${FAKE_KEY}";`],
+    ["blank line after ??", String.raw`token = opts?.env?.AGENT_API_TOKEN ??` + "\n\n" + String.raw`  "${FAKE_KEY}";`],
+    ["fallback wrapped after ??", String.raw`token = opts?.env?.AGENT_API_TOKEN ??` + "\n" + String.raw`  "${FAKE_KEY}";`],
+    ["fallback on the next line", String.raw`token = opts?.env?.AGENT_API_TOKEN` + "\n" + String.raw`  ?? "${FAKE_KEY}";`],
+    ["|| on the next line", String.raw`token = env.AGENT_API_TOKEN` + "\n" + String.raw`  || "${FAKE_KEY}"`],
+    ["backtick fallback", "token = opts?.env?.AGENT_API_TOKEN ?? `" + FAKE_KEY + "`;"],
+    ["backtick concatenation", "token = ctx.env.AGENT_API_TOKEN + `" + FAKE_KEY + "`;"],
+    ["ternary", "token = opts?.env?.AGENT_API_TOKEN ? opts.env.AGENT_API_TOKEN : `" + FAKE_KEY + "`;"],
+    ["python or", String.raw`token = ctx.env.AGENT_API_TOKEN or FALLBACK_TOKEN_VALUE`],
+    ["getenv default", String.raw`token = System.getenv("AGENT_API_TOKEN", "${FAKE_KEY}");`],
+    ["call left open", String.raw`token = System.getenv("AGENT_API_TOKEN")`],
+  ])("fires as on main: %s", (_, input) => {
+    expect(kv(input)).toBe(true);
+  });
+  test.each([
+    ["multi-line call", ["client = OpenAI(", String.raw`    api_key=os.getenv("OPENAI_API_KEY"),`, String.raw`    base_url="https://api.openai.com/v1",`, ")"].join("\n")],
+    ["object property", String.raw`  token: opts?.env?.AGENT_API_TOKEN,`],
+    ["code span in a comment", "token = make_token(user)  // `process.env.AGENT_API_TOKEN` fallback"],
+    ["getenv with an empty default", String.raw`token = os.getenv("OPENROUTER_API_KEY_V2", "")`],
+  ])("still exempt, as on main: %s", (_, input) => {
+    expect(kv(input)).toBe(false);
+  });
+});
+
+describe("env.kv: a lowercase fixture label that names itself a secret, in a test file only (GST-1894)", () => {
+  const kv = (text: string, sourcePath?: string) => scan(text, { repoVisibility: "private", sourcePath }).findings.some((f) => f.id === "env.kv");
+  test.each([
+    ["memory-registry-pg.test.ts", '      clientSecret: "personal-secret",'],
+    ["memory-registry-pg.test.ts", '  signingSecret: "memory-authority-signing-secret",'],
+    ["auth.spec.tsx", "    apiKey: 'staging-api-key',"],
+    ["test_auth.py", '    password = "admin_password_fixture"'],
+    ["auth_test.go", '  token: "refresh-token-value",'],
+  ])("clean in %s: %s", (file, input) => {
+    expect(kv(input, file)).toBe(false);
+  });
+  test.each([
+    ["memory-registry.ts", '      clientSecret: "personal-secret",'],
+    [undefined, '      clientSecret: "personal-secret",'],
+    ["deploy.env", "CLIENT_SECRET=personal-secret"],
+    ["memory-registry-pg.test.ts", '      clientSecret: "correct-horse-battery-staple",'],
+    ["memory-registry-pg.test.ts", '      clientSecret: "Personal-Secret-9x",'],
+    ["memory-registry-pg.test.ts", '      clientSecret: "secretive-horse",'],
+    ["memory-registry-pg.test.ts", '      clientSecret: "Zx9Qw8Er7Ty6Ui5Op4As3Df2",'],
+    ["memory-registry-pg.test.ts.bak", '      clientSecret: "personal-secret",'],
+    ["src/ab-test.ts", '      clientSecret: "personal-secret",'],
+    ["scripts/load-test.ts", '      clientSecret: "personal-secret",'],
+    ["src/test_helpers.ts", '      clientSecret: "personal-secret",'],
+    ["spec/auth_spec.rb", '      clientSecret: "personal-secret",'],
+  ])("still MEDIUM in %s: %s", (file, input) => {
+    expect(kv(input, file)).toBe(true);
+  });
+});
+
 describe("internal.hostname: per-machine config filenames are not hosts (#2962, #2740)", () => {
   table(
     "internal.hostname",