fix(engine)!: close rule-file sandbox escapes via module allowlist#477
Merged
Conversation
`archgate check` imports and executes every companion `.rules.ts` in-process,
so `scanRuleSource()` is the only control between rule code and the machine
running the check. Its `ImportExpression` case rejected only *non-literal*
specifiers, never testing a literal one against the module ban that
`ImportDeclaration` enforced:
import { spawn } from "node:child_process"; // blocked
const cp = await import("node:child_process"); // executed
Verified end to end: a rule file ran a subprocess, wrote an arbitrary file,
and `check` reported `"pass": true` with zero violations.
The literal-`import()` hole was one spelling among many. `./evil.ts`,
`data:text/javascript,...`, bare packages, `node:module`'s `createRequire`,
`require()`, `import.meta.require()`, `process.binding("spawn_sync")` and
`export * from` all executed, none naming a banned builtin. The denylist was
the wrong shape: rule files run in-process, so any reachable module is
arbitrary code execution, and the ways to name one are unbounded.
Replace it with an allowlist (`node:path|url|util|crypto`, `node:`-prefixed
only — bare `path` is shadowable by the target project's node_modules), and
route every module-evaluating construct through it: static import, dynamic
import, and `export ... from`. Block the escapes that name no module, matching
property names in both `o.name` and `o["name"]` spellings so
`process["binding"]` and an aliased receiver are caught too.
Add a raw-text pass for bidi and invisible characters (Trojan Source,
CVE-2021-42574) — the one class the AST cannot report, since it sees the true
program while the attack targets the human reading the diff. That pass
deliberately does not search for dangerous names: it would be weaker than the
AST, which resolves the escapes a regex misses, and would false-positive on
this repo's own rules files, which name those strings as search patterns.
BREAKING CHANGE: `.rules.ts` files may now only import node:path, node:url,
node:util and node:crypto. Rules reaching for language tooling must use
`ctx.ast()` (ARCH-022), which is the sanctioned path to a subprocess.
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
`scanImportedRuleSource()` had zero production call sites. It was written and unit-tested for exactly this purpose, but nothing ever called it, so its stricter checks never ran against a single imported rule — a security control that was dead code. `archgate adr import` copies third-party `.rules.ts` from a pack into `.archgate/adrs/`, and the next `archgate check` imports and executes them. Scan them in `writeImportedAdrs()` and refuse the whole import on violations. Import time is the only point where this is possible: once a file lands in `.archgate/adrs/` it is indistinguishable from a rule the project wrote itself, and the engine has no provenance to key stricter checks off. Scanning before the first write also means a rejected pack needs no rollback. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
ARCH-022 depends on this boundary holding ("a rule author MUST NEVER be able
to reach Bun.spawn, child_process, or any other subprocess/filesystem
primitive directly") and mitigates its own guardrail-bypass risk by asserting
the scanner prevents it. Its References section flagged the gap: the scanner
"is not currently documented by a formal ADR". That gap is why the escapes
this ADR records went unnoticed.
Records the threat model, the denylist-to-allowlist decision and why the
unsafe set is not enumerable, the `node:`-prefix requirement, the requirement
that every module-evaluating construct share one check, and the import-time
gate for third-party rules.
Deliberately `rules: false`, and says why. The invariant is behavioural — a
rule file cannot reach child_process — while a companion rule could only
assert implementation shape. A rule checking "the loader scans before
import()" would have passed for the entire lifetime of the vulnerability: the
loader did scan, in the right order; the scan simply did not work. A
structural check cannot tell a boundary from the appearance of one.
Enforcement lives in tests/engine/rule-scanner-escapes.test.ts, where each
escape is an executable regression case.
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d1ea1911-64a0-4d16-9b76-4dbb5ded6c66) |
Deploying archgate-cli with
|
| Latest commit: |
6ba8cb6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ce76c4d0.archgate-cli.pages.dev |
| Branch Preview URL: | https://claude-declarative-crafting.archgate-cli.pages.dev |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
|
Follow-up capability work tracked in #479: |
main merged its own ARCH-023 (engine file listing) after this branch was cut, so the id collided — `check` failed with "Duplicate ADR ID: ARCH-023". Take the next free id, ARCH-024, and update the sole in-code reference in rule-scanner.ts. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e7d64cf6-be48-4ff2-9d0f-dad9b1cb7887) |
Contributor
Code Coverage
Full HTML report available in workflow artifacts. Per-directory breakdown
|
Merged
3 tasks
rhuanbarreto
added a commit
that referenced
this pull request
Jul 16, 2026
… allowlist Covers the user-facing surface of this PR and the previously-undocumented sandbox change from #477, across all three locales (en, nb, pt-br) per the GEN-002 same-changeset i18n requirement. reference/rule-api.mdx - fileAtBase() method; AstOptions { rev, comments }; CommentToken type - ast() section: base-revision parsing, comment collection (original-source loc, string-awareness, Python line-only), the two new throw cases - AstNode: note the opt-in comments array guides/writing-rules.mdx - ctx.ast opts + ctx.fileAtBase entries - worked examples: documentation-only-change detection via { rev: "base" }, and comment governance via { comments: true } - note that rule files may import only the safe four modules guides/security.mdx (#477 gap — was still describing the old denylist) - rewrite the sandbox section from denylist to allowlist model - correct the now-wrong claims (literal dynamic imports ARE blocked; the scanner sees through escapes) and add require/import.meta.require, process.binding, Trojan-Source Unicode, and imported-rule scanning - reframe the "review untrusted rules" note: strong first line of defense, not a complete sandbox (Reflect.get indirection remains a residual) Cross-page anchors normalized to be locale-agnostic. llms-full.txt regenerated. Validated: 44/44 ADR rules incl. GEN-002 i18n parity + drift. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
3 tasks
rhuanbarreto
added a commit
that referenced
this pull request
Jul 16, 2026
…x escapes (#481) ## Summary The module allowlist (#477) stopped rule files from *importing* dangerous modules — but `Bun`, `process`, and the global object are **live globals** in the rule runtime, reachable with no import at all. The scanner blocked the shapes `Bun.spawn` and `Bun[x]`, which is the same losing game a module denylist was. Two fire-tested RCEs walked straight around it, both reported `"pass": true` by `archgate check`: ```js // (1) reflection / aliasing / destructuring / global-object aliases Reflect.get(Bun, "spawn")([...]); // also: const {spawn}=Bun; const B=Bun; // globalThis.Bun.spawn / global.Bun.spawn / self.Bun.spawn // (2) Function constructor via the .constructor chain === eval — bypasses even the module allowlist (() => {}).constructor("return import('node:child_process')")(); ``` ## The fix — apply the allowlist philosophy to globals Block **naming the capability source**, not the shapes of using it. Any code reference to `Bun`, `process`, `globalThis`, `global`, `self`, `Reflect`, `eval`, `Function`, `fetch`, `WebSocket`, `XMLHttpRequest`, `EventSource`, or `require` is refused — in any position except a property-key slot (`foo.process`, `{ process: 1 }` name a property, not the global, and are fine). Blocking the *identifier* closes aliasing, destructuring, and reflection in one rule. `global`/`self` are included because Bun binds the global object under all three names. Additionally block **`.constructor`** access (dotted + computed-literal) on any receiver — the property-chain route to the `Function` constructor (= eval). **This simplified the scanner** (net **−157 lines**): the per-shape `Bun`/`process` member denylists and the separate `eval`/`Function`/`fetch`/`require` call checks are gone, subsumed by the single identifier block. First-party and imported scans **converge** — `scanImportedRuleSource()` now delegates — because a first-party rule runs with full privilege too and a malicious PR can add one. An audit confirmed **zero** of the repo's own `.rules.ts` reference these globals as executable code (every mention is a searched-for string), so there are **no false positives**. ## Known residual (documented + regression-tested) A property name built at runtime — `const c = "constructor"; (() => {})[c]` — is unknowable to a static scanner, and blocking *all* computed access would reject ordinary `arr[i]`/`obj[key]`. That route to eval is left to **execution-time isolation**; the scan is defense-in-depth that raises the bar from a trivial one-liner to runtime string construction, not a jail. A test asserts this residual explicitly so it's a deliberate, known gap. ## Test plan - [x] `tests/engine/rule-scanner-escapes.test.ts` — new "reflective and aliased access to runtime globals" block: aliasing, destructuring, `Reflect.get`, the three global-object aliases, `Object.getOwnPropertyDescriptor`, the Function-constructor chain (dotted + computed-literal), eval/Function/fetch/require aliasing; plus "legitimate global-adjacent code still passes" (`Object.keys`, a property named `process`, a normal `ctx`-only rule) and the explicit computed-variable residual - [x] Fire test — both RCEs now refused end-to-end (`"pass": false`), no payload written - [x] `bun run validate` — 44/44 ADR rules (dogfooded on the repo's own rules), 1527 tests, lint, typecheck, build > [!WARNING] > **BREAKING:** rule files may no longer name `Bun`/`process`/`globalThis`/etc., even for benign reads (`Bun.env`, `process.platform`, `Bun.Glob`). Rules interact with the project only through `ctx`; a rule that genuinely needs such data is a `ctx` feature request. Small false-positive surface: a rule using one of these names as a local variable (`self`, `global`) must rename. ## ADR Amends [ARCH-024](https://github.com/archgate/cli/blob/main/.archgate/adrs/ARCH-024-rule-file-sandbox-boundary.md), clause 4: rewrites the "escapes that name no module" model to "block naming the global," documents the `.constructor` closure, the scan convergence, and the computed-variable-key residual as the static-analysis limit that execution-time isolation answers. ## Doc coordination `guides/security.mdx` currently names `Reflect.get(Bun,"spawn")` as a residual bypass (on `main`, and reworded in #480's rewrite). This PR closes that specific residual, so that note should be updated to reflect the *new* residual (a runtime-computed `.constructor` key). To avoid editing soon-replaced text and redundant trilingual i18n churn here, I'll make that doc update on the #480 branch (which owns the rewritten security section) so the two don't conflict — flagging for whoever sequences the merges. --------- Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
rhuanbarreto
added a commit
that referenced
this pull request
Jul 16, 2026
…) (#480) Closes #479. Both capability gaps that the sandbox tightening (#477 / ARCH-024) exposed, delivered through the sanctioned `ctx.ast()` door. Two commits, reviewable separately. ## Front A — base-revision access (`39e511f`) Rules can inspect a file at its **base git revision**, not only the working tree — the capability whose absence forced rule authors to shell out to a subprocess. - **`ast(path, language, { rev: "base" })`** — parses the file at the comparison base commit. Same shapes, same throw contract. - **`fileAtBase(path): Promise<string | null>`** — raw base source, or `null` when no base is resolved or the file was added since the base. The base is the **merge base of `--base` and HEAD** (the commit `changedFiles` diffs against). No new privileged path: git reads live in the already-sanctioned `git-files.ts`; all four `ast()` guardrails run in order; TS/JS parse in-process, Python/Ruby base content goes to an OS-temp file outside the project tree with the same `-I` isolation. `ast({rev:'base'})` throws distinguishably (no-base vs added-file); `fileAtBase` returns `null` so a rule can branch without a `try/catch`. Unblocks documentation-only / no-op change detection with zero subprocess use. ## Front B — comment access (`95db9bd`) Comment-governance rules can work against structured comment tokens instead of fragile line-regex. - **`ast(path, language, { comments: true })`** attaches a `comments` array of `{ type: "line" | "block"; value; loc }` to the tree. - **typescript / javascript / python** supported; **ruby** throws a clear "not supported yet" (Ripper.lex deferred). - `value` has delimiters stripped. **TS/JS comments are scanned from the ORIGINAL source** (Bun.Transpiler strips them before meriyah) — so comment `loc` is original-source-accurate even for TS, unlike the tree's own transpiled-relative `loc`. String/template aware; regex literals are a known blind spot. - **Python** comments come from `tokenize` via a second serializer (`{_tree, comments}`), same `convert()` preamble, same `-I`. Folded into `ast()` (not a new method) so it rides the existing four-guardrail flow and `single-ast-method` stays satisfied. No new subprocess site, no new guardrail. ## Why this is safe Both features add **no new subprocess site and no new guardrail**. ARCH-022's four companion rules (`ast-guardrail-ordering`, `no-unsanctioned-engine-subprocess`, `single-ast-method`, `python-subprocess-isolated`) all still pass — the new code rides entirely inside them. ## Test plan - [x] `tests/engine/runner-ast-base.test.ts` — 10 cases: git reads, `fileAtBase`, TS `{rev:'base'}` vs working tree, comment-only structural equivalence, both throw cases, Python doc-only detection - [x] `tests/engine/runner-ast-comments.test.ts` — 7 cases: TS line/block extraction (stripped values + original-source loc), string-awareness, JS, opt-in-only, ruby-throws, Python tokenize (incl. `#`-in-string exclusion + base+comments composition) - [x] `bun run validate` — 44/44 ADR rules, 1523 tests, lint, typecheck, format, knip, build ## ADR Amends [ARCH-022](https://github.com/archgate/cli/blob/main/.archgate/adrs/ARCH-022-ast-aware-rule-context.md) with both surfaces, their guardrail-compliance arguments, the throw-vs-null contract, the original-source-loc advantage for TS comments, the honest limitations (regex blind spot; `comments` as a mild root-shape addition), and cross-references to ARCH-024. --------- Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
rhuanbarreto
pushed a commit
that referenced
this pull request
Jul 16, 2026
# archgate ## [0.49.0](v0.48.4...v0.49.0) (2026-07-16) ### ⚠ BREAKING CHANGES * **engine:** block naming runtime globals to close reflective sandbox escapes (#481) * **engine:** close rule-file sandbox escapes via module allowlist (#477) * **cli:** emit lean agent-facing JSON payloads by default (#476) ### Features * **cli:** emit lean agent-facing JSON payloads by default ([#476](#476)) ([04affa3](04affa3)) * **engine:** base-revision + comment access for ctx.ast() (closes [#479](#479)) ([#480](#480)) ([49feb1f](49feb1f)), closes [#477](#477) ### Bug Fixes * **engine:** block naming runtime globals to close reflective sandbox escapes ([#481](#481)) ([6666df2](6666df2)), closes [#477](#477) [#480](#480) * **engine:** close rule-file sandbox escapes via module allowlist ([#477](#477)) ([18db14d](18db14d)) --- This PR was generated with [simple-release](https://github.com/TrigenSoftware/simple-release). <details> <summary>📄 Cheatsheet</summary> <br> You can configure the bot's behavior through a pull request comment using the `!simple-release/set-options` command. ### Command Format ````md !simple-release/set-options ```json { "bump": {}, "publish": {} } ``` ```` ### Useful Parameters #### Bump | Parameter | Type | Description | |-----------|------|-------------| | `version` | `string` | Force set specific version | | `as` | `'major' \| 'minor' \| 'patch' \| 'prerelease'` | Release type | | `prerelease` | `string` | Pre-release identifier (e.g., "alpha", "beta") | | `firstRelease` | `boolean` | Whether this is the first release | | `skip` | `boolean` | Skip version bump | | `byProject` | `Record<string, object>` | Per-project bump options for monorepos | #### Publish | Parameter | Type | Description | |-----------|------|-------------| | `skip` | `boolean` | Skip publishing | | `access` | `'public' \| 'restricted'` | Package access level | | `tag` | `string` | Tag for npm publication | ### Usage Examples #### Force specific version ````md !simple-release/set-options ```json { "bump": { "version": "2.0.0" } } ``` ```` #### Force major bump ````md !simple-release/set-options ```json { "bump": { "as": "major" } } ``` ```` #### Create alpha pre-release ````md !simple-release/set-options ```json { "bump": { "prerelease": "alpha" } } ``` ```` #### Publish with specific access and tag ````md !simple-release/set-options ```json { "bump": { "prerelease": "beta" }, "publish": { "access": "public", "tag": "beta" } } ``` ```` ### Access Restrictions The command can only be used by users with permissions: - repository owner - organization member - collaborator ### Notes - The last comment with `!simple-release/set-options` command takes priority - JSON must be valid, otherwise the command will be ignored - Parameters apply only to the current release execution - The command can be updated by editing the comment or adding a new one </details> <!-- Please do not edit this comment. simple-release-pull-request: true simple-release-branch-from: release simple-release-branch-to: main --> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
archgate checkimports and executes every companion.rules.tsin-process, soscanRuleSource()is the only control between rule code and the machine running the check. It was bypassable with a one-token change.ImportExpressionrejected only non-literal specifiers, so a literal one was never tested against the module ban thatImportDeclarationenforced. Confirmed end-to-end: a rule file ran a subprocess, exfiltrated the result into a file it wrote to disk, andarchgate checkreported"pass": truewith zero violations.This is reachable by anyone who can open a PR that adds a file, since
checkis what CI runs — and by any third-party pack, sincearchgate adr importcopies.rules.tsfrom a registry and the nextcheckexecutes them.Root cause: the denylist was the wrong shape
The literal-
import()hole was one spelling among many. Every one of these executed, and none names a banned builtin:await import("./evil.ts")await import("data:text/javascript,...")import x from "some-npm-pkg"import { createRequire } from "node:module"require("node:child_process")import.meta.require(...)ImportExpressionnor a banned identifier.process.binding("spawn_sync")export * from "node:child_process"Rule files run in-process, so any reachable module is arbitrary code execution — the unsafe set is "everything that resolves to code" and is not enumerable. The safe set is four modules.
Changes
Allowlist replaces the denylist.
node:path,node:url,node:util,node:crypto—node:-prefixed only, since barepathis shadowable by anode_modules/pathin the target project, handing execution back to the untrusted code the scanner exists to contain.One check for every module-evaluating construct — static import, dynamic import (literal and non-literal),
export ... from,export * from. A gap in one is a gap in all; that was this bug's exact shape.Escapes that name no module —
require(),import.meta.require(),process.binding/dlopen, matched in botho.nameando["name"]spellings soprocess["binding"]and an aliased receiver (const p = process) are caught.Raw-text pass for bidi/invisible characters (Trojan Source, CVE-2021-42574) — the one class the AST cannot report: it sees the true program, while the attack targets the human reading an imported pack's diff. It deliberately does not search for dangerous names — that would be weaker than the AST (the raw text of
await import("\x6eode:child_process")doesn't containnode:child_process; the parser resolves it and the allowlist catches it) and would false-positive on this repo's own ARCH-007/014/022 rules, which name those strings as search patterns.Import-time gate —
scanImportedRuleSource()had zero production call sites. It was written and tested for untrusted packs, but nothing called it, so its stricter checks never ran. Now enforced inwriteImportedAdrs(), refusing before anything reaches disk.Governance
Adds ARCH-024 — Rule File Sandbox Boundary, closing a gap ARCH-022 explicitly flagged ("this mechanism itself is not currently documented by a formal ADR"). ARCH-022's guardrail-bypass mitigation depends on this scanner holding.
It is deliberately
rules: false. The invariant is behavioural; a companion rule could only assert implementation shape — and a rule checking "the loader scans beforeimport()" would have passed for the vulnerability's entire lifetime, because the loader did scan; the scan just didn't work. Enforcement is executable:tests/engine/rule-scanner-escapes.test.ts.Test plan
tests/engine/rule-scanner-escapes.test.ts— 38 cases: every escape above, obfuscated specifiers, computed/aliased access, the text pass, plus legitimate rule shapes that must still passPWNED.txtnever writtenbun run validate— 1485 pass / 0 fail, 43/43 ADR rules, knip, buildWarning
BREAKING:
.rules.tsmay now only importnode:path,node:url,node:util,node:crypto. Rules needing language tooling must usectx.ast()(ARCH-022), the sanctioned path to a subprocess. This breaks rule files doing legitimate work by illegitimate means, which is intended.Known limits (recorded in ARCH-024)
p[k]) is unknowable without value tracking; blocking all computed access would reject ordinaryo[key].ARCH-024 names execution-time isolation (worker/subprocess/restricted resolver) as the sanctioned direction to strengthen the boundary's nature, in its own ADR.