diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..b04ba6a --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,10 @@ +# Sentinel Security Journal + +## 2025-05-15 - [Path Traversal in LocalJsonStore] +**Vulnerability:** `LocalJsonStore` allowed reading and writing files outside of its intended `baseDirectory` via `..` segments in the keys. +**Learning:** Simple string replacement or filtering of `/` isn't enough to prevent path traversal in Node.js when using `path.join`. Attackers can use `..` or absolute paths. +**Prevention:** +1. Disallow absolute paths in keys. +2. Filter out `.` and `..` segments during key normalization. +3. Use a cross-platform regex `/[\\\/]/` for splitting path segments. +4. Always verify the final resolved absolute path starts with the resolved `baseDirectory` (using a trailing separator to prevent prefix bypass attacks like `/data/store` vs `/data/store-leak`). diff --git a/packages/documents/src/index.ts b/packages/documents/src/index.ts index afd98de..2a81e05 100644 --- a/packages/documents/src/index.ts +++ b/packages/documents/src/index.ts @@ -48,13 +48,27 @@ export class LocalJsonStore { } private toPath(key: string) { - const relative = key - .split("/") - .filter(Boolean) - .map(sanitizeSegment) - .join(path.sep); + if (path.isAbsolute(key)) { + throw new Error(`Absolute paths are not allowed in JSON store keys: ${key}`); + } + + const segments = key.split(/[\\\/]/).filter((s) => s && s !== "." && s !== ".."); + const relative = segments.map(sanitizeSegment).join(path.sep); + + const filePath = path.join(this.baseDirectory, `${relative}.json`); + const absoluteBase = path.resolve(this.baseDirectory); + const absoluteResolved = path.resolve(filePath); + + // Ensure the resolved path is actually within the base directory + const baseWithTrailingSep = absoluteBase.endsWith(path.sep) + ? absoluteBase + : `${absoluteBase}${path.sep}`; + + if (!absoluteResolved.startsWith(baseWithTrailingSep) && absoluteResolved !== absoluteBase) { + throw new Error(`Path traversal detected in key: ${key}`); + } - return path.join(this.baseDirectory, `${relative}.json`); + return filePath; } private readPath(filePath: string) { diff --git a/packages/documents/src/security.test.ts b/packages/documents/src/security.test.ts new file mode 100644 index 0000000..2d754a0 --- /dev/null +++ b/packages/documents/src/security.test.ts @@ -0,0 +1,40 @@ +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; + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "json-store-test-")); + const storeDir = path.join(tempDir, "store"); + store = new LocalJsonStore(storeDir); + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + it("should not allow path traversal out of base directory", () => { + const secretFile = path.join(tempDir, "secret.json"); + fs.writeFileSync(secretFile, JSON.stringify({ secret: "data" })); + + // Attempt to read the secret file using path traversal + // The store is at tempDir/store + // So ../secret should point to tempDir/secret.json + const trappedData = store.read("../secret"); + + expect(trappedData).toBeUndefined(); + }); + + it("should not allow writing outside of base directory", () => { + const traversalKey = "../evil"; + store.write(traversalKey, { evil: "data" }); + + const evilFile = path.join(tempDir, "evil.json"); + expect(fs.existsSync(evilFile)).toBe(false); + }); +});