From ed960ba4eb3b3cfbe589ecff986b92cb802e3980 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 19:54:19 +0100 Subject: [PATCH] refactor(security): centralize path guard helpers --- src/agents/sandbox-paths.ts | 12 +-- src/infra/archive.test.ts | 5 +- src/infra/archive.ts | 207 +++++++++++++++++++++++------------- src/infra/fs-safe.ts | 21 ++-- src/infra/path-guards.ts | 35 ++++++ 5 files changed, 178 insertions(+), 102 deletions(-) create mode 100644 src/infra/path-guards.ts diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index c7a5192bc..c5547291c 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -2,6 +2,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { isNotFoundPathError, isPathInside } from "../infra/path-guards.js"; const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g; const HTTP_URL_RE = /^https?:\/\//i; @@ -129,8 +130,7 @@ async function assertNoSymlinkEscape( current = target; } } catch (err) { - const anyErr = err as { code?: string }; - if (anyErr.code === "ENOENT") { + if (isNotFoundPathError(err)) { return; } throw err; @@ -146,14 +146,6 @@ async function tryRealpath(value: string): Promise { } } -function isPathInside(root: string, target: string): boolean { - const relative = path.relative(root, target); - if (!relative || relative === "") { - return true; - } - return !(relative.startsWith("..") || path.isAbsolute(relative)); -} - function shortPath(value: string) { if (value.startsWith(os.homedir())) { return `~${value.slice(os.homedir().length)}`; diff --git a/src/infra/archive.test.ts b/src/infra/archive.test.ts index 434cc266d..2f07cbb10 100644 --- a/src/infra/archive.test.ts +++ b/src/infra/archive.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import JSZip from "jszip"; import * as tar from "tar"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import type { ArchiveSecurityError } from "./archive.js"; import { extractArchive, resolveArchiveKind, resolvePackedRootDir } from "./archive.js"; let fixtureRoot = ""; @@ -95,7 +96,9 @@ describe("archive utils", () => { await expect( extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }), - ).rejects.toThrow(/symlink/i); + ).rejects.toMatchObject({ + code: "destination-symlink-traversal", + } satisfies Partial); const outsideFile = path.join(outsideDir, "pwn.txt"); const outsideExists = await fs diff --git a/src/infra/archive.ts b/src/infra/archive.ts index 7d3d90457..0fba57976 100644 --- a/src/infra/archive.ts +++ b/src/infra/archive.ts @@ -10,6 +10,7 @@ import { stripArchivePath, validateArchiveEntryPath, } from "./archive-path.js"; +import { isNotFoundPathError, isPathInside, isSymlinkOpenError } from "./path-guards.js"; export type ArchiveKind = "tar" | "zip"; @@ -32,6 +33,21 @@ export type ArchiveExtractLimits = { maxEntryBytes?: number; }; +export type ArchiveSecurityErrorCode = + | "destination-not-directory" + | "destination-symlink" + | "destination-symlink-traversal"; + +export class ArchiveSecurityError extends Error { + code: ArchiveSecurityErrorCode; + + constructor(code: ArchiveSecurityErrorCode, message: string, options?: ErrorOptions) { + super(message, options); + this.code = code; + this.name = "ArchiveSecurityError"; + } +} + /** @internal */ export const DEFAULT_MAX_ARCHIVE_BYTES_ZIP = 256 * 1024 * 1024; /** @internal */ @@ -196,43 +212,27 @@ function createExtractBudgetTransform(params: { }); } -function isNodeError(value: unknown): value is NodeJS.ErrnoException { - return Boolean( - value && typeof value === "object" && "code" in (value as Record), +function symlinkTraversalError(originalPath: string): ArchiveSecurityError { + return new ArchiveSecurityError( + "destination-symlink-traversal", + `${ERROR_ARCHIVE_ENTRY_TRAVERSES_SYMLINK}: ${originalPath}`, ); } -function isNotFoundError(value: unknown): boolean { - return isNodeError(value) && (value.code === "ENOENT" || value.code === "ENOTDIR"); -} - -function isSymlinkOpenError(value: unknown): boolean { - return ( - isNodeError(value) && - (value.code === "ELOOP" || value.code === "EINVAL" || value.code === "ENOTSUP") - ); -} - -function symlinkTraversalError(originalPath: string): Error { - return new Error(`${ERROR_ARCHIVE_ENTRY_TRAVERSES_SYMLINK}: ${originalPath}`); -} - async function assertDestinationDirReady(destDir: string): Promise { const stat = await fs.lstat(destDir); if (stat.isSymbolicLink()) { - throw new Error("archive destination is a symlink"); + throw new ArchiveSecurityError("destination-symlink", "archive destination is a symlink"); } if (!stat.isDirectory()) { - throw new Error("archive destination is not a directory"); + throw new ArchiveSecurityError( + "destination-not-directory", + "archive destination is not a directory", + ); } return await fs.realpath(destDir); } -function pathInside(root: string, target: string): boolean { - const rel = path.relative(root, target); - return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); -} - async function assertNoSymlinkTraversal(params: { rootDir: string; relPath: string; @@ -246,7 +246,7 @@ async function assertNoSymlinkTraversal(params: { try { stat = await fs.lstat(current); } catch (err) { - if (isNotFoundError(err)) { + if (isNotFoundPathError(err)) { continue; } throw err; @@ -266,12 +266,12 @@ async function assertResolvedInsideDestination(params: { try { resolved = await fs.realpath(params.targetPath); } catch (err) { - if (isNotFoundError(err)) { + if (isNotFoundPathError(err)) { return; } throw err; } - if (!pathInside(params.destinationRealDir, resolved)) { + if (!isPathInside(params.destinationRealDir, resolved)) { throw symlinkTraversalError(params.originalPath); } } @@ -292,7 +292,7 @@ async function cleanupPartialRegularFile(filePath: string): Promise { try { stat = await fs.lstat(filePath); } catch (err) { - if (isNotFoundError(err)) { + if (isNotFoundPathError(err)) { return; } throw err; @@ -310,6 +310,8 @@ type ZipEntry = { async: (type: "nodebuffer") => Promise; }; +type ZipExtractBudget = ReturnType; + async function readZipEntryStream(entry: ZipEntry): Promise { if (typeof entry.nodeStream === "function") { return entry.nodeStream(); @@ -319,6 +321,90 @@ async function readZipEntryStream(entry: ZipEntry): Promise { + await assertNoSymlinkTraversal({ + rootDir: params.destinationDir, + relPath: params.relPath, + originalPath: params.originalPath, + }); + + if (params.isDirectory) { + await fs.mkdir(params.outPath, { recursive: true }); + await assertResolvedInsideDestination({ + destinationRealDir: params.destinationRealDir, + targetPath: params.outPath, + originalPath: params.originalPath, + }); + return; + } + + const parentDir = path.dirname(params.outPath); + await fs.mkdir(parentDir, { recursive: true }); + await assertResolvedInsideDestination({ + destinationRealDir: params.destinationRealDir, + targetPath: parentDir, + originalPath: params.originalPath, + }); +} + +async function writeZipFileEntry(params: { + entry: ZipEntry; + outPath: string; + budget: ZipExtractBudget; +}): Promise { + const handle = await openZipOutputFile(params.outPath, params.entry.name); + params.budget.startEntry(); + const readable = await readZipEntryStream(params.entry); + const writable = handle.createWriteStream(); + + try { + await pipeline( + readable, + createExtractBudgetTransform({ onChunkBytes: params.budget.addBytes }), + writable, + ); + } catch (err) { + await cleanupPartialRegularFile(params.outPath).catch(() => undefined); + throw err; + } + + // Best-effort permission restore for zip entries created on unix. + if (typeof params.entry.unixPermissions === "number") { + const mode = params.entry.unixPermissions & 0o777; + if (mode !== 0) { + await fs.chmod(params.outPath, mode).catch(() => undefined); + } + } +} + async function extractZip(params: { archivePath: string; destDir: string; @@ -342,63 +428,32 @@ async function extractZip(params: { const budget = createByteBudgetTracker(limits); for (const entry of entries) { - validateArchiveEntryPath(entry.name); - - const relPath = stripArchivePath(entry.name, strip); - if (!relPath) { + const output = resolveZipOutputPath({ + entryPath: entry.name, + strip, + destinationDir: params.destDir, + }); + if (!output) { continue; } - validateArchiveEntryPath(relPath); - const outPath = resolveArchiveOutputPath({ - rootDir: params.destDir, - relPath, - originalPath: entry.name, - }); - await assertNoSymlinkTraversal({ - rootDir: params.destDir, - relPath, + await prepareZipOutputPath({ + destinationDir: params.destDir, + destinationRealDir, + relPath: output.relPath, + outPath: output.outPath, originalPath: entry.name, + isDirectory: entry.dir, }); if (entry.dir) { - await fs.mkdir(outPath, { recursive: true }); - await assertResolvedInsideDestination({ - destinationRealDir, - targetPath: outPath, - originalPath: entry.name, - }); continue; } - await fs.mkdir(path.dirname(outPath), { recursive: true }); - await assertResolvedInsideDestination({ - destinationRealDir, - targetPath: path.dirname(outPath), - originalPath: entry.name, + await writeZipFileEntry({ + entry, + outPath: output.outPath, + budget, }); - const handle = await openZipOutputFile(outPath, entry.name); - budget.startEntry(); - const readable = await readZipEntryStream(entry); - const writable = handle.createWriteStream(); - - try { - await pipeline( - readable, - createExtractBudgetTransform({ onChunkBytes: budget.addBytes }), - writable, - ); - } catch (err) { - await cleanupPartialRegularFile(outPath).catch(() => undefined); - throw err; - } - - // Best-effort permission restore for zip entries created on unix. - if (typeof entry.unixPermissions === "number") { - const mode = entry.unixPermissions & 0o777; - if (mode !== 0) { - await fs.chmod(outPath, mode).catch(() => undefined); - } - } } } diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 5c35a5303..7b6c648ee 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -3,6 +3,7 @@ import { constants as fsConstants } from "node:fs"; import type { FileHandle } from "node:fs/promises"; import fs from "node:fs/promises"; import path from "node:path"; +import { isNotFoundPathError, isPathInside, isSymlinkOpenError } from "./path-guards.js"; export type SafeOpenErrorCode = | "invalid-path" @@ -34,27 +35,17 @@ export type SafeLocalReadResult = { stat: Stats; }; -const NOT_FOUND_CODES = new Set(["ENOENT", "ENOTDIR"]); const SUPPORTS_NOFOLLOW = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants; const OPEN_READ_FLAGS = fsConstants.O_RDONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); const ensureTrailingSep = (value: string) => (value.endsWith(path.sep) ? value : value + path.sep); -const isNodeError = (err: unknown): err is NodeJS.ErrnoException => - Boolean(err && typeof err === "object" && "code" in (err as Record)); - -const isNotFoundError = (err: unknown) => - isNodeError(err) && typeof err.code === "string" && NOT_FOUND_CODES.has(err.code); - -const isSymlinkOpenError = (err: unknown) => - isNodeError(err) && (err.code === "ELOOP" || err.code === "EINVAL" || err.code === "ENOTSUP"); - async function openVerifiedLocalFile(filePath: string): Promise { let handle: FileHandle; try { handle = await fs.open(filePath, OPEN_READ_FLAGS); } catch (err) { - if (isNotFoundError(err)) { + if (isNotFoundPathError(err)) { throw new SafeOpenError("not-found", "file not found"); } if (isSymlinkOpenError(err)) { @@ -87,7 +78,7 @@ async function openVerifiedLocalFile(filePath: string): Promise if (err instanceof SafeOpenError) { throw err; } - if (isNotFoundError(err)) { + if (isNotFoundPathError(err)) { throw new SafeOpenError("not-found", "file not found"); } throw err; @@ -102,14 +93,14 @@ export async function openFileWithinRoot(params: { try { rootReal = await fs.realpath(params.rootDir); } catch (err) { - if (isNotFoundError(err)) { + if (isNotFoundPathError(err)) { throw new SafeOpenError("not-found", "root dir not found"); } throw err; } const rootWithSep = ensureTrailingSep(rootReal); const resolved = path.resolve(rootWithSep, params.relativePath); - if (!resolved.startsWith(rootWithSep)) { + if (!isPathInside(rootWithSep, resolved)) { throw new SafeOpenError("invalid-path", "path escapes root"); } @@ -128,7 +119,7 @@ export async function openFileWithinRoot(params: { throw err; } - if (!opened.realPath.startsWith(rootWithSep)) { + if (!isPathInside(rootWithSep, opened.realPath)) { await opened.handle.close().catch(() => {}); throw new SafeOpenError("invalid-path", "path escapes root"); } diff --git a/src/infra/path-guards.ts b/src/infra/path-guards.ts new file mode 100644 index 000000000..55330fa8b --- /dev/null +++ b/src/infra/path-guards.ts @@ -0,0 +1,35 @@ +import path from "node:path"; + +const NOT_FOUND_CODES = new Set(["ENOENT", "ENOTDIR"]); +const SYMLINK_OPEN_CODES = new Set(["ELOOP", "EINVAL", "ENOTSUP"]); + +export function isNodeError(value: unknown): value is NodeJS.ErrnoException { + return Boolean( + value && typeof value === "object" && "code" in (value as Record), + ); +} + +export function hasNodeErrorCode(value: unknown, code: string): boolean { + return isNodeError(value) && value.code === code; +} + +export function isNotFoundPathError(value: unknown): boolean { + return isNodeError(value) && typeof value.code === "string" && NOT_FOUND_CODES.has(value.code); +} + +export function isSymlinkOpenError(value: unknown): boolean { + return isNodeError(value) && typeof value.code === "string" && SYMLINK_OPEN_CODES.has(value.code); +} + +export function isPathInside(root: string, target: string): boolean { + const resolvedRoot = path.resolve(root); + const resolvedTarget = path.resolve(target); + + if (process.platform === "win32") { + const relative = path.win32.relative(resolvedRoot.toLowerCase(), resolvedTarget.toLowerCase()); + return relative === "" || (!relative.startsWith("..") && !path.win32.isAbsolute(relative)); + } + + const relative = path.relative(resolvedRoot, resolvedTarget); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); +}