diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..5b89cae --- /dev/null +++ b/.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. diff --git a/packages/documents/src/index.ts b/packages/documents/src/index.ts index afd98de..8dfdbf0 100644 --- a/packages/documents/src/index.ts +++ b/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 { 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 +} diff --git a/packages/documents/src/security.test.ts b/packages/documents/src/security.test.ts new file mode 100644 index 0000000..671883c --- /dev/null +++ b/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/) + }) +})