diff --git a/changelog.d/stop-secrets-check-flagging-env-var-reads.md b/changelog.d/stop-secrets-check-flagging-env-var-reads.md new file mode 100644 index 000000000..01755c455 --- /dev/null +++ b/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[""]`, which passed because the check skipped the quote the match had taken. diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index 1d0a54f7e..63561334d 100644 --- a/lib/redact-patterns.ts +++ b/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 = /\?\?|\|\||(?")`) or a + * literal used as the name (`os.environ[""]`) 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, }, { diff --git a/test/pregate-secrets-source-context.test.ts b/test/pregate-secrets-source-context.test.ts index 6db4f99c9..d89f5be53 100644 --- a/test/pregate-secrets-source-context.test.ts +++ b/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 }) {\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(); diff --git a/test/redact-context-exemptions.test.ts b/test/redact-context-exemptions.test.ts index b7f51d1af..208e76a10 100644 --- a/test/redact-context-exemptions.test.ts +++ b/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",