refactor(archive): share archive path safety helpers

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];

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");
+  });
+});

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;
+}

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<T>(
   }
 }
 
-// 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<ArchiveExtractLimits>;
 
 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" ||