diff --git a/.changeset/528-workspace-path-safety.md b/.changeset/528-workspace-path-safety.md new file mode 100644 index 00000000..99b67348 --- /dev/null +++ b/.changeset/528-workspace-path-safety.md @@ -0,0 +1,5 @@ +--- +"@gh-symphony/cli": patch +--- + +Harden workspace path validation against traversal and symlink escapes across platforms (hojinzs/github-symphony#528). diff --git a/packages/core/src/core-conformance.test.ts b/packages/core/src/core-conformance.test.ts index 23818763..4f0c493f 100644 --- a/packages/core/src/core-conformance.test.ts +++ b/packages/core/src/core-conformance.test.ts @@ -16,6 +16,9 @@ import { resolveIssueWorkspaceDirectory, scheduleRetryAt, } from "./index.js"; +import { mkdtempSync, rmSync, symlinkSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { RunDispatchedEvent } from "./observability/structured-events.js"; describe("deriveWorkspaceKey", () => { @@ -80,6 +83,12 @@ describe("resolveIssueWorkspaceDirectory", () => { expect(result).toBe("/runtime/orchestrator/abc123"); }); + it("accepts Windows-style separators in an issue workspace key", () => { + expect(() => + resolveIssueWorkspaceDirectory("/runtime/orchestrator", "abc\\nested") + ).not.toThrow(); + }); + it("rejects path traversal that escapes the root", () => { expect(() => resolveIssueWorkspaceDirectory( @@ -97,6 +106,35 @@ describe("resolveIssueWorkspaceDirectory", () => { resolveIssueWorkspaceDirectory("/runtime/orchestrator", ".lock") ).toThrow("reserved"); }); + + it("rejects an issue workspace symlink that resolves outside the runtime root", () => { + const root = mkdtempSync(join(tmpdir(), "symphony-runtime-root-")); + const outside = mkdtempSync(join(tmpdir(), "symphony-runtime-outside-")); + symlinkSync(outside, join(root, "linked")); + + try { + expect(() => resolveIssueWorkspaceDirectory(root, "linked")).toThrow( + "Issue workspace path escapes" + ); + } finally { + rmSync(root, { force: true, recursive: true }); + rmSync(outside, { force: true, recursive: true }); + } + }); + + it("rejects a dangling issue workspace symlink that points outside the runtime root", () => { + const root = mkdtempSync(join(tmpdir(), "symphony-runtime-root-")); + const outside = join(tmpdir(), "symphony-runtime-missing-target"); + symlinkSync(outside, join(root, "linked")); + + try { + expect(() => resolveIssueWorkspaceDirectory(root, "linked")).toThrow( + "Issue workspace path escapes" + ); + } finally { + rmSync(root, { force: true, recursive: true }); + } + }); }); describe("resolveIssueRepositoryPath", () => { diff --git a/packages/core/src/workspace-safety.test.ts b/packages/core/src/workspace-safety.test.ts index f8e747f8..d2da7ff3 100644 --- a/packages/core/src/workspace-safety.test.ts +++ b/packages/core/src/workspace-safety.test.ts @@ -1,11 +1,22 @@ -import { describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync, symlinkSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; import { assertRepositoryAllowed, resolveWorkspaceDirectory } from "./index.js"; +const temporaryDirectories: string[] = []; + +afterEach(() => { + for (const directory of temporaryDirectories.splice(0)) { + rmSync(directory, { force: true, recursive: true }); + } +}); + describe("resolveWorkspaceDirectory", () => { it("keeps workspaces inside the configured root", () => { - expect(resolveWorkspaceDirectory("/tmp/github-symphony", "workspace-1")).toBe( - "/tmp/github-symphony/workspace-1" - ); + expect( + resolveWorkspaceDirectory("/tmp/github-symphony", "workspace-1") + ).toBe("/tmp/github-symphony/workspace-1"); }); it("rejects path traversal", () => { @@ -13,13 +24,44 @@ describe("resolveWorkspaceDirectory", () => { resolveWorkspaceDirectory("/tmp/github-symphony", "../outside") ).toThrow("Workspace path escapes"); }); + + it("rejects a symlink that resolves outside the workspace root", () => { + const root = mkdtempSync(join(tmpdir(), "symphony-workspace-root-")); + const outside = mkdtempSync(join(tmpdir(), "symphony-workspace-outside-")); + temporaryDirectories.push(root, outside); + symlinkSync(outside, join(root, "linked")); + + expect(() => resolveWorkspaceDirectory(root, "linked")).toThrow( + "Workspace path escapes" + ); + expect(() => resolveWorkspaceDirectory(root, "linked/new")).toThrow( + "Workspace path escapes" + ); + }); + + it("rejects a dangling symlink that points outside the workspace root", () => { + const root = mkdtempSync(join(tmpdir(), "symphony-workspace-root-")); + const outside = join(tmpdir(), "symphony-workspace-missing-target"); + temporaryDirectories.push(root); + symlinkSync(outside, join(root, "linked")); + + expect(() => resolveWorkspaceDirectory(root, "linked")).toThrow( + "Workspace path escapes" + ); + }); + + it("accepts paths containing Windows-style separators as workspace names", () => { + expect(() => + resolveWorkspaceDirectory("/tmp/github-symphony", "workspace\\nested") + ).not.toThrow(); + }); }); describe("assertRepositoryAllowed", () => { it("rejects repositories outside the workspace allowlist", () => { expect(() => assertRepositoryAllowed("https://github.com/acme/other.git", [ - "https://github.com/acme/platform.git" + "https://github.com/acme/platform.git", ]) ).toThrow("Repository is not in the workspace allowlist"); }); diff --git a/packages/core/src/workspace/identity.ts b/packages/core/src/workspace/identity.ts index 5149a455..8f08afd2 100644 --- a/packages/core/src/workspace/identity.ts +++ b/packages/core/src/workspace/identity.ts @@ -1,6 +1,7 @@ import { resolve, join } from "node:path"; import { createHash } from "node:crypto"; import type { IssueSubjectIdentity } from "../domain/issue.js"; +import { isPathWithinRoot } from "./path-safety.js"; const RESERVED_WORKSPACE_KEYS = new Set([ "cache", @@ -72,7 +73,7 @@ export function resolveIssueWorkspaceDirectory( const normalizedRuntimeRoot = resolve(runtimeRoot); const candidate = resolve(normalizedRuntimeRoot, workspaceKey); - if (!candidate.startsWith(`${normalizedRuntimeRoot}/`)) { + if (!isPathWithinRoot(normalizedRuntimeRoot, candidate, false)) { throw new Error( "Issue workspace path escapes the configured runtime root." ); @@ -87,8 +88,7 @@ export function resolveIssueWorkspaceDirectory( function isReservedWorkspaceKey(workspaceKey: string): boolean { return ( - workspaceKey.startsWith(".") || - RESERVED_WORKSPACE_KEYS.has(workspaceKey) + workspaceKey.startsWith(".") || RESERVED_WORKSPACE_KEYS.has(workspaceKey) ); } diff --git a/packages/core/src/workspace/path-safety.ts b/packages/core/src/workspace/path-safety.ts new file mode 100644 index 00000000..c8ece318 --- /dev/null +++ b/packages/core/src/workspace/path-safety.ts @@ -0,0 +1,59 @@ +import { lstatSync, realpathSync } from "node:fs"; +import { basename, dirname, isAbsolute, join, relative } from "node:path"; + +function realpathWithMissingTail(path: string): string | null { + let current = path; + const missingTail: string[] = []; + + while (true) { + try { + return join(realpathSync(current), ...missingTail); + } catch (error) { + if ( + !(error instanceof Error && "code" in error && error.code === "ENOENT") + ) { + return null; + } + } + + const parent = dirname(current); + if (parent === current) { + return null; + } + + try { + if (lstatSync(current).isSymbolicLink()) { + return null; + } + } catch (error) { + if ( + !(error instanceof Error && "code" in error && error.code === "ENOENT") + ) { + return null; + } + } + + missingTail.unshift(basename(current)); + current = parent; + } +} + +export function isPathWithinRoot( + root: string, + candidate: string, + allowRoot = true +): boolean { + const realRoot = realpathWithMissingTail(root); + const realCandidate = realpathWithMissingTail(candidate); + + if (!realRoot || !realCandidate) { + return false; + } + + const rel = relative(realRoot, realCandidate); + + return ( + (allowRoot && rel === "") || + (rel !== "" && !rel.startsWith("..") && !isAbsolute(rel)) + ); +} diff --git a/packages/core/src/workspace/safety.ts b/packages/core/src/workspace/safety.ts index 17ff632f..6e944499 100644 --- a/packages/core/src/workspace/safety.ts +++ b/packages/core/src/workspace/safety.ts @@ -1,4 +1,5 @@ import { resolve } from "node:path"; +import { isPathWithinRoot } from "./path-safety.js"; export function resolveWorkspaceDirectory( workspaceRoot: string, @@ -7,7 +8,7 @@ export function resolveWorkspaceDirectory( const normalizedRoot = resolve(workspaceRoot); const candidate = resolve(normalizedRoot, workspaceId); - if (candidate !== normalizedRoot && !candidate.startsWith(`${normalizedRoot}/`)) { + if (!isPathWithinRoot(normalizedRoot, candidate)) { throw new Error("Workspace path escapes the configured workspace root."); } @@ -19,6 +20,8 @@ export function assertRepositoryAllowed( allowedRepositoryCloneUrls: string[] ): void { if (!allowedRepositoryCloneUrls.includes(targetRepositoryCloneUrl)) { - throw new Error(`Repository is not in the workspace allowlist: ${targetRepositoryCloneUrl}`); + throw new Error( + `Repository is not in the workspace allowlist: ${targetRepositoryCloneUrl}` + ); } }