diff --git a/packages/core/src/filesystem.ts b/packages/core/src/filesystem.ts index 7f0c047de0..477122f12f 100644 --- a/packages/core/src/filesystem.ts +++ b/packages/core/src/filesystem.ts @@ -1,5 +1,5 @@ import { NodeFileSystem } from "@effect/platform-node" -import { dirname, isAbsolute, join, relative, resolve as pathResolve } from "path" +import { dirname, isAbsolute, join, relative, resolve as pathResolve, sep } from "path" import { realpathSync } from "fs" import * as NFS from "fs/promises" import { lookup } from "mime-types" @@ -233,13 +233,11 @@ export namespace AppFileSystem { } export function overlaps(a: string, b: string) { - const relA = relative(a, b) - const relB = relative(b, a) - return !relA || !relA.startsWith("..") || !relB || !relB.startsWith("..") + return contains(a, b) || contains(b, a) } export function contains(parent: string, child: string) { - const rel = relative(parent, child) - return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)) + const result = relative(parent, child) + return result === "" || (!isAbsolute(result) && result !== ".." && !result.startsWith(`..${sep}`)) } } diff --git a/packages/core/test/filesystem/filesystem.test.ts b/packages/core/test/filesystem/filesystem.test.ts index 1d9405333d..973d2eacd5 100644 --- a/packages/core/test/filesystem/filesystem.test.ts +++ b/packages/core/test/filesystem/filesystem.test.ts @@ -354,13 +354,21 @@ describe("AppFileSystem", () => { test("contains checks path containment", () => { expect(AppFileSystem.contains("/a/b", "/a/b/c")).toBe(true) + expect(AppFileSystem.contains("/a/b", "/a/b")).toBe(true) + expect(AppFileSystem.contains("/a/b", "/a/b/..c")).toBe(true) expect(AppFileSystem.contains("/a/b", "/a/c")).toBe(false) + expect(AppFileSystem.contains("/a/b", "/a")).toBe(false) + expect(AppFileSystem.contains("/a/b", "/a/bc")).toBe(false) }) test("overlaps detects overlapping paths", () => { expect(AppFileSystem.overlaps("/a/b", "/a/b/c")).toBe(true) + expect(AppFileSystem.overlaps("/a/b", "/a/b")).toBe(true) + expect(AppFileSystem.overlaps("/a/b", "/a/b/..c")).toBe(true) expect(AppFileSystem.overlaps("/a/b/c", "/a/b")).toBe(true) + expect(AppFileSystem.overlaps("/a/b", "/a")).toBe(true) expect(AppFileSystem.overlaps("/a", "/b")).toBe(false) + expect(AppFileSystem.overlaps("/a/b", "/a/bc")).toBe(false) }) }) }) diff --git a/packages/opencode/src/util/filesystem.ts b/packages/opencode/src/util/filesystem.ts index 696603adbb..6317cf1155 100644 --- a/packages/opencode/src/util/filesystem.ts +++ b/packages/opencode/src/util/filesystem.ts @@ -1,7 +1,7 @@ import { chmod, mkdir, readFile, stat as statFile, writeFile } from "fs/promises" import { createWriteStream, existsSync, statSync } from "fs" import { realpathSync } from "fs" -import { dirname, isAbsolute, join, relative, resolve as pathResolve, win32 } from "path" +import { dirname, isAbsolute, join, relative, resolve as pathResolve, sep, win32 } from "path" import { Readable } from "stream" import { pipeline } from "stream/promises" import { Glob } from "@opencode-ai/core/util/glob" @@ -163,13 +163,12 @@ export function windowsPath(p: string): string { ) } export function overlaps(a: string, b: string) { - const relA = relative(a, b) - const relB = relative(b, a) - return !relA || !relA.startsWith("..") || !relB || !relB.startsWith("..") + return contains(a, b) || contains(b, a) } export function contains(parent: string, child: string) { - return !relative(parent, child).startsWith("..") + const result = relative(parent, child) + return result === "" || (!isAbsolute(result) && result !== ".." && !result.startsWith(`..${sep}`)) } export async function findUp( diff --git a/packages/opencode/test/file/path-traversal.test.ts b/packages/opencode/test/file/path-traversal.test.ts index 1b0dec5aad..6850339e9c 100644 --- a/packages/opencode/test/file/path-traversal.test.ts +++ b/packages/opencode/test/file/path-traversal.test.ts @@ -25,6 +25,7 @@ describe("Filesystem.contains", () => { expect(Filesystem.contains("/project", "/project/src")).toBe(true) expect(Filesystem.contains("/project", "/project/src/file.ts")).toBe(true) expect(Filesystem.contains("/project", "/project")).toBe(true) + expect(Filesystem.contains("/project", "/project/..config")).toBe(true) }), ) @@ -33,6 +34,7 @@ describe("Filesystem.contains", () => { expect(Filesystem.contains("/project", "/project/../etc")).toBe(false) expect(Filesystem.contains("/project", "/project/src/../../etc")).toBe(false) expect(Filesystem.contains("/project", "/etc/passwd")).toBe(false) + expect(Filesystem.contains("/project", "/")).toBe(false) }), ) @@ -52,6 +54,29 @@ describe("Filesystem.contains", () => { ) }) +describe("Filesystem.overlaps", () => { + it.effect("detects containment in either direction", () => + Effect.sync(() => { + expect(Filesystem.overlaps("/project", "/project/src")).toBe(true) + expect(Filesystem.overlaps("/project/src", "/project")).toBe(true) + expect(Filesystem.overlaps("/project", "/project")).toBe(true) + }), + ) + + it.effect("keeps child names beginning with .. inside the parent", () => + Effect.sync(() => { + expect(Filesystem.overlaps("/project", "/project/..config")).toBe(true) + }), + ) + + it.effect("rejects unrelated sibling and parent-only paths", () => + Effect.sync(() => { + expect(Filesystem.overlaps("/project", "/project-other")).toBe(false) + expect(Filesystem.overlaps("/project/src", "/etc")).toBe(false) + }), + ) +}) + /* * Integration tests for read() and list() path traversal protection. *