From e9de3a9d8be13422bb60190fee5b3a64ebc7f4b0 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 9 Nov 2026 10:24:44 -0800 Subject: [PATCH] Stop the secret check from blocking on Django's self.client.session (#4209) (#4228) * Add failing tests for env.kv flagging a Python attribute read like self.client.session (#4209) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Read a Python or Dart attribute reference assigned with = as a name in env.kv (#4209) 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/fix-env-kv-django-session-4209.md | 4 + lib/redact-patterns.ts | 31 ++++++- test/pregate-secrets-source-context.test.ts | 16 ++++ test/redact-engine.test.ts | 3 +- .../redact-env-kv-attribute-reference.test.ts | 84 +++++++++++++++++++ 5 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fix-env-kv-django-session-4209.md create mode 100644 test/redact-env-kv-attribute-reference.test.ts diff --git a/changelog.d/fix-env-kv-django-session-4209.md b/changelog.d/fix-env-kv-django-session-4209.md new file mode 100644 index 000000000..e04d00aa5 --- /dev/null +++ b/changelog.d/fix-env-kv-django-session-4209.md @@ -0,0 +1,4 @@ +--- +important: blocker +--- +- **The pregate secrets check no longer stops /ship on Django's `session = self.client.session`.** env.kv read any assignment to a name ending in `session`, `token` or `key` as a credential when the value looked random enough, so a Python test that grabbed the test client's session failed a check nobody can downgrade, and the only way past it was a hand-edited allowlist (#4209). In Python and Dart source, an unquoted dotted name such as `self.client.session` or `request.auth`, alone on the right of `=`, now reads as an attribute. A quoted value, a random-looking dotted token, the same line inside a `"""` string, anything after it on the line, and every .env, YAML, config and TypeScript file still report. Reported by @dgrant in #4209. diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index dbdc060ce..10536ed74 100644 --- a/lib/redact-patterns.ts +++ b/lib/redact-patterns.ts @@ -832,8 +832,9 @@ function isCallExpression(span: string, match: RegExpExecArray): boolean { * `API_KEY=VelvetRiverOrbitSunset;` in a .env or YAML file is still a literal. * #3630 (#3418, #3419): Dart and Python (.py, .pyi) qualify on the same grammar * fact, no unquoted string literal, so `'password': request.password` and - * `session: HttpSession,` are names. Python's `=` forms - * (`password=request.password`) still fire: the separator must be `:`. + * `session: HttpSession,` are names. Python's keyword form + * (`password=request.password,`) still fires: the separator must be `:`. A + * plain `=` assignment of a dotted name is isAttributeReference's (#4209). * Without a known source path nothing is exempt. */ const CODE_SOURCE = /\.(?:[cm]?tsx?|jsx|dart|pyi?)$/i; @@ -1086,6 +1087,30 @@ function isSourceExpression(span: string, match: RegExpExecArray, sourcePath: st return !insideString(match.input, start, sourcePath); } +/** + * #4209: in Python and Dart source, an unquoted dotted name chain that is the + * whole right-hand side of an `=` assignment (`session = self.client.session`, + * `token = request.auth`) reads an attribute; it holds no credential. Only + * Python and Dart, where the whole file's strings are known (insideString), so + * the same line inside a `"""`/`'''` block still reports. Every segment must be + * name-shaped (snake_case, camelCase, PascalCase or CONSTANT_CASE), so a dotted + * random token still reports, and only a `;` and a comment may follow it on + * the line: a literal after it (`or "…"`, `, "…"`), a continuation or a + * keyword argument's comma still reports. A quoted value, a bare name, a + * config, .env or TS file, and a scan without a path are unchanged. + */ +const ATTRIBUTE_SEGMENT = /^_*(?:[a-z][a-z0-9]*(?:_+[a-z0-9]+)*|[A-Z][A-Z0-9]*(?:_+[A-Z0-9]+)*|[A-Za-z][a-z0-9]*(?:[A-Z][a-z0-9]+)*)_*$/; +function isAttributeReference(span: string, match: RegExpExecArray, sourcePath: string | undefined): boolean { + if (!sourcePath || !STRING_SPAN_SOURCE.test(sourcePath)) return false; + const chain = span.replace(/;$/, "").split("."); + if (chain.length < 2 || !chain.every((segment) => ATTRIBUTE_SEGMENT.test(segment))) return false; + const { start, end } = spanBounds(match); + if (!/=[ \t]*$/.test(match.input.slice(match.index, start))) return false; + const rest = match.input.slice(end).split("\n", 1)[0]; + if (!(/\.dart$/i.test(sourcePath) ? /^[ \t]*(?:\/\/.*)?\r?$/ : /^[ \t]*(?:#.*)?\r?$/).test(rest)) return false; + return !insideString(match.input, start, sourcePath); +} + /** The files isPropertyRead exempts in: TS, Dart and Python. JSX text in .tsx and .jsx can hold a host on any line, so those report as before. */ const PROPERTY_READ_SOURCE = /\.(?:[cm]?ts|dart|pyi?)$/i; /** How far back on its line isPropertyRead reads from a span. A span further from its line start than this is never exempted. */ @@ -1549,6 +1574,7 @@ export const PATTERNS: RedactPattern[] = [ // itself (`process.env.X||"…"`) is not an exact read and still fires. // #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). + // #4209: so is a dotted attribute read assigned with = in Python or Dart (isAttributeReference). // #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) => @@ -1558,6 +1584,7 @@ export const PATTERNS: RedactPattern[] = [ !isBareEnvRead(span, match) && !isCallExpression(span, match) && !isSourceExpression(span, match, opts?.sourcePath) && + !isAttributeReference(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 2a350a1a6..87dd74d7a 100644 --- a/test/pregate-secrets-source-context.test.ts +++ b/test/pregate-secrets-source-context.test.ts @@ -88,6 +88,22 @@ describe('pregate secrets: source context and diff integrity', () => { expect(repo({ 'notes.md': `logs are under /home/garry/work/run.log\nkey ${AWS}\n` }).secrets().status).toBe('fail'); }); + // Value: protects=a Django test that adds `session = self.client.session` passes the never-downgradable secrets check while the same line in a string or an env file still fails it; + // fails_when=the #4209 false positive returns, or the exemption reaches a """ block or a .env value; why_new=#4209 stopped /ship for Python users; seam=fixture repo; no_claim=Dart files at the pregate level + test('a Django test client session read passes; the same line in a string or a .env file still fails (#4209)', () => { + const base = 'from django.test import TestCase\n\n\nclass ViewTests(TestCase):\n def test_index(self):\n self.client.get("/")\n'; + const fx: FixtureRepo = makeFixtureRepo({ pregate: BASE_PREGATE, baseFiles: { 'tests/test_views.py': base } }); + try { + const secrets = () => JSON.parse(runBin(fx, 'gstack-pregate', ['--base', 'main', '--only', 'secrets', '--json']).out).checks.find((c: { id: string }) => c.id === 'secrets'); + const added = base + ' session = self.client.session\n session["cart"] = [1]\n session.save()\n'; + commit(fx, { 'tests/test_views.py': added }); + expect(secrets().status).toBe('pass'); + commit(fx, { 'tests/test_views.py': added + '\nFIXTURE = """\n session = self.client.session\n"""\n', '.env': 'SESSION=self.client.session\n' }); + const check = secrets(); + expect({ status: check.status, rows: (check.lines as string[]).filter((l) => l.includes('env.kv')) }).toEqual({ status: 'fail', rows: [expect.stringContaining('.env, added line 1)'), expect.stringContaining('test_views.py, line 12)')] }); + } finally { cleanup(fx); } + }); + test('parseAddedLineNumbers numbers each added line from its hunk header', () => { const diff = ['diff --git a/a.py b/a.py', '--- a/a.py', '+++ b/a.py', '@@ -3,0 +4,2 @@', '+one', '+two', '@@ -9 +11 @@', '-old', '+new'].join('\n'); expect([...parseAddedLineNumbers(diff)]).toEqual([['a.py', [4, 5, 11]]]); diff --git a/test/redact-engine.test.ts b/test/redact-engine.test.ts index 98bf89f50..34a46d9c7 100644 --- a/test/redact-engine.test.ts +++ b/test/redact-engine.test.ts @@ -314,6 +314,8 @@ describe("MEDIUM demoted credential-shaped patterns (TENSION-1)", () => { [" login('it\\'s', password: request.password);", "lib/a.dart"], [' "user": "a", "password": request.password,', "app.py"], // The triple-quote guard is Dart/Python only: a stray ''' in a TS string leaves #3048 as it was. ["const s = \"'''\";\n session: SessionState,", "src/a.ts"], + // #4209: a plain assignment of a dotted name reads an attribute; the keyword form below still fires. + [" password = request.password", "app/api/handlers.py"], ] as const) expect({ code, file, kv: kv(code, file) }).toEqual({ code, file, kv: false }); for (const [literal, file] of [ [" session: HttpSession,", "deploy/values.yaml"], @@ -322,7 +324,6 @@ describe("MEDIUM demoted credential-shaped patterns (TENSION-1)", () => { [` "password": "${v}",`, "app/api/handlers.py"], [` password = "${v}"`, "app/config.py"], [" password=request.password,", "app/api/handlers.py"], - [" password = request.password", "app/api/handlers.py"], [`CONFIG = """\npassword: ${vIdent}\n"""`, "tests/conftest.py"], [` 'password': '${v}',`, "backend/lib/src/handlers/onboarding_handlers.dart"], [` password: "${v}",`, "backend/lib/src/config.dart"], diff --git a/test/redact-env-kv-attribute-reference.test.ts b/test/redact-env-kv-attribute-reference.test.ts new file mode 100644 index 000000000..79962c7f7 --- /dev/null +++ b/test/redact-env-kv-attribute-reference.test.ts @@ -0,0 +1,84 @@ +/** + * #4209: env.kv flagged the Django test-client idiom `session = self.client.session` + * in a Python test file, and the pregate secrets stage (never downgradable) + * stopped /ship on it. An unquoted dotted attribute chain that is the whole + * right-hand side of an `=` assignment in Python or Dart source reads a + * name; it holds no credential. The hold cases below are the same shapes + * carrying a real value, or the same line where it is text, and must still fire. + */ +// Value: protects=env.kv stays fail-closed on literals, tokens, env files and strings while a source attribute read passes; fails_when=the attribute-reference exemption reaches a quoted, token-shaped, string or config value; why_new=#4209 had no test of an = assignment reading an attribute; seam=none; no_claim=scanner-level only; the pregate end-to-end case lives in pregate-secrets-source-context.test.ts +import { describe, expect, test } from "bun:test"; +import { scan } from "../lib/redact-engine"; + +const kv = (text: string, sourcePath?: string) => + scan(text, { repoVisibility: "private", processEnv: {}, ...(sourcePath ? { sourcePath } : {}) }).findings.some((f) => f.id === "env.kv"); +const v = "8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ"; +const tokenShaped = ["NzE4MjkzNjQ1MDcyODM5Mjk0", "Gx7QpL", "Kq9wErTy5UiOp3AsDf7GhJkL2Zx"].join("."); +const django = " session = self.client.session"; + +describe("env.kv: an attribute reference assigned in source is a name (#4209)", () => { + test("hold: quoted literal values still fire", () => { + for (const [literal, file] of [ + [` session = "${v}"`, "tests/test_views.py"], + [` session = '${v}'`, "tests/test_views.py"], + [' session = "self.client.session"', "tests/test_views.py"], + [" session = 'self.client.session'", "tests/test_views.py"], + [' session = "widget.session.value";', "lib/a.dart"], + ] as const) expect({ literal, file, kv: kv(literal, file) }).toEqual({ literal, file, kv: true }); + }); + + test("hold: high-entropy tokens still fire, unquoted ones included", () => { + for (const [literal, file] of [ + [` token = ${v}`, "app/views.py"], + [` token = ${tokenShaped}`, "app/views.py"], + [` session = ${tokenShaped};`, "lib/a.dart"], + [" token = 8Fk2pQ9v.Xz4wL7mN3rT6", "app/views.py"], + [" token = Fk2p-Q9vX.z4wL7mN3rT6", "app/views.py"], + [" token = abc.def/ghi+jkl==", "app/views.py"], + ] as const) expect({ literal, file, kv: kv(literal, file) }).toEqual({ literal, file, kv: true }); + }); + + test("hold: env-style assignments and config files still fire, a dotted value included", () => { + for (const [literal, file] of [ + [`SESSION_TOKEN=${v}`, ".env"], + ["SESSION=self.client.session", ".env"], + ["export SESSION=self.client.session", "deploy/env.sh"], + ["API_KEY=app.settings.api_key", ".env.production"], + ["session = self.client.session", "config/app.ini"], + ["session = self.client.session", "pyproject.toml"], + ["session: self.client.session", "deploy/values.yaml"], + [" session = this.client.session;", "src/a.ts"], + [django, undefined], + [django, "added.txt"], + ] as const) expect({ literal, file, kv: kv(literal, file) }).toEqual({ literal, file, kv: true }); + }); + + test("hold: the same line inside a string or followed by more value still fires", () => { + for (const [literal, file] of [ + [`CONFIG = """\n${django}\n"""`, "tests/conftest.py"], + [`CONFIG = f'''\n${django}\n'''`, "tests/conftest.py"], + [`CFG = "db \\\n${django} \\\n"`, "app.py"], + [`const fixture = '''\n session = widget.client.session;\n''';`, "test/fixtures.dart"], + [`${django} or "${v}"`, "tests/test_views.py"], + [`${django}, "${v}"`, "tests/test_views.py"], + [`${django} + "${v}"`, "tests/test_views.py"], + [`${django} \\\n + "${v}"`, "tests/test_views.py"], + [" password=request.password,", "app/api/handlers.py"], + ] as const) expect({ literal, file, kv: kv(literal, file) }).toEqual({ literal, file, kv: true }); + }); + + test("the Django test-client line and other attribute reads in Python and Dart source pass", () => { + for (const [code, file] of [ + [django, "tests/test_views.py"], + [django, "app/tests/test_views.py"], + [`class T(TestCase):\n def test_x(self):\n${django}\n session["k"] = 1\n`, "tests/test_views.py"], + [`${django} # Django test client`, "tests/test_views.py"], + [`${django}\r`, "tests/test_views.py"], + [" token = request.auth", "app/views.py"], + [" self.session = self.client.session", "tests/test_views.py"], + [" password = request.password", "app/api/handlers.py"], + [" session = widget.session;", "lib/a.dart"], + [" session = widget.client.session; // from the parent", "lib/a.dart"], + ] as const) expect({ code, file, kv: kv(code, file) }).toEqual({ code, file, kv: false }); + }); +});