🛡️ Sentinel: [CRITICAL] Fix path traversal in LocalJsonStore

.jules/sentinel.md

@@ -0,0 +1,4 @@
+## 2025-05-15 - [Path Traversal in LocalJsonStore]
+**Vulnerability:** Path traversal in `LocalJsonStore` allowed reading/writing files outside the intended base directory using `..` segments or absolute paths.
+**Learning:** Initial implementation only filtered empty segments, which was insufficient. Sanitizing individual segments and verifying the final resolved path against the base directory is necessary.
+**Prevention:** Always use `path.resolve` and check if the result starts with the base directory (ensuring a trailing separator is handled to prevent prefix attacks). Explicitly block absolute paths if relative keys are expected.

packages/documents/src/index.ts

@@ -6,124 +6,137 @@ import {
   renameSync,
   rmSync,
   statSync,
-  writeFileSync
-} from "node:fs";
-import path from "node:path";
+  writeFileSync,
+} from "node:fs"
+import path from "node:path"
 
 const sanitizeSegment = (segment: string) => {
-  return segment.replace(/[<>:"|?*]/g, "_");
-};
+  return segment.replace(/[<>:"|?*]/g, "_")
+}
 
 const walkJsonFiles = (directory: string, files: string[] = []) => {
   if (!existsSync(directory)) {
-    return files;
+    return files
   }
 
   for (const entry of readdirSync(directory)) {
-    const fullPath = path.join(directory, entry);
-    const stats = statSync(fullPath);
+    const fullPath = path.join(directory, entry)
+    const stats = statSync(fullPath)
 
     if (stats.isDirectory()) {
-      walkJsonFiles(fullPath, files);
-      continue;
+      walkJsonFiles(fullPath, files)
+      continue
     }
 
     if (fullPath.endsWith(".json")) {
-      files.push(fullPath);
+      files.push(fullPath)
     }
   }
 
-  return files;
-};
+  return files
+}
 
 const blockingSleep = (durationMs: number) => {
-  const buffer = new SharedArrayBuffer(4);
-  const view = new Int32Array(buffer);
-  Atomics.wait(view, 0, 0, durationMs);
-};
+  const buffer = new SharedArrayBuffer(4)
+  const view = new Int32Array(buffer)
+  Atomics.wait(view, 0, 0, durationMs)
+}
 
 export class LocalJsonStore<T> {
   constructor(private readonly baseDirectory: string) {
-    mkdirSync(baseDirectory, { recursive: true });
+    mkdirSync(baseDirectory, { recursive: true })
   }
 
   private toPath(key: string) {
-    const relative = key
-      .split("/")
-      .filter(Boolean)
-      .map(sanitizeSegment)
-      .join(path.sep);
+    if (path.isAbsolute(key)) {
+      throw new Error(`JSON store key "${key}" must be a relative path.`)
+    }
+
+    const segments = key.split(/[\\/]/).filter((s) => s && s !== "." && s !== "..")
 
-    return path.join(this.baseDirectory, `${relative}.json`);
+    if (segments.length === 0) {
+      throw new Error(`JSON store key "${key}" contains no valid segments.`)
+    }
+
+    const relative = segments.map(sanitizeSegment).join(path.sep)
+    const resolved = path.resolve(this.baseDirectory, `${relative}.json`)
+    const base = path.resolve(this.baseDirectory)
+    const baseWithTrailingSep = base.endsWith(path.sep) ? base : `${base}${path.sep}`
+
+    if (!resolved.startsWith(baseWithTrailingSep) && resolved !== base) {
+      throw new Error(`Path traversal detected in key "${key}".`)
+    }
+
+    return resolved
   }
 
   private readPath(filePath: string) {
     for (let attempt = 0; attempt < 5; attempt += 1) {
       try {
-        const raw = readFileSync(filePath, "utf8");
+        const raw = readFileSync(filePath, "utf8")
         if (!raw.trim()) {
-          throw new SyntaxError(`JSON store file "${filePath}" is empty.`);
+          throw new SyntaxError(`JSON store file "${filePath}" is empty.`)
         }
 
-        return JSON.parse(raw) as T;
+        return JSON.parse(raw) as T
       } catch (error) {
-        const code = error && typeof error === "object" && "code" in error ? error.code : undefined;
+        const code = error && typeof error === "object" && "code" in error ? error.code : undefined
         if (code === "ENOENT") {
-          return undefined;
+          return undefined
         }
 
         if (error instanceof SyntaxError && attempt < 4) {
-          blockingSleep(10);
-          continue;
+          blockingSleep(10)
+          continue
         }
 
-        throw error;
+        throw error
       }
     }
 
-    return undefined;
+    return undefined
   }
 
   read(key: string) {
-    const filePath = this.toPath(key);
+    const filePath = this.toPath(key)
     if (!existsSync(filePath)) {
-      return undefined;
+      return undefined
     }
 
-    return this.readPath(filePath);
+    return this.readPath(filePath)
   }
 
   write(key: string, value: T) {
-    const filePath = this.toPath(key);
-    mkdirSync(path.dirname(filePath), { recursive: true });
-    const tempPath = `${filePath}.${process.pid}.${Date.now()}.tmp`;
-    writeFileSync(tempPath, JSON.stringify(value, null, 2), "utf8");
-    rmSync(filePath, { force: true });
-    renameSync(tempPath, filePath);
-    return value;
+    const filePath = this.toPath(key)
+    mkdirSync(path.dirname(filePath), { recursive: true })
+    const tempPath = `${filePath}.${process.pid}.${Date.now()}.tmp`
+    writeFileSync(tempPath, JSON.stringify(value, null, 2), "utf8")
+    rmSync(filePath, { force: true })
+    renameSync(tempPath, filePath)
+    return value
   }
 
   list() {
     return walkJsonFiles(this.baseDirectory).flatMap((filePath) => {
-      const record = this.readPath(filePath);
-      return record === undefined ? [] : [record];
-    });
+      const record = this.readPath(filePath)
+      return record === undefined ? [] : [record]
+    })
   }
 
   delete(key: string) {
-    const filePath = this.toPath(key);
+    const filePath = this.toPath(key)
     if (existsSync(filePath)) {
-      rmSync(filePath);
+      rmSync(filePath)
     }
   }
 
   clear() {
-    rmSync(this.baseDirectory, { recursive: true, force: true });
-    mkdirSync(this.baseDirectory, { recursive: true });
+    rmSync(this.baseDirectory, { recursive: true, force: true })
+    mkdirSync(this.baseDirectory, { recursive: true })
   }
 }
 
 export const ensureDirectory = (directory: string) => {
-  mkdirSync(directory, { recursive: true });
-  return directory;
-};
+  mkdirSync(directory, { recursive: true })
+  return directory
+}

packages/documents/src/security.test.ts

@@ -0,0 +1,41 @@
+import { describe, it, expect, beforeEach, afterEach } from "vitest"
+import { LocalJsonStore } from "./index.js"
+import path from "node:path"
+import fs from "node:fs"
+import os from "node:os"
+
+describe("LocalJsonStore Security", () => {
+  let tempDir: string
+  let store: LocalJsonStore<{ secret: string }>
+
+  beforeEach(() => {
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "jeanbot-test-"))
+    const storeDir = path.join(tempDir, "store")
+    store = new LocalJsonStore(storeDir)
+  })
+
+  afterEach(() => {
+    fs.rmSync(tempDir, { recursive: true, force: true })
+  })
+
+  it("should prevent path traversal via .. segments", () => {
+    const sensitiveFile = path.join(tempDir, "sensitive.json")
+    fs.writeFileSync(sensitiveFile, JSON.stringify({ secret: "password123" }))
+
+    // This should NOT be able to read the sensitive file outside the store directory
+    // It should either return undefined because .. is stripped, or throw an error
+    const data = store.read("../sensitive")
+
+    expect(data).toBeUndefined()
+  })
+
+  it("should prevent absolute path bypass", () => {
+    const sensitiveFile = path.join(tempDir, "absolute.json")
+    fs.writeFileSync(sensitiveFile, JSON.stringify({ secret: "absolute-secret" }))
+
+    // Passing an absolute path should throw because of path.isAbsolute(key)
+    expect(() => {
+      store.read(sensitiveFile.replace(".json", ""))
+    }).toThrow(/must be a relative path/)
+  })
+})