Skip to content

Commit 73a4bde

Browse files
committed
Merge origin/main into claude/spicy-booping-muffin
#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>
2 parents 6b0091a + a555f9d commit 73a4bde

5 files changed

Lines changed: 288 additions & 55 deletions

File tree

.archgate/adrs/ARCH-022-ast-aware-rule-context.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ This method dispatches internally based on `language`, and the dispatch mechanis
4141

4242
**Guardrail ordering — this is the core architectural constraint of this ADR.** A rule author MUST NEVER be able to reach `Bun.spawn`, `child_process`, or any other subprocess/filesystem primitive directly; `ctx.ast()` is the only door, exactly as `glob`/`grep`/`readFile` are today, and this is consistent with the sandbox `rule-scanner.ts` already enforces on `.rules.ts` source (which explicitly blocks `Bun.spawn` and `Bun.spawnSync` from rule code). All of the following MUST execute inside `createRuleContext()` in `src/engine/runner.ts`, in this order, before any subprocess is spawned:
4343

44-
1. **Path safety** — the requested `path` MUST pass through the same `safePath()` sandboxing already applied to `readFile`/`glob` (no traversal outside `scopedFiles`, no symlink escapes).
44+
1. **Path safety** — the requested `path` MUST pass through the same `safePath()` sandboxing already applied to `readFile`/`glob` (no traversal outside `scopedFiles`, no symlink escapes). "No symlink escapes" means **every component below the project root**, not just the leaf: with `<root>/docs` linked outside the project, `<root>/docs/secret.txt` is an ordinary file, so a lexical containment check and an `lstat` of the leaf both pass while the OS resolves through the link and reads outside. `assertNoSymlinkInPath()` in `src/engine/safe-path.ts` walks each component and rejects any that is a link. Two deliberate limits on that walk: components at or above the root are NOT inspected (the root's own location is the user's business, and macOS's temp prefix is itself a symlink — `/var` → `/private/var` — which would otherwise reject every temp-dir root), and each component is tested with a boolean `lstat` rather than compared against `realpath`, which case-canonicalizes on Windows and macOS and would reject case-mismatched-but-legitimate paths.
4545
2. **Language plausibility check** — the file's extension and/or leading content MUST be sanity-checked against the requested `language` before any interpreter is invoked on it. A rule calling `ctx.ast("config.json", "python")` MUST fail this check rather than hand arbitrary file content to a Python interpreter.
4646
3. **Interpreter availability probe** — for `"python"`/`"ruby"`, an availability check (e.g. `Bun.spawn([candidate, "--version"])` wrapped in `try/catch`, following the exact pattern `isClaudeCliAvailable()` uses in ARCH-007) MUST run before the real invocation. `python3` is not a universal PATH alias on Windows (the common installer exposes `python`, not `python3`); the probe MUST try platform-appropriate candidate executable names in order (e.g. `python3` then `python` on non-Windows; `python`, then `python3`, then the `py` launcher on Windows — the python.org installer registers `py` even when "Add python.exe to PATH" is unchecked — using [ARCH-009](./ARCH-009-platform-detection-helper.md)'s `isWindows()`) and use the first one that resolves for both the probe and the real invocation. This probe result MUST be cached once per `check` invocation, not re-run per file.
4747
4. **Guarded invocation** — the actual `Bun.spawn` call MUST use array-based arguments only, per ARCH-007, with no shell interpolation of file contents or paths.

src/engine/runner.ts

Lines changed: 2 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// SPDX-License-Identifier: Apache-2.0
22
// Copyright 2026 Archgate
3-
import { lstatSync } from "node:fs";
4-
import { relative, resolve, isAbsolute } from "node:path";
3+
import { relative, resolve } from "node:path";
54

65
import type {
76
AstLanguage,
@@ -16,7 +15,6 @@ import type {
1615
ViolationDetail,
1716
} from "../formats/rules";
1817
import { logDebug, logWarn } from "../helpers/log";
19-
import { UserError } from "../helpers/user-error";
2018
import {
2119
AST_LANGUAGE_EXTENSIONS,
2220
PYTHON_AST_PROGRAM,
@@ -48,59 +46,9 @@ import {
4846
import { listMatchingFiles, matchLines } from "./glob-utils";
4947
import { parseTsOrJsSource } from "./js-parser";
5048
import { type LoadResult, blockedToRuleResult } from "./loader";
49+
import { isWithinRoot, resolveUserPath, safePath } from "./safe-path";
5150
import { applySuppressions, type SuppressionWarning } from "./suppressions";
5251

53-
/**
54-
* Resolve a user-supplied path against projectRoot without any boundary check.
55-
*/
56-
function resolveUserPath(resolvedRoot: string, userPath: string): string {
57-
return isAbsolute(userPath)
58-
? resolve(userPath)
59-
: resolve(resolvedRoot, userPath);
60-
}
61-
62-
/**
63-
* Check whether an already-resolved absolute path stays within projectRoot.
64-
* On Windows, paths on different drives produce a full absolute relative()
65-
* result rather than a ".." prefix — use startsWith on the normalized paths.
66-
*/
67-
function isWithinRoot(resolvedRoot: string, absPath: string): boolean {
68-
return (
69-
absPath.startsWith(resolvedRoot + "/") ||
70-
absPath.startsWith(resolvedRoot + "\\") ||
71-
absPath === resolvedRoot
72-
);
73-
}
74-
75-
/**
76-
* Resolve a user-supplied path and ensure it stays within projectRoot.
77-
*
78-
* @throws {UserError} When the resolved path escapes the project boundary or
79-
* is a symlink.
80-
*/
81-
function safePath(resolvedRoot: string, userPath: string): string {
82-
const absPath = resolveUserPath(resolvedRoot, userPath);
83-
if (!isWithinRoot(resolvedRoot, absPath)) {
84-
throw new UserError(
85-
`Path "${userPath}" escapes project root — access denied`
86-
);
87-
}
88-
// Reject symlinks to prevent following links to files outside the project
89-
try {
90-
if (lstatSync(absPath).isSymbolicLink()) {
91-
throw new UserError(
92-
`Path "${userPath}" is a symbolic link — access denied`
93-
);
94-
}
95-
} catch (err) {
96-
// Re-throw our own errors; ignore ENOENT (file may not exist yet for glob results)
97-
if (err instanceof Error && err.message.includes("access denied")) {
98-
throw err;
99-
}
100-
}
101-
return absPath;
102-
}
103-
10452
const RULE_TIMEOUT_MS = 30_000;
10553

10654
/**

src/engine/safe-path.ts

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// Copyright 2026 Archgate
3+
import { lstatSync } from "node:fs";
4+
import { resolve, isAbsolute, join, relative } from "node:path";
5+
6+
import { UserError } from "../helpers/user-error";
7+
8+
/**
9+
* Resolve a user-supplied path against projectRoot without any boundary check.
10+
*/
11+
export function resolveUserPath(
12+
resolvedRoot: string,
13+
userPath: string
14+
): string {
15+
return isAbsolute(userPath)
16+
? resolve(userPath)
17+
: resolve(resolvedRoot, userPath);
18+
}
19+
20+
/**
21+
* Check whether an already-resolved absolute path stays within projectRoot.
22+
* On Windows, paths on different drives produce a full absolute relative()
23+
* result rather than a ".." prefix — use startsWith on the normalized paths.
24+
*/
25+
export function isWithinRoot(resolvedRoot: string, absPath: string): boolean {
26+
return (
27+
absPath.startsWith(resolvedRoot + "/") ||
28+
absPath.startsWith(resolvedRoot + "\\") ||
29+
absPath === resolvedRoot
30+
);
31+
}
32+
33+
/**
34+
* Directories verified as real (non-symlink), memoized per process: safePath
35+
* runs once per glob result and siblings share every ancestor, so without this
36+
* the walk below re-lstats the same directories thousands of times per run.
37+
*/
38+
const verifiedRealDirs = new Set<string>();
39+
40+
/**
41+
* Reject a symlink anywhere in the path below the project root — the leaf OR
42+
* any ancestor, since a linked ancestor makes the leaf look like an ordinary
43+
* file to both `isWithinRoot` and a leaf `lstat`. Components at or above the
44+
* root are deliberately not inspected, and each test is a boolean `lstat`
45+
* rather than a `realpath` comparison.
46+
*
47+
* @throws {UserError} When any component below the root is a symbolic link.
48+
* @see ARCH-022 — why the walk stops at the root and avoids `realpath`
49+
*/
50+
function assertNoSymlinkInPath(
51+
resolvedRoot: string,
52+
absPath: string,
53+
userPath: string
54+
): void {
55+
const rel = relative(resolvedRoot, absPath);
56+
// Empty (the root itself) or escaping — nothing below the root to walk.
57+
if (!rel || rel.startsWith("..")) return;
58+
59+
const segments = rel.split(/[/\\]/u);
60+
let current = resolvedRoot;
61+
for (const [index, segment] of segments.entries()) {
62+
current = join(current, segment);
63+
if (verifiedRealDirs.has(current)) continue;
64+
let stat;
65+
try {
66+
stat = lstatSync(current);
67+
} catch {
68+
// Does not exist (glob result, not-yet-created file): nothing can be
69+
// traversed through it, and the eventual read fails on its own.
70+
return;
71+
}
72+
if (stat.isSymbolicLink()) {
73+
throw new UserError(
74+
index === segments.length - 1
75+
? `Path "${userPath}" is a symbolic link — access denied`
76+
: `Path "${userPath}" traverses symbolic link "${segment}" — access denied`
77+
);
78+
}
79+
if (stat.isDirectory()) verifiedRealDirs.add(current);
80+
}
81+
}
82+
83+
/**
84+
* Resolve a user-supplied path and ensure it stays within projectRoot.
85+
* Throws if the resolved path escapes the project boundary, is a symlink, or
86+
* reaches its target through a symlinked ancestor directory.
87+
*/
88+
export function safePath(resolvedRoot: string, userPath: string): string {
89+
const absPath = resolveUserPath(resolvedRoot, userPath);
90+
if (!isWithinRoot(resolvedRoot, absPath)) {
91+
throw new UserError(
92+
`Path "${userPath}" escapes project root — access denied`
93+
);
94+
}
95+
assertNoSymlinkInPath(resolvedRoot, absPath, userPath);
96+
return absPath;
97+
}

tests/commands/check-security.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,77 @@ describe("check command security", () => {
184184
rmSync(outsideDir, { recursive: true, force: true });
185185
});
186186

187+
test("blocks reads that tunnel out through a symlinked ancestor directory", async () => {
188+
// The leaf here is an ordinary file — only an ANCESTOR is the symlink, so
189+
// a leaf-only lstat reports "not a link" while the OS still resolves the
190+
// real path outside the project and reads it.
191+
const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
192+
writeFileSync(join(outsideDir, "secret.txt"), "sensitive data");
193+
194+
try {
195+
// "junction" is ignored on POSIX (plain symlink) but on Windows creates
196+
// a directory junction, which needs no admin privileges and which
197+
// lstat() reports as a symbolic link — so unlike the file-symlink test
198+
// above, this case runs for real on every platform instead of skipping.
199+
symlinkSync(outsideDir, join(tempDir, "src", "linkdir"), "junction");
200+
} catch {
201+
rmSync(outsideDir, { recursive: true, force: true });
202+
return;
203+
}
204+
205+
writeAdrAndRule(
206+
"SEC-007",
207+
`export default {
208+
rules: {
209+
"read-through-symlinked-dir": {
210+
description: "Attempt to read a real file via a symlinked parent",
211+
async check(ctx) {
212+
await ctx.readFile("src/linkdir/secret.txt");
213+
},
214+
},
215+
},
216+
};
217+
`
218+
);
219+
220+
const loaded = await loadRuleAdrs(tempDir);
221+
const result = await runChecks(tempDir, loaded);
222+
expect(result.results[0].error).toContain("access denied");
223+
expect(result.results[0].error).toContain("symbolic link");
224+
225+
rmSync(outsideDir, { recursive: true, force: true });
226+
});
227+
228+
test("allows reads under real nested directories (no false positives)", async () => {
229+
// Guards the ancestor walk against over-rejecting: a deep, entirely real
230+
// directory chain must still be readable.
231+
mkdirSync(join(tempDir, "src", "a", "b", "c"), { recursive: true });
232+
writeFileSync(join(tempDir, "src", "a", "b", "c", "deep.ts"), "export {};");
233+
234+
writeAdrAndRule(
235+
"SEC-008",
236+
`export default {
237+
rules: {
238+
"read-deep-real-path": {
239+
description: "Read a file nested under real directories",
240+
async check(ctx) {
241+
const content = await ctx.readFile("src/a/b/c/deep.ts");
242+
if (!content.includes("export")) {
243+
ctx.report.violation({ message: "unexpected content" });
244+
}
245+
},
246+
},
247+
},
248+
};
249+
`
250+
);
251+
252+
const loaded = await loadRuleAdrs(tempDir);
253+
const result = await runChecks(tempDir, loaded);
254+
expect(result.results[0].error).toBeUndefined();
255+
expect(result.results[0].violations).toHaveLength(0);
256+
});
257+
187258
test("allows legitimate file reads within project", async () => {
188259
writeFileSync(join(tempDir, "src", "app.ts"), "export const x = 1;\n");
189260

tests/engine/safe-path.test.ts

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// Copyright 2026 Archgate
3+
import { describe, expect, test, beforeEach, afterEach } from "bun:test";
4+
import {
5+
mkdtempSync,
6+
rmSync,
7+
mkdirSync,
8+
symlinkSync,
9+
writeFileSync,
10+
} from "node:fs";
11+
import { tmpdir } from "node:os";
12+
import { join, resolve } from "node:path";
13+
14+
import {
15+
isWithinRoot,
16+
resolveUserPath,
17+
safePath,
18+
} from "../../src/engine/safe-path";
19+
20+
describe("safe-path", () => {
21+
let tempDir: string;
22+
23+
beforeEach(() => {
24+
tempDir = resolve(mkdtempSync(join(tmpdir(), "archgate-safe-path-")));
25+
});
26+
27+
afterEach(() => {
28+
rmSync(tempDir, { recursive: true, force: true });
29+
});
30+
31+
describe("resolveUserPath", () => {
32+
test("resolves a relative path against the root", () => {
33+
expect(resolveUserPath(tempDir, "src/app.ts")).toBe(
34+
resolve(tempDir, "src", "app.ts")
35+
);
36+
});
37+
38+
test("keeps an absolute path as-is (resolved)", () => {
39+
const abs = join(tempDir, "config.yml");
40+
expect(resolveUserPath(tempDir, abs)).toBe(resolve(abs));
41+
});
42+
});
43+
44+
describe("isWithinRoot", () => {
45+
test("accepts the root itself and paths under it", () => {
46+
expect(isWithinRoot(tempDir, tempDir)).toBe(true);
47+
expect(isWithinRoot(tempDir, join(tempDir, "a", "b"))).toBe(true);
48+
});
49+
50+
test("rejects siblings and parents", () => {
51+
expect(isWithinRoot(tempDir, resolve(tempDir, ".."))).toBe(false);
52+
expect(isWithinRoot(tempDir, `${tempDir}-sibling`)).toBe(false);
53+
});
54+
});
55+
56+
describe("safePath", () => {
57+
test("returns the absolute path for an in-root file", () => {
58+
writeFileSync(join(tempDir, "ok.txt"), "x");
59+
expect(safePath(tempDir, "ok.txt")).toBe(join(tempDir, "ok.txt"));
60+
});
61+
62+
test("accepts a deep chain of real directories", () => {
63+
mkdirSync(join(tempDir, "a", "b", "c"), { recursive: true });
64+
writeFileSync(join(tempDir, "a", "b", "c", "deep.txt"), "x");
65+
expect(() => safePath(tempDir, "a/b/c/deep.txt")).not.toThrow();
66+
});
67+
68+
test("does not throw for a non-existent in-root path", () => {
69+
expect(() => safePath(tempDir, "missing/file.txt")).not.toThrow();
70+
});
71+
72+
test("accepts the root itself", () => {
73+
expect(safePath(tempDir, ".")).toBe(tempDir);
74+
});
75+
76+
test("throws on traversal outside the root", () => {
77+
expect(() => safePath(tempDir, "../outside.txt")).toThrow(
78+
/escapes project root/u
79+
);
80+
});
81+
82+
test("throws when an ANCESTOR directory is a symlink", () => {
83+
// The leaf is an ordinary file — only the parent is a link, which a
84+
// leaf-only lstat cannot see.
85+
const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
86+
writeFileSync(join(outsideDir, "secret.txt"), "sensitive");
87+
try {
88+
// "junction" is ignored on POSIX; on Windows it needs no admin rights
89+
// and lstat reports it as a symlink, so this runs on every platform.
90+
symlinkSync(outsideDir, join(tempDir, "linkdir"), "junction");
91+
} catch {
92+
rmSync(outsideDir, { recursive: true, force: true });
93+
return;
94+
}
95+
expect(() => safePath(tempDir, "linkdir/secret.txt")).toThrow(
96+
/traverses symbolic link "linkdir" access denied/u
97+
);
98+
rmSync(outsideDir, { recursive: true, force: true });
99+
});
100+
101+
test("throws when the LEAF itself is a symlink", () => {
102+
const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
103+
writeFileSync(join(outsideDir, "secret.txt"), "sensitive");
104+
try {
105+
symlinkSync(join(outsideDir, "secret.txt"), join(tempDir, "link.txt"));
106+
} catch {
107+
// File symlinks still need admin/developer mode on Windows — skip.
108+
rmSync(outsideDir, { recursive: true, force: true });
109+
return;
110+
}
111+
expect(() => safePath(tempDir, "link.txt")).toThrow(
112+
/is a symbolic link access denied/u
113+
);
114+
rmSync(outsideDir, { recursive: true, force: true });
115+
});
116+
});
117+
});

0 commit comments

Comments
 (0)