fix(engine): reject rule-file reads through a symlinked ancestor directory by rhuanbarreto · Pull Request #499 · archgate/cli

rhuanbarreto · 2026-07-25T00:12:20Z

Split out of #497 at your request, following the same call you made on #491 (pre-existing security finding → standalone PR).

Surfaced by CodeRabbit while reviewing #497. The finding is real and predates that PR — #497 only moved this code verbatim out of runner.ts, so the gap exists on main today.

The escape

safePath() lstat'd only the final path component. Given a symlinked ancestor:

<root>/linkdir  ->  /somewhere/outside

then ctx.readFile("linkdir/secret.txt") gets through every gate:

resolveUserPath() uses resolve(), which is purely lexical — it does not resolve symlinks, so the path stays <root>/linkdir/secret.txt.
isWithinRoot() is a string-prefix test on that lexical path — passes.
lstatSync(absPath) inspects only the leaf, which is an ordinary file, not a link — passes.
Bun.file(absPath).text() hands the path to the OS, which does resolve linkdir — and reads outside the project.

So a .rules.ts file could read arbitrary files outside the project through a symlinked directory, which is exactly the boundary ARCH-024 exists to hold.

The fix

The leaf check and the ancestor check become one walk over every path component below the project root.

Two deliberate constraints:

Nothing at or above the root is inspected. The project root's own location is the user's business, and on macOS the temp prefix is itself a symlink (/var → /private/var) — walking above the root would reject every path under a temp-dir root, breaking a large share of the test suite for no security gain.
Each component is a boolean lstat, not a string comparison against realpath. A realpath-and-compare implementation is one syscall instead of N and was my first instinct, but realpath case-canonicalizes on Windows and macOS, so it would reject case-mismatched-but-legitimate paths. The boolean question has no such failure mode.

Verified directories are memoized per process. safePath runs once per glob result and sibling files share every ancestor, so without the memo the walk would re-lstat the same handful of directories thousands of times per check run (grepFiles × 40+ rules).

Tests

tests/engine/safe-path.test.ts — ancestor symlink rejected, leaf symlink rejected, deep real-directory chain accepted, non-existent in-root path tolerated, root itself accepted, traversal rejected
tests/commands/check-security.test.ts — end-to-end through runChecks: a rule reading through a symlinked parent is blocked, and a rule reading a deeply nested real path still succeeds (guards against over-rejection)

The regression tests actually run on Windows. They create the directory link with symlink type "junction" — ignored on POSIX (plain symlink), but on Windows a junction needs no admin privileges and lstat reports it as a symbolic link. The pre-existing file-symlink test still skips on Windows, since file symlinks require elevation or Developer Mode; that limitation is now isolated to that one case rather than covering the whole symlink surface.

I confirmed the fix is load-bearing rather than trusting a green suite: with the guard call stubbed out, the ancestor test fails; with it in place, it passes.

Note on overlap with #497

The helpers move to src/engine/safe-path.ts here because runner.ts sits exactly at the 500-line max-lines cap on main — the fix cannot land in place. #497 performs the same extraction independently for the same reason. The two versions of the file are identical apart from this fix, so whichever merges second needs a trivial rebase.

bun run validate passes (lint, typecheck, format:check, full suite, archgate check 44/44, knip, build:check).

…ctory safePath() lstat'd only the FINAL path component, so a symlinked ancestor escaped the sandbox: with <root>/docs a link to somewhere outside the project, <root>/docs/secret.txt is an ordinary file — resolve() is purely lexical so isWithinRoot() passes, and the leaf lstat reports a regular file — yet the OS resolves through the link and a .rules.ts file reads outside the project. The leaf check and the new ancestor check are now one walk over every path component below the project root. Components at or above the root are deliberately not inspected: the root's own location is the user's business, and on macOS the temp prefix is itself a symlink (/var -> /private/var), which would otherwise reject every path under a temp-dir root. Each component is a boolean lstat rather than a string comparison against realpath output, which case-canonicalizes on Windows and macOS and would reject case-mismatched-but-legitimate paths. Verified directories are memoized per process — safePath runs per glob result and siblings share every ancestor, so the walk would otherwise re-lstat the same directories thousands of times per check run. The helpers move to src/engine/safe-path.ts because runner.ts sits exactly at the 500-line max-lines cap, so the fix cannot land in place. PR #497 performs the same extraction independently; whichever merges second needs a trivial rebase. The regression tests use symlink type "junction", ignored on POSIX but on Windows creating a directory junction that needs no admin privileges and that lstat reports as a symlink — so the ancestor case runs for real on every platform, unlike file symlinks which still require elevation. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>

coderabbitai · 2026-07-25T00:12:29Z

Warning

Review limit reached

@rhuanbarreto, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 84e77e82-ccbb-498f-99f8-833014f32713

📥 Commits

Reviewing files that changed from the base of the PR and between 0015542 and a370b8f.

📒 Files selected for processing (4)

src/engine/runner.ts
src/engine/safe-path.ts
tests/commands/check-security.test.ts
tests/engine/safe-path.test.ts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

cloudflare-workers-and-pages · 2026-07-25T00:13:29Z

Deploying archgate-cli with Cloudflare Pages

Latest commit:	`a370b8f`
Status:	✅ Deploy successful!
Preview URL:	https://63213d2b.archgate-cli.pages.dev
Branch Preview URL:	https://fix-safepath-ancestor-symlin.archgate-cli.pages.dev

View logs

github-actions · 2026-07-25T00:16:08Z

Code Coverage

Metric	Value
Lines	91.3% (8165 / 8942)
Threshold	90% minimum — met
Platforms	Linux + Windows

Full HTML report available in workflow artifacts.

Per-directory breakdown

Directory	Coverage	Lines
`src/commands/`	89.0%	1959 / 2200
`src/engine/`	93.9%	2048 / 2181
`src/formats/`	98.7%	148 / 150
`src/helpers/`	90.9%	4010 / 4411

rhuanbarreto · 2026-07-25T00:37:31Z

There's one caveat here to take care of: what if the symlink is on a project main worktree? We need to check what to do.

rhuanbarreto · 2026-07-25T09:57:24Z

Note on ordering: #497 is now stacked on this branch (its base is fix/safepath-ancestor-symlink), so this PR should merge first — GitHub will retarget #497 to main automatically.

The reason is that #497 is the PR that creates src/engine/safe-path.ts (the extraction is forced by runner.ts sitting exactly at the 500-line max-lines cap). Merging it ahead of this one would land a freshly-added file containing the known ancestor-symlink escape. Stacking makes that ordering structural instead of a note in a PR description.

Since both branches now share this commit, the earlier caveat about "whichever merges second needs a trivial rebase" no longer applies — there is no duplicate safe-path.ts to reconcile.

CodeRabbit hasn't reviewed this PR yet — it was rate-limited when the PR opened.

@throws

#499 extracted safePath/isWithinRoot/resolveUserPath from runner.ts into src/engine/safe-path.ts and hardened them against symlinked ancestor directories; take main's side for that move. The new file's 11-line symlink rationale exceeded GEN-004's bound. ARCH-024 explicitly scopes itself out of ctx.readFile/ctx.glob path sandboxing, so the rationale moves to ARCH-022 clause 1, which already depends on safePath's 'no symlink escapes' guarantee but did not say what it covers. The comment keeps the invariant plus @throws and a pointer. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>

rhuanbarreto · 2026-07-25T10:09:52Z

You were right, and it's worse than a caveat — it's a regression this PR introduced, now on main. Fix in #500.

The check rejected any symlinked component below the root regardless of where it pointed. But a link resolving back inside the project escapes nothing, and symlinked directories inside a repository are an ordinary layout — pnpm/npm/yarn/bun all symlink node_modules/<pkg> → packages/<pkg>, and projects symlink shared source dirs. So archgate would refuse to read files it is meant to govern, failing the rule with access denied.

Reproduced before fixing (junction, so it runs on Windows too):

INSIDE-pointing symlink: REJECTED <<< FALSE POSITIVE
  Path "apps/web/shared/index.ts" traverses symbolic link "shared" — access denied
OUTSIDE-pointing symlink: REJECTED (correct)

My mistake was conflating "is a symlink" with "escapes the project". The boundary is the target, not the link. #500 resolves each component and rejects only when the target lands outside the root — the escape closed here stays closed, including for a chain that hops inside first and then out.

Worth noting the sequencing trap I walked into: I verified this fix was load-bearing (stubbing the guard made the test fail) but never tested the opposite direction — that legitimate in-project symlinks still work. A guard test proves the gate closes, not that it opens for what it should. That is what let an over-broad rejection through a green suite.

The ancestor-symlink check from #499 rejected any symlinked path component below the project root, regardless of where it pointed. That over-rejects: a link resolving back inside the project escapes nothing, and symlinked directories inside a repository are an ordinary layout. Workspace monorepos symlink node_modules/<pkg> to packages/<pkg> (pnpm, npm, yarn and bun all do this), and projects symlink shared source directories — so archgate refused to read files it is meant to govern, failing the rule with "access denied". The gate is now where a symlink POINTS rather than that one exists: each component below the root is resolved and rejected only when its target lands outside the root. The escape #499 closed stays closed — an outside-pointing link is still rejected at any component, including after an earlier hop that legitimately stayed inside. Comparison is realpath-against-realpath, never realpath-against-lexical: realpath case-canonicalizes on Windows and macOS, so mixing the two would reject case-mismatched-but-legitimate paths. Both sides are resolved so the same canonicalization applies to each, and the root's own realpath is memoized alongside the per-component memo. The component memo is keyed by (root, component) rather than component alone: the same component can sit under two roots with different verdicts, since a link leaving root A may land inside an enclosing root B. Note this also relaxes the pre-existing LEAF symlink check, which likewise rejected in-project links. Leaving the leaf strict while allowing ancestors would be arbitrary — a file reachable as packages/shared/index.ts should not become unreadable because it is also linked as index.ts. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>

Closes #490. ## Context #490 asked for shared helper modules importable from `.rules.ts` files. That was declined in #491 — following relative imports would turn the scanner's single-file view into a recursive resolver plus containment checker, which becomes the security boundary (exactly what ARCH-024 declined and named as a tracked risk). The agreed path for the underlying duplication pain was to move the common helpers onto `ctx` instead, and `readYAML` (with frontmatter support) plus `checkCase` were the two named there. This PR adds both. The sandbox boundary is untouched — no new subprocess site, no new resolver, no new dependency (`Bun.YAML` is a runtime built-in, per ARCH-006). ## `ctx.readYAML(path)` Returns one object covering both YAML documents and Markdown frontmatter: ```typescript interface ReadYamlResult { frontmatter: Record<string, YamlValue> | null; content: YamlValue; } ``` Dispatch is **extension-based**, not content-sniffing: - **`.yml` / `.yaml`** — the whole document parses as YAML. `frontmatter` is always `null`, `content` is the parsed value. Because dispatch is by extension, a multi-document stream's `---` separators (Kubernetes manifests, CI configs) are never misread as a frontmatter block. - **Every other file** (typically Markdown) — the leading `---`-delimited block parses as `frontmatter`: `null` when absent, `{}` when present but empty. `content` is the remaining body text, trimmed, and is **not** parsed as YAML. `null` for "no frontmatter" is deliberate and mirrors `fileAtBase()`: "does this file have frontmatter?" is ordinary rule control flow, checkable with one null test rather than a `try/catch`. Values are typed `YamlValue` (the JSON-like data YAML's core schema produces) rather than `unknown`, so after a `typeof` check a rule can index into mappings and sequences without casting. Fail-closed like `ast()`: a malformed `.yml` file, or a frontmatter block that is invalid YAML or parses to a non-mapping (scalar/sequence), **throws** — surfacing as a rule execution error (exit 2) instead of a silent false pass. Reads go through the same `safePath` sandbox as `readFile`, and share the per-run file-text cache; the parsed value is deliberately **not** cached, matching `readJSON` (rules receive a mutable object, and sharing one instance would leak mutations between rules). ## `ctx.checkCase(value, scheme)` Pure and synchronous — no `await`. Schemes: `kebab-case`, `camelCase`, `PascalCase`, `snake_case`, `SCREAMING_SNAKE_CASE`. Matching is all-or-nothing and ASCII-only; the empty string matches no scheme. `camelCase`/`PascalCase` follow the ecosystem convention (typescript-eslint's `naming-convention`): a leading lower/uppercase letter followed by any ASCII alphanumerics, so acronym runs (`parseURL`, `HTTPServer`) match. An unrecognized scheme name **throws** rather than returning `false`, so a typo surfaces as a rule error instead of a silent false pass/fail. ## Incidental: ARCH-022 gains a fifth rule Wiring these helpers made it concrete that the `RuleContext` mirror between `src/formats/rules.ts` and the generated ambient shim in `src/helpers/rules-shim.ts` is maintained entirely **by hand**, and that only the `ast()` signature was enforced (`single-ast-method`). A drifted shim silently hands rule authors wrong types with no signal. The new `rulecontext-shim-parity` rule extracts the `interface RuleContext` member names from both surfaces and fails on any member present in one but missing from the other, in **both** directions. Extraction is a regex over raw text rather than `ctx.ast()` — `Bun.Transpiler` erases type-only interface declarations before the tree is built, so the AST never contains them. Both failure directions were fire-tested against a deliberately drifted shim before committing; the rule is member-name parity only, so full signature/JSDoc parity stays a documented manual review item. ## Stacked on #499 > **Base branch is `fix/safepath-ancestor-symlink` (#499), not `main`.** Merge #499 first; GitHub retargets this PR to `main` automatically. `safePath`/`isWithinRoot`/`resolveUserPath` move out of `runner.ts` into `src/engine/safe-path.ts` because `runner.ts` sits exactly at the 500-line `max-lines` cap. Review flagged that the extracted `safePath` only `lstat`s the **final** path component, so a symlinked ancestor directory escapes the sandbox. That gap is pre-existing — `git diff` against `main` shows the extraction is byte-for-byte apart from line-wrapping — so the fix went to its own fast-trackable PR (#499), per the standing preference for pre-existing security findings. But this is the PR that *creates* `safe-path.ts`, so merging it first would land a freshly-added file containing a known escape. Rather than leave that to merge-order convention, this PR is stacked on #499: the fix is present in this branch, the review thread is addressed in this PR's own diff, and the ordering hazard is structural rather than advisory. ## Tests - `tests/engine/yaml-utils.test.ts` — whole-document vs frontmatter dispatch, multi-document stream not misread as frontmatter, BOM, CRLF, empty block, body-never-parsed-as-YAML, invalid YAML, non-mapping block - `tests/engine/check-case.test.ts` — accept/reject tables per scheme, empty string, non-ASCII, unknown-scheme throw - `tests/engine/runner-yaml-case.test.ts` — end-to-end through `runChecks`, including the `safePath` sandbox on `readYAML` and a realistic kebab-case filename rule - `tests/engine/safe-path.test.ts` — extracted sandbox helpers ## Docs `reference/rule-api.mdx` in all three locales (en, nb, pt-br) per GEN-002: the `RuleContext` interface listing plus new `readYAML` and `checkCase` sections. `bun run validate` passes (lint, typecheck, format:check, 1600+ tests, `archgate check` 45/45, knip, build:check). --------- Signed-off-by: Rhuan Barreto <rhuan@barreto.work> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

# archgate ## [0.51.0](v0.50.0...v0.51.0) (2026-07-26) ### Features * **adrs:** enforce concise, forward-only code comments (GEN-004) ([#496](#496)) ([9a114b3](9a114b3)), references [#2123](https://github.com/archgate/cli/issues/2123) * **adrs:** flag stray files at the repository root (GEN-005) ([#535](#535)) ([6a6e765](6a6e765)), closes [#514](#514), references [#500](#500) * **engine:** add ctx.readYAML and ctx.checkCase rule helpers ([#497](#497)) ([c5d82c5](c5d82c5)), closes [#490](#490), references [#490](#490) [#491](#491) [#499](#499) [#499](#499) [#499](#499) [#499](#499) * report truncated ADR briefings, trim the ADR corpus 15.6%, add GEN-005 briefing budget ([#501](#501)) ([a9dab40](a9dab40)) ### Bug Fixes * **docs:** relocate ADR content to clear briefing-budget warnings ([#531](#531)) ([c7419b3](c7419b3)) * **docs:** restore pt-br diacritics and enforce locale content integrity ([#523](#523)) ([db39104](db39104)), closes [#516](#516), references [#231](#231) * **engine:** allow symlinks that resolve inside the project root ([#500](#500)) ([387bf15](387bf15)) * **engine:** reject rule-file reads through a symlinked ancestor directory ([#499](#499)) ([a555f9d](a555f9d)), references [#497](#497) [#491](#491) [#497](#497) [#497](#497) [#497](#497) [#497](#497) * **engine:** scan top-level export declarations with a null source ([#493](#493)) ([d07db03](d07db03)), closes [#491](#491) * **engine:** stop dropping AST nodes with exotic literal values ([#494](#494)) ([0015542](0015542)), closes [#493](#493) [#493](#493) * **lint:** resolve no-bare-env-restore by captured key and lexical scope ([#524](#524)) ([7094a3a](7094a3a)), closes [#498](#498) * **rules:** make ARCH-020 and ARCH-023 match ctx.ast() instead of raw text ([#533](#533)) ([ad5529b](ad5529b)), closes [#513](#513), references [#486](#486) * **tests:** replace bun:test anti-patterns with idiomatic patterns ([#512](#512)) ([bcb086f](bcb086f)) --- 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" } } ``` ```` ### Custom Changelog Preamble You can add custom markdown to the top of the changelog (right after the version header) using the `!simple-release/set-preamble` command. The markdown after the command line becomes the preamble. ```md !simple-release/set-preamble ## What's new? - The website was completely redesigned - The new API gives you awesome possibilities ``` In a monorepo, pass the full package name after the command to target a single package's changelog. Wrap the name in backticks so GitHub keeps it as text instead of a mention: ```md !simple-release/set-preamble `@your-org/core` ## Core changes - New plugin system ``` Use one comment per package, plus one without a name for the whole release. ### Access Restrictions The commands 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 - The last `!simple-release/set-preamble` comment per package takes priority - JSON must be valid, otherwise the `set-options` command will be ignored - Parameters apply only to the current release execution - The commands can be updated by editing the comment or adding a new one </details>  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>

rhuanbarreto mentioned this pull request Jul 25, 2026

feat(engine): add ctx.readYAML and ctx.checkCase rule helpers #497

Merged

rhuanbarreto merged commit a555f9d into main Jul 25, 2026
22 checks passed

rhuanbarreto deleted the fix/safepath-ancestor-symlink branch July 25, 2026 00:36

archgatebot Bot mentioned this pull request Jul 24, 2026

chore(release): 0.51.0 #495

Merged

rhuanbarreto mentioned this pull request Jul 25, 2026

fix(engine): allow symlinks that resolve inside the project root #500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(engine): reject rule-file reads through a symlinked ancestor directory - #499

fix(engine): reject rule-file reads through a symlinked ancestor directory#499
rhuanbarreto merged 1 commit into
mainfrom
fix/safepath-ancestor-symlink

rhuanbarreto commented Jul 25, 2026

Uh oh!

coderabbitai Bot commented Jul 25, 2026

Review limit reached

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 25, 2026

Uh oh!

github-actions Bot commented Jul 25, 2026

Uh oh!

Uh oh!

rhuanbarreto commented Jul 25, 2026

Uh oh!

rhuanbarreto commented Jul 25, 2026

Uh oh!

rhuanbarreto commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rhuanbarreto commented Jul 25, 2026

The escape

The fix

Tests

Note on overlap with #497

Uh oh!

coderabbitai Bot commented Jul 25, 2026

Review limit reached

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 25, 2026

Deploying archgate-cli with Cloudflare Pages

Uh oh!

github-actions Bot commented Jul 25, 2026

Code Coverage

Uh oh!

Uh oh!

rhuanbarreto commented Jul 25, 2026

Uh oh!

rhuanbarreto commented Jul 25, 2026

Uh oh!

rhuanbarreto commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant