From 8e5691b05cdb28c4f2dd9f257ccb9980569bf0d1 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Tue, 18 Aug 2026 10:46:21 -0400 Subject: [PATCH] fix(routes): canonicalize the workspace before comparing it to a resolved path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateSessionFilePath realpath-resolves the candidate path but compared it against the raw sessionWorkingDir. When the workspace is itself reached through a symlink the two sides live in different namespaces, so relative() reports a spurious `../` and every file in that workspace is judged an escape — reads and writes in the session are refused wholesale. That is not an exotic setup: os.tmpdir() hands back a symlinked path on macOS (/tmp -> /private/tmp), and symlinked project directories and bind-mounted case paths hit it too. Resolve both sides and compare canonical to canonical. This only makes the comparison honest — it does not widen it. The candidate keeps its own realpath, so a symlink pointing out of the workspace and a ../ traversal are still refused, and a workspace that cannot be resolved now fails closed. Three stubs in file-routes.test.ts used a blanket realpathSync.mockReturnValue(escapeTarget), which answers the same path for the workspace and the candidate; with both sides resolved that makes an escape look contained. They now use the input-aware mockImplementation idiom the rest of that file already uses, so the workspace resolves to itself and only the candidate escapes. Verified they still bite: removing the confinement check turns all of them red. Adds test/route-helpers-symlink-confinement.test.ts, which exercises the function against a real symlinked workspace on disk and pins the negative cases (../ escape, symlink-out, missing file) alongside the fix. --- src/web/route-helpers.ts | 16 +++- .../route-helpers-symlink-confinement.test.ts | 83 +++++++++++++++++++ test/routes/file-routes.test.ts | 12 ++- 3 files changed, 105 insertions(+), 6 deletions(-) create mode 100644 test/route-helpers-symlink-confinement.test.ts diff --git a/src/web/route-helpers.ts b/src/web/route-helpers.ts index 50990b70..8585358c 100644 --- a/src/web/route-helpers.ts +++ b/src/web/route-helpers.ts @@ -63,19 +63,29 @@ export async function readJsonConfig(filePath: string, logLabel: string, defa * Validates that a file path (possibly containing symlinks) resolves to a location * within the given session working directory. Returns the resolved and relative paths, * or null if the path escapes the directory or doesn't exist. + * + * BOTH sides are realpath-resolved before they are compared. Resolving only the + * candidate leaves the two paths in different namespaces whenever the workspace + * itself is reached through a symlink, and `relative()` then reports a spurious + * `../` for a file that is genuinely inside it — refusing every read and write in + * that session. A symlinked workspace is ordinary: `os.tmpdir()` returns one on + * macOS (`/tmp` -> `/private/tmp`), as do symlinked project dirs and bind-mounted + * case paths. Canonicalizing the base only makes the comparison honest; escapes + * are still refused, since the candidate keeps its own realpath. */ export function validateSessionFilePath( sessionWorkingDir: string, filePath: string ): { resolvedPath: string; relativePath: string } | null { - const fullPath = resolve(sessionWorkingDir, filePath); + let resolvedWorkingDir: string; let resolvedPath: string; try { - resolvedPath = realpathSync(fullPath); + resolvedWorkingDir = realpathSync(sessionWorkingDir); + resolvedPath = realpathSync(resolve(sessionWorkingDir, filePath)); } catch { return null; } - const relativePath = relative(sessionWorkingDir, resolvedPath); + const relativePath = relative(resolvedWorkingDir, resolvedPath); if (relativePath.startsWith('..') || isAbsolute(relativePath)) { return null; } diff --git a/test/route-helpers-symlink-confinement.test.ts b/test/route-helpers-symlink-confinement.test.ts new file mode 100644 index 00000000..0f545701 --- /dev/null +++ b/test/route-helpers-symlink-confinement.test.ts @@ -0,0 +1,83 @@ +// Port: none (pure function over a real temp filesystem). +// +// `validateSessionFilePath` is the shared confinement gate for the file-serving +// and file-writing routes: it answers "does this path resolve to somewhere +// inside the session workspace". It realpath-resolves the CANDIDATE so a +// symlink cannot smuggle a path out of the workspace — but the workspace it +// compares against must be canonical too, or the two sides are expressed in +// different namespaces and `relative()` reports a spurious `../`. +// +// That is not exotic: a symlinked workspace is the norm on macOS, where +// `/tmp` is a symlink to `/private/tmp` and `os.tmpdir()` hands back the +// symlinked form, and it also covers symlinked project dirs and bind-mounted +// case paths. The effect is a workspace whose own files are all judged to be +// outside it, so every read and write in that session is refused. +import { mkdtempSync, mkdirSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, describe, expect, it } from 'vitest'; + +import { validateSessionFilePath } from '../src/web/route-helpers.js'; + +// realpath the root itself so the fixture controls which side is symlinked, +// rather than inheriting whatever os.tmpdir() happens to be on this platform. +const root = realpathSync(mkdtempSync(join(tmpdir(), 'codeman-confinement-'))); +const realWorkspace = join(root, 'real-workspace'); +const linkedWorkspace = join(root, 'linked-workspace'); + +mkdirSync(join(realWorkspace, 'nested'), { recursive: true }); +writeFileSync(join(realWorkspace, 'notes.md'), '# notes\n'); +writeFileSync(join(realWorkspace, 'nested', 'deep.txt'), 'deep\n'); +symlinkSync(realWorkspace, linkedWorkspace, 'dir'); + +afterAll(() => rmSync(root, { recursive: true, force: true })); + +describe('validateSessionFilePath', () => { + it('accepts a file inside a workspace reached through a symlink', () => { + // The regression: the candidate is realpath'd to /…/real-workspace/notes.md + // while the base stays /…/linked-workspace, so a naive relative() yields + // '../real-workspace/notes.md' and the file is refused as an escape. + const result = validateSessionFilePath(linkedWorkspace, 'notes.md'); + + expect(result).not.toBeNull(); + expect(result!.relativePath).toBe('notes.md'); + expect(result!.resolvedPath).toBe(join(realWorkspace, 'notes.md')); + }); + + it('keeps the relative path usable for nested files under a symlinked workspace', () => { + // relativePath is what callers hand back to the client and re-join later, + // so an absolute or ../-prefixed value is a bug even when non-null. + const result = validateSessionFilePath(linkedWorkspace, 'nested/deep.txt'); + + expect(result).not.toBeNull(); + expect(result!.relativePath).toBe(join('nested', 'deep.txt')); + }); + + it('accepts the same file through the canonical workspace path', () => { + const result = validateSessionFilePath(realWorkspace, 'notes.md'); + + expect(result).not.toBeNull(); + expect(result!.relativePath).toBe('notes.md'); + }); + + it('still refuses a traversal escape from a symlinked workspace', () => { + // The point of canonicalizing the base is to make the comparison honest, + // NOT to loosen it: an escape must stay refused on both spellings. + writeFileSync(join(root, 'outside.txt'), 'outside\n'); + + expect(validateSessionFilePath(linkedWorkspace, '../outside.txt')).toBeNull(); + expect(validateSessionFilePath(realWorkspace, '../outside.txt')).toBeNull(); + }); + + it('still refuses a symlink that points out of the workspace', () => { + // The candidate-side realpath must keep doing its job. + writeFileSync(join(root, 'secret.txt'), 'secret\n'); + symlinkSync(join(root, 'secret.txt'), join(realWorkspace, 'escape.txt')); + + expect(validateSessionFilePath(linkedWorkspace, 'escape.txt')).toBeNull(); + }); + + it('returns null for a path that does not exist', () => { + expect(validateSessionFilePath(linkedWorkspace, 'nope.md')).toBeNull(); + }); +}); diff --git a/test/routes/file-routes.test.ts b/test/routes/file-routes.test.ts index 1bcdcf40..9552c32a 100644 --- a/test/routes/file-routes.test.ts +++ b/test/routes/file-routes.test.ts @@ -668,7 +668,9 @@ describe('file-routes', () => { it('rejects path traversal attempts', async () => { // realpathSync resolves the symlink to a path outside workingDir - mockedRealpathSync.mockReturnValue('/etc/passwd' as never); + mockedRealpathSync.mockImplementation( + (p: string) => (p === harness.ctx._session.workingDir ? p : '/etc/passwd') as never + ); const res = await harness.app.inject({ method: 'GET', @@ -773,7 +775,9 @@ describe('file-routes', () => { }); it('rejects path traversal in raw file serving', async () => { - mockedRealpathSync.mockReturnValue('/etc/shadow' as never); + mockedRealpathSync.mockImplementation( + (p: string) => (p === harness.ctx._session.workingDir ? p : '/etc/shadow') as never + ); const res = await harness.app.inject({ method: 'GET', @@ -868,7 +872,9 @@ describe('file-routes', () => { }); it('rejects symlink targets that escape the session working directory', async () => { - mockedRealpathSync.mockReturnValue('/tmp/outside-workdir/link.log' as never); + mockedRealpathSync.mockImplementation( + (p: string) => (p === harness.ctx._session.workingDir ? p : '/tmp/outside-workdir/link.log') as never + ); const res = await harness.app.inject({ method: 'GET',