HIGH security: confine the file IPC to the workspace when symlinks are involved

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<string | null> {
+  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<boolean> {
+  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<string | null> {
+  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<FileHandle> {
+  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,18 +231,20 @@ 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<ArrayBuffer>; 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.
     if (!s.isFile()) return { ok: false, error: 'not a regular file' };
     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(() => {});
   }
 }
 

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<GitDiff | { ok: false; error: string }> {
-  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:<path>` — 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) {

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);