From d4bf07d07576a9750e7968a7e646b612ba4acb85 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 17:23:24 +0000 Subject: [PATCH] refactor(security): unify hardened install and fs write flows --- src/agents/sandbox/fs-bridge.ts | 137 +++++++++----- src/agents/skills-install-download.ts | 223 +---------------------- src/agents/skills-install-extract.ts | 144 +++++++++++++++ src/agents/skills-install-tar-verbose.ts | 80 ++++++++ src/hooks/install.ts | 46 +++-- src/infra/fs-safe.ts | 116 ++---------- src/infra/install-package-dir.ts | 46 ++--- src/infra/install-target.ts | 41 +++++ src/plugins/install.ts | 43 +++-- 9 files changed, 435 insertions(+), 441 deletions(-) create mode 100644 src/agents/skills-install-extract.ts create mode 100644 src/agents/skills-install-tar-verbose.ts create mode 100644 src/infra/install-target.ts diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 4d28def07d2..e1cca2912eb 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -26,6 +26,11 @@ type PathSafetyOptions = { allowedType?: SafeOpenSyncAllowedType; }; +type PathSafetyCheck = { + target: SandboxResolvedFsPath; + options: PathSafetyOptions; +}; + export type SandboxResolvedPath = { hostPath: string; relativePath: string; @@ -97,8 +102,9 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - await this.assertPathSafety(target, { action: "read files" }); - const result = await this.runCommand('set -eu; cat -- "$1"', { + const result = await this.runCheckedCommand({ + checks: [{ target, options: { action: "read files" } }], + script: 'set -eu; cat -- "$1"', args: [target.containerPath], signal: params.signal, }); @@ -127,8 +133,10 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }); try { - await this.assertPathSafety(target, { action: "write files", requireWritable: true }); - await this.runCommand('set -eu; mv -f -- "$1" "$2"', { + await this.runCheckedCommand({ + checks: [{ target, options: { action: "write files", requireWritable: true } }], + recheckBeforeCommand: true, + script: 'set -eu; mv -f -- "$1" "$2"', args: [tempPath, target.containerPath], signal: params.signal, }); @@ -141,12 +149,18 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "create directories"); - await this.assertPathSafety(target, { - action: "create directories", - requireWritable: true, - allowedType: "directory", - }); - await this.runCommand('set -eu; mkdir -p -- "$1"', { + await this.runCheckedCommand({ + checks: [ + { + target, + options: { + action: "create directories", + requireWritable: true, + allowedType: "directory", + }, + }, + ], + script: 'set -eu; mkdir -p -- "$1"', args: [target.containerPath], signal: params.signal, }); @@ -161,21 +175,23 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "remove files"); - await this.assertPathSafety(target, { - action: "remove files", - requireWritable: true, - aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, - }); const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter( Boolean, ); const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm"; - await this.assertPathSafety(target, { - action: "remove files", - requireWritable: true, - aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, - }); - await this.runCommand(`set -eu; ${rmCommand} -- "$1"`, { + await this.runCheckedCommand({ + checks: [ + { + target, + options: { + action: "remove files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }, + }, + ], + recheckBeforeCommand: true, + script: `set -eu; ${rmCommand} -- "$1"`, args: [target.containerPath], signal: params.signal, }); @@ -191,31 +207,30 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { const to = this.resolveResolvedPath({ filePath: params.to, cwd: params.cwd }); this.ensureWriteAccess(from, "rename files"); this.ensureWriteAccess(to, "rename files"); - await this.assertPathSafety(from, { - action: "rename files", - requireWritable: true, - aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + await this.runCheckedCommand({ + checks: [ + { + target: from, + options: { + action: "rename files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }, + }, + { + target: to, + options: { + action: "rename files", + requireWritable: true, + }, + }, + ], + recheckBeforeCommand: true, + script: + 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', + args: [from.containerPath, to.containerPath], + signal: params.signal, }); - await this.assertPathSafety(to, { - action: "rename files", - requireWritable: true, - }); - await this.assertPathSafety(from, { - action: "rename files", - requireWritable: true, - aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, - }); - await this.assertPathSafety(to, { - action: "rename files", - requireWritable: true, - }); - await this.runCommand( - 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', - { - args: [from.containerPath, to.containerPath], - signal: params.signal, - }, - ); } async stat(params: { @@ -224,8 +239,9 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - await this.assertPathSafety(target, { action: "stat files" }); - const result = await this.runCommand('set -eu; stat -c "%F|%s|%Y" -- "$1"', { + const result = await this.runCheckedCommand({ + checks: [{ target, options: { action: "stat files" } }], + script: 'set -eu; stat -c "%F|%s|%Y" -- "$1"', args: [target.containerPath], signal: params.signal, allowFailure: true, @@ -272,6 +288,33 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }); } + private async runCheckedCommand(params: { + checks: PathSafetyCheck[]; + script: string; + args?: string[]; + stdin?: Buffer | string; + allowFailure?: boolean; + signal?: AbortSignal; + recheckBeforeCommand?: boolean; + }): Promise { + await this.assertPathChecks(params.checks); + if (params.recheckBeforeCommand) { + await this.assertPathChecks(params.checks); + } + return await this.runCommand(params.script, { + args: params.args, + stdin: params.stdin, + allowFailure: params.allowFailure, + signal: params.signal, + }); + } + + private async assertPathChecks(checks: PathSafetyCheck[]): Promise { + for (const check of checks) { + await this.assertPathSafety(check.target, check.options); + } + } + private async assertPathSafety(target: SandboxResolvedFsPath, options: PathSafetyOptions) { const lexicalMount = this.resolveMountByContainerPath(target.containerPath); if (!lexicalMount) { diff --git a/src/agents/skills-install-download.ts b/src/agents/skills-install-download.ts index 2b657956bfb..345fd1a3698 100644 --- a/src/agents/skills-install-download.ts +++ b/src/agents/skills-install-download.ts @@ -1,24 +1,19 @@ -import { createHash, randomUUID } from "node:crypto"; +import { randomUUID } from "node:crypto"; import fs from "node:fs"; 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 } from "../infra/archive-path.js"; -import { - createTarEntrySafetyChecker, - extractArchive as extractArchiveSafe, -} from "../infra/archive.js"; import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js"; import { assertCanonicalPathWithinBase } from "../infra/install-safe-path.js"; import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; import { isWithinDir } from "../infra/path-safety.js"; -import { runCommandWithTimeout } from "../process/exec.js"; import { ensureDir, resolveUserPath } from "../utils.js"; +import { extractArchive } from "./skills-install-extract.js"; import { formatInstallFailureMessage } from "./skills-install-output.js"; import type { SkillInstallResult } from "./skills-install.js"; import type { SkillEntry, SkillInstallSpec } from "./skills.js"; -import { hasBinary } from "./skills.js"; import { resolveSkillToolsRootDir } from "./skills/tools-dir.js"; function isNodeReadableStream(value: unknown): value is NodeJS.ReadableStream { @@ -64,101 +59,6 @@ function resolveArchiveType(spec: SkillInstallSpec, filename: string): string | return undefined; } -const TAR_VERBOSE_MONTHS = new Set([ - "Jan", - "Feb", - "Mar", - "Apr", - "May", - "Jun", - "Jul", - "Aug", - "Sep", - "Oct", - "Nov", - "Dec", -]); -const ISO_DATE_PATTERN = /^\d{4}-\d{2}-\d{2}$/; - -function mapTarVerboseTypeChar(typeChar: string): string { - switch (typeChar) { - case "l": - return "SymbolicLink"; - case "h": - return "Link"; - case "b": - return "BlockDevice"; - case "c": - return "CharacterDevice"; - case "p": - return "FIFO"; - case "s": - return "Socket"; - case "d": - return "Directory"; - default: - return "File"; - } -} - -function parseTarVerboseSize(line: string): number { - const tokens = line.trim().split(/\s+/).filter(Boolean); - if (tokens.length < 6) { - throw new Error(`unable to parse tar verbose metadata: ${line}`); - } - - let dateIndex = tokens.findIndex((token) => TAR_VERBOSE_MONTHS.has(token)); - if (dateIndex > 0) { - const size = Number.parseInt(tokens[dateIndex - 1] ?? "", 10); - if (!Number.isFinite(size) || size < 0) { - throw new Error(`unable to parse tar entry size: ${line}`); - } - return size; - } - - dateIndex = tokens.findIndex((token) => ISO_DATE_PATTERN.test(token)); - if (dateIndex > 0) { - const size = Number.parseInt(tokens[dateIndex - 1] ?? "", 10); - if (!Number.isFinite(size) || size < 0) { - throw new Error(`unable to parse tar entry size: ${line}`); - } - return size; - } - - throw new Error(`unable to parse tar verbose metadata: ${line}`); -} - -function parseTarVerboseMetadata(stdout: string): Array<{ type: string; size: number }> { - const lines = stdout - .split("\n") - .map((line) => line.trim()) - .filter(Boolean); - return lines.map((line) => { - const typeChar = line[0] ?? ""; - if (!typeChar) { - throw new Error("unable to parse tar entry type"); - } - return { - type: mapTarVerboseTypeChar(typeChar), - size: parseTarVerboseSize(line), - }; - }); -} - -async function hashFileSha256(filePath: string): Promise { - const hash = createHash("sha256"); - const stream = fs.createReadStream(filePath); - return await new Promise((resolve, reject) => { - stream.on("data", (chunk) => { - hash.update(chunk as Buffer); - }); - stream.on("error", reject); - stream.on("end", () => { - resolve(hash.digest("hex")); - }); - }); -} - async function downloadFile(params: { url: string; rootDir: string; @@ -201,125 +101,6 @@ async function downloadFile(params: { } } -async function extractArchive(params: { - archivePath: string; - archiveType: string; - targetDir: string; - stripComponents?: number; - timeoutMs: number; -}): Promise<{ stdout: string; stderr: string; code: number | null }> { - const { archivePath, archiveType, targetDir, stripComponents, timeoutMs } = params; - const strip = - typeof stripComponents === "number" && Number.isFinite(stripComponents) - ? Math.max(0, Math.floor(stripComponents)) - : 0; - - try { - if (archiveType === "zip") { - await extractArchiveSafe({ - archivePath, - destDir: targetDir, - timeoutMs, - kind: "zip", - stripComponents: strip, - }); - return { stdout: "", stderr: "", code: 0 }; - } - - if (archiveType === "tar.gz") { - await extractArchiveSafe({ - archivePath, - destDir: targetDir, - timeoutMs, - kind: "tar", - stripComponents: strip, - tarGzip: true, - }); - return { stdout: "", stderr: "", code: 0 }; - } - - if (archiveType === "tar.bz2") { - if (!hasBinary("tar")) { - return { stdout: "", stderr: "tar not found on PATH", code: null }; - } - - const preflightHash = await hashFileSha256(archivePath); - - // Preflight list to prevent zip-slip style traversal before extraction. - const listResult = await runCommandWithTimeout(["tar", "tf", archivePath], { timeoutMs }); - if (listResult.code !== 0) { - return { - stdout: listResult.stdout, - stderr: listResult.stderr || "tar list failed", - code: listResult.code, - }; - } - const entries = listResult.stdout - .split("\n") - .map((line) => line.trim()) - .filter(Boolean); - - const verboseResult = await runCommandWithTimeout(["tar", "tvf", archivePath], { timeoutMs }); - if (verboseResult.code !== 0) { - return { - stdout: verboseResult.stdout, - stderr: verboseResult.stderr || "tar verbose list failed", - code: verboseResult.code, - }; - } - const metadata = parseTarVerboseMetadata(verboseResult.stdout); - if (metadata.length !== entries.length) { - return { - stdout: verboseResult.stdout, - stderr: `tar verbose/list entry count mismatch (${metadata.length} vs ${entries.length})`, - code: 1, - }; - } - const checkTarEntrySafety = createTarEntrySafetyChecker({ - rootDir: targetDir, - stripComponents: strip, - escapeLabel: "targetDir", - }); - for (let i = 0; i < entries.length; i += 1) { - const entryPath = entries[i]; - const entryMeta = metadata[i]; - if (!entryPath || !entryMeta) { - return { - stdout: verboseResult.stdout, - stderr: "tar metadata parse failure", - code: 1, - }; - } - checkTarEntrySafety({ - path: entryPath, - type: entryMeta.type, - size: entryMeta.size, - }); - } - - const postPreflightHash = await hashFileSha256(archivePath); - if (postPreflightHash !== preflightHash) { - return { - stdout: "", - stderr: "tar archive changed during safety preflight; refusing to extract", - code: 1, - }; - } - - const argv = ["tar", "xf", archivePath, "-C", targetDir]; - if (strip > 0) { - argv.push("--strip-components", String(strip)); - } - return await runCommandWithTimeout(argv, { timeoutMs }); - } - - return { stdout: "", stderr: `unsupported archive type: ${archiveType}`, code: null }; - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - return { stdout: "", stderr: message, code: 1 }; - } -} - export async function installDownloadSpec(params: { entry: SkillEntry; spec: SkillInstallSpec; diff --git a/src/agents/skills-install-extract.ts b/src/agents/skills-install-extract.ts new file mode 100644 index 00000000000..4578935378f --- /dev/null +++ b/src/agents/skills-install-extract.ts @@ -0,0 +1,144 @@ +import { createHash } from "node:crypto"; +import fs from "node:fs"; +import { + createTarEntrySafetyChecker, + extractArchive as extractArchiveSafe, +} from "../infra/archive.js"; +import { runCommandWithTimeout } from "../process/exec.js"; +import { parseTarVerboseMetadata } from "./skills-install-tar-verbose.js"; +import { hasBinary } from "./skills.js"; + +export type ArchiveExtractResult = { stdout: string; stderr: string; code: number | null }; + +async function hashFileSha256(filePath: string): Promise { + const hash = createHash("sha256"); + const stream = fs.createReadStream(filePath); + return await new Promise((resolve, reject) => { + stream.on("data", (chunk) => { + hash.update(chunk as Buffer); + }); + stream.on("error", reject); + stream.on("end", () => { + resolve(hash.digest("hex")); + }); + }); +} + +export async function extractArchive(params: { + archivePath: string; + archiveType: string; + targetDir: string; + stripComponents?: number; + timeoutMs: number; +}): Promise { + const { archivePath, archiveType, targetDir, stripComponents, timeoutMs } = params; + const strip = + typeof stripComponents === "number" && Number.isFinite(stripComponents) + ? Math.max(0, Math.floor(stripComponents)) + : 0; + + try { + if (archiveType === "zip") { + await extractArchiveSafe({ + archivePath, + destDir: targetDir, + timeoutMs, + kind: "zip", + stripComponents: strip, + }); + return { stdout: "", stderr: "", code: 0 }; + } + + if (archiveType === "tar.gz") { + await extractArchiveSafe({ + archivePath, + destDir: targetDir, + timeoutMs, + kind: "tar", + stripComponents: strip, + tarGzip: true, + }); + return { stdout: "", stderr: "", code: 0 }; + } + + if (archiveType === "tar.bz2") { + if (!hasBinary("tar")) { + return { stdout: "", stderr: "tar not found on PATH", code: null }; + } + + const preflightHash = await hashFileSha256(archivePath); + + // Preflight list to prevent zip-slip style traversal before extraction. + const listResult = await runCommandWithTimeout(["tar", "tf", archivePath], { timeoutMs }); + if (listResult.code !== 0) { + return { + stdout: listResult.stdout, + stderr: listResult.stderr || "tar list failed", + code: listResult.code, + }; + } + const entries = listResult.stdout + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); + + const verboseResult = await runCommandWithTimeout(["tar", "tvf", archivePath], { timeoutMs }); + if (verboseResult.code !== 0) { + return { + stdout: verboseResult.stdout, + stderr: verboseResult.stderr || "tar verbose list failed", + code: verboseResult.code, + }; + } + const metadata = parseTarVerboseMetadata(verboseResult.stdout); + if (metadata.length !== entries.length) { + return { + stdout: verboseResult.stdout, + stderr: `tar verbose/list entry count mismatch (${metadata.length} vs ${entries.length})`, + code: 1, + }; + } + const checkTarEntrySafety = createTarEntrySafetyChecker({ + rootDir: targetDir, + stripComponents: strip, + escapeLabel: "targetDir", + }); + for (let i = 0; i < entries.length; i += 1) { + const entryPath = entries[i]; + const entryMeta = metadata[i]; + if (!entryPath || !entryMeta) { + return { + stdout: verboseResult.stdout, + stderr: "tar metadata parse failure", + code: 1, + }; + } + checkTarEntrySafety({ + path: entryPath, + type: entryMeta.type, + size: entryMeta.size, + }); + } + + const postPreflightHash = await hashFileSha256(archivePath); + if (postPreflightHash !== preflightHash) { + return { + stdout: "", + stderr: "tar archive changed during safety preflight; refusing to extract", + code: 1, + }; + } + + const argv = ["tar", "xf", archivePath, "-C", targetDir]; + if (strip > 0) { + argv.push("--strip-components", String(strip)); + } + return await runCommandWithTimeout(argv, { timeoutMs }); + } + + return { stdout: "", stderr: `unsupported archive type: ${archiveType}`, code: null }; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return { stdout: "", stderr: message, code: 1 }; + } +} diff --git a/src/agents/skills-install-tar-verbose.ts b/src/agents/skills-install-tar-verbose.ts new file mode 100644 index 00000000000..fb1ce93b12d --- /dev/null +++ b/src/agents/skills-install-tar-verbose.ts @@ -0,0 +1,80 @@ +const TAR_VERBOSE_MONTHS = new Set([ + "Jan", + "Feb", + "Mar", + "Apr", + "May", + "Jun", + "Jul", + "Aug", + "Sep", + "Oct", + "Nov", + "Dec", +]); +const ISO_DATE_PATTERN = /^\d{4}-\d{2}-\d{2}$/; + +function mapTarVerboseTypeChar(typeChar: string): string { + switch (typeChar) { + case "l": + return "SymbolicLink"; + case "h": + return "Link"; + case "b": + return "BlockDevice"; + case "c": + return "CharacterDevice"; + case "p": + return "FIFO"; + case "s": + return "Socket"; + case "d": + return "Directory"; + default: + return "File"; + } +} + +function parseTarVerboseSize(line: string): number { + const tokens = line.trim().split(/\s+/).filter(Boolean); + if (tokens.length < 6) { + throw new Error(`unable to parse tar verbose metadata: ${line}`); + } + + let dateIndex = tokens.findIndex((token) => TAR_VERBOSE_MONTHS.has(token)); + if (dateIndex > 0) { + const size = Number.parseInt(tokens[dateIndex - 1] ?? "", 10); + if (!Number.isFinite(size) || size < 0) { + throw new Error(`unable to parse tar entry size: ${line}`); + } + return size; + } + + dateIndex = tokens.findIndex((token) => ISO_DATE_PATTERN.test(token)); + if (dateIndex > 0) { + const size = Number.parseInt(tokens[dateIndex - 1] ?? "", 10); + if (!Number.isFinite(size) || size < 0) { + throw new Error(`unable to parse tar entry size: ${line}`); + } + return size; + } + + throw new Error(`unable to parse tar verbose metadata: ${line}`); +} + +export function parseTarVerboseMetadata(stdout: string): Array<{ type: string; size: number }> { + const lines = stdout + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); + return lines.map((line) => { + const typeChar = line[0] ?? ""; + if (!typeChar) { + throw new Error("unable to parse tar entry type"); + } + return { + type: mapTarVerboseTypeChar(typeChar), + size: parseTarVerboseSize(line), + }; + }); +} diff --git a/src/hooks/install.ts b/src/hooks/install.ts index fe236ceae7b..a41651e4338 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -8,16 +8,16 @@ import { resolveTimedInstallModeOptions, } from "../infra/install-mode-options.js"; import { installPackageDir } from "../infra/install-package-dir.js"; -import { - assertCanonicalPathWithinBase, - resolveSafeInstallDir, - unscopedPackageName, -} from "../infra/install-safe-path.js"; +import { resolveSafeInstallDir, unscopedPackageName } from "../infra/install-safe-path.js"; import { type NpmIntegrityDrift, type NpmSpecResolution, resolveArchiveSourcePath, } from "../infra/install-source-utils.js"; +import { + ensureInstallTargetAvailable, + resolveCanonicalInstallTarget, +} from "../infra/install-target.js"; import { finalizeNpmSpecArchiveInstall, installFromNpmSpecArchiveWithInstaller, @@ -106,26 +106,12 @@ async function resolveInstallTargetDir( hooksDir?: string, ): Promise<{ ok: true; targetDir: string } | { ok: false; error: string }> { const baseHooksDir = hooksDir ? resolveUserPath(hooksDir) : path.join(CONFIG_DIR, "hooks"); - await fs.mkdir(baseHooksDir, { recursive: true }); - - const targetDirResult = resolveSafeInstallDir({ + return await resolveCanonicalInstallTarget({ baseDir: baseHooksDir, id, invalidNameMessage: "invalid hook name: path traversal detected", + boundaryLabel: "hooks directory", }); - if (!targetDirResult.ok) { - return { ok: false, error: targetDirResult.error }; - } - try { - await assertCanonicalPathWithinBase({ - baseDir: baseHooksDir, - candidatePath: targetDirResult.path, - boundaryLabel: "hooks directory", - }); - } catch (err) { - return { ok: false, error: err instanceof Error ? err.message : String(err) }; - } - return { ok: true, targetDir: targetDirResult.path }; } async function resolveHookNameFromDir(hookDir: string): Promise { @@ -202,8 +188,13 @@ async function installHookPackageFromDir(params: { return { ok: false, error: targetDirResult.error }; } const targetDir = targetDirResult.targetDir; - if (mode === "install" && (await fileExists(targetDir))) { - return { ok: false, error: `hook pack already exists: ${targetDir} (delete it first)` }; + const availability = await ensureInstallTargetAvailable({ + mode, + targetDir, + alreadyExistsError: `hook pack already exists: ${targetDir} (delete it first)`, + }); + if (!availability.ok) { + return availability; } const resolvedHooks = [] as string[]; @@ -294,8 +285,13 @@ async function installHookFromDir(params: { return { ok: false, error: targetDirResult.error }; } const targetDir = targetDirResult.targetDir; - if (mode === "install" && (await fileExists(targetDir))) { - return { ok: false, error: `hook already exists: ${targetDir} (delete it first)` }; + const availability = await ensureInstallTargetAvailable({ + mode, + targetDir, + alreadyExistsError: `hook already exists: ${targetDir} (delete it first)`, + }); + if (!availability.ok) { + return availability; } if (dryRun) { diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 5a48e474eb1..95d20e1b7fe 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -418,8 +418,11 @@ export async function copyFileWithinRoot(params: { relativePath: string; maxBytes?: number; mkdir?: boolean; + rejectSourceHardlinks?: boolean; }): Promise { - const source = await openVerifiedLocalFile(params.sourcePath); + const source = await openVerifiedLocalFile(params.sourcePath, { + rejectHardlinks: params.rejectSourceHardlinks, + }); if (params.maxBytes !== undefined && source.stat.size > params.maxBytes) { await source.handle.close().catch(() => {}); throw new SafeOpenError( @@ -471,108 +474,11 @@ export async function writeFileFromPathWithinRoot(params: { sourcePath: string; mkdir?: boolean; }): Promise { - const { rootReal, rootWithSep, resolved } = await resolvePathWithinRoot(params); - try { - await assertNoPathAliasEscape({ - absolutePath: resolved, - rootPath: rootReal, - boundaryLabel: "root", - }); - } catch (err) { - throw new SafeOpenError("invalid-path", "path alias escape blocked", { cause: err }); - } - if (params.mkdir !== false) { - await fs.mkdir(path.dirname(resolved), { recursive: true }); - } - - const source = await openVerifiedLocalFile(params.sourcePath, { rejectHardlinks: true }); - let ioPath = resolved; - try { - const resolvedRealPath = await fs.realpath(resolved); - if (!isPathInside(rootWithSep, resolvedRealPath)) { - throw new SafeOpenError("outside-workspace", "file is outside workspace root"); - } - ioPath = resolvedRealPath; - } catch (err) { - if (err instanceof SafeOpenError) { - await source.handle.close().catch(() => {}); - throw err; - } - if (!isNotFoundPathError(err)) { - await source.handle.close().catch(() => {}); - throw err; - } - } - - let handle: FileHandle; - let createdForWrite = false; - try { - try { - handle = await fs.open(ioPath, OPEN_WRITE_EXISTING_FLAGS, 0o600); - } catch (err) { - if (!isNotFoundPathError(err)) { - throw err; - } - handle = await fs.open(ioPath, OPEN_WRITE_CREATE_FLAGS, 0o600); - createdForWrite = true; - } - } catch (err) { - await source.handle.close().catch(() => {}); - if (isNotFoundPathError(err)) { - throw new SafeOpenError("not-found", "file not found"); - } - if (isSymlinkOpenError(err)) { - throw new SafeOpenError("invalid-path", "symlink open blocked", { cause: err }); - } - throw err; - } - - let openedRealPath: string | null = null; - try { - const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(ioPath)]); - if (lstat.isSymbolicLink() || !stat.isFile()) { - throw new SafeOpenError("invalid-path", "path is not a regular file under root"); - } - if (stat.nlink > 1) { - throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); - } - if (!sameFileIdentity(stat, lstat)) { - throw new SafeOpenError("path-mismatch", "path changed during write"); - } - - const realPath = await fs.realpath(ioPath); - openedRealPath = realPath; - const realStat = await fs.stat(realPath); - if (!sameFileIdentity(stat, realStat)) { - throw new SafeOpenError("path-mismatch", "path mismatch"); - } - if (realStat.nlink > 1) { - throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); - } - if (!isPathInside(rootWithSep, realPath)) { - throw new SafeOpenError("outside-workspace", "file is outside workspace root"); - } - - if (!createdForWrite) { - await handle.truncate(0); - } - const chunk = Buffer.allocUnsafe(64 * 1024); - let sourceOffset = 0; - while (true) { - const { bytesRead } = await source.handle.read(chunk, 0, chunk.length, sourceOffset); - if (bytesRead <= 0) { - break; - } - await handle.write(chunk.subarray(0, bytesRead), 0, bytesRead, null); - sourceOffset += bytesRead; - } - } catch (err) { - if (createdForWrite && err instanceof SafeOpenError && openedRealPath) { - await fs.rm(openedRealPath, { force: true }).catch(() => {}); - } - throw err; - } finally { - await source.handle.close().catch(() => {}); - await handle.close().catch(() => {}); - } + await copyFileWithinRoot({ + sourcePath: params.sourcePath, + rootDir: params.rootDir, + relativePath: params.relativePath, + mkdir: params.mkdir, + rejectSourceHardlinks: true, + }); } diff --git a/src/infra/install-package-dir.ts b/src/infra/install-package-dir.ts index a2841d42c07..22b2293f465 100644 --- a/src/infra/install-package-dir.ts +++ b/src/infra/install-package-dir.ts @@ -49,6 +49,19 @@ async function sanitizeManifestForNpmInstall(targetDir: string): Promise { await fs.writeFile(manifestPath, `${JSON.stringify(manifest, null, 2)}\n`, "utf-8"); } +async function assertInstallBoundaryPaths(params: { + installBaseDir: string; + candidatePaths: string[]; +}): Promise { + for (const candidatePath of params.candidatePaths) { + await assertCanonicalPathWithinBase({ + baseDir: params.installBaseDir, + candidatePath, + boundaryLabel: "install directory", + }); + } +} + export async function installPackageDir(params: { sourceDir: string; targetDir: string; @@ -63,20 +76,18 @@ export async function installPackageDir(params: { params.logger?.info?.(`Installing to ${params.targetDir}…`); const installBaseDir = path.dirname(params.targetDir); await fs.mkdir(installBaseDir, { recursive: true }); - await assertCanonicalPathWithinBase({ - baseDir: installBaseDir, - candidatePath: params.targetDir, - boundaryLabel: "install directory", + await assertInstallBoundaryPaths({ + installBaseDir, + candidatePaths: [params.targetDir], }); let backupDir: string | null = null; if (params.mode === "update" && (await fileExists(params.targetDir))) { const backupRoot = path.join(path.dirname(params.targetDir), ".openclaw-install-backups"); backupDir = path.join(backupRoot, `${path.basename(params.targetDir)}-${Date.now()}`); await fs.mkdir(backupRoot, { recursive: true }); - await assertCanonicalPathWithinBase({ - baseDir: installBaseDir, - candidatePath: backupDir, - boundaryLabel: "install directory", + await assertInstallBoundaryPaths({ + installBaseDir, + candidatePaths: [backupDir], }); await fs.rename(params.targetDir, backupDir); } @@ -85,25 +96,18 @@ export async function installPackageDir(params: { if (!backupDir) { return; } - await assertCanonicalPathWithinBase({ - baseDir: installBaseDir, - candidatePath: params.targetDir, - boundaryLabel: "install directory", - }); - await assertCanonicalPathWithinBase({ - baseDir: installBaseDir, - candidatePath: backupDir, - boundaryLabel: "install directory", + await assertInstallBoundaryPaths({ + installBaseDir, + candidatePaths: [params.targetDir, backupDir], }); await fs.rm(params.targetDir, { recursive: true, force: true }).catch(() => undefined); await fs.rename(backupDir, params.targetDir).catch(() => undefined); }; try { - await assertCanonicalPathWithinBase({ - baseDir: installBaseDir, - candidatePath: params.targetDir, - boundaryLabel: "install directory", + await assertInstallBoundaryPaths({ + installBaseDir, + candidatePaths: [params.targetDir], }); await fs.cp(params.sourceDir, params.targetDir, { recursive: true }); } catch (err) { diff --git a/src/infra/install-target.ts b/src/infra/install-target.ts new file mode 100644 index 00000000000..38dd103c01c --- /dev/null +++ b/src/infra/install-target.ts @@ -0,0 +1,41 @@ +import fs from "node:fs/promises"; +import { fileExists } from "./archive.js"; +import { assertCanonicalPathWithinBase, resolveSafeInstallDir } from "./install-safe-path.js"; + +export async function resolveCanonicalInstallTarget(params: { + baseDir: string; + id: string; + invalidNameMessage: string; + boundaryLabel: string; +}): Promise<{ ok: true; targetDir: string } | { ok: false; error: string }> { + await fs.mkdir(params.baseDir, { recursive: true }); + const targetDirResult = resolveSafeInstallDir({ + baseDir: params.baseDir, + id: params.id, + invalidNameMessage: params.invalidNameMessage, + }); + if (!targetDirResult.ok) { + return { ok: false, error: targetDirResult.error }; + } + try { + await assertCanonicalPathWithinBase({ + baseDir: params.baseDir, + candidatePath: targetDirResult.path, + boundaryLabel: params.boundaryLabel, + }); + } catch (err) { + return { ok: false, error: err instanceof Error ? err.message : String(err) }; + } + return { ok: true, targetDir: targetDirResult.path }; +} + +export async function ensureInstallTargetAvailable(params: { + mode: "install" | "update"; + targetDir: string; + alreadyExistsError: string; +}): Promise<{ ok: true } | { ok: false; error: string }> { + if (params.mode === "install" && (await fileExists(params.targetDir))) { + return { ok: false, error: params.alreadyExistsError }; + } + return { ok: true }; +} diff --git a/src/plugins/install.ts b/src/plugins/install.ts index 8cce6b06615..ed6a0c3755d 100644 --- a/src/plugins/install.ts +++ b/src/plugins/install.ts @@ -9,7 +9,6 @@ import { } from "../infra/install-mode-options.js"; import { installPackageDir } from "../infra/install-package-dir.js"; import { - assertCanonicalPathWithinBase, resolveSafeInstallDir, safeDirName, unscopedPackageName, @@ -19,6 +18,10 @@ import { type NpmSpecResolution, resolveArchiveSourcePath, } from "../infra/install-source-utils.js"; +import { + ensureInstallTargetAvailable, + resolveCanonicalInstallTarget, +} from "../infra/install-target.js"; import { finalizeNpmSpecArchiveInstall, installFromNpmSpecArchiveWithInstaller, @@ -224,32 +227,23 @@ async function installPluginFromPackageDir(params: { const extensionsDir = params.extensionsDir ? resolveUserPath(params.extensionsDir) : path.join(CONFIG_DIR, "extensions"); - await fs.mkdir(extensionsDir, { recursive: true }); - - const targetDirResult = resolveSafeInstallDir({ + const targetDirResult = await resolveCanonicalInstallTarget({ baseDir: extensionsDir, id: pluginId, invalidNameMessage: "invalid plugin name: path traversal detected", + boundaryLabel: "extensions directory", }); if (!targetDirResult.ok) { return { ok: false, error: targetDirResult.error }; } - const targetDir = targetDirResult.path; - try { - await assertCanonicalPathWithinBase({ - baseDir: extensionsDir, - candidatePath: targetDir, - boundaryLabel: "extensions directory", - }); - } catch (err) { - return { ok: false, error: err instanceof Error ? err.message : String(err) }; - } - - if (mode === "install" && (await fileExists(targetDir))) { - return { - ok: false, - error: `plugin already exists: ${targetDir} (delete it first)`, - }; + const targetDir = targetDirResult.targetDir; + const availability = await ensureInstallTargetAvailable({ + mode, + targetDir, + alreadyExistsError: `plugin already exists: ${targetDir} (delete it first)`, + }); + if (!availability.ok) { + return availability; } if (dryRun) { @@ -393,8 +387,13 @@ export async function installPluginFromFile(params: { } const targetFile = path.join(extensionsDir, `${safeFileName(pluginId)}${path.extname(filePath)}`); - if (mode === "install" && (await fileExists(targetFile))) { - return { ok: false, error: `plugin already exists: ${targetFile} (delete it first)` }; + const availability = await ensureInstallTargetAvailable({ + mode, + targetDir: targetFile, + alreadyExistsError: `plugin already exists: ${targetFile} (delete it first)`, + }); + if (!availability.ok) { + return availability; } if (dryRun) {