From b11bfb90c81e8ba52602301ffe03c0cf143021d2 Mon Sep 17 00:00:00 2001 From: Ashish Soni Date: Wed, 17 Jun 2026 14:17:23 +0530 Subject: [PATCH 1/4] fix(security): enforce strict path normalization to prevent workspace escape (#2285) --- apps/server/src/routes/files.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/apps/server/src/routes/files.ts b/apps/server/src/routes/files.ts index afc55cd52..e3c203b19 100644 --- a/apps/server/src/routes/files.ts +++ b/apps/server/src/routes/files.ts @@ -1,6 +1,6 @@ import { createReadStream } from "node:fs"; import { readFile, readdir, rename, rm, stat, writeFile } from "node:fs/promises"; -import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; +import { basename, dirname, isAbsolute, join, relative, resolve, sep, posix } from "node:path"; import { Readable } from "node:stream"; import { recordAudit } from "../audit.js"; import { ApiError } from "../errors.js"; @@ -63,6 +63,11 @@ export function normalizeWorkspaceRelativePath(input: string, options: { allowSu normalized = normalized.replace(/^workspace\//, ""); normalized = normalized.replace(/^\/+/, ""); + normalized = posix.normalize(normalized); + if (normalized.startsWith("../") || normalized === "..") { + throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); + } + const parts = normalized.split("/").filter(Boolean); if (!parts.length) { throw new ApiError(400, "invalid_path", "Path is required"); @@ -115,7 +120,14 @@ function resolveSafeChildPath(root: string, child: string): string { if (candidate === rootResolved) { throw new ApiError(400, "invalid_path", "Path must point to a file"); } - if (!candidate.startsWith(rootResolved + sep)) { + + const isWindows = process.platform === "win32"; + const rootPrefix = rootResolved + sep; + const isSafe = isWindows + ? candidate.toLowerCase().startsWith(rootPrefix.toLowerCase()) + : candidate.startsWith(rootPrefix); + + if (!isSafe) { throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); } return candidate; From d3e26a8ef98eff022f91133ecb86455b48dad762 Mon Sep 17 00:00:00 2001 From: Ashish Soni Date: Wed, 17 Jun 2026 17:52:24 +0530 Subject: [PATCH 2/4] fix(security): address reviewer comments for macOS case-insensitivity --- .../server/src/extensions/openai-image-generation.ts | 12 +++++++++++- apps/server/src/routes/files.ts | 4 ++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/apps/server/src/extensions/openai-image-generation.ts b/apps/server/src/extensions/openai-image-generation.ts index b1e7daca1..d1d29c140 100644 --- a/apps/server/src/extensions/openai-image-generation.ts +++ b/apps/server/src/extensions/openai-image-generation.ts @@ -101,7 +101,17 @@ function workspaceForContext(config: ServerConfig, context: Record Date: Wed, 17 Jun 2026 18:28:00 +0530 Subject: [PATCH 3/4] refactor(security): extract shared path-security primitives; add regression tests for #2285 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Address all reviewer feedback from PR #2301 before merge. ### Changes **NEW: apps/server/src/path-security.ts** - Single source of truth for ormalizeWorkspaceRelativePath and esolveSafeChildPath shared across the server codebase. - Eliminates logic drift between the two previous copies of esolveSafeChildPath (routes/files.ts and extensions/openai-image-generation.ts). - Detailed JSDoc rationale for every design decision (case-insensitive comparison, trailing separator, posix.normalize pre-pass, symlink note). **NEW: apps/server/src/path-security.test.ts** - 26 regression + unit tests that directly reproduce the three escape vectors from issue #2285: - [#2285-a] raw .. traversal (bare, prefix, workspace/../, deeply nested, Windows backslash form) - [#2285-b] case-mismatch false-rejection on Windows / macOS - [#2285-c] sibling-workspace-prefix bypass (/ws vs /ws-evil) - All 26 tests pass on both POSIX and Windows. **MODIFY: apps/server/src/routes/files.ts** - Remove local implementations of both security primitives. - Import from ../path-security.js (the canonical source). - Re-export ormalizeWorkspaceRelativePath for backward compatibility with callers that import from routes/files.ts or server.ts. **MODIFY: apps/server/src/extensions/openai-image-generation.ts** - Remove duplicate esolveSafeChildPath implementation (was still on the old case-sensitive logic — the exact drift Reviewer 1 flagged). - Import from ../path-security.js; logic is now identical to files.ts. ### Reviewer concerns addressed | Concern (Abhijeet1005 / Pablosinyores) | Resolution | |---|---| | No regression test for #2285 escape | 26 tests in path-security.test.ts | | Duplicate resolveSafeChildPath, drifting logic | Shared module, one copy | | macOS case-insensitivity (darwin) missing | darwin added alongside win32 | | posix.normalize belt-and-suspenders comment | Documented in JSDoc | | Symlink note | Explicitly documented as out-of-scope | Closes #2285 --- .../src/extensions/openai-image-generation.ts | 21 +- apps/server/src/path-security.test.ts | 245 ++++++++++++++++++ apps/server/src/path-security.ts | 139 ++++++++++ apps/server/src/routes/files.ts | 65 +---- 4 files changed, 393 insertions(+), 77 deletions(-) create mode 100644 apps/server/src/path-security.test.ts create mode 100644 apps/server/src/path-security.ts diff --git a/apps/server/src/extensions/openai-image-generation.ts b/apps/server/src/extensions/openai-image-generation.ts index d1d29c140..8b3344206 100644 --- a/apps/server/src/extensions/openai-image-generation.ts +++ b/apps/server/src/extensions/openai-image-generation.ts @@ -2,6 +2,7 @@ import { mkdir, writeFile } from "node:fs/promises"; import { dirname, resolve, sep } from "node:path"; import { ApiError } from "../errors.js"; +import { resolveSafeChildPath } from "../path-security.js"; import type { EnvService } from "../env-file.js"; import type { ServerConfig, WorkspaceInfo } from "../types.js"; @@ -98,24 +99,8 @@ function workspaceForContext(config: ServerConfig, context: Record { + + // ── Basic acceptance ─────────────────────────────────────────────────────── + + test("accepts a plain workspace-relative path", () => { + expect(normalizeWorkspaceRelativePath("notes.md", { allowSubdirs: true })).toBe("notes.md"); + }); + + test("accepts a nested path when allowSubdirs is true", () => { + expect(normalizeWorkspaceRelativePath("reports/q1.csv", { allowSubdirs: true })).toBe( + "reports/q1.csv", + ); + }); + + test("normalises benign in-bounds dot-dot (a/b/../c -> a/c)", () => { + // This is a VALID path — the segment just collapses within the root. + expect( + normalizeWorkspaceRelativePath("reports/archive/../q1.csv", { allowSubdirs: true }), + ).toBe("reports/q1.csv"); + }); + + // ── Prefix stripping ─────────────────────────────────────────────────────── + + test("strips workspace/ prefix", () => { + expect(normalizeWorkspaceRelativePath("workspace/notes.md", { allowSubdirs: true })).toBe( + "notes.md", + ); + expect( + normalizeWorkspaceRelativePath("workspace/dir/notes.md", { allowSubdirs: true }), + ).toBe("dir/notes.md"); + }); + + test("strips Workspace// prefix from rendered artifact paths", () => { + expect( + normalizeWorkspaceRelativePath("Workspace/32423/reports/artifact-eval.md", { + allowSubdirs: true, + }), + ).toBe("reports/artifact-eval.md"); + expect( + normalizeWorkspaceRelativePath("workspaces/demo/reports/artifact-eval.csv", { + allowSubdirs: true, + }), + ).toBe("reports/artifact-eval.csv"); + }); + + test("strips /workspace/ prefix", () => { + expect(normalizeWorkspaceRelativePath("/workspace/notes.md", { allowSubdirs: true })).toBe( + "notes.md", + ); + expect( + normalizeWorkspaceRelativePath("//workspace/dir/notes.md", { allowSubdirs: true }), + ).toBe("dir/notes.md"); + }); + + test("strips ./workspace/ prefix", () => { + expect(normalizeWorkspaceRelativePath("./workspace/notes.md", { allowSubdirs: true })).toBe( + "notes.md", + ); + }); + + // ── Windows back-slash payloads ──────────────────────────────────────────── + + test("converts Windows back-slashes before checking", () => { + expect(normalizeWorkspaceRelativePath("reports\\q1.csv", { allowSubdirs: true })).toBe( + "reports/q1.csv", + ); + }); + + // ── Traversal rejection ──────────────────────────────────────────────────── + + // Regression #2285 — vector (a): raw `..` traversal + test("[Regression #2285-a] rejects bare .. traversal", () => { + expect(() => + normalizeWorkspaceRelativePath("..", { allowSubdirs: true }), + ).toThrow(); + }); + + test("[Regression #2285-a] rejects ../ prefix", () => { + expect(() => + normalizeWorkspaceRelativePath("../secrets.md", { allowSubdirs: true }), + ).toThrow(); + }); + + test("[Regression #2285-a] rejects workspace/../secrets.md", () => { + expect(() => + normalizeWorkspaceRelativePath("workspace/../secrets.md", { allowSubdirs: true }), + ).toThrow(); + }); + + test("[Regression #2285-a] rejects /workspace/../secrets.md", () => { + expect(() => + normalizeWorkspaceRelativePath("/workspace/../secrets.md", { allowSubdirs: true }), + ).toThrow(); + }); + + test("[Regression #2285-a] rejects deeply nested traversal (a/b/../../../secret)", () => { + // After posix.normalize: a/b/../../../secret → ../../secret (escapes root) + expect(() => + normalizeWorkspaceRelativePath("a/b/../../../secret", { allowSubdirs: true }), + ).toThrow(); + }); + + test("[Regression #2285-a] rejects Windows-backslash traversal (..\\..\\secret)", () => { + // Back-slashes are converted to forward-slashes first, then posix.normalize + // catches the traversal. + expect(() => + normalizeWorkspaceRelativePath("..\\..\\secret", { allowSubdirs: true }), + ).toThrow(); + }); + + // ── Other invalid inputs ─────────────────────────────────────────────────── + + test("rejects null bytes", () => { + expect(() => + normalizeWorkspaceRelativePath("file\u0000.md", { allowSubdirs: true }), + ).toThrow(); + }); + + test("rejects empty string", () => { + expect(() => normalizeWorkspaceRelativePath("", { allowSubdirs: true })).toThrow(); + }); + + test("enforces allowSubdirs: false", () => { + expect(() => + normalizeWorkspaceRelativePath("workspace/dir/notes.md", { allowSubdirs: false }), + ).toThrow(); + }); + + test("treats workspace/ with no file as invalid", () => { + expect(() => normalizeWorkspaceRelativePath("workspace/", { allowSubdirs: true })).toThrow(); + expect(() => + normalizeWorkspaceRelativePath("/workspace/", { allowSubdirs: true }), + ).toThrow(); + }); +}); + +// ─── resolveSafeChildPath ──────────────────────────────────────────────────── + +describe("resolveSafeChildPath", () => { + // Use the OS temp dir as a stable root that always exists as an absolute path. + // We are NOT actually reading/writing — only testing string comparisons. + const root = nodePath.resolve( + nodePath.join( + process.platform === "win32" ? "C:\\projects\\ws" : "/projects/ws", + ), + ); + + // ── Acceptance ───────────────────────────────────────────────────────────── + + test("accepts a file directly inside the workspace root", () => { + const result = resolveSafeChildPath(root, "notes.md"); + expect(result.startsWith(root)).toBe(true); + expect(result).toContain("notes.md"); + }); + + test("accepts a nested file inside the workspace root", () => { + const result = resolveSafeChildPath(root, "reports/q1.csv"); + expect(result.startsWith(root)).toBe(true); + }); + + // Regression #2285 — vector (b): case-mismatch on Windows/macOS + test("[Regression #2285-b] accepts path whose casing differs from root (case-insensitive OS fix)", () => { + // On Windows and macOS the filesystem is case-insensitive, so a path + // differing only in case should still be accepted rather than rejected + // with a false-positive traversal error. + const upperRoot = root.toUpperCase(); + // Build a candidate that resolves to the same location but with different case. + const child = "notes.md"; + // We test the function against the lower-cased root; on a real Windows/macOS + // system `resolve` would normalise the case; here we validate the guard itself. + // The function should NOT throw for a valid child. + expect(() => resolveSafeChildPath(upperRoot, child)).not.toThrow(); + }); + + // ── Rejection ───────────────────────────────────────────────────────────── + + // Regression #2285 — vector (a) at the resolveSafeChildPath layer + test("[Regression #2285-a] rejects a path that resolves to the root itself", () => { + // Passing "." or "" resolves to the root — must be rejected. + expect(() => resolveSafeChildPath(root, ".")).toThrow(/Path must point to a file/); + }); + + test("[Regression #2285-a] rejects a path that escapes via ..", () => { + expect(() => resolveSafeChildPath(root, "../escape.md")).toThrow( + /Path traversal is not allowed/, + ); + }); + + test("[Regression #2285-a] rejects a deeply nested traversal", () => { + expect(() => resolveSafeChildPath(root, "a/b/../../../escape.md")).toThrow( + /Path traversal is not allowed/, + ); + }); + + // Regression #2285 — vector (c): sibling-workspace-prefix bypass + test("[Regression #2285-c] rejects a sibling workspace whose name starts with the same prefix", () => { + // /projects/ws-evil must not be accepted when root is /projects/ws. + // The trailing separator in `rootPrefix = root + sep` prevents this. + const siblingEscape = process.platform === "win32" + ? root.replace("ws", "ws-evil") + "\\secret.md" + : root.replace("ws", "ws-evil") + "/secret.md"; + expect(() => resolveSafeChildPath(root, nodePath.relative(root, siblingEscape))).toThrow( + /Path traversal is not allowed/, + ); + }); + + test("rejects an absolute path to an unrelated directory", () => { + const outside = process.platform === "win32" ? "C:\\Windows\\System32\\cmd.exe" : "/etc/passwd"; + expect(() => resolveSafeChildPath(root, outside)).toThrow(/Path traversal is not allowed/); + }); +}); diff --git a/apps/server/src/path-security.ts b/apps/server/src/path-security.ts new file mode 100644 index 000000000..ef96329fa --- /dev/null +++ b/apps/server/src/path-security.ts @@ -0,0 +1,139 @@ +/** + * path-security.ts + * + * Shared workspace path-security primitives. + * + * These functions are the single source of truth for sandbox boundary checks. + * Both `routes/files.ts` and `extensions/openai-image-generation.ts` import + * from here so the logic can never drift between the two copies. + * + * Design decisions + * ──────────────── + * 1. Case-insensitive comparison on win32 *and* darwin. + * APFS / HFS+ are case-insensitive by default on macOS, so the same + * lower-casing guard that fixes Windows also prevents false-rejections on + * macOS without ever relaxing the actual containment check. + * + * 2. Trailing separator in the root prefix (`rootResolved + sep`). + * This is the classic "sibling-prefix" guard: it makes sure that a + * workspace at /projects/ws cannot be escaped by /projects/ws-secret. + * `startsWith("/projects/ws/")` is false for "/projects/ws-secret/…". + * + * 3. `posix.normalize` before the per-part `..` check in + * `normalizeWorkspaceRelativePath`. + * `posix.normalize` collapses `a/b/../c` → `a/c`, which means benign + * dot-dot segments inside an otherwise valid path are accepted rather than + * rejected as traversal. Any traversal that *leaves* the root produces a + * leading `../` or exactly `..`, which is then caught and rejected. + * + * 4. Symlinks are deliberately NOT followed (no `realpath`). + * Resolving symlinks is an async FS operation and would change the + * signature of `resolveSafeChildPath` from sync to async everywhere it is + * called. Symlink-based escapes require the attacker to control a symlink + * *inside* the workspace, which is a much narrower threat model; that + * mitigation is noted as out-of-scope and can be layered on separately. + */ + +import { posix, resolve, sep } from "node:path"; +import { ApiError } from "./errors.js"; + +/** + * Returns true when the current OS uses a case-insensitive filesystem by + * default (Windows and macOS). Used to decide whether path-prefix checks + * should fold case. + */ +function isCaseInsensitiveFs(): boolean { + return process.platform === "win32" || process.platform === "darwin"; +} + +/** + * Normalise a *workspace-relative* path supplied by the client. + * + * - Converts back-slashes to forward-slashes (Windows payloads). + * - Strips common "workspace/…" or "/workspace/…" prefix variants that + * the OpenWork UI and agent tooling tend to emit. + * - Collapses benign in-bounds dot-dot segments (`a/b/../c` → `a/c`). + * - Rejects null bytes, absolute paths, and traversal that escapes the root. + * + * @throws {ApiError} 400 on any invalid or traversal path. + */ +export function normalizeWorkspaceRelativePath( + input: string, + options: { allowSubdirs: boolean }, +): string { + const raw = String(input ?? "").trim(); + if (!raw) { + throw new ApiError(400, "invalid_path", "Path is required"); + } + if (raw.includes("\u0000")) { + throw new ApiError(400, "invalid_path", "Path contains null byte"); + } + + // Normalise separators first so every subsequent regex works on forward- + // slash paths regardless of the client OS. + let normalized = raw.replace(/\\/g, "/"); + normalized = normalized.replace(/^\/+/, ""); + normalized = normalized.replace(/^\.\//, ""); + normalized = normalized.replace(/^workspaces\/[^/]+\//i, ""); + normalized = normalized.replace(/^workspace\/(?:ws_[^/]+|\d+|[0-9a-f-]{6,})\//i, ""); + normalized = normalized.replace(/^workspace\//, ""); + normalized = normalized.replace(/^\/+/, ""); + + // Collapse benign in-bounds dot-dot segments (e.g. a/b/../c → a/c). + // Any traversal that still escapes the root after collapsing produces a + // leading "../" or exactly "..", which we catch below. + normalized = posix.normalize(normalized); + if (normalized.startsWith("../") || normalized === "..") { + throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); + } + + const parts = normalized.split("/").filter(Boolean); + if (!parts.length) { + throw new ApiError(400, "invalid_path", "Path is required"); + } + if (!options.allowSubdirs && parts.length > 1) { + throw new ApiError(400, "invalid_path", "Subdirectories are not allowed"); + } + // Belt-and-suspenders: posix.normalize only leaves a leading ".." (already + // caught above); this loop catches any surviving bare ".." or "." parts. + for (const part of parts) { + if (part === "." || part === "..") { + throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); + } + } + return parts.join("/"); +} + +/** + * Resolve `child` relative to `root` and assert it stays inside `root`. + * + * Uses a case-insensitive prefix check on win32 and darwin to avoid + * false-rejections caused by OS-level path normalisation that changes + * letter-case (e.g. `C:\Users\Me\ws` vs `c:\users\me\ws`). + * + * The trailing separator in the prefix (`rootResolved + sep`) prevents the + * sibling-workspace-prefix attack: `/projects/ws` cannot be escaped via + * `/projects/ws-evil`. + * + * @throws {ApiError} 400 when `child` resolves to `root` itself (not a file) + * or to any path outside `root`. + */ +export function resolveSafeChildPath(root: string, child: string): string { + const rootResolved = resolve(root); + const candidate = resolve(rootResolved, child); + + if (candidate === rootResolved) { + throw new ApiError(400, "invalid_path", "Path must point to a file"); + } + + const rootPrefix = rootResolved + sep; + const safe = isCaseInsensitiveFs() + ? candidate.toLowerCase().startsWith(rootPrefix.toLowerCase()) + : candidate.startsWith(rootPrefix); + + if (!safe) { + throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); + } + + return candidate; +} diff --git a/apps/server/src/routes/files.ts b/apps/server/src/routes/files.ts index b265b7834..91dc9d1ad 100644 --- a/apps/server/src/routes/files.ts +++ b/apps/server/src/routes/files.ts @@ -1,9 +1,10 @@ import { createReadStream } from "node:fs"; import { readFile, readdir, rename, rm, stat, writeFile } from "node:fs/promises"; -import { basename, dirname, isAbsolute, join, relative, resolve, sep, posix } from "node:path"; +import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; import { Readable } from "node:stream"; import { recordAudit } from "../audit.js"; import { ApiError } from "../errors.js"; +import { normalizeWorkspaceRelativePath, resolveSafeChildPath } from "../path-security.js"; import { FileSessionStore } from "../file-sessions.js"; import type { ApprovalRequest, ServerConfig, TokenScope, WorkspaceInfo } from "../types.js"; import { ensureDir, exists, shortId } from "../utils.js"; @@ -43,45 +44,10 @@ function resolveOutboxDir(workspaceRoot: string): string { return join(workspaceRoot, ".opencode", "openwork", "outbox"); } -export function normalizeWorkspaceRelativePath(input: string, options: { allowSubdirs: boolean }): string { - const raw = String(input ?? "").trim(); - if (!raw) { - throw new ApiError(400, "invalid_path", "Path is required"); - } - if (raw.includes("\u0000")) { - throw new ApiError(400, "invalid_path", "Path contains null byte"); - } - - // A lot of user-facing surfaces (artifacts, tool logs) reference files as - // `workspace/` or `/workspace/`. The server API expects - // workspace-relative paths, so normalize those common prefixes here. - let normalized = raw.replace(/\\/g, "/"); - normalized = normalized.replace(/^\/+/, ""); - normalized = normalized.replace(/^\.\//, ""); - normalized = normalized.replace(/^workspaces\/[^/]+\//i, ""); - normalized = normalized.replace(/^workspace\/(?:ws_[^/]+|\d+|[0-9a-f-]{6,})\//i, ""); - normalized = normalized.replace(/^workspace\//, ""); - normalized = normalized.replace(/^\/+/, ""); - - normalized = posix.normalize(normalized); - if (normalized.startsWith("../") || normalized === "..") { - throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); - } - - const parts = normalized.split("/").filter(Boolean); - if (!parts.length) { - throw new ApiError(400, "invalid_path", "Path is required"); - } - if (!options.allowSubdirs && parts.length > 1) { - throw new ApiError(400, "invalid_path", "Subdirectories are not allowed"); - } - for (const part of parts) { - if (part === "." || part === "..") { - throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); - } - } - return parts.join("/"); -} +// normalizeWorkspaceRelativePath and resolveSafeChildPath are imported from +// ../path-security.ts — the single shared implementation used by both this +// module and extensions/openai-image-generation.ts. +export { normalizeWorkspaceRelativePath } from "../path-security.js"; export function isSupportedWorkspaceTextFilePath(relativePath: string): boolean { const lowered = relativePath.toLowerCase(); @@ -114,25 +80,6 @@ export function isSupportedWorkspaceTextFilePath(relativePath: string): boolean ); } -function resolveSafeChildPath(root: string, child: string): string { - const rootResolved = resolve(root); - const candidate = resolve(rootResolved, child); - if (candidate === rootResolved) { - throw new ApiError(400, "invalid_path", "Path must point to a file"); - } - - const isCaseInsensitive = process.platform === "win32" || process.platform === "darwin"; - const rootPrefix = rootResolved + sep; - const isSafe = isCaseInsensitive - ? candidate.toLowerCase().startsWith(rootPrefix.toLowerCase()) - : candidate.startsWith(rootPrefix); - - if (!isSafe) { - throw new ApiError(400, "invalid_path", "Path traversal is not allowed"); - } - return candidate; -} - function encodeArtifactId(path: string): string { return Buffer.from(path, "utf8").toString("base64url"); } From 1f36cb7da18e8fef53fdcd5db46fc3739bfbb757 Mon Sep 17 00:00:00 2001 From: Ashish Soni Date: Thu, 18 Jun 2026 09:49:00 +0530 Subject: [PATCH 4/4] test(security): fix vacuous #2285-b regression test to actually exercise case-insensitive guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous test called resolveSafeChildPath(upperRoot, 'notes.md') with a relative child. Since resolve() builds the candidate FROM upperRoot, both root and candidate shared the same casing — so the old case-sensitive startsWith() check passed trivially, making the test vacuous (it would pass even on pre-PR code). Fix: supply an ABSOLUTE child path whose casing differs from the root string. With that, the old code (case-sensitive) throws a false-rejection, and the new code (case-insensitive via toLowerCase()) correctly accepts it. Split into two platform-aware tests: - win32: c:\projects\ws vs C:\PROJECTS\WS\notes.md - darwin: /projects/ws vs /Projects/WS/notes.md - linux: correctly rejected (legitimately different path on case-sensitive FS) 27 tests pass (was 26). pnpm --filter ./apps/server typecheck -> clean. Addresses review feedback from @Abhijeet1005. --- apps/server/src/path-security.test.ts | 53 +++++++++++++++++++++------ 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/apps/server/src/path-security.test.ts b/apps/server/src/path-security.test.ts index e1a75a137..4f41a2b0f 100644 --- a/apps/server/src/path-security.test.ts +++ b/apps/server/src/path-security.test.ts @@ -193,17 +193,48 @@ describe("resolveSafeChildPath", () => { }); // Regression #2285 — vector (b): case-mismatch on Windows/macOS - test("[Regression #2285-b] accepts path whose casing differs from root (case-insensitive OS fix)", () => { - // On Windows and macOS the filesystem is case-insensitive, so a path - // differing only in case should still be accepted rather than rejected - // with a false-positive traversal error. - const upperRoot = root.toUpperCase(); - // Build a candidate that resolves to the same location but with different case. - const child = "notes.md"; - // We test the function against the lower-cased root; on a real Windows/macOS - // system `resolve` would normalise the case; here we validate the guard itself. - // The function should NOT throw for a valid child. - expect(() => resolveSafeChildPath(upperRoot, child)).not.toThrow(); + // + // The fix: on case-insensitive filesystems (win32, darwin), the boundary + // check folds both root and candidate to lower-case before comparing. + // + // Critical: the child must be an *absolute* path whose casing differs from + // the root string. If the child is relative, resolve() builds the candidate + // FROM the root and both strings share the same case — so the old + // case-sensitive check would pass too, making the test vacuous. + // + // With an absolute child whose case differs: + // OLD (case-sensitive): candidate.startsWith(rootPrefix) → false → throws ❌ + // NEW (case-insensitive): candidate.toLowerCase().startsWith(…toLowerCase()) → true → ok ✔ + test("[Regression #2285-b] accepts absolute child whose casing differs from root on Windows", () => { + if (process.platform !== "win32") return; // Windows-specific case-folding + const lowerRoot = "c:\\projects\\ws"; + // Absolute child with UPPER-case root prefix — same location, different casing. + const absoluteChildUpper = "C:\\PROJECTS\\WS\\notes.md"; + // Pre-PR (case-sensitive): startsWith("c:\\projects\\ws\\") is FALSE for + // "C:\\PROJECTS\\WS\\notes.md" → would throw a false-rejection. + // Post-PR (case-insensitive): toLowerCase() match → correctly accepted. + expect(() => resolveSafeChildPath(lowerRoot, absoluteChildUpper)).not.toThrow(); + }); + + test("[Regression #2285-b] accepts absolute child whose casing differs from root on macOS/POSIX", () => { + if (process.platform === "win32") return; // POSIX/darwin variant + const lowerRoot = "/projects/ws"; + // Absolute path — same location, artificially upper-cased to simulate a + // case-insensitive FS where the OS hands back a differently-cased path. + // We bypass resolve() by constructing the absolute string directly so the + // pre/post-PR difference is visible in the string comparison. + const mixedCaseChild = "/Projects/WS/notes.md"; + // Pre-PR (case-sensitive): startsWith("/projects/ws/") → false → throws. + // Post-PR (case-insensitive on darwin): toLowerCase() match → accepted. + if (process.platform === "darwin") { + expect(() => resolveSafeChildPath(lowerRoot, mixedCaseChild)).not.toThrow(); + } else { + // On Linux (case-sensitive FS) this IS a legitimately different path and + // SHOULD be rejected — the guard is correct to throw here. + expect(() => resolveSafeChildPath(lowerRoot, mixedCaseChild)).toThrow( + /Path traversal is not allowed/, + ); + } }); // ── Rejection ─────────────────────────────────────────────────────────────