From 2b8f1bade0eb33bbc06970397751069e187acc2a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 18 Feb 2026 16:46:51 +0000 Subject: [PATCH] refactor(archive): share archive path safety helpers --- src/agents/skills-install-download.ts | 74 ++++++--------------------- src/infra/archive-path.test.ts | 46 +++++++++++++++++ src/infra/archive-path.ts | 63 +++++++++++++++++++++++ src/infra/archive.ts | 71 ++++++------------------- 4 files changed, 139 insertions(+), 115 deletions(-) create mode 100644 src/infra/archive-path.test.ts create mode 100644 src/infra/archive-path.ts diff --git a/src/agents/skills-install-download.ts b/src/agents/skills-install-download.ts index a586a36438..e184a3d804 100644 --- a/src/agents/skills-install-download.ts +++ b/src/agents/skills-install-download.ts @@ -3,9 +3,15 @@ import path from "node:path"; import { Readable } from "node:stream"; import { pipeline } from "node:stream/promises"; import type { ReadableStream as NodeReadableStream } from "node:stream/web"; +import { + isWindowsDrivePath, + resolveArchiveOutputPath, + stripArchivePath, + validateArchiveEntryPath, +} from "../infra/archive-path.js"; import { extractArchive as extractArchiveSafe } from "../infra/archive.js"; import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; -import { isWithinDir, resolveSafeBaseDir } from "../infra/path-safety.js"; +import { isWithinDir } from "../infra/path-safety.js"; import { runCommandWithTimeout } from "../process/exec.js"; import { ensureDir, resolveUserPath } from "../utils.js"; import { formatInstallFailureMessage } from "./skills-install-output.js"; @@ -18,10 +24,6 @@ function isNodeReadableStream(value: unknown): value is NodeJS.ReadableStream { return Boolean(value && typeof (value as NodeJS.ReadableStream).pipe === "function"); } -function isWindowsDrivePath(p: string): boolean { - return /^[a-zA-Z]:[\\/]/.test(p); -} - function resolveDownloadTargetDir(entry: SkillEntry, spec: SkillInstallSpec): string { const safeRoot = resolveSkillToolsRootDir(entry); const raw = spec.targetDir?.trim(); @@ -61,57 +63,6 @@ function resolveArchiveType(spec: SkillInstallSpec, filename: string): string | return undefined; } -function normalizeArchiveEntryPath(raw: string): string { - return raw.replaceAll("\\", "/"); -} - -function validateArchiveEntryPath(entryPath: string): void { - if (!entryPath || entryPath === "." || entryPath === "./") { - return; - } - if (isWindowsDrivePath(entryPath)) { - throw new Error(`archive entry uses a drive path: ${entryPath}`); - } - const normalized = path.posix.normalize(normalizeArchiveEntryPath(entryPath)); - if (normalized === ".." || normalized.startsWith("../")) { - throw new Error(`archive entry escapes targetDir: ${entryPath}`); - } - if (path.posix.isAbsolute(normalized) || normalized.startsWith("//")) { - throw new Error(`archive entry is absolute: ${entryPath}`); - } -} - -function stripArchivePath(entryPath: string, stripComponents: number): string | null { - const raw = normalizeArchiveEntryPath(entryPath); - if (!raw || raw === "." || raw === "./") { - return null; - } - - // Important: tar's --strip-components semantics operate on raw path segments, - // before any normalization that would collapse "..". We mimic that so we - // can detect strip-induced escapes like "a/../b" with stripComponents=1. - const parts = raw.split("/").filter((part) => part.length > 0 && part !== "."); - const strip = Math.max(0, Math.floor(stripComponents)); - const stripped = strip === 0 ? parts.join("/") : parts.slice(strip).join("/"); - const result = path.posix.normalize(stripped); - if (!result || result === "." || result === "./") { - return null; - } - return result; -} - -function validateExtractedPathWithinRoot(params: { - rootDir: string; - relPath: string; - originalPath: string; -}): void { - const safeBase = resolveSafeBaseDir(params.rootDir); - const outPath = path.resolve(params.rootDir, params.relPath); - if (!outPath.startsWith(safeBase)) { - throw new Error(`archive entry escapes targetDir: ${params.originalPath}`); - } -} - async function downloadFile( url: string, destPath: string, @@ -219,13 +170,18 @@ async function extractArchive(params: { } for (const entry of entries) { - validateArchiveEntryPath(entry); + validateArchiveEntryPath(entry, { escapeLabel: "targetDir" }); const relPath = stripArchivePath(entry, strip); if (!relPath) { continue; } - validateArchiveEntryPath(relPath); - validateExtractedPathWithinRoot({ rootDir: targetDir, relPath, originalPath: entry }); + validateArchiveEntryPath(relPath, { escapeLabel: "targetDir" }); + resolveArchiveOutputPath({ + rootDir: targetDir, + relPath, + originalPath: entry, + escapeLabel: "targetDir", + }); } const argv = ["tar", "xf", archivePath, "-C", targetDir]; diff --git a/src/infra/archive-path.test.ts b/src/infra/archive-path.test.ts new file mode 100644 index 0000000000..bc900c6964 --- /dev/null +++ b/src/infra/archive-path.test.ts @@ -0,0 +1,46 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { + resolveArchiveOutputPath, + stripArchivePath, + validateArchiveEntryPath, +} from "./archive-path.js"; + +describe("archive path helpers", () => { + it("uses custom escape labels in traversal errors", () => { + expect(() => + validateArchiveEntryPath("../escape.txt", { + escapeLabel: "targetDir", + }), + ).toThrow("archive entry escapes targetDir: ../escape.txt"); + }); + + it("preserves strip-induced traversal for follow-up validation", () => { + const stripped = stripArchivePath("a/../escape.txt", 1); + expect(stripped).toBe("../escape.txt"); + expect(() => + validateArchiveEntryPath(stripped ?? "", { + escapeLabel: "targetDir", + }), + ).toThrow("archive entry escapes targetDir: ../escape.txt"); + }); + + it("keeps resolved output paths inside the root", () => { + const rootDir = path.join(path.sep, "tmp", "archive-root"); + const safe = resolveArchiveOutputPath({ + rootDir, + relPath: "sub/file.txt", + originalPath: "sub/file.txt", + }); + expect(safe).toBe(path.resolve(rootDir, "sub/file.txt")); + + expect(() => + resolveArchiveOutputPath({ + rootDir, + relPath: "../escape.txt", + originalPath: "../escape.txt", + escapeLabel: "targetDir", + }), + ).toThrow("archive entry escapes targetDir: ../escape.txt"); + }); +}); diff --git a/src/infra/archive-path.ts b/src/infra/archive-path.ts new file mode 100644 index 0000000000..43da416f28 --- /dev/null +++ b/src/infra/archive-path.ts @@ -0,0 +1,63 @@ +import path from "node:path"; +import { resolveSafeBaseDir } from "./path-safety.js"; + +export function isWindowsDrivePath(value: string): boolean { + return /^[a-zA-Z]:[\\/]/.test(value); +} + +export function normalizeArchiveEntryPath(raw: string): string { + return raw.replaceAll("\\", "/"); +} + +export function validateArchiveEntryPath( + entryPath: string, + params?: { escapeLabel?: string }, +): void { + if (!entryPath || entryPath === "." || entryPath === "./") { + return; + } + if (isWindowsDrivePath(entryPath)) { + throw new Error(`archive entry uses a drive path: ${entryPath}`); + } + const normalized = path.posix.normalize(normalizeArchiveEntryPath(entryPath)); + const escapeLabel = params?.escapeLabel ?? "destination"; + if (normalized === ".." || normalized.startsWith("../")) { + throw new Error(`archive entry escapes ${escapeLabel}: ${entryPath}`); + } + if (path.posix.isAbsolute(normalized) || normalized.startsWith("//")) { + throw new Error(`archive entry is absolute: ${entryPath}`); + } +} + +export function stripArchivePath(entryPath: string, stripComponents: number): string | null { + const raw = normalizeArchiveEntryPath(entryPath); + if (!raw || raw === "." || raw === "./") { + return null; + } + + // Mimic tar --strip-components semantics (raw segments before normalization) + // so strip-induced escapes like "a/../b" are visible to validators. + const parts = raw.split("/").filter((part) => part.length > 0 && part !== "."); + const strip = Math.max(0, Math.floor(stripComponents)); + const stripped = strip === 0 ? parts.join("/") : parts.slice(strip).join("/"); + const result = path.posix.normalize(stripped); + if (!result || result === "." || result === "./") { + return null; + } + return result; +} + +export function resolveArchiveOutputPath(params: { + rootDir: string; + relPath: string; + originalPath: string; + escapeLabel?: string; +}): string { + const safeBase = resolveSafeBaseDir(params.rootDir); + const outPath = path.resolve(params.rootDir, params.relPath); + const escapeLabel = params.escapeLabel ?? "destination"; + if (!outPath.startsWith(safeBase)) { + throw new Error(`archive entry escapes ${escapeLabel}: ${params.originalPath}`); + } + return outPath; +} diff --git a/src/infra/archive.ts b/src/infra/archive.ts index f7d0843732..46d0605984 100644 --- a/src/infra/archive.ts +++ b/src/infra/archive.ts @@ -5,7 +5,11 @@ import { Readable, Transform } from "node:stream"; import { pipeline } from "node:stream/promises"; import JSZip from "jszip"; import * as tar from "tar"; -import { resolveSafeBaseDir } from "./path-safety.js"; +import { + resolveArchiveOutputPath, + stripArchivePath, + validateArchiveEntryPath, +} from "./archive-path.js"; export type ArchiveKind = "tar" | "zip"; @@ -102,59 +106,6 @@ export async function withTimeout( } } -// Path hygiene. -function normalizeArchivePath(raw: string): string { - // Archives may contain Windows separators; treat them as separators. - return raw.replaceAll("\\", "/"); -} - -function isWindowsDrivePath(p: string): boolean { - return /^[a-zA-Z]:[\\/]/.test(p); -} - -function validateArchiveEntryPath(entryPath: string): void { - if (!entryPath || entryPath === "." || entryPath === "./") { - return; - } - if (isWindowsDrivePath(entryPath)) { - throw new Error(`archive entry uses a drive path: ${entryPath}`); - } - const normalized = path.posix.normalize(normalizeArchivePath(entryPath)); - if (normalized === ".." || normalized.startsWith("../")) { - throw new Error(`archive entry escapes destination: ${entryPath}`); - } - if (path.posix.isAbsolute(normalized) || normalized.startsWith("//")) { - throw new Error(`archive entry is absolute: ${entryPath}`); - } -} - -function stripArchivePath(entryPath: string, stripComponents: number): string | null { - const raw = normalizeArchivePath(entryPath); - if (!raw || raw === "." || raw === "./") { - return null; - } - - // Important: mimic tar --strip-components semantics (raw segments before - // normalization) so strip-induced escapes like "a/../b" are not hidden. - const parts = raw.split("/").filter((part) => part.length > 0 && part !== "."); - const strip = Math.max(0, Math.floor(stripComponents)); - const stripped = strip === 0 ? parts.join("/") : parts.slice(strip).join("/"); - const result = path.posix.normalize(stripped); - if (!result || result === "." || result === "./") { - return null; - } - return result; -} - -function resolveCheckedOutPath(destDir: string, relPath: string, original: string): string { - const safeBase = resolveSafeBaseDir(destDir); - const outPath = path.resolve(destDir, relPath); - if (!outPath.startsWith(safeBase)) { - throw new Error(`archive entry escapes destination: ${original}`); - } - return outPath; -} - type ResolvedArchiveExtractLimits = Required; function clampLimit(value: number | undefined): number | undefined { @@ -286,7 +237,11 @@ async function extractZip(params: { } validateArchiveEntryPath(relPath); - const outPath = resolveCheckedOutPath(params.destDir, relPath, entry.name); + const outPath = resolveArchiveOutputPath({ + rootDir: params.destDir, + relPath, + originalPath: entry.name, + }); if (entry.dir) { await fs.mkdir(outPath, { recursive: true }); continue; @@ -379,7 +334,11 @@ export async function extractArchive(params: { return; } validateArchiveEntryPath(relPath); - resolveCheckedOutPath(params.destDir, relPath, info.path); + resolveArchiveOutputPath({ + rootDir: params.destDir, + relPath, + originalPath: info.path, + }); if ( info.type === "SymbolicLink" ||