From 0bcea95b74643214f431cdd45fd49f70119b6045 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sun, 26 Jul 2026 07:43:17 +0800 Subject: [PATCH] fix(scripts): detect env reads inside a for-of over a literal-name array gen-selfhost-env-reference.ts recognized computed env[X] access only via a string literal, the envString(env, "X") helper, or a helper whose name argument is a literal at the call site. It had no case for a name sourced from iterating a local array of string literals, so the four critical secret tokens src/selfhost/preflight.ts reads only through for (const name of CRITICAL_SECRET_VARS) { const value = nonBlank(env[name]); ... } (GITHUB_WEBHOOK_SECRET, LOOPOVER_API_TOKEN, LOOPOVER_MCP_TOKEN, INTERNAL_JOB_TOKEN) were absent from the operator-facing self-host env reference -- exactly the high-risk secrets that table exists to warn operators about. Extend the scan: a pre-pass collects every locally-declared const array of only string literals (unwrapping a trailing 'as const'), and a new for-of branch surfaces the array's names when the loop iterates it with a single identifier loop variable whose body reads env[loopVar]. Generalizes to any such array -- no var name is special-cased. Regenerated reference adds exactly the four tokens. Test: a fixture-driven regression test drives the new array-loop path directly, with positive and negative shapes (non-env loop, destructured loop var, non-identifier iterable, unknown identifier, numeric/empty arrays) exercising every new branch. --- .../src/lib/selfhost-env-reference.ts | 20 +++++++ scripts/gen-selfhost-env-reference.ts | 58 +++++++++++++++++++ .../selfhost-env-reference-script.test.ts | 52 +++++++++++++++++ 3 files changed, 130 insertions(+) diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index 0c0684aae3..fd0bd65936 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -245,10 +245,22 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "GITHUB_PUBLIC_TOKEN", firstReference: "src/queue/ai-review-orchestration.ts", }, + { + name: "GITHUB_WEBHOOK_SECRET", + firstReference: "src/selfhost/preflight.ts", + }, { name: "HOME", firstReference: "src/selfhost/ai.ts", }, + { + name: "INTERNAL_JOB_TOKEN", + firstReference: "src/selfhost/preflight.ts", + }, + { + name: "LOOPOVER_API_TOKEN", + firstReference: "src/selfhost/preflight.ts", + }, { name: "LOOPOVER_ENABLE_PAGERDUTY", firstReference: "src/services/notify-pagerduty.ts", @@ -257,6 +269,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER", firstReference: "src/selfhost/ai.ts", }, + { + name: "LOOPOVER_MCP_TOKEN", + firstReference: "src/selfhost/preflight.ts", + }, { name: "LOOPOVER_REPO_CONFIG_DIR", firstReference: "src/server.ts", @@ -626,9 +642,13 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `GITHUB_INSTALLATION_CONCURRENCY_ENABLED` | `src/selfhost/installation-concurrency-admission.ts` |", "| `GITHUB_INSTALLATION_CONCURRENCY_LIMIT` | `src/selfhost/installation-concurrency-admission.ts` |", "| `GITHUB_PUBLIC_TOKEN` | `src/queue/ai-review-orchestration.ts` |", + "| `GITHUB_WEBHOOK_SECRET` | `src/selfhost/preflight.ts` |", "| `HOME` | `src/selfhost/ai.ts` |", + "| `INTERNAL_JOB_TOKEN` | `src/selfhost/preflight.ts` |", + "| `LOOPOVER_API_TOKEN` | `src/selfhost/preflight.ts` |", "| `LOOPOVER_ENABLE_PAGERDUTY` | `src/services/notify-pagerduty.ts` |", "| `LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER` | `src/selfhost/ai.ts` |", + "| `LOOPOVER_MCP_TOKEN` | `src/selfhost/preflight.ts` |", "| `LOOPOVER_REPO_CONFIG_DIR` | `src/server.ts` |", "| `LOOPOVER_REVIEW_CONTINUOUS` | `src/queue/processors.ts` |", "| `LOOPOVER_VERSION` | `src/selfhost/otel.ts` |", diff --git a/scripts/gen-selfhost-env-reference.ts b/scripts/gen-selfhost-env-reference.ts index 9720124655..a9f006cff6 100644 --- a/scripts/gen-selfhost-env-reference.ts +++ b/scripts/gen-selfhost-env-reference.ts @@ -76,6 +76,10 @@ function collectEnvReads(source: string, fileName: string): EnvRead[] { if (!ENV_NAME_RE.test(name) || INJECTED_BINDING_NAMES.has(name)) return; reads.push({ name }); }; + // Locally-declared `const NAME = ["A", "B", ...]` literal-string arrays, so a `for (const x of NAME)` loop + // whose body reads `env[x]` can be resolved back to the concrete var names -- src/selfhost/preflight.ts's + // CRITICAL_SECRET_VARS loop reads four tokens (GITHUB_WEBHOOK_SECRET etc.) this way and nowhere else (#8652). + const literalArrays = collectLiteralStringArrays(sourceFile); const visit = (node: ts.Node) => { if (ts.isPropertyAccessExpression(node) && isEnvContainer(node.expression)) { addRead(node.name.text); @@ -95,6 +99,8 @@ function collectEnvReads(source: string, fileName: string): EnvRead[] { const arg = node.arguments[argIndex]; if (arg && ts.isStringLiteralLike(arg)) addRead(arg.text); } + } else if (ts.isForOfStatement(node)) { + for (const name of envReadingForOfArrayLiterals(node, literalArrays)) addRead(name); } ts.forEachChild(node, visit); }; @@ -141,6 +147,58 @@ function isEnvNameLiteralArgHelperCall(node: ts.CallExpression): boolean { return argIndexes !== undefined && argIndexes.some((argIndex) => node.arguments.length > argIndex && ts.isStringLiteralLike(node.arguments[argIndex]!)); } +// Collect every locally-declared `const NAME = ["A", "B", ...]` whose initializer is an array of only string +// literals (unwrapping a trailing `as const`). Used to resolve `for (const x of NAME) { env[x] }` loops back to +// concrete var names. Generalizes to any such array -- no var name is special-cased. +function collectLiteralStringArrays(sourceFile: ts.SourceFile): Map { + const arrays = new Map(); + const walk = (node: ts.Node) => { + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.initializer) { + const init = unwrapEnvExpression(node.initializer); + if (ts.isArrayLiteralExpression(init) && init.elements.length > 0 && init.elements.every((element) => ts.isStringLiteralLike(element))) { + arrays.set( + node.name.text, + init.elements.map((element) => (element as ts.StringLiteralLike).text), + ); + } + } + ts.forEachChild(node, walk); + }; + walk(sourceFile); + return arrays; +} + +// If `node` iterates a known literal-string array with a single identifier loop variable whose body reads +// `env[]`, return that array's literal names; otherwise []. This is the `for (const name of +// LOCAL_ARRAY) { env[name] }` computed-read pattern the plain element-access branch can't see (the argument is +// an identifier, not a string literal). +function envReadingForOfArrayLiterals(node: ts.ForOfStatement, literalArrays: Map): string[] { + const iterable = unwrapEnvExpression(node.expression); + if (!ts.isIdentifier(iterable)) return []; + const literals = literalArrays.get(iterable.text); + if (!literals) return []; + if (!ts.isVariableDeclarationList(node.initializer) || node.initializer.declarations.length !== 1) return []; + const loopVar = node.initializer.declarations[0]!.name; + if (!ts.isIdentifier(loopVar)) return []; + return bodyReadsEnvByName(node.statement, loopVar.text) ? literals : []; +} + +// True if `body` reads `env[]` anywhere -- a computed element access whose object is an env container +// and whose argument is the loop variable identifier. +function bodyReadsEnvByName(body: ts.Statement, loopVar: string): boolean { + let found = false; + const walk = (node: ts.Node) => { + if (found) return; + if (ts.isElementAccessExpression(node) && isEnvContainer(node.expression) && ts.isIdentifier(node.argumentExpression) && node.argumentExpression.text === loopVar) { + found = true; + return; + } + ts.forEachChild(node, walk); + }; + walk(body); + return found; +} + function bindingElementName(element: ts.BindingElement): string | null { const candidate = element.propertyName ?? element.name; if (ts.isIdentifier(candidate) || ts.isStringLiteralLike(candidate)) return candidate.text; diff --git a/test/unit/selfhost-env-reference-script.test.ts b/test/unit/selfhost-env-reference-script.test.ts index 015bf49699..5817ccd0e2 100644 --- a/test/unit/selfhost-env-reference-script.test.ts +++ b/test/unit/selfhost-env-reference-script.test.ts @@ -102,6 +102,58 @@ describe("gen-selfhost-env-reference (#2081)", () => { expect(after).toEqual(before); }); + it("REGRESSION (#8652): detects env reads inside a for-of over a locally-declared literal-name array", () => { + const root = mkdtempSync(join(tmpdir(), "gt-env-reference-forof-")); + mkdirSync(join(root, "src", "selfhost"), { recursive: true }); + // Mirrors src/selfhost/preflight.ts's CRITICAL_SECRET_VARS loop: the var names come from iterating a local + // const array of string literals, and `env[name]` is a computed access whose argument is the loop variable, + // not a string literal -- invisible to the plain element-access branch. + writeFileSync( + join(root, "src", "selfhost", "loops.ts"), + [ + 'const SECRET_VARS = ["ALPHA_TOKEN", "BETA_TOKEN"] as const;', + "for (const name of SECRET_VARS) {", + " const value = nonBlank(env[name]);", + " void value;", + "}", + // Negative: a literal-name array whose loop never touches env must NOT be surfaced. + 'const UNUSED_VARS = ["GAMMA_UNUSED"];', + "for (const label of UNUSED_VARS) {", + " console.log(label);", + "}", + // Negative: destructuring loop variable over a known literal array is not a plain `env[name]` read. + "for (const [first] of SECRET_VARS) {", + " void first;", + "}", + // Negative: an assignment-target loop (no `const` declaration list) is ignored. + "let reused;", + "for (reused of SECRET_VARS) {", + " void reused;", + "}", + // Negative: iterable is a call expression, not an identifier -> ignored. + "for (const other of Object.keys(env)) {", + " void other;", + "}", + // Negative: iterable identifier is not a collected literal-string array -> ignored. + "for (const missing of NOT_DECLARED_HERE) {", + " void env[missing];", + "}", + // Non-string / empty arrays are never treated as literal-name arrays. + "const NUMBERS = [1, 2, 3];", + "const EMPTY_ARRAY = [];", + "for (const n of NUMBERS) {", + " void n;", + "}", + "void EMPTY_ARRAY;", + "", + ].join("\n"), + ); + expect(collectSelfHostEnvVars({ rootDir: root })).toEqual([ + { name: "ALPHA_TOKEN", firstReference: "src/selfhost/loops.ts" }, + { name: "BETA_TOKEN", firstReference: "src/selfhost/loops.ts" }, + ]); + }); + it("scans configured JavaScript roots and rejects file-shaped directories", () => { const root = fixtureRoot();