diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index af27a9f..4af75a8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,7 +73,10 @@ jobs: - name: Check licenses if: matrix.node-version == 22 - run: npx --yes license-checker --failOn "GPL-2.0;GPL-3.0;AGPL-3.0" + # Pin the top-level package version instead of resolving "latest" at CI + # time, matching the SHA/version-pinning discipline used for gitleaks and + # actions elsewhere in this template. Bump this alongside other deps. + run: npx --yes license-checker@25.0.1 --failOn "GPL-2.0;GPL-3.0;AGPL-3.0" - name: Verify EN/KO docs heading parity if: matrix.node-version == 22 diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 5ce096c..033f1a0 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -157,7 +157,7 @@ jobs: - name: Create GitHub Release if: github.event_name == 'push' || inputs.dry_run == false - uses: softprops/action-gh-release@v3 + uses: softprops/action-gh-release@718ea10b132b3b2eba29c1007bb80653f286566b # v3.0.1 with: tag_name: v${{ steps.pkg.outputs.version }} name: v${{ steps.pkg.outputs.version }} diff --git a/src/audit-cd.ts b/src/audit-cd.ts index 8e680a9..d87b3f4 100644 --- a/src/audit-cd.ts +++ b/src/audit-cd.ts @@ -441,7 +441,15 @@ function probeGithubReleases( const out = execFileSync( "gh", ["release", "view", "--json", "tagName,publishedAt", "-R", id], - { encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }, + { + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + // Bound the outbound GitHub API call so a slow/unreachable host or a + // gh auth prompt can't hang the single-process MCP server. On timeout + // execFileSync throws and the catch below degrades to an error status. + timeout: 10_000, + killSignal: "SIGKILL", + }, ); const data = JSON.parse(out) as { tagName?: string; publishedAt?: string }; const tag = data.tagName ?? null; diff --git a/src/audit-security.ts b/src/audit-security.ts index bf2da76..5d8f348 100644 --- a/src/audit-security.ts +++ b/src/audit-security.ts @@ -251,7 +251,15 @@ function checkSecretScanning( const out = execFileSync( "gh", ["api", `repos/${remote}`, "--jq", ".security_and_analysis.secret_scanning.status // \"\""], - { encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }, + { + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + // Bound the outbound GitHub API call so a slow/unreachable host or a + // gh auth prompt can't hang the single-process MCP server. On timeout + // execFileSync throws and the catch below falls through gracefully. + timeout: 10_000, + killSignal: "SIGKILL", + }, ).trim(); if (out === "enabled") { return { diff --git a/src/download.ts b/src/download.ts index 5963e49..1ec1e5e 100644 --- a/src/download.ts +++ b/src/download.ts @@ -1,5 +1,5 @@ import { mkdir } from "node:fs/promises"; -import { isAbsolute, normalize } from "node:path"; +import { resolve, sep } from "node:path"; import { Readable } from "node:stream"; import { pipeline } from "node:stream/promises"; import { extract } from "tar"; @@ -115,6 +115,10 @@ async function fetchOnce(args: FetchOnceArgs): Promise { if (done) break; total += value.byteLength; if (total > maxSizeBytes) { + // Tear down the underlying fetch/socket the same way the timeout path + // does, so the response body isn't left undrained (fd/socket leak) on + // this DoS-defense branch. + controller.abort(); throw new DownloadError( `download exceeded limit ${maxSizeBytes} bytes`, "SIZE_EXCEEDED", @@ -147,26 +151,41 @@ async function fetchOnce(args: FetchOnceArgs): Promise { /** * Reject tar entries that would escape `destDir` (zip-slip / path-traversal). * - * `tar.extract`'s `strip: 1` peels the leading "-/" segment off - * GitHub archives, so by the time `filter` sees a path it should be a - * relative path inside the project. Anything else — absolute paths, paths - * containing `..`, Windows drive letters — is a malicious or corrupt entry. + * node-tar invokes the user `filter` on the RAW entry path — BEFORE the + * `strip: 1` that peels the leading "-/" segment off GitHub + * archives is applied. So this guard must replicate `strip: 1` itself: it + * drops the first raw segment, then resolves the remainder against `destDir` + * and requires the result to stay strictly inside `destDir`. Doing so on the + * post-strip path is what node-tar will actually extract to, and resolving + * against `destDir` is robust regardless of `..`, absolute paths, or Windows + * drive letters. + * + * IMPORTANT: the first raw segment is dropped BEFORE any normalization — + * normalizing first would let `repo/../../escape` collapse to `../escape` and + * then falsely drop `..` as the "first segment". * * `tar.extract` also rejects symlinks/hardlinks that target outside cwd by * default; we keep that behavior and add an explicit path check on top. */ -export function isSafeTarEntry(path: string): boolean { - if (!path) return false; - if (isAbsolute(path)) return false; - // Reject Windows drive letters that look relative on POSIX (e.g. "C:foo"). - if (/^[A-Za-z]:/.test(path)) return false; - // `normalize` collapses `a/../b` → `b`. If the result still starts with - // `..` it means the entry escapes cwd. - const normalized = normalize(path); - if (normalized.startsWith("..") || normalized.includes(`${"/"}..${"/"}`)) { - return false; - } - return true; +export function isSafeTarEntry(rawPath: string, destDir: string): boolean { + if (!rawPath) return false; + // Replicate node-tar's `strip: 1` on the RAW path first (drop the leading + // "-/" segment). + const stripped = stripTopLevelTarSegment(rawPath); + // Entry was just the top-level directory itself — nothing to extract. + if (!stripped) return false; + // Resolve against destDir and require the result to stay strictly inside it. + // Using `resolvedDest + sep` (not a bare prefix) prevents a sibling like + // "/dest-evil" from matching "/dest". + const resolvedDest = resolve(destDir); + const target = resolve(resolvedDest, stripped); + return target === resolvedDest || target.startsWith(resolvedDest + sep); +} + +function stripTopLevelTarSegment(rawPath: string): string { + const parts = rawPath.replace(/\\/g, "/").split("/"); + parts.shift(); + return parts.join("/"); } export async function extractTarball( @@ -183,7 +202,10 @@ export async function extractTarball( cwd: destDir, strip: 1, filter: (path) => { - if (!isSafeTarEntry(path)) { + if (!stripTopLevelTarSegment(path)) { + return false; + } + if (!isSafeTarEntry(path, destDir)) { // Capture the first offender so we can surface it in the error // instead of silently skipping (which is what `filter` does). rejected ??= path; diff --git a/tests/download.test.ts b/tests/download.test.ts index 261b0d7..e927aa6 100644 --- a/tests/download.test.ts +++ b/tests/download.test.ts @@ -114,39 +114,50 @@ describe("fetchTarball", () => { }); describe("isSafeTarEntry (zip-slip / path-traversal guard)", () => { + // node-tar invokes `filter` on the RAW entry path (before `strip: 1`), so + // every entry from a GitHub archive carries a leading "-/" + // segment. isSafeTarEntry replicates strip:1 itself, then checks the result + // stays inside destDir. Tests therefore use raw (pre-strip) paths + a destDir. + const destDir = "/tmp/create-starter-dest"; const safe = [ - "package.json", - "src/index.ts", - "deeply/nested/file.txt", - "with-dashes/and_underscores.md", - "trailing/slash/", // tar can emit directory entries + "repo-abc/package.json", + "repo-abc/src/index.ts", + "repo-abc/deeply/nested/file.txt", + "repo-abc/with-dashes/and_underscores.md", + "repo-abc/trailing/slash/", // tar can emit directory entries ]; const unsafe = [ "", - "/etc/passwd", - "/absolute/path", - "../escape", - "ok/then/../../../escape", // collapses to ../escape — escapes cwd - "C:windows", // Windows drive letter - "C:\\Users\\Public", - "..", - "../", - "../../", + "repo-abc/../escape.txt", // after strip:1 → ../escape.txt — escapes destDir + "repo-abc/../../escape.txt", // after strip:1 → ../../escape.txt + "repo-abc/ok/then/../../../escape", // after strip:1 collapses to ../escape + "repo-abc", // top-level dir only — nothing to extract after strip + "repo-abc/", // ditto + "repo-abc/..", + "repo-abc/../", ]; - it("accepts paths that resolve back inside cwd even with .. mid-path", () => { - // `ok/then/../../escape` → `escape` (still inside cwd) — safe. - assert.equal(isSafeTarEntry("ok/then/../../escape"), true); + it("accepts raw paths that resolve back inside destDir even with .. mid-path", () => { + // `repo-abc/ok/then/../../escape` → strip → `ok/then/../../escape` + // → resolves to `escape` (still inside destDir) — safe. + assert.equal(isSafeTarEntry("repo-abc/ok/then/../../escape", destDir), true); + }); + + it("strips the RAW first segment before normalizing (double-.. escape)", () => { + // If it normalized first, `repo-abc/../../escape` would collapse to + // `../escape`, then dropping `..` as the "first segment" would falsely + // accept `escape`. Stripping the raw segment first correctly rejects it. + assert.equal(isSafeTarEntry("repo-abc/../../escape", destDir), false); }); for (const p of safe) { it(`accepts ${JSON.stringify(p)}`, () => { - assert.equal(isSafeTarEntry(p), true); + assert.equal(isSafeTarEntry(p, destDir), true); }); } for (const p of unsafe) { it(`rejects ${JSON.stringify(p)}`, () => { - assert.equal(isSafeTarEntry(p), false); + assert.equal(isSafeTarEntry(p, destDir), false); }); } });