fix(engine)!: block naming runtime globals to close reflective sandbox escapes

.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