From 70cf9ab507706d0ec176d3262a015c821f511a99 Mon Sep 17 00:00:00 2001 From: Aaron Coville Date: Wed, 26 Aug 2026 11:23:05 -0400 Subject: [PATCH] fix: confine the file IPC to the workspace when symlinks are involved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A symlink inside the selected workspace let the file IPC read and overwrite files outside it. `/notes.txt -> ../elsewhere/target` reads as an ordinary in-workspace relative name, and the read/write/list call then reached the external target. Root cause: the shared containment guard was lexical only. `resolve`, `normalize` and `relative` are string math and know nothing about symlinks, while the `readFile`/`writeFile`/`readdir` that runs afterwards is resolved by the kernel, which does follow them. Every consumer of the guard was affected — the text read, the binary read, the write, the directory listing, and both git path operations — because they all share it. The guard becomes an async `safeResolve` that all of them call, in three layers: 1. canonicalize the workspace root and the target with `realpath` and re-check containment on the canonical paths. The not-yet-existing tail of a path is re-attached to its deepest existing ancestor, so creating a new file in a real workspace directory still works. 2. reject any path component that is a symlink, via an `lstat` walk. Not redundant with (1): `realpath` throws ENOENT on a dangling symlink, so canonicalization alone treats it as a name to be created and the write follows the link and creates the file at its external target. 3. open reads and writes with `O_NOFOLLOW` (plus `O_NONBLOCK`, so a FIFO cannot park the open and with it the IPC call), so containment does not rest on the path still meaning the same thing between the check and the open. Those flags are POSIX-only, which is why (2) is a check in its own right and not merely a pre-filter. Layer 3 is why the git diff's working-side read had to change shape rather than just gain a call to the guard. It was a plain `stat` + `readFile` on the cleared path, which is a second and third resolution of a name the kernel is free to look up differently — anything that swaps that final component for a symlink after the check is what the read actually gets, and the external file then travels to the renderer as diff text. It now opens through the shared `openForRead` and stats through the returned handle, like every other read of a confined path, so the flags are stated in exactly one place. The boundary is the WORKSPACE, not symlinks. A link whose target exists and is itself inside the workspace is followed, and what comes back is the canonical path of that target: it reaches nothing the caller could not already reach by the target's real name, and refusing it would make an ordinary `node_modules` or monorepo checkout unbrowsable for no gain. What is refused is a link that leaves the workspace, and a dangling link, whose target does not exist yet and so cannot be shown to land inside it. Both directions of that are pinned by tests, so the policy cannot be flipped by accident later. The lexical form is no longer exported — a caller reaching for it would reintroduce exactly this bug. --- src/main/fs.ts | 175 ++++++++++++++++++--- src/main/git.ts | 25 ++- src/renderer/src/markdown/mdLinks.ts | 2 +- test/fs-path-containment.test.cjs | 217 +++++++++++++++++++++++++++ test/ide-image.test.cjs | 4 +- 5 files changed, 394 insertions(+), 29 deletions(-) create mode 100644 test/fs-path-containment.test.cjs diff --git a/src/main/fs.ts b/src/main/fs.ts index 327f76b54..e79198e20 100644 --- a/src/main/fs.ts +++ b/src/main/fs.ts @@ -1,22 +1,146 @@ -import { readdir, readFile, writeFile, stat } from 'node:fs/promises'; -import { isAbsolute, join, normalize, relative, resolve } from 'node:path'; +import { readdir, lstat, open, realpath, stat } from 'node:fs/promises'; +import type { FileHandle } from 'node:fs/promises'; +import { constants } from 'node:fs'; +import { basename, dirname, isAbsolute, join, normalize, relative, resolve, sep } from 'node:path'; import { homedir } from 'node:os'; import { imageMimeForPath } from '../shared/imageTypes'; /** - * Confines `path` inside `root` to prevent path-traversal escapes. - * Returns the resolved absolute path on success, or null on violation. + * Lexical containment — pure string math, no filesystem access. * - * Exported so other main-process modules (e.g. git.ts) validate caller-supplied - * relative paths against a workspace root with the SAME guard — there is exactly - * one path-escape policy in the app and it lives here. + * PRIVATE on purpose. On its own this is NOT a containment guarantee: `resolve`, + * `normalize` and `relative` know nothing about symlinks, while the `readFile` / + * `writeFile` / `readdir` that runs afterwards is resolved by the kernel, which + * follows them. `safeResolve` is the guard every consumer must use. */ -export function safeJoin(root: string, rel: string): string | null { +function lexicalJoin(root: string, rel: string): { absRoot: string; absPath: string } | null { const absRoot = resolve(root); const absPath = isAbsolute(rel) ? normalize(rel) : resolve(absRoot, rel); const rel2 = relative(absRoot, absPath); if (rel2.startsWith('..') || isAbsolute(rel2)) return null; - return absPath; + return { absRoot, absPath }; +} + +/** + * Canonicalize `absPath` and re-check containment against the canonical root. + * + * `realpath` throws ENOENT for a path that does not exist yet — which is normal + * for a write that creates a file — so the deepest EXISTING ancestor is + * canonicalized and the not-yet-existing tail is re-attached to it. That keeps + * "create a new file in a real workspace directory" working while still pinning + * every existing component to where it actually lives on disk. + */ +async function canonicalize(realRoot: string, absPath: string): Promise { + let probe = absPath; + const tail: string[] = []; + for (;;) { + try { + const real = await realpath(probe); + const full = tail.length ? resolve(real, ...tail) : real; + const r = relative(realRoot, full); + if (r.startsWith('..') || isAbsolute(r)) return null; + return full; + } catch (e) { + if ((e as NodeJS.ErrnoException).code !== 'ENOENT') return null; + const parent = dirname(probe); + if (parent === probe) return null; // walked past the filesystem root + tail.unshift(basename(probe)); + probe = parent; + } + } +} + +/** + * True if any component of `absPath` below `realRoot` is a symlink. + * + * Runs on the CANONICALIZED path, so every link that `realpath` could resolve is + * already gone by the time it walks — which is exactly why an in-workspace link + * to an existing in-workspace target is followed rather than refused. + * + * What is left for it to catch is the DANGLING link, and that is load-bearing: a + * link whose target does not exist yet makes `realpath` throw ENOENT, so + * canonicalization treats it as a to-be-created name and lets it through. A write + * then follows the link and creates the file at its EXTERNAL target. An `lstat` + * walk sees the link itself and refuses it regardless of where it points. + */ +async function hasSymlinkComponent(realRoot: string, absPath: string): Promise { + let cur = realRoot; + for (const part of relative(realRoot, absPath).split(sep).filter(Boolean)) { + cur = resolve(cur, part); + try { + if ((await lstat(cur)).isSymbolicLink()) return true; + } catch (e) { + // Nothing there yet — the rest of the path cannot exist either, so there is + // no link left to find. Anything else is unreadable metadata: fail closed. + if ((e as NodeJS.ErrnoException).code === 'ENOENT') return false; + return true; + } + } + return false; +} + +/** + * Confines `rel` inside `root` and returns the CANONICAL absolute path, or null + * on violation. + * + * Exported so other main-process modules (e.g. git.ts) validate caller-supplied + * relative paths against a workspace root with the SAME guard — there is exactly + * one path-escape policy in the app and it lives here. + * + * Async because containment cannot be decided without touching the filesystem. + * + * The boundary is the WORKSPACE, not "no symlinks at all". A link whose target + * exists and is itself inside the workspace is followed, and what comes back is + * the canonical path of that target — it reaches nothing the caller could not + * already reach by its real name, and refusing it would make an ordinary + * `node_modules` or monorepo checkout unbrowsable. What is refused is a link that + * leaves the workspace, and a DANGLING link, whose target does not exist yet and + * so cannot be proven to land inside it. + */ +export async function safeResolve(root: string, rel: string): Promise { + const lex = lexicalJoin(root, rel); + if (!lex) return null; + let realRoot: string; + try { + realRoot = await realpath(lex.absRoot); + } catch { + return null; + } + const abs = await canonicalize(realRoot, lex.absPath); + if (!abs) return null; + if (await hasSymlinkComponent(realRoot, abs)) return null; + return abs; +} + +/** + * Open-time guards, so containment does not depend on the path still meaning the + * same thing between the check above and the open below. + * + * O_NOFOLLOW makes the kernel refuse the final component if it is a symlink; + * O_NONBLOCK keeps a FIFO from parking the open (and with it the IPC call and the + * renderer's loading state) forever. Both are POSIX-only — on Windows they are + * undefined and OR in as 0, which is why the `lstat` walk above is a check in its + * own right and not merely a pre-filter. + */ +const READ_FLAGS = constants.O_RDONLY | (constants.O_NOFOLLOW | 0) | (constants.O_NONBLOCK | 0); +const WRITE_FLAGS = + constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC | (constants.O_NOFOLLOW | 0); + +/** + * Open a path that `safeResolve` has ALREADY cleared, for reading. + * + * Exported, and the only way any main-process module opens a confined path, + * because clearing a path and reading it are two separate resolutions: the guard + * inspects the path, and then the kernel looks it up again at open time. Whatever + * replaces the final component in between is what the read actually gets, so the + * open has to refuse it on its own — a caller that reaches for a plain `readFile` + * after `safeResolve` has no final-component protection at all. + * + * Callers still have to check `fstat` THROUGH the returned handle (not `stat` on + * the path, which resolves it a third time) before trusting what they opened. + */ +export function openForRead(abs: string): Promise { + return open(abs, READ_FLAGS); } export interface DirEntry { @@ -29,7 +153,7 @@ export interface DirEntry { export async function listDir(root: string, rel: string): Promise<{ ok: true; entries: DirEntry[]; path: string; } | { ok: false; error: string }> { - const abs = safeJoin(root, rel); + const abs = await safeResolve(root, rel); if (!abs) return { ok: false, error: 'path escapes root' }; try { const names = await readdir(abs); @@ -56,19 +180,24 @@ const MAX_READ_BYTES = 2 * 1024 * 1024; // 2 MB export async function readFileText(root: string, rel: string): Promise<{ ok: true; content: string; path: string; size: number; } | { ok: false; error: string }> { - const abs = safeJoin(root, rel); + const abs = await safeResolve(root, rel); if (!abs) return { ok: false, error: 'path escapes root' }; + let fh; try { - const s = await stat(abs); + fh = await openForRead(abs); + const s = await fh.stat(); + if (!s.isFile()) return { ok: false, error: 'not a regular file' }; if (s.size > MAX_READ_BYTES) { return { ok: false, error: `file too large (${(s.size / 1024 / 1024).toFixed(1)} MB)` }; } - const buf = await readFile(abs); + const buf = await fh.readFile(); // Reject obvious binary files based on null-byte sniff if (buf.includes(0)) return { ok: false, error: 'binary file (not displayable)' }; return { ok: true, content: buf.toString('utf8'), path: abs, size: s.size }; } catch (e) { return { ok: false, error: e instanceof Error ? e.message : String(e) }; + } finally { + await fh?.close().catch(() => {}); } } @@ -83,7 +212,7 @@ export async function readFileText(root: string, rel: string): Promise<{ const MAX_BINARY_READ_BYTES = 10 * 1024 * 1024; // 10 MB /** - * Read a file as raw BYTES, confined to `root` by the same `safeJoin` guard as + * Read a file as raw BYTES, confined to `root` by the same `safeResolve` guard as * every other fs entry point here. * * Exists because the text path deliberately refuses binary content (the @@ -102,10 +231,12 @@ const MAX_BINARY_READ_BYTES = 10 * 1024 * 1024; // 10 MB export async function readFileBinary(root: string, rel: string, maxBytes = MAX_BINARY_READ_BYTES): Promise<{ ok: true; bytes: Uint8Array; mime: string; path: string; size: number; } | { ok: false; error: string }> { - const abs = safeJoin(root, rel); + const abs = await safeResolve(root, rel); if (!abs) return { ok: false, error: 'path escapes root' }; + let fh; try { - const s = await stat(abs); + fh = await openForRead(abs); + const s = await fh.stat(); // Directories and FIFOs are the trap here: readFile on a directory throws // (fine) but on a FIFO it BLOCKS forever with no size to check against, which // would hang the IPC call and, with it, the renderer's loading state. @@ -113,7 +244,7 @@ export async function readFileBinary(root: string, rel: string, maxBytes = MAX_B if (s.size > maxBytes) { return { ok: false, error: `file too large (${(s.size / 1024 / 1024).toFixed(1)} MB)` }; } - const buf = await readFile(abs); + const buf = await fh.readFile(); if (buf.byteLength > maxBytes) { // The file grew between stat and read. Rare, but the cap is a memory // guarantee for the renderer, not an advisory. @@ -136,19 +267,25 @@ export async function readFileBinary(root: string, rel: string, maxBytes = MAX_B }; } catch (e) { return { ok: false, error: e instanceof Error ? e.message : String(e) }; + } finally { + await fh?.close().catch(() => {}); } } export async function writeFileText(root: string, rel: string, content: string): Promise<{ ok: true; path: string; } | { ok: false; error: string }> { - const abs = safeJoin(root, rel); + const abs = await safeResolve(root, rel); if (!abs) return { ok: false, error: 'path escapes root' }; + let fh; try { - await writeFile(abs, content, 'utf8'); + fh = await open(abs, WRITE_FLAGS, 0o666); + await fh.writeFile(content, 'utf8'); return { ok: true, path: abs }; } catch (e) { return { ok: false, error: e instanceof Error ? e.message : String(e) }; + } finally { + await fh?.close().catch(() => {}); } } diff --git a/src/main/git.ts b/src/main/git.ts index 38297dba8..fd20e1e8f 100644 --- a/src/main/git.ts +++ b/src/main/git.ts @@ -1,6 +1,5 @@ import { spawn } from 'node:child_process'; -import { readFile, stat } from 'node:fs/promises'; -import { safeJoin } from './fs'; +import { openForRead, safeResolve } from './fs'; /** Run git in `cwd` with `args`. Returns stdout text or an error. */ function runGit(cwd: string, args: string[], timeoutMs = 8000): Promise<{ @@ -170,7 +169,7 @@ export interface GitDiff { export async function getDiff( cwd: string, relPath: string ): Promise { - const abs = safeJoin(cwd, relPath); + const abs = await safeResolve(cwd, relPath); if (!abs) return { ok: false, error: 'path escapes repository root' }; // HEAD side: `git show HEAD:` — errors (untracked / new file) → no head version. @@ -179,21 +178,33 @@ export async function getDiff( const show = await runGit(cwd, ['show', `HEAD:${relPath}`]); if (show.ok) { head = show.stdout; headExists = true; } - // Working side: read the on-disk file. ENOENT → deleted from the working tree. + // Working side: read the on-disk file through the same guarded open as the file + // IPC, so the final component is re-checked by the kernel at open time rather + // than trusted from the `safeResolve` above. ENOENT (or a refused open) → + // nothing diffable in the working tree. let working = ''; let workingExists = false; let workingBinary = false; + let fh; try { - const s = await stat(abs); + fh = await openForRead(abs); + // Stat THROUGH the handle: it describes what is actually open, where a second + // `stat` on the path would resolve it again. A directory has no diffable + // content, and a FIFO would park the read — and the IPC call behind it — + // forever. + const s = await fh.stat(); + if (!s.isFile()) throw new Error('not a regular file'); if (s.size > MAX_DIFF_BYTES) { return { ok: false, error: `file too large to diff (${(s.size / 1048576).toFixed(1)} MB)` }; } - const buf = await readFile(abs); + const buf = await fh.readFile(); workingExists = true; if (buf.includes(0)) workingBinary = true; else working = buf.toString('utf8'); } catch { workingExists = false; + } finally { + await fh?.close().catch(() => {}); } const isBinary = workingBinary || head.includes('\0'); @@ -412,7 +423,7 @@ export async function getFileAtRev(cwd: string, rev: string, relPath: string): P { ok: true; exists: boolean; isBinary: boolean; content: string } | { ok: false; error: string } > { if (!isSafeRev(rev)) return { ok: false, error: 'invalid revision' }; - if (!safeJoin(cwd, relPath)) return { ok: false, error: 'path escapes repository root' }; + if (!(await safeResolve(cwd, relPath))) return { ok: false, error: 'path escapes repository root' }; const size = await runGit(cwd, ['cat-file', '-s', `${rev}:${relPath}`]); if (!size.ok) return { ok: true, exists: false, isBinary: false, content: '' }; if ((parseInt(size.stdout.trim(), 10) || 0) > MAX_SHOW_BYTES) { diff --git a/src/renderer/src/markdown/mdLinks.ts b/src/renderer/src/markdown/mdLinks.ts index 67602a9b6..e4125dc9e 100644 --- a/src/renderer/src/markdown/mdLinks.ts +++ b/src/renderer/src/markdown/mdLinks.ts @@ -13,7 +13,7 @@ import { isImagePath } from '@shared/imageTypes'; * Note that `..` can only ever pop segments that this function itself pushed — * once `parts` is empty a `..` is dropped — so the result is always a path * UNDER the workspace root, never a sibling of it. That is a convenience, not - * the security boundary: the real containment check is `safeJoin` in the main + * the security boundary: the real containment check is `safeResolve` in the main * process, which every read goes through. */ export function resolveRel(baseRel: string | undefined, href: string): string { const baseDir = (baseRel ?? '').split('/').slice(0, -1); diff --git a/test/fs-path-containment.test.cjs b/test/fs-path-containment.test.cjs new file mode 100644 index 000000000..984346b63 --- /dev/null +++ b/test/fs-path-containment.test.cjs @@ -0,0 +1,217 @@ +'use strict'; + +/** + * The workspace root is the confinement boundary for the file IPC: the renderer + * may only reach files INSIDE the selected workspace. + * + * A purely lexical containment check cannot enforce that. `resolve`/`normalize`/ + * `relative` are string math — they know nothing about symlinks — while the + * `readFile`/`writeFile`/`readdir` that runs afterwards is resolved by the + * kernel, which DOES follow them. So a symlink planted inside the workspace + * (agent-generated content and cloned repos both routinely contain them) reads + * as an in-root relative name to the guard and as an external file to the sink. + * + * These tests pin the boundary against that: every escape below must be refused + * by the shared guard, on every consumer of it, while ordinary in-workspace + * reads and writes keep working. + * + * The boundary is the workspace, not "no symlinks at all" — a link to a target + * that is itself inside the workspace is followed — so the legitimate case is + * pinned here too, in both directions. + */ + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const loadTs = require('./load-ts.cjs'); + +const { readFileText, readFileBinary, writeFileText, listDir } = loadTs('src/main/fs.ts'); +const { getDiff } = loadTs('src/main/git.ts'); + +const SECRET = 'external-secret-contents\n'; + +/** + * A throwaway workspace with an `outside` sibling standing in for the rest of + * the user's filesystem. Every symlink here points at that sibling — no test + * touches a real file outside its own temp dir. + */ +function makeWorkspace() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-containment-')); + const root = path.join(dir, 'workspace'); + const outside = path.join(dir, 'outside'); + fs.mkdirSync(path.join(root, 'docs'), { recursive: true }); + fs.mkdirSync(path.join(outside, 'dir'), { recursive: true }); + + fs.writeFileSync(path.join(outside, 'secret.txt'), SECRET); + fs.writeFileSync(path.join(outside, 'secret.bin'), Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00, 0x01])); + fs.writeFileSync(path.join(outside, 'dir', 'deep.txt'), SECRET); + fs.writeFileSync(path.join(outside, 'overwrite-me.txt'), 'original\n'); + + // Ordinary, legitimate workspace content. + fs.writeFileSync(path.join(root, 'real.txt'), 'in-workspace\n'); + fs.writeFileSync(path.join(root, 'docs', 'shot.bin'), Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00, 0x02])); + + // (a) final-component symlinks to external targets + fs.symlinkSync(path.join(outside, 'secret.txt'), path.join(root, 'innocent.txt')); + fs.symlinkSync(path.join(outside, 'secret.bin'), path.join(root, 'innocent.bin')); + fs.symlinkSync(path.join(outside, 'overwrite-me.txt'), path.join(root, 'notes.txt')); + // (b) an intermediate directory symlink — the escape is a path COMPONENT + fs.symlinkSync(path.join(outside, 'dir'), path.join(root, 'sub')); + // (c) a DANGLING final symlink: its target does not exist yet, so a write + // through it CREATES the external file. + fs.symlinkSync(path.join(outside, 'newly-created.txt'), path.join(root, 'dangling.txt')); + // (d) links that stay INSIDE the workspace and point at something that exists. + // These are the legitimate case — node_modules and monorepo checkouts are + // full of them — and they must keep working. + fs.symlinkSync(path.join(root, 'real.txt'), path.join(root, 'alias.txt')); + fs.symlinkSync(path.join(root, 'docs'), path.join(root, 'docs-link')); + + return { dir, root, outside }; +} + +function withWorkspace(fn) { + const ws = makeWorkspace(); + return Promise.resolve(fn(ws)).finally(() => fs.rmSync(ws.dir, { recursive: true, force: true })); +} + +// ─── reads ────────────────────────────────────────────────────────────────── + +test('a final-component symlink cannot be read as text', () => withWorkspace(async ({ root }) => { + const res = await readFileText(root, 'innocent.txt'); + assert.equal(res.ok, false, 'a symlink out of the workspace must not be readable'); + assert.equal(res.content, undefined); +})); + +test('a final-component symlink cannot be read as bytes', () => withWorkspace(async ({ root }) => { + const res = await readFileBinary(root, 'innocent.bin'); + assert.equal(res.ok, false, 'the binary reader shares the boundary and must refuse too'); +})); + +test('an intermediate directory symlink cannot be read through', () => withWorkspace(async ({ root }) => { + const res = await readFileText(root, 'sub/deep.txt'); + assert.equal(res.ok, false, 'the escape can be any component, not just the last one'); +})); + +// ─── writes ───────────────────────────────────────────────────────────────── + +test('a final-component symlink cannot be written through', () => withWorkspace(async ({ root, outside }) => { + const target = path.join(outside, 'overwrite-me.txt'); + const before = fs.readFileSync(target, 'utf8'); + const res = await writeFileText(root, 'notes.txt', 'clobbered\n'); + assert.equal(res.ok, false, 'writing through a symlink must be refused'); + assert.equal(fs.readFileSync(target, 'utf8'), before, 'the external file must be untouched'); +})); + +test('a dangling symlink cannot be used to create a file outside the workspace', () => + withWorkspace(async ({ root, outside }) => { + // The link target does not exist, so `realpath` cannot see where this leads — + // canonicalization alone would let the write through and CREATE the external + // file. Only an lstat walk (and an O_NOFOLLOW open) catches this one. + const res = await writeFileText(root, 'dangling.txt', 'created outside\n'); + assert.equal(res.ok, false, 'a dangling symlink is still a symlink'); + assert.equal( + fs.existsSync(path.join(outside, 'newly-created.txt')), false, + 'no file may be created outside the workspace' + ); + })); + +// ─── the other consumers of the same guard ────────────────────────────────── + +test('listDir cannot list a directory outside the workspace', () => withWorkspace(async ({ root }) => { + const res = await listDir(root, 'sub'); + assert.equal(res.ok, false, 'the directory listing shares the boundary'); +})); + +test('the git diff path check refuses a symlink escape', () => withWorkspace(async ({ root }) => { + const res = await getDiff(root, 'innocent.txt'); + assert.equal(res.ok, false, 'git path operations validate against the same boundary'); + assert.equal(res.working, undefined, 'the external file contents must never be returned'); +})); + +// ─── and ordinary workspace use still works ───────────────────────────────── + +test('ordinary in-workspace reads and writes still succeed', () => withWorkspace(async ({ root }) => { + const text = await readFileText(root, 'real.txt'); + assert.equal(text.ok, true, text.ok ? '' : text.error); + assert.equal(text.content, 'in-workspace\n'); + + const bin = await readFileBinary(root, 'docs/shot.bin'); + assert.equal(bin.ok, true, bin.ok ? '' : bin.error); + assert.equal(bin.size, 6); + + const created = await writeFileText(root, 'docs/new.txt', 'hello\n'); + assert.equal(created.ok, true, created.ok ? '' : created.error); + assert.equal(fs.readFileSync(path.join(root, 'docs', 'new.txt'), 'utf8'), 'hello\n'); + + const overwritten = await writeFileText(root, 'real.txt', 'replaced\n'); + assert.equal(overwritten.ok, true, overwritten.ok ? '' : overwritten.error); + assert.equal(fs.readFileSync(path.join(root, 'real.txt'), 'utf8'), 'replaced\n'); + + const listed = await listDir(root, 'docs'); + assert.equal(listed.ok, true, listed.ok ? '' : listed.error); + assert.ok(listed.entries.some((e) => e.name === 'shot.bin')); +})); + +test('an in-workspace symlink to an in-workspace target is followed, not refused', () => + withWorkspace(async ({ root }) => { + // The boundary is the workspace, not "no symlinks at all": a link whose + // target is itself inside the workspace reaches nothing the caller could not + // already reach by its real name. Refusing it would make an ordinary + // node_modules or monorepo checkout unbrowsable for no security gain. This + // pins that so the policy cannot be flipped by accident. + const viaLink = await readFileText(root, 'alias.txt'); + assert.equal(viaLink.ok, true, viaLink.ok ? '' : viaLink.error); + assert.equal(viaLink.content, 'in-workspace\n'); + assert.equal( + viaLink.path, path.join(fs.realpathSync(root), 'real.txt'), + 'the link resolves to the canonical path of its target, not to the link' + ); + + const listed = await listDir(root, 'docs-link'); + assert.equal(listed.ok, true, listed.ok ? '' : listed.error); + assert.ok(listed.entries.some((e) => e.name === 'shot.bin'), 'a directory link lists its target'); + })); + +test('the git diff read refuses a final component swapped for a symlink mid-call', { + skip: process.platform === 'win32' ? 'needs a POSIX shell shim and symlinks' : false +}, () => withWorkspace(async ({ root, outside, dir }) => { + // Validating a path and then reading it are two separate resolutions, and the + // kernel redoes the lookup at open time. `getDiff` runs `git show HEAD:` + // between the two, so shadowing `git` with a script that plants the symlink + // puts an attacker in exactly that window — deterministically, rather than + // hoping to win a race. The read must refuse what it is handed, not trust that + // the earlier check still holds. + const target = path.join(root, 'real.txt'); + const binDir = path.join(dir, 'bin'); + fs.mkdirSync(binDir); + const shim = path.join(binDir, 'git'); + fs.writeFileSync( + shim, + `#!/bin/sh\nrm -f '${target}'\nln -s '${path.join(outside, 'secret.txt')}' '${target}'\nexit 1\n` + ); + fs.chmodSync(shim, 0o755); + + const savedPath = process.env.PATH; + process.env.PATH = `${binDir}${path.delimiter}${savedPath}`; + let res; + try { + res = await getDiff(root, 'real.txt'); + } finally { + process.env.PATH = savedPath; + } + + assert.equal(fs.lstatSync(target).isSymbolicLink(), true, 'the shim must have run inside the window'); + assert.equal(res.ok, true, res.ok ? '' : res.error); + assert.equal(res.workingExists, false, 'a symlink is not the regular file the guard cleared'); + assert.equal(res.working, '', 'the external file contents must never reach the renderer'); +})); + +test('lexical traversal out of the root is still rejected', () => withWorkspace(async ({ root, dir }) => { + for (const rel of ['../outside/secret.txt', 'docs/../../outside/secret.txt', path.join(dir, 'outside', 'secret.txt')]) { + const res = await readFileText(root, rel); + assert.equal(res.ok, false, `${rel} must not be readable`); + assert.equal(res.error, 'path escapes root'); + } +})); diff --git a/test/ide-image.test.cjs b/test/ide-image.test.cjs index b68c2811e..2ce9836a9 100644 --- a/test/ide-image.test.cjs +++ b/test/ide-image.test.cjs @@ -117,7 +117,7 @@ test('non-image sources are refused even when local', () => { }); test('`..` can never climb above the workspace root textually', () => { - // Not the security boundary (safeJoin is), but it must not even try. + // Not the security boundary (safeResolve is), but it must not even try. assert.equal(resolveRel('a/b/c.md', '../../../../../etc/passwd'), 'etc/passwd'); assert.equal(resolveLocalImageRel('a/b.md', '../../../../x.png'), 'x.png'); }); @@ -172,7 +172,7 @@ test('path traversal out of the root is rejected', async () => { assert.equal(res.ok, false, `${rel} must not be readable`); assert.equal(res.error, 'path escapes root'); } - // …while an absolute path INSIDE the root is still fine (safeJoin's rule). + // …while an absolute path INSIDE the root is still fine (safeResolve's rule). const inside = await readFileBinary(root, path.join(root, 'docs', 'shot.png')); assert.equal(inside.ok, true); } finally {