diff --git a/changelog.d/capy-gst-1450-redact-property-read.md b/changelog.d/capy-gst-1450-redact-property-read.md new file mode 100644 index 000000000..829611f84 --- /dev/null +++ b/changelog.d/capy-gst-1450-redact-property-read.md @@ -0,0 +1,3 @@ +### Fixed + +- **`gstack-redact` no longer reports a property read in code as an internal hostname (#3987).** The `internal.hostname` rule matches any name ending in `.local`, `.prod`, `.staging`, `.internal`, `.corp` or `.lan`, so `server.local('/x')`, `if (config.prod)` and `if settings.internal:` were reported as leaked hosts. The pregate secrets stage listed each one as a warning, and /ship then asked for a confirm. In gstack's own source files, 57 of the 239 findings this rule made were property reads like these. Now, in a TypeScript (`.ts`, `.mts`, `.cts`), Dart or Python file, a name is treated as code when it sits outside every string and comment on its line and has code on both sides of it: an operator, bracket or expression keyword before it, and a bracket, operator, method call or the line end after it. A quoted host, a host in a comment, a host and port (`db.internal:5432`), a host and path (`proxy.corp/api`), prose, a host inside a TypeScript template literal or block comment, every host in a `.tsx`, `.jsx`, config, dotenv or Markdown file, and a scan without a file path still report. The check reads at most 1,000 characters back on a line and reads each line once. A 4 MB file of ordinary source scans in the same time as before; a buffer packed with these names on every line takes up to about 40% longer. Thanks to @nazarli-shabnam (#4026) for the approach. diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index e4e29a5df..1833ec1c9 100644 --- a/lib/redact-patterns.ts +++ b/lib/redact-patterns.ts @@ -792,6 +792,7 @@ let stringSpansCache: { text: string; lang: string; spans: Array<[number, number /** Drops the cached text once a scan ends, so a long-lived process holds no scanned input. */ export function clearStringSpanCache(): void { stringSpansCache = null; + codeLineCache = null; } /** The index after the comment at `i`: a line comment ends at its newline, a Dart block comment at its matching `*\/` (they nest). */ @@ -926,9 +927,40 @@ function stringSpans(text: string, lang: "py" | "dart"): Spans { return spans; } -function insideString(text: string, offset: number, sourcePath: string): boolean { - if (!STRING_SPAN_SOURCE.test(sourcePath)) return false; - const spans = stringSpans(text, /\.dart$/i.test(sourcePath) ? "dart" : "py"); +/** + * JS template literals and block comments, from one pass over the whole input: + * the context a line cannot show on its own. A quote ends at its line end, a + * line comment is skipped, and `${…}` stays template text. A regex literal is + * not recognized, so a `//` or quote inside one can hide a template that opens + * later on its line, and a host in that template is exempt as it was before + * jsSpans. Cached and cleared with stringSpans. + */ +function jsSpans(text: string): Spans { + if (stringSpansCache?.text === text && stringSpansCache.lang === "js") return stringSpansCache.spans; + const spans: Spans = []; + const n = text.length; + for (let i = 0; i < n; ) { + const c = text[i]!; + if (text.startsWith("//", i)) { + const newline = text.indexOf("\n", i); + i = newline < 0 ? n : newline; + } else if (text.startsWith("/*", i)) { + const close = text.indexOf("*/", i + 2); + const end = close < 0 ? n : close + 2; + spans.push([i, end]); + i = end; + } else if (c === '"' || c === "'" || c === "`") { + let j = i + 1; + while (j < n && text[j] !== c && (c === "`" || text[j] !== "\n")) j += text[j] === "\\" ? 2 : 1; + if (c === "`") spans.push([i, Math.min(j + 1, n)]); + i = j + 1; + } else i += 1; + } + stringSpansCache = { text, lang: "js", spans }; + return spans; +} + +function inSpans(spans: Spans, offset: number): boolean { let lo = 0; let hi = spans.length - 1; while (lo <= hi) { @@ -940,6 +972,11 @@ function insideString(text: string, offset: number, sourcePath: string): boolean return false; } +function insideString(text: string, offset: number, sourcePath: string): boolean { + if (!STRING_SPAN_SOURCE.test(sourcePath)) return false; + return inSpans(stringSpans(text, /\.dart$/i.test(sourcePath) ? "dart" : "py"), offset); +} + /** The rest of a name's expression on its line, up to the comma, semicolon, bracket or comment that ends it; null when it continues on the next line. */ function expressionTail(line: string, comment: string): string | null { let rest = line; @@ -981,6 +1018,143 @@ function isSourceExpression(span: string, match: RegExpExecArray, sourcePath: st 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. */ +const PROPERTY_READ_LINE_CHARS = 1000; +/** The object chain in front of a span (`this.`, `ctx.`, `a!.`), skipped before the lead is read. Matched against the last 80 characters only, so a longer chain leaves a name as the lead, which never qualifies. */ +const PROPERTY_READ_CHAIN = /(?:[\w$#]+[!?]?\.)+$/; +/** The token that opens an expression in front of the chain: an operator or bracket, or one of the language's expression keywords. Matched against the last 7 characters of the lead. */ +const PROPERTY_READ_LEAD = /(?:[([{,;?:=+]|=>|&&|\|\||\.\.\.)$/; +const PROPERTY_READ_KEYWORD = { + js: /(? offset || offset - s.at > cap) { + const head = text.slice(Math.max(0, offset - cap - 1), offset); + const newline = head.lastIndexOf("\n"); + if (newline < 0 && offset > cap) return -1; + const lineStart = offset - head.length + newline + 1; + s = codeLineCache = { text, at: lineStart, lineStart, indent: true, quote: "", comment: "", slash: false }; + } + let i = s.at; + while (i < offset) { + const c = text[i]!; + if (c === "\n") { + Object.assign(s, { lineStart: i + 1, indent: true, quote: "", comment: "", slash: false }); + i += 1; + continue; + } + if (s.indent) { + if (c === " " || c === "\t") { i += 1; continue; } + s.indent = false; + if (c === "*" && lang !== "py") s.comment = "line"; + } + if (s.comment === "line") { + i += 1; + } else if (s.comment === "block") { + if (text.startsWith("*/", i)) { s.comment = ""; i += 2; } else i += 1; + } else if (s.quote) { + if (c === "\\" && text[i + 1] !== "\n") i += 2; + else if (text.startsWith(s.quote, i)) { i += s.quote.length; s.quote = ""; } + else i += 1; + } else if (lang === "py" ? c === "#" : text.startsWith("//", i)) { + s.comment = "line"; + } else if (lang !== "py" && text.startsWith("/*", i)) { + s.comment = "block"; + i += 2; + } else if (c === '"' || c === "'" || (c === "`" && lang === "js")) { + s.quote = lang !== "js" && text.startsWith(c.repeat(3), i) ? c.repeat(3) : c; + i += s.quote.length; + } else { + if (c === "/" && lang === "js") s.slash = true; + i += 1; + } + } + s.at = Math.max(i, offset); + return s.quote || s.comment || s.slash || offset - s.lineStart > cap ? -1 : s.lineStart; +} + +/** + * True when an `internal.hostname` span is a property read in source code + * (`server.local('/x')`, `if (config.prod)`, `if settings.internal:`) rather + * than a host (#3987). The pattern ends in `.local|.prod|.staging|…`, so any + * object with a property of that name reported, and the pregate secrets stage + * warned on every one in a pushed diff. + * + * Exempts only when all of these hold: the scan has a TS, Dart or Python + * file's path (PROPERTY_READ_SOURCE; without one nothing is exempt, and .tsx + * and .jsx report as before), the span starts with an + * identifier, it sits outside every string and comment on its line (and, in + * Python and Dart, outside every string of the file: insideString), and the + * line shows code syntax on both sides of it: an operator, bracket or one of + * the language's expression keywords before its object chain (or the line + * start, when a call, index or assignment follows), and a bracket, operator, + * method call or line end after it. A quoted host, a host in a comment, + * `db.internal:5432`, `build-7.internal`, `proxy.corp/api`, prose, and every + * host in a config, dotenv or Markdown file still report. In TS, a span + * inside a template literal or block comment of the scanned text (jsSpans) is + * never exempt. A scan of added lines only cannot see a + * template literal that opened above them, so in JS a line that is only a key + * and `:` or `=` before the span (`host: db.internal`, `DB_HOST=db.internal`) + * is never exempt either. + */ +function isPropertyRead(match: RegExpExecArray, sourcePath: string | undefined): boolean { + if (!sourcePath || !PROPERTY_READ_SOURCE.test(sourcePath)) return false; + const input = match.input ?? ""; + const { start, end } = spanBounds(match); + if (!/^[A-Za-z_$][\w$]*\./.test(input.slice(start, end))) return false; + const after = input.slice(end, end + 40).split("\n", 1)[0]!; + if (!PROPERTY_READ_TAIL.test(after)) return false; + const lang = /\.pyi?$/i.test(sourcePath) ? "py" : /\.dart$/i.test(sourcePath) ? "dart" : "js"; + const lineStart = codeLineStart(input, start, lang); + if (lineStart < 0) return false; + const before = input.slice(lineStart, start); + let k = before.length - (PROPERTY_READ_CHAIN.exec(before.slice(-80))?.[0].length ?? 0); + while (k > 0 && before[k - 1] === "!") k -= 1; + while (k > 0 && (before[k - 1] === " " || before[k - 1] === "\t")) k -= 1; + const lead = before.slice(Math.max(0, k - 7), k); + if (lang === "js" && PROPERTY_READ_BARE_KEY.test(before.slice(0, k))) return false; + if (k === 0 ? !PROPERTY_READ_LINE_START_TAIL.test(after) : !PROPERTY_READ_LEAD.test(lead) && !PROPERTY_READ_KEYWORD[lang].test(lead)) return false; + return lang === "js" ? !inSpans(jsSpans(input), start) : !insideString(input, start, sourcePath); +} + export const PATTERNS: RedactPattern[] = [ // ===== HIGH — genuinely-secret credentials (block) ===== { @@ -1386,8 +1560,10 @@ export const PATTERNS: RedactPattern[] = [ description: "Internal hostname (*.internal/.corp/.local/.prod/.staging)", regex: /\b([a-z0-9][a-z0-9\-]*\.(?:internal|corp|local|lan|prod|staging))\b/i, // `.env.local`, `CLAUDE.local.md` and friends are filenames, not hosts. - // See isDotenvFilename and isLocalConfigFilename. - validate: (_span, match) => !isDotenvFilename(match) && !isLocalConfigFilename(match), + // See isDotenvFilename and isLocalConfigFilename. A property read in + // code (`config.prod`) is not a host either: see isPropertyRead (#3987). + validate: (_span, match, opts) => + !isDotenvFilename(match) && !isLocalConfigFilename(match) && !isPropertyRead(match, opts?.sourcePath), }, { id: "internal.url_private", diff --git a/test/redact-hostname-property-read.test.ts b/test/redact-hostname-property-read.test.ts new file mode 100644 index 000000000..1fb5245a0 --- /dev/null +++ b/test/redact-hostname-property-read.test.ts @@ -0,0 +1,109 @@ +/** + * internal.hostname vs property reads in source code (#3987). + * + * The hostname pattern ends in `.local|.prod|.staging|.internal|.corp|.lan`, + * so a property read in code (`server.local(...)`, `config.prod`) reported as + * a leaked internal host. The pregate secrets stage scans each file under its + * own name, so every such read in a pushed diff became a WARN, and /ship asked + * for a confirm on it. The first ten CODE lines are taken from gstack's own + * source (three trimmed to one line); the issue's examples and other shapes + * follow them. + * + * The exemption needs a code file's path, a span outside any string or + * comment on its line, and code syntax on both sides of it, so every real host + * below still reports. + */ +import { describe, test, expect } from "bun:test"; +import { scan } from "../lib/redact-engine"; + +const flagsHost = (input: string, sourcePath?: string): boolean => + scan(input, { repoVisibility: "private", ...(sourcePath ? { sourcePath } : {}) }).findings.some( + (f) => f.id === "internal.hostname", + ); + +describe("internal.hostname: a property read in code is not a host", () => { + const CODE: [string, string][] = [ + ["browse/test/server-auth.test.ts", " const post = (headers: Record) => server.local('/extension-token', { method: 'POST', headers });"], + ["browse/test/server-auth.test.ts", " expect(await statusAndJson(await server.local(route))).toEqual(UNAUTHORIZED);"], + ["browse/test/dual-listener.test.ts", " for (const call of [server.local, server.tunnel]) {"], + ["lib/code-intelligence/sourcebot-adapter.ts", " this.local = isLoopback(this.#baseUrl);"], + ["lib/pregate/config.ts", " if (!Array.isArray(obj.local) || obj.local.some(l => typeof l !== 'string')) errors.push({ code: 'X' });"], + ["lib/pregate/config.ts", " else (obj.local as string[]).forEach((l, i) => { if (!LANE_ID.test(l)) errors.push(l); });"], + ["lib/pregate/config.ts", ` return { declaresLanes: LANE_KEYS.some(k => k in o), config: { $schema: o.$schema, runner: o.runner ?? null, dependencies: o.dependencies ?? {}, empty_lanes: o.empty_lanes ?? [], lanes: o.lanes ?? {}, remote: o.remote ?? [], local: o.local ?? [] } };`], + ["lib/gate-diff/git.ts", " if (base.local === 'fallback') return `${base.unrelatedRemote ? `${base.unrelatedRemote} shares no history with HEAD` : `no remote-tracking branch for ${name}`}; using the local branch`;"], + ["lib/pregate/lane-selection.ts", " if (lane.tier === 'platform' && ctx.config.local.includes(lane.id)) ctx.explain.push(lane.id);"], + ["test/code-intelligence.test.ts", " expect(g.local).toBe(false);"], + ["src/a.ts", " return m.local"], + ["src/a.ts", "if (config.prod) start();"], + ["src/a.ts", "const host = env.staging ? a : b;"], + ["src/a.ts", "} else if (!env.staging) {"], + ["app/x.py", "if settings.internal:\n pass\n"], + ["app/x.py", "if ready and not cfg.local: # dev only\n pass\n"], + ["app/x.py", "mode = 'a' if cfg.prod else 'b'\n"], + ["lib/a.dart", " if (config.prod) return;"], + ]; + for (const [path, line] of CODE) { + test(`${path}: ${line.trim()}`, () => { + expect(flagsHost(line, path)).toBe(false); + }); + } +}); + +describe("internal.hostname: real hosts still report", () => { + const HOSTS: [string, string | undefined, string][] = [ + ["quoted URL in code", "src/a.ts", 'const url = "http://build.corp/api";'], + ["quoted host in code", "src/a.ts", "const host = 'db.internal';"], + ["template literal in code", "src/a.ts", "const u = `http://build-7.internal/health`;"], + ["quoted host in a call", "src/a.ts", "connect('db.internal', 5432);"], + ["line comment in code", "src/a.ts", "const port = 5432; // primary is db.internal"], + ["block comment in code", "src/a.ts", "/* (db.internal) */"], + ["JSDoc continuation line", "src/a.ts", " * Falls back to (printer.local) when mDNS answers."], + ["host and port in code", "src/a.ts", "dial(db.internal:5432);"], + ["hyphenated host in code", "src/a.ts", "if (build-7.internal) ok();"], + ["host followed by a path", "src/a.ts", "return proxy.corp/api;"], + ["prose in a code file", "src/a.ts", "connect to db.internal on port 5432"], + ["keyword then prose", "src/a.ts", "if api.corp is down, page the on-call"], + ["return in prose", "src/a.ts", "then return to printer.local for a retry"], + ["shell command", "src/a.ts", "ssh jump.corp"], + ["and before a host in a JS file", "src/a.ts", "deploy to staging.prod and prod.internal"], + ["if in prose, closed by a paren", "src/a.ts", "(only if build.internal)"], + ["bang in prose", "src/a.ts", "Done! build.internal"], + ["a host alone on its line", "src/a.ts", "const hosts = `\ndb.internal\ncache.internal,\n`;"], + ["a YAML key in a multi-line template literal", "src/a.ts", "const cfg = `\nhost: db.internal\nport: 5432\n`;"], + ["a dotenv line in a multi-line template literal", "src/a.ts", "const env = `\nDB_HOST=db.internal\n`;"], + ["a connection string in a multi-line template literal", "src/a.ts", "const c = `\nServer=db.internal;Database=app\n`;"], + ["a quote inside a regex literal before a quoted host", "src/a.ts", 'if (/"/.test(x) && s === "(db.internal)") go();'], + ["JSX text in a .tsx file", "src/status.tsx", "export const Status = () =>

Primary database (db.internal)

;"], + ["multi-line JSX text in a .tsx file", "src/a.tsx", "export const S = () => (\n

\n Primary database (db.internal)\n

\n);"], + ["JSX text after an expression in a .tsx file", "src/a.tsx", "

{label} Primary database (db.internal)

"], + ["a comparison before a property read in a .tsx file", "src/a.tsx", "const x = n > 0 && config.prod;"], + ["a property read in a .tsx file", "src/a.tsx", "if (config.prod) start();"], + ["a YAML flow list in a multi-line template literal", "src/config.ts", "const cfg = `\nupstreams: [db.internal, cache.corp]\n`;"], + ["a host in a block comment opened on an earlier line", "src/a.ts", "/*\nPrimary database (db.internal)\n*/"], + ["a Python sentence ending in a colon", "app/x.py", "if the VPN is down, use backup.corp:\n"], + ["a Python host before a port", "app/x.py", "if db.internal:5432 is reachable\n"], + ["Python string", "app/x.py", 'HOST = "db.internal"\n'], + ["Python comment", "app/x.py", "x = 1 # if (db.internal)\n"], + ["Python docstring line", "app/x.py", '"""\nFalls back to (printer.local).\n"""\n'], + ["prose", "README.md", "Connect to db.internal on port 5432"], + ["config value", "config/app.yaml", "host: db.internal"], + ["config assignment", ".env", "HOST=db.internal"], + ["URL host field", "deploy.json", '{ "host": "api.staging", "url": "https://build.corp/x" }'], + ["a code shape without a path", undefined, "if (config.prod) start();"], + ["a code shape past the context cap", "src/a.ts", `if (${"x".repeat(1200)} && m.local) go();`], + ]; + for (const [label, path, input] of HOSTS) { + test(label, () => { + expect(flagsHost(input, path)).toBe(true); + }); + } +}); + +describe("internal.hostname: the property-read check stays bounded", () => { + test("a long line of property reads finishes quickly", () => { + const line = `if (${"!".repeat(20000)}${"cfg.local && ".repeat(5000)}ok) go();`; + const started = performance.now(); + flagsHost(line, "src/a.ts"); + expect(performance.now() - started).toBeLessThan(2000); + }); +});