diff --git a/.archgate/adrs/ARCH-024-rule-file-sandbox-boundary.md b/.archgate/adrs/ARCH-024-rule-file-sandbox-boundary.md index df459f5f..b136886a 100644 --- a/.archgate/adrs/ARCH-024-rule-file-sandbox-boundary.md +++ b/.archgate/adrs/ARCH-024-rule-file-sandbox-boundary.md @@ -68,9 +68,17 @@ That is: static `import`, dynamic `import()` with a literal specifier, dynamic ` This clause exists because a gap in any one construct is a gap in all of them. The incident above was precisely that shape: `ImportDeclaration` enforced the ban correctly and `ImportExpression` did not, and the weaker of the two set the boundary's real strength. -**4. Escapes that name no module MUST be blocked at the same boundary.** +**4. Naming a dangerous runtime global MUST be blocked — not the shapes of using it.** -`require()`, `import.meta.require()`, `eval`/`Function`, computed access on `Bun`/`globalThis`, the `BLOCKED_BUN_PROPS` set, and process internals (`binding`, `_linkedBinding`, `dlopen`) all reach executable code without naming a module specifier, and MUST be refused. Process-internal property names MUST be matched on the property alone, not on a receiver named `process` — `globalThis.process.binding(...)` is the same capability as `process.binding(...)`, and a check pinned to the identifier `process` catches only one spelling. +A rule file runs in-process, so `Bun`, `process`, and the global object are live and expose subprocess, filesystem, and native capabilities with **no import at all**. The module allowlist (clause 1) does nothing here: `Bun.spawn(...)` needs no `import`. Blocking specific _shapes_ of reaching these globals — `Bun.spawn` dotted, `Bun[x]` computed — is the identical losing game clause 1 rejects for modules, because the ways to name the same capability are unbounded. All of the following reach `Bun.spawn` without matching any per-shape check, and were live RCEs before this clause (each ran arbitrary code while `archgate check` reported a pass): aliasing (`const B = Bun; B.spawn(...)`), destructuring (`const { spawn } = Bun`), reflection (`Reflect.get(Bun, "spawn")`), and global-object aliases (`globalThis.Bun.spawn`, `global.Bun.spawn`, `self.Bun.spawn` — Bun binds the global object under all three names). + +The scanner therefore refuses any **code reference to a dangerous global identifier**, in any position other than a property-key slot (`foo.process` and `{ process: 1 }` name a property, not the global, and are fine): `Bun`, `process`, `globalThis`, `global`, `self`, `Reflect`, `eval`, `Function`, `fetch`, `WebSocket`, `XMLHttpRequest`, `EventSource`, and `require` MUST be blocked. Blocking the _identifier_ (not the call) is what closes the aliases: `const f = Function` and `const r = require` are refused the same as `Function(...)`/`require(...)`. `import.meta.require(...)` is handled separately, since it is a `MetaProperty` member rather than a bare identifier. + +Because naming `Function`/`eval` is now blocked, the scanner MUST ALSO refuse `.constructor` access — dotted (`x.constructor`) and computed-literal (`x["constructor"]`) — on **any** receiver. `(() => {}).constructor` **is** the `Function` constructor, i.e. `eval`: `f = (() => {}).constructor; f("return import('node:child_process')")()` runs arbitrary, unscanned code and bypasses even the module allowlist. This was the second fire-tested RCE. `.constructor` is also reachable through a **destructuring binding pattern** — `const { constructor: F } = (() => {})` reads the same property through an `ObjectPattern` the member-expression check never visits — so the block MUST cover the destructured forms too: the renamed key (`{ constructor: F }`), the computed-string key (`{ ["constructor"]: F }`), and the shorthand (`{ constructor }`). An object _literal_ `{ constructor: 1 }` merely names a property and is fine; only the binding-pattern form performs the read. + +This clause **subsumed and simplified** the prior per-shape checks: the `BLOCKED_BUN_PROPS`/process-internals member denylists and the separate `eval`/`Function`/`fetch`/`require` call checks were removed, replaced by the single identifier block. First-party and imported scans have **converged** as a result — the previously imported-only restrictions (`Bun`/`process` environment reads, `require`, `WebSocket`) are now blocked for every rule file, so `scanImportedRuleSource()` delegates to `scanRuleSource()`. This is deliberate: a first-party rule file also executes with full privilege, and a malicious pull request can add one, so the aliasing bypass had to close for _all_ rules, not only imported ones. An audit confirmed zero of this repository's own `.rules.ts` files reference any of these globals as executable code — every mention is a string the rule searches _for_ — so the block has no false positives on real rules. + +**Known residual (a static-analysis limit, not an oversight).** A property name built at runtime — `const c = "constructor"; (() => {})[c]`, or its destructured twin `const { [c]: F } = (() => {})` — is unknowable to a scanner that does not track values, and blocking _all_ computed member access (or computed destructuring) would reject ordinary `arr[i]`/`obj[key]`/`const { [k]: v } = obj`. So the computed-_variable_-key route to `.constructor`, and thus to `eval`, remains open in both the member and destructuring spellings. This is the same class as the computed non-literal `import()` clause 3 already refuses only when it cannot resolve the specifier, and it is exactly why this ADR names execution-time isolation as the complete answer: the static scan is defense-in-depth that raises the bar from a trivial one-liner to requiring runtime string construction, not a jail. A regression test asserts this residual explicitly, so it is a deliberate, documented gap rather than an accidental one. Property matching MUST additionally read the key from **both** spellings, `o.name` and `o["name"]` (`staticPropName()` in `src/engine/rule-scanner.ts`). A member expression has two syntaxes for one capability, and reading only `prop.name` sees one of them: `process["binding"]("spawn_sync")` was reachable for exactly this reason after the first pass at fixing this ADR's incident. Matching the property name in either spelling closes the aliased receiver too, since `const p = process; p["binding"](...)` is caught by the key, not the object. @@ -109,7 +117,9 @@ A denylist is legitimate _here_ and nowhere else in this ADR. Clause 1 rejects a - **DO** add a failing regression case to `tests/engine/rule-scanner-escapes.test.ts` **before** fixing any newly discovered escape, so the test demonstrably catches it - **DO** verify a scanner change against a real payload, not only unit assertions — an escape is only closed when a `.rules.ts` that actually attempts it is refused by `archgate check` - **DO** direct rule authors who need language tooling to `ctx.ast()` per [ARCH-022](./ARCH-022-ast-aware-rule-context.md), which is the sanctioned door to a subprocess -- **DO** match a blocked property name in both the `o.name` and `o["name"]` spellings via `staticPropName()` — one capability, two syntaxes +- **DO** block _naming_ a dangerous runtime global (`Bun`, `process`, `globalThis`/`global`/`self`, `Reflect`, `eval`, `Function`, `fetch`, `WebSocket`, `require`, …) rather than the shapes of using it — aliasing, destructuring, and reflection all reach the same capability, so the identifier is the only durable anchor +- **DO** block `.constructor` access on any receiver in every spelling that statically reads it — dotted (`x.constructor`), computed-literal (`x["constructor"]`), and destructuring binding patterns (`const { constructor: F } = x`, including the computed-string and shorthand keys) — it is the property-chain route to the `Function` constructor, which is `eval` +- **DO** keep the first-party and imported scans converged (`scanImportedRuleSource()` delegates to `scanRuleSource()`) unless a genuinely imported-only restriction is ever needed — a first-party rule executes with full privilege too - **DO** keep the raw-text pass scoped to character-level integrity, and reach for the AST for anything semantic — the parser is the stronger tool everywhere it applies - **DO** spell blocked code points numerically (`0x202e`), never as literal characters and never as `\u` escapes. A literal would hide inside the scanner's own source where no reviewer could see it, and an escape is not durable: a formatter may normalise it back into the literal character. This is not hypothetical — it happened twice while implementing this ADR, once in a source comment and once in a test fixture, where a `n` silently became a plain `n` and turned an obfuscation test into an ordinary one @@ -125,6 +135,8 @@ A denylist is legitimate _here_ and nowhere else in this ADR. Clause 1 rejects a - **DON'T** treat a passing `archgate check` as evidence the sandbox holds — it reported `"pass": true` throughout the incident described above - **DON'T** add a text search for `child_process`, `Bun.spawn`, or any other dangerous name. It is weaker than the AST, which resolves the escapes that defeat a regex, and it false-positives on this repository's own rule files, which name those strings as the patterns they search for - **DON'T** write an obfuscation test fixture as inline escape text without a guard asserting it is still obfuscated — a normalised escape turns the test into a no-op that passes for the wrong reason +- **DON'T** reintroduce a per-shape denylist of `Bun`/`process` members (`Bun.spawn`, `process.binding`) — an alias, destructure, or reflection walks straight around it, exactly as the module denylist was walked around; block the identifier instead +- **DON'T** try to close the computed-variable-key route to `.constructor` by blocking all computed member access — it would reject ordinary `obj[key]`; that residual belongs to execution-time isolation, not to more pattern-matching ## Consequences @@ -135,13 +147,15 @@ A denylist is legitimate _here_ and nowhere else in this ADR. Clause 1 rejects a - **ARCH-022's mitigation becomes true.** ARCH-022 mitigates its guardrail-bypass risk by asserting `createRuleContext()` is the only code path that can spawn a subprocess. That assertion held only if the scanner did; now it does. - **Third-party rule code is gated where provenance still exists.** Scanning in `writeImportedAdrs()` catches untrusted rules at the one moment the system knows they are untrusted, and refuses before writing anything. - **The safe set is small enough to review.** Four `node:`-prefixed modules can be reasoned about exhaustively, which is not true of a ban list that must anticipate every future resolver behaviour. +- **The reflective/aliasing class is closed with one rule, and the scanner got simpler.** Blocking the global identifier collapses aliasing, destructuring, reflection, and global-object aliases into a single check; the scattered per-shape `Bun`/`process`/`eval`/`fetch` member and call checks were deleted, and the first-party and imported scans converged. ### Negative - **Breaking change for existing rule files.** Any `.rules.ts` importing outside the four allowed modules now fails, including rules doing legitimate work by illegitimate means. The migration is real: a rule shelling out to a language parser must move to `ctx.ast()` ([ARCH-022](./ARCH-022-ast-aware-rule-context.md)), which is a rewrite, not a find-and-replace. - **Legitimate helper reuse across rule files is refused.** A relative import of a shared helper is blocked along with `./evil.ts`, because the scanner cannot distinguish them — it never reads either. Rule files must be self-contained. - **The allowlist is a maintenance surface.** Every genuine future need for a safe module requires an explicit review and an edit here, rather than "it wasn't banned, so it worked." -- **Static analysis remains the boundary.** This ADR hardens the scan but does not change its nature: the scanner still reasons about source text, and a bug in it is still an escape. Execution-time isolation would not have this property. +- **Static analysis remains the boundary.** This ADR hardens the scan but does not change its nature: the scanner still reasons about source text, and a bug in it is still an escape. Execution-time isolation would not have this property. The clause-4 residual (a `.constructor` reached via a runtime-computed key) is the concrete face of this — closable only by isolation, not by more pattern-matching. +- **Rules can no longer name these globals at all, even for benign reads.** `Bun.env`, `process.platform`, and `Bun.Glob` are refused along with `Bun.spawn`, a real capability reduction for first-party rules. It is accepted because rules interact with the project only through `ctx` and the audit found no rule that needed a global; a rule that genuinely wants such data is a `ctx` feature request, not a reason to reopen the alias. There is also a small false-positive surface: a rule using one of these names as a local variable or parameter (`self`, `global`) is refused and must rename. ### Risks @@ -155,6 +169,8 @@ A denylist is legitimate _here_ and nowhere else in this ADR. Clause 1 rejects a - **Mitigation:** fixtures in `tests/engine/rule-scanner-escapes.test.ts` are built from a concatenated backslash constant (`const BS = "\\"`), which no formatter can collapse, and the suite carries an explicit guard test asserting each fixture does **not** contain the plain text it is meant to hide. That guard is not decorative: it caught exactly this during implementation, after an inline `n` had already been normalised to `n`. - **A scanner regression ships because the test suite encodes the bug as intended behaviour.** This is not hypothetical: the suite contained `test("allows import with literal string")` with the comment "allowed by dynamic import check," which asserted the vulnerability was correct. It passed for the vulnerability's entire lifetime. - **Mitigation:** escape regression tests are consolidated in `tests/engine/rule-scanner-escapes.test.ts`, where each case is framed as an attack that must be blocked rather than a behaviour that is permitted. Reviewers are directed to read a permissive assertion in that file as a claim requiring justification. +- **A future Bun/Node release exposes the global object or a capability under a new alias, or a new `eval` path appears**, reopening the reflective/global class. + - **Mitigation:** the block is on the identifier set, so a new alias is a one-line addition — with a matching case in the "reflective and aliased access to runtime globals" block of `tests/engine/rule-scanner-escapes.test.ts`, which encodes every known route (aliasing, destructuring, reflection, the three global-object aliases, and the `Function`-constructor chain reached by both member access and destructuring) plus the documented computed-variable residual. The first-party/imported convergence keeps that coverage identical for both entry points, so a new alias cannot be closed for one and left open for the other. ## Compliance and Enforcement @@ -166,7 +182,7 @@ The invariant here is behavioural — _a rule file cannot reach `child_process`_ Enforcement therefore lives where behaviour can actually be observed: -- **`tests/engine/rule-scanner-escapes.test.ts`** — the authoritative enforcement artifact. Every known escape is encoded as a case asserting the scanner blocks it, alongside cases asserting legitimate rule files still pass. A regression fails the suite. Coverage spans all four clauses that can be exercised: module specifiers in every construct, escapes that name no module (including computed and aliased property access), the raw-text pass (bidi and invisible characters, leading-BOM tolerance, reporting through a parse failure), and the obfuscated-specifier cases that demonstrate the AST resolving what a text search would miss — guarded by a test asserting those fixtures are genuinely obfuscated. +- **`tests/engine/rule-scanner-escapes.test.ts`** — the authoritative enforcement artifact. Every known escape is encoded as a case asserting the scanner blocks it, alongside cases asserting legitimate rule files still pass. A regression fails the suite. Coverage spans the clauses that can be exercised: module specifiers in every construct; the reflective/global class of clause 4 — a "reflective and aliased access to runtime globals" block covering aliasing, destructuring, `Reflect.get`, the three global-object aliases, and the `Function`-constructor chain in both its member-access and destructuring (`{ constructor: F }`) spellings, plus the explicit computed-variable-key residual tests for both and "legitimate global-adjacent code still passes" cases (`Object.keys`, a property merely named `process`, a normal `ctx`-only rule); the raw-text pass (bidi and invisible characters, leading-BOM tolerance, reporting through a parse failure); and the obfuscated-specifier cases that demonstrate the AST resolving what a text search would miss — guarded by a test asserting those fixtures are genuinely obfuscated. The message and position assertions for the converged identifier model live in `tests/engine/rule-scanner.test.ts` and `tests/engine/rule-scanner-positions.test.ts`. - **`tests/helpers/adr-import.test.ts`** — asserts `writeImportedAdrs()` refuses a rule file reaching `child_process` and writes nothing, including no ADR markdown. - **`bun run validate`** — runs both suites and blocks the pipeline on failure. @@ -181,7 +197,8 @@ Code reviewers MUST verify, for any PR touching `src/engine/rule-scanner.ts`, `s 5. A permissive assertion in the escape suite (any test named "allows...") is justified explicitly. The suite's default posture is refusal. 6. `scanRuleSource()` still runs before `import()` in `loader.ts`, and `scanImportedRuleSource()` still runs before the first `writeFileSync()` in `writeImportedAdrs()`. 7. The raw-text pass has not grown a search for dangerous names, and blocked code points are still spelled numerically rather than as literals or escapes. -8. Any new blocked property check reads its key via `staticPropName()`, so it covers `o.name` and `o["name"]` alike. +8. Dangerous globals are blocked by **naming** (the banned-identifier set), not by per-shape member/call checks. A newly added blocked global or `.constructor`-style property check arrives with a matching case in the reflective-globals block of the escape suite, and covers the `o.name`/`o["name"]` member spellings and the `{ name: v }` destructuring spelling via `staticPropName()`. +9. The first-party and imported scans are still converged (`scanImportedRuleSource()` delegates), so a new global block cannot be closed for one entry point and left open for the other. ### Exceptions diff --git a/src/engine/rule-scanner.ts b/src/engine/rule-scanner.ts index 60dabfb8..2de393e0 100644 --- a/src/engine/rule-scanner.ts +++ b/src/engine/rule-scanner.ts @@ -31,17 +31,43 @@ const ALLOWED_MODULES = new Set([ "node:crypto", ]); -/** Bun API properties that bypass the RuleContext sandbox. */ -const BLOCKED_BUN_PROPS = new Set(["spawn", "spawnSync", "write", "$", "file"]); - /** - * Property names that reach process internals or native code from any object - * reference. Matched on the property name alone, regardless of what it is - * accessed on: `process.binding(...)` and `globalThis.process.binding(...)` - * are the same capability, and pinning the check to an object named `process` - * only catches the first spelling. + * Live globals in the rule runtime whose mere *naming* is blocked — in any + * position, code only (a same-named property key or string is fine). + * + * This is the module allowlist's logic (above) applied to globals. A rule file + * runs in-process, so `Bun`, `process`, and the global object are live and + * expose subprocess/filesystem/network/native capabilities directly — no import + * needed. Blocking specific *shapes* of reaching them (`Bun.spawn`, `Bun[x]`) + * is the same losing game as a module denylist: `const B = Bun; B.spawn(...)`, + * `const { spawn } = Bun`, `Reflect.get(Bun, "spawn")`, and `global.Bun.spawn` + * all reach the identical capability without matching any of those shapes. + * Enumerating the evasions is unwinnable; refusing to let rule code *name* the + * capability source is not. Rules touch the project only through `ctx`. + * + * Grouped by what each reaches: + * - the global object and its aliases, and reflection over it; + * - dynamic code execution (`eval`, and `Function` — the `Function` constructor + * is `eval`), which also subsumes the `.constructor` chain blocked below; + * - network; + * - module loading (`require`; `import.meta.require` is handled separately as it + * is a MetaProperty member, not a bare identifier). */ -const BLOCKED_INTERNAL_PROPS = new Set(["binding", "_linkedBinding", "dlopen"]); +const BANNED_GLOBALS = new Set([ + "globalThis", + "global", + "self", + "Bun", + "process", + "Reflect", + "eval", + "Function", + "fetch", + "WebSocket", + "XMLHttpRequest", + "EventSource", + "require", +]); /** * Characters that let source render differently from how it parses. @@ -280,13 +306,10 @@ export function scanRuleSource( } /** - * The property name a member expression reads, when it is knowable - * statically — from `o.name` and from `o["name"]` alike. - * - * Both spellings are the same capability, so a check that only reads - * `prop.name` sees the first and misses the second. `o[k]` with a computed - * key returns undefined: unknowable here, and out of reach of a scanner that - * does not track values (see ARCH-024 on the limits of the static boundary). + * The statically-knowable property name of a member expression — from + * `o.name` and from `o["name"]` alike. `o[k]` with a computed, non-literal + * key returns undefined (unknowable to a scanner that does not track values; + * see ARCH-024 on the limits of the static boundary). */ function staticPropName( prop: AstNode, @@ -296,6 +319,37 @@ export function scanRuleSource( return typeof prop.value === "string" ? prop.value : undefined; } + /** + * Flag a code reference to a banned global (`checkBannedIdentifier`), skipping + * property-key positions — `foo.process` and `{ process: 1 }` name a property, + * not the global. Called from the recursion, which knows the parent context. + */ + function checkBannedIdentifier(node: AstNode, isPropertyKey: boolean): void { + if ( + node.type !== "Identifier" || + typeof node.name !== "string" || + !BANNED_GLOBALS.has(node.name) + ) { + return; + } + // Advance the occurrence counter for EVERY code-position occurrence of the + // name — property-key slots included — so it stays aligned with the position + // remapper, which counts all code occurrences (it skips only strings and + // comments, not property keys). A property key (`{ Bun: 1 }`, `foo.Bun`) + // names a property, not the global, so it is counted but never emitted; + // skipping the count here would make a later *real* reference remap onto the + // earlier key. Bypasses `pushViolation` because it must count-without-emit. + const count = seenCounts.get(node.name) ?? 0; + seenCounts.set(node.name, count + 1); + if (!isPropertyKey) { + rawViolations.push({ + message: `Reference to the "${node.name}" global is blocked in rule files. Rules reach the project only through the RuleContext API (ctx); naming a runtime global — even to alias, destructure, or reflect over it — is not permitted.`, + searchText: node.name, + occurrence: count, + }); + } + } + /** * Enforce the module allowlist for any construct that names a module and * causes it to be evaluated. `import`, `export ... from`, and `export * from` @@ -327,91 +381,68 @@ export function scanRuleSource( checkModuleSpecifier(node); break; } + case "ObjectPattern": { + // Destructuring `const { constructor: F } = obj` READS `obj.constructor` + // — the Function constructor (= eval), the same reach as `.constructor` + // member access, but through a binding pattern the MemberExpression case + // never sees. An object *literal* `{ constructor: 1 }` (ObjectExpression) + // only names a property and is fine; only the pattern form performs the + // read. `staticPropName` covers `{ constructor: F }`, `{ ["constructor"]: + // F }`, and the shorthand `{ constructor }`. A runtime-computed key + // (`{ [c]: F }`) is the same documented static-analysis residual as the + // computed-variable member case (see ARCH-024). + const props = Array.isArray(node.properties) ? node.properties : []; + for (const raw of props) { + const p = parseNode(raw); + if (!p || p.type !== "Property") continue; + const key = parseNode(p.key); + if ( + key && + staticPropName(key, p.computed ?? false) === "constructor" + ) { + pushViolation( + "Destructuring `.constructor` is blocked in rule files — it reaches the Function constructor, which is equivalent to eval.", + "constructor" + ); + } + } + break; + } case "MemberExpression": { const obj = node.object; const prop = node.property; if (!obj || !prop) break; const computed = node.computed ?? false; - // Block Bun.spawn, Bun.write, Bun.$, Bun.file, Bun.spawnSync. - // Only the dotted spelling needs naming here: `Bun["spawn"]` is caught - // by the blanket computed-access rule below, which is stricter. - if ( - obj.name === "Bun" && - !computed && - BLOCKED_BUN_PROPS.has(prop.name ?? "") - ) { + // Block `.constructor` (dotted or computed-literal), on ANY receiver. + // `(() => {}).constructor` is the `Function` constructor — i.e. eval — + // so `f = (() => {}).constructor; f("return import('node:fs')")()` + // would run arbitrary, unscanned code, bypassing every other check + // including the module allowlist. Naming `Function`/`eval` directly is + // already blocked as a global; this closes the property-chain route. + if (staticPropName(prop, computed) === "constructor") { pushViolation( - `Bun.${prop.name}() is blocked in rule files. Use the RuleContext API instead.`, - `Bun.${prop.name}` - ); - } - - // Block computed access: Bun[x], globalThis[x] - if (computed && (obj.name === "Bun" || obj.name === "globalThis")) { - pushViolation( - `Computed property access on ${obj.name} is blocked in rule files.`, - `${obj.name}[` - ); - } - - // Block process.binding / process.dlopen and friends, on any receiver - // and in either spelling. Matching the property name rather than an - // object named `process` is what catches an aliased receiver - // (`const p = process; p.binding(...)`), and reading the key from a - // computed literal is what catches `process["binding"](...)`. - const propName = staticPropName(prop, computed); - if (propName !== undefined && BLOCKED_INTERNAL_PROPS.has(propName)) { - pushViolation( - `.${propName}() is blocked in rule files — it reaches process internals and native code. Use the RuleContext API instead.`, - computed ? `["${propName}"]` : `.${propName}` + "Access to `.constructor` is blocked in rule files — it reaches the Function constructor, which is equivalent to eval.", + computed ? `["constructor"]` : ".constructor" ); } // Block `import.meta.require(...)` — a require() escape that names no - // banned module and is not an ImportExpression. + // banned module and is a MetaProperty member, not a bare identifier + // (so the banned-globals check does not see it). `staticPropName` covers + // both `.require` and `["require"]`; the latter also reaches here via + // the transpiler, which rewrites the bracket form to dotted. if ( - !computed && obj.type === "MetaProperty" && - prop.name === "require" + staticPropName(prop, computed) === "require" ) { + // Anchor on `import.meta` — common to `.require` and `["require"]`. + // Anchoring on the dotted spelling would miss (remap to line 0) when + // the original source used brackets, since the remapper searches the + // untransformed source, not the normalised AST. pushViolation( "import.meta.require() is blocked in rule files. Use the RuleContext API instead.", - "import.meta.require" - ); - } - break; - } - case "CallExpression": { - const name = node.callee?.name; - if (name === "eval") { - pushViolation("eval() is blocked in rule files.", "eval("); - } - if (name === "Function") { - pushViolation( - "Function() constructor is blocked in rule files.", - "Function(" - ); - } - if (name === "fetch") { - pushViolation( - "fetch() is blocked in rule files. Rules should not make network requests.", - "fetch(" - ); - } - if (name === "require") { - pushViolation( - "require() is blocked in rule files. Use the RuleContext API instead.", - "require(" - ); - } - break; - } - case "NewExpression": { - if (node.callee?.name === "Function") { - pushViolation( - "new Function() is blocked in rule files.", - "new Function(" + "import.meta" ); } break; @@ -444,40 +475,35 @@ export function scanRuleSource( } break; } - case "AssignmentExpression": { - const left = node.left; - if (left && left.type === "MemberExpression") { - if (left.object?.name === "globalThis") { - pushViolation( - "Mutating globalThis is blocked in rule files.", - "globalThis." - ); - } - if ( - left.object?.name === "process" && - left.property?.name === "env" - ) { - const target = `${left.object.name}.${left.property.name}`; - pushViolation( - `Mutating ${target} is blocked in rule files.`, - target - ); - } - } - break; - } } - // Recurse into child nodes - for (const value of Object.values(node)) { + // Recurse into child nodes, checking each for a banned-global reference as + // we descend. The parent knows whether a child sits in a property-key slot + // (`foo.process`, `{ process: 1 }`) — a name there, not the global — so the + // check is done here rather than in a per-node case. Assignments that mutate + // a global (e.g. its `env`) are caught by this same reference check. + for (const [key, value] of Object.entries(node)) { + const isPropertyKey = + (node.type === "MemberExpression" && + key === "property" && + !(node.computed ?? false)) || + (node.type === "Property" && + key === "key" && + !(node.computed ?? false)); if (Array.isArray(value)) { for (const item of value) { const child = parseNode(item); - if (child) walk(child); + if (child) { + checkBannedIdentifier(child, false); + walk(child); + } } } else { const child = parseNode(value); - if (child) walk(child); + if (child) { + checkBannedIdentifier(child, isPropertyKey); + walk(child); + } } } } @@ -490,139 +516,19 @@ export function scanRuleSource( } /** - * Extra patterns blocked for imported (untrusted) rule files. + * Scan an imported (untrusted) `.rules.ts` source. * - * `require` is deliberately absent: `scanRuleSource()` now blocks it for every - * rule file, first-party or not, and listing it here too would report the same - * call twice. - */ -const IMPORTED_BLOCKED_GLOBALS = new Set(["WebSocket"]); - -/** - * Scan an imported (untrusted) `.rules.ts` source with stricter checks. - * - * Runs the standard `scanRuleSource()` first, then adds extra checks for - * patterns that are acceptable in first-party rules but dangerous in - * imported rules: - * - `Bun.env` access - * - environment variable reads via process - * - `require()` calls - * - `WebSocket` usage + * Historically this added stricter checks than `scanRuleSource()` — imported + * rules were forbidden the environment reads (via `Bun` and `process`), + * `require()`, and `WebSocket` that first-party rules were allowed. Those are + * all now blocked for *every* rule file: naming `Bun`, `process`, `require`, or + * `WebSocket` + * (or any other runtime global) is refused by the banned-globals check in + * `scanRuleSource()`. First-party and imported scans have therefore converged, + * and this delegates. It remains a distinct export so the `adr import` call + * site reads intentionally, and so the two can diverge again if a future + * imported-only restriction is ever needed. */ export function scanImportedRuleSource(source: string): ScanViolation[] { - let js: string; - try { - js = tsTranspiler.transformSync(source); - } catch (err) { - const msg = - err instanceof AggregateError && err.errors.length > 0 - ? String(err.errors[0]) - : err instanceof Error - ? err.message - : String(err); - return [ - { - message: `Parse error: ${msg}`, - line: 1, - column: 0, - endLine: 1, - endColumn: 0, - }, - ]; - } - - let ast: MeriyahProgram; - try { - ast = parseJsModule(js); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - return [ - { - message: `Parse error: ${msg}`, - line: 1, - column: 0, - endLine: 1, - endColumn: 0, - }, - ]; - } - const rawViolations: RawViolation[] = []; - - const seenCounts = new Map(); - function pushViolation(message: string, searchText: string) { - const count = seenCounts.get(searchText) ?? 0; - seenCounts.set(searchText, count + 1); - rawViolations.push({ message, searchText, occurrence: count }); - } - - function walkImported(node: AstNode): void { - if (!node || typeof node !== "object") return; - - switch (node.type) { - case "MemberExpression": { - const obj = node.object; - const prop = node.property; - if (!obj || !prop) break; - const computed = node.computed ?? false; - - // Block Bun.env - if (obj.name === "Bun" && !computed && prop.name === "env") { - pushViolation( - "Bun.env access is blocked in imported rule files.", - "Bun.env" - ); - } - - // Block process env reads - if (obj.name === "process" && !computed && prop.name === "env") { - const target = `${obj.name}.${prop.name}`; - pushViolation( - `${target} access is blocked in imported rule files.`, - target - ); - } - break; - } - case "CallExpression": { - const name = node.callee?.name; - if (name && IMPORTED_BLOCKED_GLOBALS.has(name)) { - pushViolation( - `${name}() is blocked in imported rule files.`, - `${name}(` - ); - } - break; - } - case "NewExpression": { - const name = node.callee?.name; - if (name && IMPORTED_BLOCKED_GLOBALS.has(name)) { - pushViolation( - `new ${name}() is blocked in imported rule files.`, - `new ${name}(` - ); - } - break; - } - } - - for (const value of Object.values(node)) { - if (Array.isArray(value)) { - for (const item of value) { - const child = parseNode(item); - if (child) walkImported(child); - } - } else { - const child = parseNode(value); - if (child) walkImported(child); - } - } - } - - const importedRoot = parseNode(ast); - if (importedRoot) walkImported(importedRoot); - - // Combine: standard scan + imported-only scan (reuse transpiled JS) - const standardViolations = scanRuleSource(source, js); - const importedViolations = remapViolations(source, rawViolations); - return standardViolations.concat(importedViolations); + return scanRuleSource(source); } diff --git a/tests/engine/rule-scanner-adversarial.test.ts b/tests/engine/rule-scanner-adversarial.test.ts index bfdd6801..142c2dc2 100644 --- a/tests/engine/rule-scanner-adversarial.test.ts +++ b/tests/engine/rule-scanner-adversarial.test.ts @@ -94,11 +94,11 @@ describe("scanRuleSource adversarial position mapping", () => { const violations = scanRuleSource(source); expect(violations).toHaveLength(3); expect(violations[0].line).toBe(5); - expect(violations[0].message).toContain("Bun.spawn()"); + expect(violations[0].message).toContain(`"Bun" global`); expect(violations[1].line).toBe(6); - expect(violations[1].message).toContain("Bun.file()"); + expect(violations[1].message).toContain(`"Bun" global`); expect(violations[2].line).toBe(7); - expect(violations[2].message).toContain("fetch()"); + expect(violations[2].message).toContain(`"fetch" global`); }); test("eval in comment then eval in code", () => { diff --git a/tests/engine/rule-scanner-escapes.test.ts b/tests/engine/rule-scanner-escapes.test.ts index ce8b0b45..98d81173 100644 --- a/tests/engine/rule-scanner-escapes.test.ts +++ b/tests/engine/rule-scanner-escapes.test.ts @@ -15,7 +15,10 @@ */ import { describe, expect, test } from "bun:test"; -import { scanRuleSource } from "../../src/engine/rule-scanner"; +import { + scanImportedRuleSource, + scanRuleSource, +} from "../../src/engine/rule-scanner"; describe("rule sandbox escapes", () => { // Regression: the ImportExpression case rejected only *non-literal* @@ -87,13 +90,14 @@ describe("rule sandbox escapes", () => { }); describe("require and process internals", () => { + // `require` is a banned global identifier, so naming it in any position + // (call, alias, argument) is refused. test("blocks require()", () => { const violations = scanRuleSource( `const cp = require("node:child_process");` ); - expect( - violations.filter((v) => v.message.includes("require()")) - ).toHaveLength(1); + expect(violations.length).toBeGreaterThan(0); + expect(violations[0].message).toContain('"require" global'); }); test("blocks import.meta.require()", () => { @@ -104,15 +108,28 @@ describe("rule sandbox escapes", () => { expect(violations[0].message).toContain("import.meta.require()"); }); - // process.binding / dlopen reach spawn and native code without importing - // anything. Matched on the property name, so an aliased receiver such as - // `globalThis.process` cannot spell around the check. + // The computed spelling `import.meta["require"]` reaches the same require() + // escape. It must be blocked AND report a real position — the anchor is + // `import.meta` (common to both spellings) so the violation does not remap + // to line 0 when the original source used brackets. + test("blocks computed import.meta['require']() with a real position", () => { + const violations = scanRuleSource( + `const cp = import.meta["require"]("node:child_process");` + ); + expect(violations.length).toBeGreaterThan(0); + expect(violations[0].message).toContain("import.meta.require()"); + expect(violations[0].line).toBe(1); + }); + + // process internals (`binding`, `dlopen`) reach spawn and native code, but + // they are reached *through* the `process` global — which is itself banned, + // so naming `process` in any spelling is what the scanner refuses. test("blocks process.binding()", () => { const violations = scanRuleSource( `const cp = process.binding("spawn_sync");` ); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain(".binding()"); + expect(violations.length).toBeGreaterThan(0); + expect(violations[0].message).toContain('"process" global'); }); test("blocks globalThis.process.binding()", () => { @@ -152,9 +169,9 @@ describe("rule sandbox escapes", () => { expect(scanRuleSource(source)).toHaveLength(0); }); }); - // Property access has two spellings, and a receiver can be aliased. Matching - // `prop.name` on an object named `process` sees only `process.binding(...)`; - // these are the same capability wearing different clothes. + // A capability reached through a banned global is refused wherever the global + // is named — dotted, computed, aliased, or chained — because the block is on + // naming `process`/`globalThis`, not on the property spelling. describe("computed access and aliased receivers", () => { const spellings: Array<[string, string]> = [ ["computed literal key", `const cp = process["binding"]("spawn_sync");`], @@ -283,4 +300,155 @@ const b = 2${RLO};`); expect(scanRuleSource(IDENT).length).toBeGreaterThan(0); }); }); + + // `Bun`, `process`, and the global object are LIVE globals in the rule + // runtime — reachable with no import at all. Blocking the syntactic shapes + // (`Bun.spawn`, `Bun[x]`) is the same losing game the module denylist was: + // aliasing, destructuring, reflection, and global-object aliases all reach + // the identical capability. The scanner instead blocks *naming* the global. + describe("reflective and aliased access to runtime globals", () => { + const reachSpawn: Array<[string, string]> = [ + ["direct Bun.spawn", `Bun.spawn(["ls"]);`], + ["Reflect.get(Bun, ...)", `Reflect.get(Bun, "spawn")(["ls"]);`], + ["destructuring Bun", `const { spawn } = Bun;\nspawn(["ls"]);`], + ["aliasing Bun", `const B = Bun;\nB.spawn(["ls"]);`], + ["globalThis.Bun.spawn", `globalThis.Bun.spawn(["ls"]);`], + ["global.Bun.spawn (Node alias)", `global.Bun.spawn(["ls"]);`], + ["self.Bun.spawn (Web alias)", `self.Bun.spawn(["ls"]);`], + [ + "Object.getOwnPropertyDescriptor(Bun, ...)", + `Object.getOwnPropertyDescriptor(Bun, "spawn").value(["ls"]);`, + ], + ["Reflect.get(process, ...)", `Reflect.get(process, "binding")("x");`], + ]; + + for (const [label, source] of reachSpawn) { + test(`blocks ${label}`, () => { + const violations = scanRuleSource(source); + expect(violations.length).toBeGreaterThan(0); + }); + } + + // Every eval-equivalent identifier is banned, and — crucially — so is + // aliasing it, which the old callee-name checks missed. + const codegen: Array<[string, string]> = [ + ["eval()", `eval("x");`], + ["aliased eval", `const e = eval;\ne("x");`], + ["Function()", `Function("return 1")();`], + ["new Function()", `new Function("return 1");`], + ["aliased fetch", `const f = fetch;\nf("http://x");`], + ["aliased require", `const r = require;\nr("fs");`], + ["WebSocket", `new WebSocket("ws://x");`], + ["XMLHttpRequest", `new XMLHttpRequest();`], + ["EventSource", `new EventSource("http://x");`], + ]; + + for (const [label, source] of codegen) { + test(`blocks ${label}`, () => { + expect(scanRuleSource(source).length).toBeGreaterThan(0); + }); + } + + // `.constructor` reaches the Function constructor (= eval) from any object, + // which would otherwise bypass every check including the module allowlist. + test("blocks the Function-constructor chain (dotted)", () => { + const violations = scanRuleSource( + `(() => {}).constructor("return 1")();` + ); + expect(violations.length).toBeGreaterThan(0); + expect(violations[0].message).toContain(".constructor"); + }); + + test("blocks .constructor via a computed string literal", () => { + expect( + scanRuleSource(`(() => {})["constructor"]("return 1")();`).length + ).toBeGreaterThan(0); + }); + + // Destructuring reaches `.constructor` through a binding pattern the + // MemberExpression case never sees: `const { constructor: F } = x` READS + // `x.constructor`. Same eval reach, so the same block must apply. + const destructured: Array<[string, string]> = [ + [ + "renamed key", + `const { constructor: F } = (() => {});\nF("return 1")();`, + ], + ["computed string key", `const { ["constructor"]: F } = (() => {});`], + ["shorthand key", `const { constructor } = (() => {});`], + ]; + + for (const [label, source] of destructured) { + test(`blocks .constructor destructured via ${label}`, () => { + const violations = scanRuleSource(source); + expect(violations.length).toBeGreaterThan(0); + expect(violations[0].message).toContain("constructor"); + }); + } + + // The computed-*variable* key is the same static-analysis residual as the + // member form above: `{ [c]: F }` with `c` bound at runtime is unknowable + // without value tracking, so it is left to execution-time isolation rather + // than chased (blocking all computed destructuring would reject ordinary + // `const { [k]: v } = obj`). Asserted so the limit stays explicit. + test("does NOT catch .constructor destructured via a runtime-computed key (known limit)", () => { + expect( + scanRuleSource( + `const c = "constructor";\nconst { [c]: F } = (() => {});` + ) + ).toHaveLength(0); + }); + + // A property name built at runtime (`obj[variable]`) is unknowable to a + // scanner that does not track values — the documented static-analysis limit + // (see ARCH-024). Blocking all computed access would reject ordinary + // `arr[i]`/`obj[key]`, so this residual is left to execution-time + // isolation, not chased with more pattern-matching. Asserted so the limit + // is explicit rather than an accidental gap. + test("does NOT catch .constructor via a runtime-computed key (known limit)", () => { + expect( + scanRuleSource(`const c = "constructor";\nconst F = (() => {})[c];`) + ).toHaveLength(0); + }); + + test("imported-rule scan applies the same block", () => { + expect( + scanImportedRuleSource(`const { spawn } = Bun;`).length + ).toBeGreaterThan(0); + }); + }); + + // Blocking the globals must not swallow the ordinary shapes rules use. + describe("legitimate global-adjacent code still passes", () => { + test("allows Object.keys / values / entries", () => { + expect( + scanRuleSource( + `const a = Object.keys({});\nconst b = Object.values({});\nconst c = Object.entries({});` + ) + ).toHaveLength(0); + }); + + test("allows a property or key that merely shares a global's name", () => { + expect( + scanRuleSource(`const cfg = { process: true };\nconst x = cfg.process;`) + ).toHaveLength(0); + }); + + // A same-named property key BEFORE a real reference must not steal the + // reported position: the key `Bun` in `{ Bun: true }` is a code occurrence + // the remapper counts, so the counter has to advance past it for the true + // `Bun.spawn` reference on line 2 to remap correctly (not onto line 1). + test("reports the real reference, not an earlier same-named key", () => { + const violations = scanRuleSource( + `const cfg = { Bun: true };\nBun.spawn([]);` + ); + expect(violations.length).toBeGreaterThan(0); + expect(violations[0].message).toContain('"Bun" global'); + expect(violations[0].line).toBe(2); + }); + + test("allows a normal RuleContext-only rule", () => { + const source = `export default { rules: { r: { description: "d", async check(ctx) { const files = await ctx.glob("**/*.ts"); const text = await ctx.readFile(files[0]); if (text.includes("TODO")) ctx.report.warning({ message: "m", file: files[0] }); } } } };`; + expect(scanRuleSource(source)).toHaveLength(0); + }); + }); }); diff --git a/tests/engine/rule-scanner-positions.test.ts b/tests/engine/rule-scanner-positions.test.ts index 3a1b82d3..7af5c95b 100644 --- a/tests/engine/rule-scanner-positions.test.ts +++ b/tests/engine/rule-scanner-positions.test.ts @@ -33,9 +33,9 @@ describe("scanRuleSource position remapping", () => { expect(violations).toHaveLength(1); expect(violations[0].line).toBe(9); expect(violations[0].column).toBe(8); - // "Bun.spawn" is 9 chars + // The banned `Bun` identifier is 3 chars: endColumn = 8 + 3 = 11. expect(violations[0].endLine).toBe(9); - expect(violations[0].endColumn).toBe(17); + expect(violations[0].endColumn).toBe(11); }); test("TypeScript type annotations stripped don't shift lines", () => { @@ -135,11 +135,11 @@ describe("scanRuleSource position remapping", () => { expect(importV).toBeDefined(); expect(importV!.line).toBe(2); - const spawnV = violations.find((v) => v.message.includes("Bun.spawn")); + const spawnV = violations.find((v) => v.message.includes(`"Bun" global`)); expect(spawnV).toBeDefined(); expect(spawnV!.line).toBe(8); - const fetchV = violations.find((v) => v.message.includes("fetch()")); + const fetchV = violations.find((v) => v.message.includes(`"fetch" global`)); expect(fetchV).toBeDefined(); expect(fetchV!.line).toBe(11); }); @@ -172,10 +172,11 @@ describe("scanRuleSource position remapping", () => { ]; const violations = scanRuleSource(lines.join("\n")); expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Bun.spawn()"); + expect(violations[0].message).toContain(`"Bun" global`); expect(violations[0].line).toBe(13); + // The banned `Bun` identifier begins at column 23; it is 3 chars. expect(violations[0].column).toBe(23); - expect(violations[0].endColumn).toBe(32); + expect(violations[0].endColumn).toBe(26); }); test("inline comments between violations", () => { @@ -186,11 +187,12 @@ describe("scanRuleSource position remapping", () => { "Bun.file('/etc/passwd'); // read a file", ].join("\n"); const violations = scanRuleSource(source); + // Both name the `Bun` global; the point is that each maps to its own line. expect(violations).toHaveLength(2); expect(violations[0].line).toBe(1); - expect(violations[0].message).toContain("Bun.spawn()"); + expect(violations[0].message).toContain(`"Bun" global`); expect(violations[1].line).toBe(4); - expect(violations[1].message).toContain("Bun.file()"); + expect(violations[1].message).toContain(`"Bun" global`); }); test("violation on first line has correct position", () => { @@ -200,7 +202,8 @@ describe("scanRuleSource position remapping", () => { expect(violations[0].line).toBe(1); expect(violations[0].column).toBe(0); expect(violations[0].endLine).toBe(1); - expect(violations[0].endColumn).toBe(9); + // "Bun" is 3 chars. + expect(violations[0].endColumn).toBe(3); }); test("violation deeply indented has correct column", () => { @@ -280,7 +283,7 @@ describe("scanRuleSource position remapping", () => { expect(violations[0].line).toBe(6); }); - test("different Bun APIs on consecutive lines", () => { + test("repeated Bun references on consecutive lines each map by occurrence", () => { const source = [ "Bun.spawn([]);", "Bun.write('/tmp/x', 'y');", @@ -288,11 +291,10 @@ describe("scanRuleSource position remapping", () => { ].join("\n"); const violations = scanRuleSource(source); expect(violations).toHaveLength(3); - expect(violations[0].line).toBe(1); - expect(violations[0].message).toContain("Bun.spawn()"); - expect(violations[1].line).toBe(2); - expect(violations[1].message).toContain("Bun.write()"); - expect(violations[2].line).toBe(3); - expect(violations[2].message).toContain("Bun.file()"); + // Each names the `Bun` global; the Nth code occurrence maps to the Nth line. + expect(violations.map((v) => v.line)).toEqual([1, 2, 3]); + expect(violations.every((v) => v.message.includes(`"Bun" global`))).toBe( + true + ); }); }); diff --git a/tests/engine/rule-scanner.test.ts b/tests/engine/rule-scanner.test.ts index 33b3ff80..3516264e 100644 --- a/tests/engine/rule-scanner.test.ts +++ b/tests/engine/rule-scanner.test.ts @@ -45,86 +45,37 @@ describe("scanRuleSource", () => { } }); - describe("dangerous Bun APIs", () => { - test("blocks Bun.spawn", () => { - const violations = scanRuleSource(`Bun.spawn(["ls"]);`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Bun.spawn()"); - }); - - test("blocks Bun.spawnSync", () => { - const violations = scanRuleSource(`Bun.spawnSync(["ls"]);`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Bun.spawnSync()"); - }); - - test("blocks Bun.write", () => { - const violations = scanRuleSource(`Bun.write("output.txt", "data");`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Bun.write()"); - }); - - test("blocks Bun.$", () => { - const violations = scanRuleSource(`Bun.$;`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Bun.$()"); - }); - - test("blocks Bun.file", () => { - const violations = scanRuleSource(`Bun.file("/etc/passwd");`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Bun.file()"); - }); - }); - - describe("computed property access", () => { - test("blocks Bun[variable]", () => { - const violations = scanRuleSource( - `const method = "spawn"; Bun[method]();` - ); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain( - "Computed property access on Bun" - ); - }); - - test("blocks globalThis[variable]", () => { - const violations = scanRuleSource( - `const key = "fetch"; globalThis[key]();` - ); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain( - "Computed property access on globalThis" - ); - }); - }); - - describe("eval and Function constructor", () => { - test("blocks eval()", () => { - const violations = scanRuleSource(`eval("console.log(1)");`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("eval()"); - }); - - test("blocks new Function()", () => { - const violations = scanRuleSource(`new Function("return 1")();`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("new Function()"); - }); - - test("blocks Function() without new", () => { - const violations = scanRuleSource(`Function("return 1")();`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Function() constructor"); - }); - }); + // Bun/process/globalThis and the eval-equivalents are blocked by naming the + // global, not by matching a call shape (see rule-scanner-escapes.test.ts for + // the aliasing/reflection cases). Each names exactly one banned global here. + describe("banned runtime globals", () => { + const cases: Array<[string, string, string]> = [ + ["Bun.spawn", `Bun.spawn(["ls"]);`, "Bun"], + ["Bun.spawnSync", `Bun.spawnSync(["ls"]);`, "Bun"], + ["Bun.write", `Bun.write("output.txt", "data");`, "Bun"], + ["Bun.$", `Bun.$;`, "Bun"], + ["Bun.file", `Bun.file("/etc/passwd");`, "Bun"], + ["Bun[variable]", `const method = "spawn"; Bun[method]();`, "Bun"], + [ + "globalThis[variable]", + `const key = "fetch"; globalThis[key]();`, + "globalThis", + ], + ["eval()", `eval("console.log(1)");`, "eval"], + ["new Function()", `new Function("return 1")();`, "Function"], + ["Function() without new", `Function("return 1")();`, "Function"], + ["fetch()", `fetch("https://example.com");`, "fetch"], + ["globalThis assignment", `globalThis.myGlobal = "value";`, "globalThis"], + ["process.env assignment", `process.env = {};`, "process"], + ]; - describe("fetch", () => { - test("blocks fetch()", () => { - const violations = scanRuleSource(`fetch("https://example.com");`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("fetch()"); - }); + for (const [label, source, global] of cases) { + test(`blocks ${label}`, () => { + const violations = scanRuleSource(source); + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain(`"${global}" global`); + }); + } }); describe("dynamic imports", () => { @@ -143,20 +94,6 @@ describe("scanRuleSource", () => { // rule-scanner-escapes.test.ts alongside the other sandbox escapes. }); - describe("global mutation", () => { - test("blocks globalThis assignment", () => { - const violations = scanRuleSource(`globalThis.myGlobal = "value";`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Mutating globalThis"); - }); - - test("blocks process.env assignment", () => { - const violations = scanRuleSource(`process.env = {};`); - expect(violations).toHaveLength(1); - expect(violations[0].message).toContain("Mutating process.env"); - }); - }); - describe("TypeScript support", () => { test("handles TypeScript syntax (interfaces, type annotations)", () => { const source = ` @@ -247,8 +184,8 @@ describe("scanRuleSource", () => { expect(violations[0].line).toBe(2); expect(violations[0].column).toBe(0); expect(violations[0].endLine).toBe(2); - // endColumn covers the search text "eval(" = 5 chars - expect(violations[0].endColumn).toBe(5); + // endColumn covers the banned-global identifier "eval" = 4 chars + expect(violations[0].endColumn).toBe(4); }); }); @@ -273,123 +210,44 @@ describe("scanRuleSource", () => { }); describe("scanImportedRuleSource", () => { - describe("imported-only: Bun.env access", () => { - test("blocks Bun.env.FOO read", () => { - const source = `const token = Bun.env.FOO;`; - const violations = scanImportedRuleSource(source); - const envViolations = violations.filter((v) => - v.message.includes("Bun.env") - ); - expect(envViolations).toHaveLength(1); - expect(envViolations[0].message).toContain( - "Bun.env access is blocked in imported rule files" - ); - }); - - test("blocks bare Bun.env access", () => { - const source = `const env = Bun.env;`; - const violations = scanImportedRuleSource(source); - const envViolations = violations.filter((v) => - v.message.includes("Bun.env") - ); - expect(envViolations).toHaveLength(1); - }); - }); - - describe("imported-only: process.env access", () => { - test("blocks process.env read", () => { - const source = `const val = process.env.SECRET;`; - const violations = scanImportedRuleSource(source); - const envViolations = violations.filter((v) => - v.message.includes("process.env") - ); - expect(envViolations).toHaveLength(1); - expect(envViolations[0].message).toContain( - "process.env access is blocked in imported rule files" - ); - }); - }); - - // require() is blocked for every rule file by scanRuleSource, not just - // imported ones, so it is reported once with the base message rather than - // twice with two. - describe("require() call", () => { - test("blocks require() call exactly once", () => { - const source = `const mod = require("some-module");`; - const violations = scanImportedRuleSource(source); - const requireViolations = violations.filter((v) => - v.message.includes("require()") - ); - expect(requireViolations).toHaveLength(1); - expect(requireViolations[0].message).toContain( - "require() is blocked in rule files" - ); - }); - }); - - describe("imported-only: WebSocket usage", () => { - test("blocks new WebSocket()", () => { - const source = `const ws = new WebSocket("ws://localhost");`; - const violations = scanImportedRuleSource(source); - const wsViolations = violations.filter((v) => - v.message.includes("WebSocket") - ); - expect(wsViolations).toHaveLength(1); - expect(wsViolations[0].message).toContain( - "new WebSocket() is blocked in imported rule files" - ); - }); - - test("blocks WebSocket() without new", () => { - const source = `const ws = WebSocket("ws://localhost");`; - const violations = scanImportedRuleSource(source); - const wsViolations = violations.filter((v) => - v.message.includes("WebSocket") - ); - expect(wsViolations).toHaveLength(1); - expect(wsViolations[0].message).toContain( - "WebSocket() is blocked in imported rule files" - ); - }); - }); - - describe("multiple imported-only violations", () => { - test("reports all imported-only violations together", () => { - const source = ` -const token = Bun.env.TOKEN; -const secret = process.env.SECRET; -const mod = require("dangerous"); -const ws = new WebSocket("ws://localhost"); -`; - const violations = scanImportedRuleSource(source); - const importedMessages = violations.map((v) => v.message); + // scanImportedRuleSource now delegates to scanRuleSource: the patterns that + // were once imported-only (Bun.env, process.env, require, WebSocket) are + // blocked for every rule file by the banned-globals check, because each names + // a banned global. These confirm the block reaches through the imported + // entry point. + describe("previously imported-only patterns are now always blocked", () => { + const cases: Array<[string, string, string]> = [ + ["Bun.env read", `const token = Bun.env.FOO;`, "Bun"], + ["process.env read", `const val = process.env.SECRET;`, "process"], + ["require()", `const mod = require("some-module");`, "require"], + [ + "new WebSocket()", + `const ws = new WebSocket("ws://localhost");`, + "WebSocket", + ], + ]; + for (const [label, source, global] of cases) { + test(`blocks ${label}`, () => { + const violations = scanImportedRuleSource(source); + expect( + violations.some((v) => v.message.includes(`"${global}" global`)) + ).toBe(true); + }); + } - expect(importedMessages.some((m) => m.includes("Bun.env"))).toBe(true); - expect(importedMessages.some((m) => m.includes("process.env"))).toBe( - true - ); - expect(importedMessages.some((m) => m.includes("require()"))).toBe(true); - expect(importedMessages.some((m) => m.includes("new WebSocket()"))).toBe( - true - ); + test("reports a banned global once, not twice", () => { + const violations = scanImportedRuleSource(`const mod = require("x");`); + expect( + violations.filter((v) => v.message.includes(`"require" global`)) + ).toHaveLength(1); }); - }); - - describe("standard violations are included", () => { - test("includes standard scanRuleSource violations alongside imported-only ones", () => { - const source = ` -import { readFileSync } from "node:fs"; -const token = Bun.env.TOKEN; -eval("code"); -`; - const violations = scanImportedRuleSource(source); - const messages = violations.map((v) => v.message); - // Standard violations (from scanRuleSource) + test("still includes standard scanRuleSource violations", () => { + const messages = scanImportedRuleSource( + `import { readFileSync } from "node:fs";\nconst token = Bun.env.TOKEN;` + ).map((v) => v.message); expect(messages.some((m) => m.includes('"node:fs"'))).toBe(true); - expect(messages.some((m) => m.includes("eval()"))).toBe(true); - // Imported-only violation - expect(messages.some((m) => m.includes("Bun.env"))).toBe(true); + expect(messages.some((m) => m.includes(`"Bun" global`))).toBe(true); }); }); @@ -442,40 +300,37 @@ export default { }); describe("violation location for imported checks", () => { - test("reports correct line and column for Bun.env", () => { + test("reports the location of the banned Bun global", () => { const source = `const x = 1;\nconst t = Bun.env.TOKEN;`; - const violations = scanImportedRuleSource(source); - const envViolation = violations.find((v) => - v.message.includes("Bun.env") + const violation = scanImportedRuleSource(source).find((v) => + v.message.includes(`"Bun" global`) ); - expect(envViolation).toBeDefined(); - expect(envViolation!.line).toBe(2); - expect(envViolation!.column).toBe(10); - // "Bun.env" is 7 chars, so endColumn = 10 + 7 = 17 - expect(envViolation!.endColumn).toBe(17); + expect(violation).toBeDefined(); + expect(violation!.line).toBe(2); + expect(violation!.column).toBe(10); + // The identifier "Bun" is 3 chars, so endColumn = 10 + 3 = 13. + expect(violation!.endColumn).toBe(13); }); - test("reports correct line and column for require()", () => { + test("reports the location of the banned require global", () => { const source = `const a = 1;\nconst b = 2;\nconst m = require("foo");`; - const violations = scanImportedRuleSource(source); - const reqViolation = violations.find((v) => - v.message.includes("require()") + const violation = scanImportedRuleSource(source).find((v) => + v.message.includes(`"require" global`) ); - expect(reqViolation).toBeDefined(); - expect(reqViolation!.line).toBe(3); - expect(reqViolation!.column).toBe(10); - // "require(" is 8 chars, so endColumn = 10 + 8 = 18 - expect(reqViolation!.endColumn).toBe(18); + expect(violation).toBeDefined(); + expect(violation!.line).toBe(3); + expect(violation!.column).toBe(10); + // "require" is 7 chars, so endColumn = 10 + 7 = 17. + expect(violation!.endColumn).toBe(17); }); - test("reports correct line for new WebSocket()", () => { + test("reports the line of the banned WebSocket global", () => { const source = `const x = 1;\nconst ws = new WebSocket("ws://localhost");`; - const violations = scanImportedRuleSource(source); - const wsViolation = violations.find((v) => - v.message.includes("WebSocket") + const violation = scanImportedRuleSource(source).find((v) => + v.message.includes(`"WebSocket" global`) ); - expect(wsViolation).toBeDefined(); - expect(wsViolation!.line).toBe(2); + expect(violation).toBeDefined(); + expect(violation!.line).toBe(2); }); }); });