From a370b8fc04e3ae432f9678db83a76fd9c33bc917 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 25 Jul 2026 02:06:53 +0200 Subject: [PATCH] fix(engine): reject rule-file reads through a symlinked ancestor directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit safePath() lstat'd only the FINAL path component, so a symlinked ancestor escaped the sandbox: with /docs a link to somewhere outside the project, /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 --- src/engine/runner.ts | 54 +----------- src/engine/safe-path.ts | 101 ++++++++++++++++++++++ tests/commands/check-security.test.ts | 71 ++++++++++++++++ tests/engine/safe-path.test.ts | 117 ++++++++++++++++++++++++++ 4 files changed, 291 insertions(+), 52 deletions(-) create mode 100644 src/engine/safe-path.ts create mode 100644 tests/engine/safe-path.test.ts diff --git a/src/engine/runner.ts b/src/engine/runner.ts index cccafa1a..939899b5 100644 --- a/src/engine/runner.ts +++ b/src/engine/runner.ts @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { lstatSync } from "node:fs"; -import { relative, resolve, isAbsolute } from "node:path"; +import { relative, resolve } from "node:path"; import type { AstLanguage, @@ -16,7 +15,6 @@ import type { ViolationDetail, } from "../formats/rules"; import { logDebug, logWarn } from "../helpers/log"; -import { UserError } from "../helpers/user-error"; import { AST_LANGUAGE_EXTENSIONS, PYTHON_AST_PROGRAM, @@ -48,57 +46,9 @@ import { import { listMatchingFiles, matchLines } from "./glob-utils"; import { parseTsOrJsSource } from "./js-parser"; import { type LoadResult, blockedToRuleResult } from "./loader"; +import { isWithinRoot, resolveUserPath, safePath } from "./safe-path"; import { applySuppressions, type SuppressionWarning } from "./suppressions"; -/** - * Resolve a user-supplied path against projectRoot without any boundary check. - */ -function resolveUserPath(resolvedRoot: string, userPath: string): string { - return isAbsolute(userPath) - ? resolve(userPath) - : resolve(resolvedRoot, userPath); -} - -/** - * Check whether an already-resolved absolute path stays within projectRoot. - * On Windows, paths on different drives produce a full absolute relative() - * result rather than a ".." prefix — use startsWith on the normalized paths. - */ -function isWithinRoot(resolvedRoot: string, absPath: string): boolean { - return ( - absPath.startsWith(resolvedRoot + "/") || - absPath.startsWith(resolvedRoot + "\\") || - absPath === resolvedRoot - ); -} - -/** - * Resolve a user-supplied path and ensure it stays within projectRoot. - * Throws if the resolved path escapes the project boundary or is a symlink. - */ -function safePath(resolvedRoot: string, userPath: string): string { - const absPath = resolveUserPath(resolvedRoot, userPath); - if (!isWithinRoot(resolvedRoot, absPath)) { - throw new UserError( - `Path "${userPath}" escapes project root — access denied` - ); - } - // Reject symlinks to prevent following links to files outside the project - try { - if (lstatSync(absPath).isSymbolicLink()) { - throw new UserError( - `Path "${userPath}" is a symbolic link — access denied` - ); - } - } catch (err) { - // Re-throw our own errors; ignore ENOENT (file may not exist yet for glob results) - if (err instanceof Error && err.message.includes("access denied")) { - throw err; - } - } - return absPath; -} - const RULE_TIMEOUT_MS = 30_000; /** diff --git a/src/engine/safe-path.ts b/src/engine/safe-path.ts new file mode 100644 index 00000000..c481b768 --- /dev/null +++ b/src/engine/safe-path.ts @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { lstatSync } from "node:fs"; +import { resolve, isAbsolute, join, relative } from "node:path"; + +import { UserError } from "../helpers/user-error"; + +/** + * Resolve a user-supplied path against projectRoot without any boundary check. + */ +export function resolveUserPath( + resolvedRoot: string, + userPath: string +): string { + return isAbsolute(userPath) + ? resolve(userPath) + : resolve(resolvedRoot, userPath); +} + +/** + * Check whether an already-resolved absolute path stays within projectRoot. + * On Windows, paths on different drives produce a full absolute relative() + * result rather than a ".." prefix — use startsWith on the normalized paths. + */ +export function isWithinRoot(resolvedRoot: string, absPath: string): boolean { + return ( + absPath.startsWith(resolvedRoot + "/") || + absPath.startsWith(resolvedRoot + "\\") || + absPath === resolvedRoot + ); +} + +/** + * Directories verified as real (non-symlink), memoized per process: safePath + * runs once per glob result and siblings share every ancestor, so without this + * the walk below re-lstats the same directories thousands of times per run. + */ +const verifiedRealDirs = new Set(); + +/** + * Reject a symlink anywhere in the path below the project root — the leaf OR + * any ancestor. A leaf-only check is insufficient: with `/docs` linked + * outside the project, `/docs/secret.txt` is an ordinary file, so the + * lexical `isWithinRoot` and an lstat of the leaf both pass while the OS + * resolves through the link and reads outside. + * + * Components at or above the root are deliberately 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 reject every temp-dir root. + * Each component is a boolean lstat rather than a string comparison against + * `realpath`, which case-canonicalizes on Windows/macOS and would reject + * case-mismatched-but-legitimate paths. + */ +function assertNoSymlinkInPath( + resolvedRoot: string, + absPath: string, + userPath: string +): void { + const rel = relative(resolvedRoot, absPath); + // Empty (the root itself) or escaping — nothing below the root to walk. + if (!rel || rel.startsWith("..")) return; + + const segments = rel.split(/[/\\]/u); + let current = resolvedRoot; + for (const [index, segment] of segments.entries()) { + current = join(current, segment); + if (verifiedRealDirs.has(current)) continue; + let stat; + try { + stat = lstatSync(current); + } catch { + // Does not exist (glob result, not-yet-created file): nothing can be + // traversed through it, and the eventual read fails on its own. + return; + } + if (stat.isSymbolicLink()) { + throw new UserError( + index === segments.length - 1 + ? `Path "${userPath}" is a symbolic link — access denied` + : `Path "${userPath}" traverses symbolic link "${segment}" — access denied` + ); + } + if (stat.isDirectory()) verifiedRealDirs.add(current); + } +} + +/** + * Resolve a user-supplied path and ensure it stays within projectRoot. + * Throws if the resolved path escapes the project boundary, is a symlink, or + * reaches its target through a symlinked ancestor directory. + */ +export function safePath(resolvedRoot: string, userPath: string): string { + const absPath = resolveUserPath(resolvedRoot, userPath); + if (!isWithinRoot(resolvedRoot, absPath)) { + throw new UserError( + `Path "${userPath}" escapes project root — access denied` + ); + } + assertNoSymlinkInPath(resolvedRoot, absPath, userPath); + return absPath; +} diff --git a/tests/commands/check-security.test.ts b/tests/commands/check-security.test.ts index 1debcded..24bf71c3 100644 --- a/tests/commands/check-security.test.ts +++ b/tests/commands/check-security.test.ts @@ -185,6 +185,77 @@ describe("check command security", () => { rmSync(outsideDir, { recursive: true, force: true }); }); + test("blocks reads that tunnel out through a symlinked ancestor directory", async () => { + // The leaf here is an ordinary file — only an ANCESTOR is the symlink, so + // a leaf-only lstat reports "not a link" while the OS still resolves the + // real path outside the project and reads it. + const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-")); + writeFileSync(join(outsideDir, "secret.txt"), "sensitive data"); + + try { + // "junction" is ignored on POSIX (plain symlink) but on Windows creates + // a directory junction, which needs no admin privileges and which + // lstat() reports as a symbolic link — so unlike the file-symlink test + // above, this case runs for real on every platform instead of skipping. + symlinkSync(outsideDir, join(tempDir, "src", "linkdir"), "junction"); + } catch { + rmSync(outsideDir, { recursive: true, force: true }); + return; + } + + writeAdrAndRule( + "SEC-007", + `export default { + rules: { + "read-through-symlinked-dir": { + description: "Attempt to read a real file via a symlinked parent", + async check(ctx) { + await ctx.readFile("src/linkdir/secret.txt"); + }, + }, + }, +}; +` + ); + + const loaded = await loadRuleAdrs(tempDir); + const result = await runChecks(tempDir, loaded); + expect(result.results[0].error).toContain("access denied"); + expect(result.results[0].error).toContain("symbolic link"); + + rmSync(outsideDir, { recursive: true, force: true }); + }); + + test("allows reads under real nested directories (no false positives)", async () => { + // Guards the ancestor walk against over-rejecting: a deep, entirely real + // directory chain must still be readable. + mkdirSync(join(tempDir, "src", "a", "b", "c"), { recursive: true }); + writeFileSync(join(tempDir, "src", "a", "b", "c", "deep.ts"), "export {};"); + + writeAdrAndRule( + "SEC-008", + `export default { + rules: { + "read-deep-real-path": { + description: "Read a file nested under real directories", + async check(ctx) { + const content = await ctx.readFile("src/a/b/c/deep.ts"); + if (!content.includes("export")) { + ctx.report.violation({ message: "unexpected content" }); + } + }, + }, + }, +}; +` + ); + + const loaded = await loadRuleAdrs(tempDir); + const result = await runChecks(tempDir, loaded); + expect(result.results[0].error).toBeUndefined(); + expect(result.results[0].violations).toHaveLength(0); + }); + test("allows legitimate file reads within project", async () => { writeFileSync(join(tempDir, "src", "app.ts"), "export const x = 1;\n"); diff --git a/tests/engine/safe-path.test.ts b/tests/engine/safe-path.test.ts new file mode 100644 index 00000000..69a62463 --- /dev/null +++ b/tests/engine/safe-path.test.ts @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { describe, expect, test, beforeEach, afterEach } from "bun:test"; +import { + mkdtempSync, + rmSync, + mkdirSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { + isWithinRoot, + resolveUserPath, + safePath, +} from "../../src/engine/safe-path"; + +describe("safe-path", () => { + let tempDir: string; + + beforeEach(() => { + tempDir = resolve(mkdtempSync(join(tmpdir(), "archgate-safe-path-"))); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + describe("resolveUserPath", () => { + test("resolves a relative path against the root", () => { + expect(resolveUserPath(tempDir, "src/app.ts")).toBe( + resolve(tempDir, "src", "app.ts") + ); + }); + + test("keeps an absolute path as-is (resolved)", () => { + const abs = join(tempDir, "config.yml"); + expect(resolveUserPath(tempDir, abs)).toBe(resolve(abs)); + }); + }); + + describe("isWithinRoot", () => { + test("accepts the root itself and paths under it", () => { + expect(isWithinRoot(tempDir, tempDir)).toBe(true); + expect(isWithinRoot(tempDir, join(tempDir, "a", "b"))).toBe(true); + }); + + test("rejects siblings and parents", () => { + expect(isWithinRoot(tempDir, resolve(tempDir, ".."))).toBe(false); + expect(isWithinRoot(tempDir, `${tempDir}-sibling`)).toBe(false); + }); + }); + + describe("safePath", () => { + test("returns the absolute path for an in-root file", () => { + writeFileSync(join(tempDir, "ok.txt"), "x"); + expect(safePath(tempDir, "ok.txt")).toBe(join(tempDir, "ok.txt")); + }); + + test("accepts a deep chain of real directories", () => { + mkdirSync(join(tempDir, "a", "b", "c"), { recursive: true }); + writeFileSync(join(tempDir, "a", "b", "c", "deep.txt"), "x"); + expect(() => safePath(tempDir, "a/b/c/deep.txt")).not.toThrow(); + }); + + test("does not throw for a non-existent in-root path", () => { + expect(() => safePath(tempDir, "missing/file.txt")).not.toThrow(); + }); + + test("accepts the root itself", () => { + expect(safePath(tempDir, ".")).toBe(tempDir); + }); + + test("throws on traversal outside the root", () => { + expect(() => safePath(tempDir, "../outside.txt")).toThrow( + /escapes project root/u + ); + }); + + test("throws when an ANCESTOR directory is a symlink", () => { + // The leaf is an ordinary file — only the parent is a link, which a + // leaf-only lstat cannot see. + const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-")); + writeFileSync(join(outsideDir, "secret.txt"), "sensitive"); + try { + // "junction" is ignored on POSIX; on Windows it needs no admin rights + // and lstat reports it as a symlink, so this runs on every platform. + symlinkSync(outsideDir, join(tempDir, "linkdir"), "junction"); + } catch { + rmSync(outsideDir, { recursive: true, force: true }); + return; + } + expect(() => safePath(tempDir, "linkdir/secret.txt")).toThrow( + /traverses symbolic link "linkdir" — access denied/u + ); + rmSync(outsideDir, { recursive: true, force: true }); + }); + + test("throws when the LEAF itself is a symlink", () => { + const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-")); + writeFileSync(join(outsideDir, "secret.txt"), "sensitive"); + try { + symlinkSync(join(outsideDir, "secret.txt"), join(tempDir, "link.txt")); + } catch { + // File symlinks still need admin/developer mode on Windows — skip. + rmSync(outsideDir, { recursive: true, force: true }); + return; + } + expect(() => safePath(tempDir, "link.txt")).toThrow( + /is a symbolic link — access denied/u + ); + rmSync(outsideDir, { recursive: true, force: true }); + }); + }); +});