Merge pull request #314 from aakhter/fix/symlink-safe-workspace-confinement

fix(routes): canonicalize the workspace before comparing it to a resolved path

src/web/route-helpers.ts

@@ -63,19 +63,29 @@ export async function readJsonConfig<T>(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;
   }

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();
+  });
+});

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',