From 2452db0bde1d0e55db536dec6becff2a3f215234 Mon Sep 17 00:00:00 2001
From: Zach Caceres <zcaceres@users.noreply.github.com>
Date: Mon, 18 May 2026 10:08:02 -0600
Subject: [PATCH] fix: close prefix-confusion and symlink bypasses in path
 allowlist

isWithinDirectory used startsWith, so `/srv/share-evil` passed a
`startsWith('/srv/share')` check. Rewrite using path.relative and reject
results starting with `..` or that are absolute (Windows cross-drive).

assertPathAllowed resolved with path.resolve+normalize but never followed
symlinks. An attacker who can write into the allowed directory (the
intended use case for MD_SHARE_DIR) could plant a symlink to ~/.ssh/id_rsa
and exfiltrate via an LLM tool call. Realpath both the input and the
allowed dirs before the containment check; walk up parents when the leaf
doesn't exist yet so dir-level symlinks are still caught.

Adds regression tests for sibling-prefix rejection, symlink escape, and
the realpath-aware happy path (also fixes macOS /tmp -> /private/tmp).

Refs #99.
---
 src/utils.test.ts | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 src/utils.ts      | 31 ++++++++++++++++++++-----
 2 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/src/utils.test.ts b/src/utils.test.ts
index 49fb8b0..fc478fe 100644
--- a/src/utils.test.ts
+++ b/src/utils.test.ts
@@ -166,6 +166,12 @@ describe("isWithinDirectory", () => {
       isWithinDirectory("/home/user/docs/../other/file.md", "/home/user/docs"),
     ).toBe(false);
   });
+
+  test("returns false for sibling directory with shared prefix", () => {
+    expect(
+      isWithinDirectory("/home/user/docs-evil/file.md", "/home/user/docs"),
+    ).toBe(false);
+  });
 });
 
 describe("validateRepoUrl", () => {
@@ -351,4 +357,56 @@ describe("getAllowedPaths / assertPathAllowed", () => {
       assertPathAllowed("/tmp/allowed/../etc/passwd"),
     ).toThrow("outside the allowed directories");
   });
+
+  test("assertPathAllowed rejects sibling directory with shared prefix", () => {
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "mdfy-prefix-"));
+    try {
+      const allowed = path.join(tmp, "share");
+      const evilSibling = path.join(tmp, "share-evil");
+      fs.mkdirSync(allowed);
+      fs.mkdirSync(evilSibling);
+      const secret = path.join(evilSibling, "secret.txt");
+      fs.writeFileSync(secret, "top secret");
+      process.env.MD_ALLOWED_PATHS = allowed;
+      expect(() => assertPathAllowed(secret)).toThrow(
+        "outside the allowed directories",
+      );
+    } finally {
+      fs.rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+
+  test("assertPathAllowed rejects symlink inside allowed dir that escapes", () => {
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "mdfy-symlink-"));
+    try {
+      const allowed = path.join(tmp, "allowed");
+      const outside = path.join(tmp, "outside");
+      fs.mkdirSync(allowed);
+      fs.mkdirSync(outside);
+      const secret = path.join(outside, "secret.txt");
+      fs.writeFileSync(secret, "top secret");
+      const link = path.join(allowed, "link.txt");
+      fs.symlinkSync(secret, link);
+      process.env.MD_ALLOWED_PATHS = allowed;
+      expect(() => assertPathAllowed(link)).toThrow(
+        "outside the allowed directories",
+      );
+    } finally {
+      fs.rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+
+  test("assertPathAllowed permits regular file inside allowed dir after realpath", () => {
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "mdfy-happy-"));
+    try {
+      const allowed = path.join(tmp, "allowed");
+      fs.mkdirSync(allowed);
+      const file = path.join(allowed, "doc.pdf");
+      fs.writeFileSync(file, "%PDF-1.4");
+      process.env.MD_ALLOWED_PATHS = allowed;
+      expect(() => assertPathAllowed(file)).not.toThrow();
+    } finally {
+      fs.rmSync(tmp, { recursive: true, force: true });
+    }
+  });
 });
diff --git a/src/utils.ts b/src/utils.ts
index 49c15ff..131d5f9 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -43,15 +43,34 @@ export function getAllowedPaths(): string[] | null {
   return dirs.length > 0 ? dirs : null;
 }
 
+function realpathOrAncestor(p: string): string {
+  let current = path.resolve(p);
+  const suffix: string[] = [];
+  while (true) {
+    try {
+      const realCurrent = fs.realpathSync.native(current);
+      return suffix.length === 0
+        ? realCurrent
+        : path.join(realCurrent, ...suffix.slice().reverse());
+    } catch {
+      const parent = path.dirname(current);
+      if (parent === current) return path.resolve(p);
+      suffix.push(path.basename(current));
+      current = parent;
+    }
+  }
+}
+
 export function assertPathAllowed(filePath: string): void {
   const allowed = getAllowedPaths();
   if (!allowed) return;
-  const resolved = path.normalize(path.resolve(expandHome(filePath)));
-  if (!allowed.some((dir) => isWithinDirectory(resolved, dir))) {
+  const resolved = realpathOrAncestor(expandHome(filePath));
+  const allowedReal = allowed.map(realpathOrAncestor);
+  if (!allowedReal.some((dir) => isWithinDirectory(resolved, dir))) {
     throw new Error(
       `Path "${filePath}" is outside the allowed directories. ` +
         `Set MD_ALLOWED_PATHS to a ${path.delimiter}-separated list that includes a parent directory ` +
-        `(currently allowed: ${allowed.join(path.delimiter)}).`,
+        `(currently allowed: ${allowedReal.join(path.delimiter)}).`,
     );
   }
 }
@@ -104,7 +123,7 @@ export function isMarkdownFile(filePath: string): boolean {
 }
 
 export function isWithinDirectory(filePath: string, directory: string): boolean {
-  const normPath = path.normalize(path.resolve(filePath));
-  const normDir = path.normalize(path.resolve(directory));
-  return normPath.startsWith(normDir);
+  const rel = path.relative(path.resolve(directory), path.resolve(filePath));
+  if (rel === "") return true;
+  return !rel.startsWith("..") && !path.isAbsolute(rel);
 }