diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index e9f7474c..c9a8f96c 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -25,13 +25,15 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code - **Commit before fire-testing a rule or guard.** The loop (mutate → confirm the check fails → restore) restores with `git checkout `, which discards all uncommitted work in that file, including the fix under test. - **Fire-test with `ARCHGATE_TELEMETRY=0` whenever the guard under test throws.** A local `bun run cli` run reports `environment: production` to Sentry, so a guard reaching the exit-2 boundary files a real issue against the product from a deliberately broken tree. A guard reporting an ADR violation is safe; one that throws is not. - **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point. -- **Confirm a fire-test fixture holds the bytes you meant before believing the result.** A `bun -e` body inside shell single quotes is still parsed as a JS string literal, so an escape written for the file collapses one level on the way in and the injected "violation" lands as ordinary well-formed text — a false negative that looks like a defect in the rule. Build such fixtures from `String.fromCharCode` and print the file back (`cat -A`) first. +- **Confirm a probe or fire-test fixture holds the bytes you meant before believing the result.** A `bun -e` body inside shell single quotes is still parsed as a JS string literal, so an escape written for the file collapses one level on the way in and the injected "violation" lands as ordinary well-formed text — a false negative that looks like a defect in the rule. A quoted `<<'EOF'` heredoc through the Bash tool is not reliably literal either: a backslash-heavy regex probe run that way reported no match for a pattern that matches fine. Write any escape-sensitive probe to a real file with the Write tool instead of inlining it in a shell command, build such fixtures from `String.fromCodePoint` (oxlint's `unicorn/prefer-code-point` rejects `fromCharCode`), and print the file back (`cat -A`) first. - **A Sentry issue whose `install_path` is a local worktree is an artifact of that worktree, not a user report.** Confirm provenance from `install_path`, `is_ci`, and the timestamp — then still ask whether the error was classified correctly, because reaching Sentry at all claims archgate has a bug rather than the caller. - **`--update-snapshots` is never on its own the fix for a failing snapshot.** `tests/helpers/__snapshots__/rules-shim.test.ts.snap` is the entire `rules.d.ts` a governed project receives, and its diff is the review artifact — read every hunk and confirm it follows from an intended `src/formats/rules.ts` edit before regenerating. Deleting the file routes around nothing: Bun fails a missing snapshot whenever `CI` is set, and passes it locally. - **`actionlint` silently skips its shellcheck-backed checks when `shellcheck` is not on PATH.** A Windows dev machine typically has none, so a local run exits 0 while CI (ubuntu-latest) fails on a `run:` block — SC2086 on an unquoted variable is the common one. A Git Bash `PATH` entry must use the Unix-style form (`/c/Users/...`), or the lookup silently fails. - **Splitting a test file for `oxlint`'s 500-line `max-lines` cap: add a sibling `-.test.ts`, don't trim coverage.** Precedent: `check-max-warnings.test.ts` beside `check.test.ts`; followed again for `reporter-strict.test.ts`, `sync-strict.test.ts`, and the `*-strict.test.ts` integration files. - **`typescript/no-unnecessary-condition` does not flag a string-literal union compared against a literal outside it.** With `typeAware: true`, a narrowed `"a" | "b"` tested against `""` passes clean while a `string !== undefined` control in the same function is flagged at once. Narrowing a type does not hand the dead comparison to the linter, so a clean lint run is not evidence either way. - **Sizing a change's blast radius by grep: search each token separately, don't require them on one line.** A pattern requiring `"check"` and `"--json"` on the same line missed `tests/commands/check.test.ts`, where they sit on adjacent lines — surfacing only when `bun run validate` failed after a "complete" migration. +- **A `test.skipIf(process.platform === "win32")` test passes vacuously on this machine — run it under WSL before believing it.** A skipped test reports as passing, so an assertion that never executes reads exactly like a verified one. Bun installs in WSL Ubuntu via `curl -fsSL https://bun.sh/install | bash`, then `wsl.exe -d Ubuntu -- bash -lc 'cd /mnt/e/... && ~/.bun/bin/bun test '` runs the Linux-only cases against the Windows checkout. Fire-test there too — a skipped fire-test proves nothing. +- **Run WSL against every test file a change touches, not just the one being written.** Compare the Windows and Linux `skip` counts: a change to a helper's signature broke three `tests/commands/upgrade-action.test.ts` cases that Windows skips, and checking only the helper's own test file missed them until CI. Two caveats: `/mnt/e` fails ~120 subprocess-integration tests (`review-context`, `stream-guards`, `session-context` spawn the CLI) that pass on real CI Linux, so a full-suite WSL run cannot be read as a gate — target the affected files and judge failures by whether they touch your change; and coverage from `/mnt/e` runs is not comparable to CI's. - **Genuine OS-level EPIPE cannot be arranged from bun:test on Windows** — a spawned child's `stdout.cancel()` leaves the child's pipe open, and Git-Bash `cmd | true` pipelines may never break the pipe even unguarded (so a passing fire-test there is inconclusive, not proof). Synthesize `process.stdout.emit("error", err)` with `code: "EPIPE"` instead, as in `tests/integration/stream-guards.test.ts`. A real break IS reproducible locally with a sustained writer piped to `head -c 100`. - **`archgate review-context`'s `--base` diffs against the local `main`/`origin/main` ref, which can be stale-but-tree-identical after a squash merge** — same content, different hash, inflating `allChangedFiles` with the last merged PR's files. Fix with `git fetch origin` + explicit `--base origin/main` (the only option in a worktree, where `fetch origin main:main` is refused because `main` is checked out in the primary tree). Only when the current branch IS the stale `main` and `git diff origin/main HEAD --stat` is empty may `git reset origin/main` (never `--hard`) realign it. - **`docs/public/llms-full.txt` is auto-regenerated by the `update-llms.yaml` PR workflow whenever `docs/src/content/docs/**` changes** — a bot commit lands on the branch shortly after pushing docs edits. Never hand-edit it; `git pull` before continuing, and when CodeRabbit flags stale wording inside it, fix the source `.mdx`. diff --git a/.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md b/.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md index 5af4df1c..2882288c 100644 --- a/.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md +++ b/.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md @@ -20,7 +20,11 @@ Four failure modes worth testing for by hand: **A review agent's verdict on non-English prose is worthless**, and it invents the detail that supports it: an orthography pass over the pt-br docs returned PASS while asserting accents the words do not contain (`depreciadas` "(á)", `governança` "(ã)"). Grep cannot settle a claim about meaning, so verify mechanically instead — does the stripped form still occur, did fenced code blocks change — and leave the language judgement to a human speaker. -**Reproduce a described failure before scheduling work from it**, including when your own scan reports zero. Of three issues one audit derived from memory files, two collapsed to nothing once the failure was actually tested (#517 Go proxy, #518 branch protection); the third was real but larger than described (#516). +**Reproduce a described failure before scheduling work from it**, including when your own scan reports zero. Of three issues one audit derived from memory files, two collapsed to nothing once the failure was actually tested (#517 Go proxy, #518 branch protection); the third was real but larger than described (#516). A second trio filed from coverage work held the same ratio: #554 was accurate, #555's central claim was false (the branch it called dead is load-bearing), and #556 understated its own bug (the leak it pinned to failure paths happens on success too). + +An issue filed by a prior session is unverified in the current checkout, and its own claim of having been verified counts for nothing: #555 opened with "verified empirically against real GNU tar output rather than inferred from reading" and was still wrong about what that output does. Re-run the check yourself. + +Test the premise separately from the defect. A report can name a real defect while its stated mechanism is wrong, and a fix built on the stated mechanism removes working code — #555 asked for the deletion of a guard branch that is load-bearing. And prove a zero is a real zero: `\b` inside a JS template literal is a backspace, not a word boundary, so a regex built that way found no corruption where 69 occurrences sat. diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 033a1609..f32f6b86 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { existsSync } from "node:fs"; +import { existsSync, rmSync } from "node:fs"; import { dirname, join } from "node:path"; import { clearLine, cursorTo } from "node:readline"; @@ -278,17 +278,28 @@ async function upgradeBinary(tag: string): Promise { logDebug("Artifact:", artifact.name, "ext:", artifact.ext); const hint = getManualInstallHint(); + // downloadReleaseBinary creates an extraction directory it cannot remove + // itself, so removing it once the binary is installed is the caller's job. + let extractDir: string | undefined; try { - const onProgress = createDownloadProgress(); - const newBinaryPath = await downloadReleaseBinary( - tag, - artifact, - onProgress - ); - finishDownloadProgress(); - logDebug("Downloaded binary to:", newBinaryPath); - logDebug("Replacing binary:", process.execPath); - replaceBinary(process.execPath, newBinaryPath); + try { + const onProgress = createDownloadProgress(); + const { binaryPath, tmpDir } = await downloadReleaseBinary( + tag, + artifact, + onProgress + ); + extractDir = tmpDir; + finishDownloadProgress(); + logDebug("Downloaded binary to:", binaryPath); + logDebug("Replacing binary:", process.execPath); + replaceBinary(process.execPath, binaryPath); + } finally { + // Runs before the handler below, which ends the process via exitWith(). + if (extractDir !== undefined) { + rmSync(extractDir, { recursive: true, force: true }); + } + } } catch (err) { if (err instanceof Error && err.name === "ExitPromptError") throw err; finishDownloadProgress(); diff --git a/src/helpers/binary-upgrade.ts b/src/helpers/binary-upgrade.ts index 2b13b173..4b76a625 100644 --- a/src/helpers/binary-upgrade.ts +++ b/src/helpers/binary-upgrade.ts @@ -1,7 +1,14 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate import { createHash } from "node:crypto"; -import { chmodSync, mkdtempSync, renameSync, unlinkSync } from "node:fs"; +import { + chmodSync, + lstatSync, + mkdtempSync, + renameSync, + rmSync, + unlinkSync, +} from "node:fs"; import { unlink } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -114,19 +121,49 @@ export type DownloadProgressCallback = (progress: DownloadProgress) => void; // Download and extract // --------------------------------------------------------------------------- +/** + * Run `cmd`, draining both pipes concurrently with the exit code. + * + * @see ARCH-007 — awaiting one pipe alone leaves the child blocked once it + * fills the other, so `proc.exited` never resolves. + */ +async function runCapture( + cmd: string[] +): Promise<{ exitCode: number; stdout: string; stderr: string }> { + const proc = Bun.spawn(cmd, { stdout: "pipe", stderr: "pipe" }); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { exitCode, stdout, stderr }; +} + +export interface DownloadedBinary { + /** Path to the extracted binary, inside {@link DownloadedBinary.tmpDir}. */ + binaryPath: string; + /** + * Extraction directory this call created. The caller removes it once the + * binary is installed; deriving it from `binaryPath` instead would delete + * whichever directory that path happens to sit in. + */ + tmpDir: string; +} + /** * Download and extract the release binary to a temp directory. - * Returns the path to the extracted binary. * * When an `onProgress` callback is provided the response body is streamed * so the caller can display incremental progress. Without the callback the * response is buffered in one shot. + * + * @returns The extracted binary and the directory the caller must remove. */ export async function downloadReleaseBinary( tag: string, artifact: ArtifactInfo, onProgress?: DownloadProgressCallback -): Promise { +): Promise { const baseUrl = `https://github.com/${GITHUB_REPO}/releases/download/${tag}`; const archiveUrl = `${baseUrl}/${artifact.name}${artifact.ext}`; const checksumUrl = `${baseUrl}/${artifact.name}${artifact.ext}.sha256`; @@ -218,62 +255,92 @@ export async function downloadReleaseBinary( logDebug("Checksum verification skipped:", err); } const tmpDir = mkdtempSync(join(tmpdir(), "archgate-upgrade-")); - const archivePath = join(tmpDir, `archgate${artifact.ext}`); - logDebug("Extracting archive to:", tmpDir); + try { + const archivePath = join(tmpDir, `archgate${artifact.ext}`); + logDebug("Extracting archive to:", tmpDir); + + await Bun.write(archivePath, buffer); + + if (artifact.ext === ".tar.gz") { + // Validate archive entries before extraction to prevent path traversal. + // Backslashes are normalized because a member stored as `..\evil` is + // listed escaped by GNU tar and literal by bsdtar; both forms reach the + // `../` check only after normalization. + const list = await runCapture(["tar", "-tzf", archivePath]); + if (list.exitCode !== 0) { + // An empty listing from a failed run would otherwise read as "no + // unsafe entries" and wave the archive through the guard below. + throw new UserError( + `Failed to read archive listing (tar exit code ${list.exitCode})` + ); + } - await Bun.write(archivePath, buffer); + for (const entry of list.stdout.split("\n").filter(Boolean)) { + const normalized = entry.replaceAll("\\", "/").trim(); + if ( + normalized.startsWith("/") || + normalized.includes("../") || + normalized === ".." + ) { + throw new Error( + `Unsafe path in release archive: "${entry}" — aborting extraction` + ); + } + } - if (artifact.ext === ".tar.gz") { - // Validate archive entries before extraction to prevent path traversal - const listProc = Bun.spawn(["tar", "-tzf", archivePath], { - stdout: "pipe", - stderr: "pipe", - }); - const listing = await new Response(listProc.stdout).text(); - await listProc.exited; - - for (const entry of listing.split("\n").filter(Boolean)) { - const normalized = entry.replaceAll("\\", "/").trim(); - if ( - normalized.startsWith("/") || - normalized.includes("../") || - normalized === ".." - ) { - throw new Error( - `Unsafe path in release archive: "${entry}" — aborting extraction` + const { exitCode } = await runCapture([ + "tar", + "-xzf", + archivePath, + "-C", + tmpDir, + ]); + if (exitCode !== 0) { + throw new UserError( + `Failed to extract archive (tar exit code ${exitCode})` + ); + } + } else { + // `-ErrorAction Stop` promotes Expand-Archive's non-terminating error to + // a terminating one; without it PowerShell exits 0 on a corrupt archive + // and extraction failure goes unnoticed. + const { exitCode } = await runCapture([ + "powershell", + "-NoProfile", + "-Command", + `Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpDir}' -Force -ErrorAction Stop`, + ]); + if (exitCode !== 0) { + throw new UserError( + `Failed to extract archive (PowerShell exit code ${exitCode})` ); } } - const proc = Bun.spawn(["tar", "-xzf", archivePath, "-C", tmpDir], { - stdout: "pipe", - stderr: "pipe", - }); - const exitCode = await proc.exited; - if (exitCode !== 0) { + const binaryPath = join(tmpDir, artifact.binaryName); + // lstat rather than existsSync: the latter follows symlinks and accepts + // directories, so an archive member that is either would be installed as + // the binary. replaceBinary renames without following, so a symlink would + // land in ~/.archgate/bin/ pointing wherever the archive chose. + const stats = lstatSync(binaryPath, { throwIfNoEntry: false }); + if (!stats) { throw new UserError( - `Failed to extract archive (tar exit code ${exitCode})` + `Extraction produced no ${artifact.binaryName} — the downloaded archive is corrupt or incomplete` ); } - } else { - const proc = Bun.spawn( - [ - "powershell", - "-NoProfile", - "-Command", - `Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpDir}' -Force`, - ], - { stdout: "pipe", stderr: "pipe" } - ); - const exitCode = await proc.exited; - if (exitCode !== 0) { + if (!stats.isFile()) { throw new UserError( - `Failed to extract archive (PowerShell exit code ${exitCode})` + `Extraction produced ${artifact.binaryName} as a ${stats.isSymbolicLink() ? "symbolic link" : "non-regular file"} — refusing to install it` ); } - } - return join(tmpDir, artifact.binaryName); + return { binaryPath, tmpDir }; + } catch (err) { + // The caller learns of tmpDir only on success, so it can only clean up + // then; every failure has to remove it here. + rmSync(tmpDir, { recursive: true, force: true }); + throw err; + } } // --------------------------------------------------------------------------- diff --git a/tests/commands/upgrade-action.test.ts b/tests/commands/upgrade-action.test.ts index d018efb3..8ba74b92 100644 --- a/tests/commands/upgrade-action.test.ts +++ b/tests/commands/upgrade-action.test.ts @@ -16,6 +16,8 @@ import { spyOn, test, } from "bun:test"; +import { existsSync, mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import { Command } from "@commander-js/extra-typings"; @@ -43,8 +45,11 @@ describe("upgrade action handler (upgrade flow)", () => { let credsSpy: Mock; let originalExecPath: string; let originalIsTTY: boolean | undefined; + /** A real directory, so the command's cleanup is exercised rather than a no-op. */ + let fakeExtractDir: string; beforeEach(() => { + fakeExtractDir = mkdtempSync(join(tmpdir(), "archgate-upgrade-test-")); logSpy = spyOn(console, "log").mockImplementation(() => {}); errorSpy = spyOn(console, "error").mockImplementation(() => {}); exitSpy = spyOn(exitModule, "exitWith").mockImplementation(() => { @@ -64,7 +69,10 @@ describe("upgrade action handler (upgrade flow)", () => { downloadSpy = spyOn( binaryUpgrade, "downloadReleaseBinary" - ).mockResolvedValue("/tmp/new-binary"); + ).mockResolvedValue({ + binaryPath: join(fakeExtractDir, "new-binary"), + tmpDir: fakeExtractDir, + }); replaceSpy = spyOn(binaryUpgrade, "replaceBinary").mockImplementation( () => {} ); @@ -93,6 +101,7 @@ describe("upgrade action handler (upgrade flow)", () => { }); afterEach(() => { + rmSync(fakeExtractDir, { recursive: true, force: true }); logSpy.mockRestore(); errorSpy.mockRestore(); exitSpy.mockRestore(); @@ -126,6 +135,9 @@ describe("upgrade action handler (upgrade flow)", () => { expect(downloadSpy).toHaveBeenCalledTimes(1); expect(replaceSpy).toHaveBeenCalledTimes(1); + // The archive inside it can exceed 100 MB, so a successful upgrade must + // not leave the extraction directory behind. + expect(existsSync(fakeExtractDir)).toBe(false); const output = logSpy.mock.calls .map((c: unknown[]) => String(c[0])) diff --git a/tests/commands/upgrade-dispatch.test.ts b/tests/commands/upgrade-dispatch.test.ts index f1833da0..22120577 100644 --- a/tests/commands/upgrade-dispatch.test.ts +++ b/tests/commands/upgrade-dispatch.test.ts @@ -464,7 +464,10 @@ describe("upgrade dispatch", () => { "downloadReleaseBinary" ).mockImplementation(async (_tag, _artifact, onProgress) => { onProgress?.({ downloadedBytes: 10, totalBytes: 100 }); - return join(tempDir, "new-binary"); + // A subdirectory, so the command's cleanup cannot reach tempDir itself. + const extractDir = join(tempDir, "extract"); + mkdirSync(extractDir, { recursive: true }); + return { binaryPath: join(extractDir, "new-binary"), tmpDir: extractDir }; }); const replaceSpy = spyOn(binaryUpgrade, "replaceBinary").mockImplementation( () => {} diff --git a/tests/helpers/binary-upgrade-archive.test.ts b/tests/helpers/binary-upgrade-archive.test.ts index 85b137b7..4c919536 100644 --- a/tests/helpers/binary-upgrade-archive.test.ts +++ b/tests/helpers/binary-upgrade-archive.test.ts @@ -9,8 +9,8 @@ import { beforeEach, afterEach, } from "bun:test"; -import { rmSync } from "node:fs"; -import { dirname } from "node:path"; +import { readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import { type ArtifactInfo, @@ -41,13 +41,16 @@ function writeHeaderField( } /** - * Build a 512-byte ustar header for a zero-length regular file. + * Build a 512-byte ustar header for a zero-length member. * * `tar` refuses to *create* an archive whose member escapes the extraction * root, so an archive carrying such a member has to be assembled byte by byte. * That is the only shape that reaches the path-traversal guard. + * + * @param linkTarget When given, the member is a symlink to it rather than a + * regular file. */ -function tarHeader(name: string): Uint8Array { +function tarHeader(name: string, linkTarget?: string): Uint8Array { const header = new Uint8Array(512); writeHeaderField(header, 0, name); writeHeaderField(header, 100, "0000644\0"); @@ -57,7 +60,8 @@ function tarHeader(name: string): Uint8Array { writeHeaderField(header, 136, "00000000000\0"); // The checksum is computed with its own field filled with spaces. writeHeaderField(header, 148, " "); - writeHeaderField(header, 156, "0"); + writeHeaderField(header, 156, linkTarget === undefined ? "0" : "2"); + if (linkTarget !== undefined) writeHeaderField(header, 157, linkTarget); writeHeaderField(header, 257, "ustar\0"); writeHeaderField(header, 263, "00"); @@ -78,6 +82,13 @@ function buildTarGz(names: string[]): Uint8Array { return Bun.gzipSync(tar); } +/** A gzipped tar whose sole member is a symlink to `linkTarget`. */ +function buildSymlinkTarGz(name: string, linkTarget: string): Uint8Array { + const tar = new Uint8Array(512 + 1024); + tar.set(tarHeader(name, linkTarget), 0); + return Bun.gzipSync(tar); +} + /** * Serve `archive` as the release download and 404 the checksum request, so the * archive reaches extraction without a checksum to satisfy. @@ -105,6 +116,57 @@ function mockArchiveDownload(archive: Uint8Array): void { }) as unknown as typeof fetch; } +/** Names of the extraction directories `downloadReleaseBinary` currently owns. */ +function upgradeTempDirs(): Set { + return new Set( + readdirSync(tmpdir()).filter((name) => name.startsWith("archgate-upgrade-")) + ); +} + +/** + * Assert that `run` rejects without leaving its extraction directory behind. + * + * @returns The rejection message, for the caller's own assertions. + */ +async function rejectionWithoutLeak(run: Promise): Promise { + const before = upgradeTempDirs(); + const message = await rejectionMessage(run); + const leaked = [...upgradeTempDirs()].filter((name) => !before.has(name)); + expect(leaked).toEqual([]); + return message; +} + +interface SpawnReply { + exitCode: number; + stdout?: string; +} + +/** + * Replace `Bun.spawn` with a stub extracting nothing, answering with either a + * fixed exit code or a per-invocation reply derived from the argv. + * + * @returns The argv of each spawn, populated as calls arrive. + */ +function stubSpawn( + reply: number | ((argv: string[]) => SpawnReply) +): string[][] { + const calls: string[][] = []; + const respond = + typeof reply === "number" ? () => ({ exitCode: reply }) : reply; + const spawnSpy = spyOn(Bun, "spawn"); + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + spawnSpy.mockImplementation(((argv: string[]) => { + calls.push(argv); + const { exitCode, stdout }: SpawnReply = respond(argv); + return { + stdout: stdout ?? "", + stderr: "", + exited: Promise.resolve(exitCode), + }; + }) as unknown as typeof Bun.spawn); + return calls; +} + describe("downloadReleaseBinary archive handling", () => { let originalFetch: typeof fetch; @@ -118,10 +180,9 @@ describe("downloadReleaseBinary archive handling", () => { mock.restore(); }); - // A backslash-separated escape (`..\evil`) has no row: GNU tar lists it with - // the backslash escaped, so the guard's backslash normalization never sees - // the shape it is written for. `test.skipIf(...).each()` also only accepts a - // mutable row array, hence no `as const` here. + // Entries listed verbatim by tar, so the message quotes them unchanged. + // `test.skipIf(...).each()` only accepts a mutable row array, hence no + // `as const` here. const unsafeEntries: string[] = [ "../evil", "pkg/../../evil", @@ -137,7 +198,7 @@ describe("downloadReleaseBinary archive handling", () => { async (entry) => { mockArchiveDownload(buildTarGz([entry])); - const message = await rejectionMessage( + const message = await rejectionWithoutLeak( downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) ); @@ -146,54 +207,127 @@ describe("downloadReleaseBinary archive handling", () => { } ); + // A backslash escape gets its own case because the quoted entry is not the + // stored name: GNU tar lists `..\evil` with the backslash doubled and bsdtar + // lists it verbatim. Normalizing either form yields a `../` the guard trips + // on, so the assertion accepts both spellings. + test.skipIf(process.platform === "win32")( + "aborts extraction for a backslash-separated escape", + async () => { + const entry = `..${String.fromCodePoint(92)}evil`; + mockArchiveDownload(buildTarGz([entry])); + + const message = await rejectionWithoutLeak( + downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) + ); + + expect(message).toContain("Unsafe path in release archive"); + expect(message).toMatch(/\.\.\\+evil/u); + } + ); + test.skipIf(process.platform === "win32")( "extracts an archive whose entries all stay inside the root", async () => { mockArchiveDownload(buildTarGz(["archgate", "nested/dir/file"])); - const binaryPath = await downloadReleaseBinary("v1.0.0", TAR_ARTIFACT); + const { binaryPath, tmpDir } = await downloadReleaseBinary( + "v1.0.0", + TAR_ARTIFACT + ); try { expect(binaryPath).toEndWith("archgate"); + expect(binaryPath).toStartWith(tmpDir); } finally { - // downloadReleaseBinary extracts into its own mkdtemp directory. - rmSync(dirname(binaryPath), { recursive: true, force: true }); + rmSync(tmpDir, { recursive: true, force: true }); } } ); + // The member name is `archgate`, so the path-traversal guard has nothing to + // object to — only the post-extraction check sees what it really is. + test.skipIf(process.platform === "win32")( + "refuses a binary that extracts as a symlink", + async () => { + mockArchiveDownload(buildSymlinkTarGz("archgate", "/bin/sh")); + + const message = await rejectionWithoutLeak( + downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) + ); + + expect(message).toBe( + "Extraction produced archgate as a symbolic link — refusing to install it" + ); + } + ); + + test("rejects when the archive listing fails", async () => { + // Not a gzip stream at all, so `tar -tzf` cannot read it. An empty listing + // must not be mistaken for an archive with no unsafe entries. + mockArchiveDownload(new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8])); + + const message = await rejectionWithoutLeak( + downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) + ); + + expect(message).toContain("Failed to read archive listing (tar exit code"); + }); + test("reports the tar exit code when extraction fails", async () => { - // Not a gzip stream at all: `tar -tzf` lists nothing, so the guard passes - // and `tar -xzf` is what rejects it. mockArchiveDownload(new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8])); + // The listing succeeds with a safe entry so the guard passes and `tar -xzf` + // is what rejects the archive, on every runner rather than only on Linux. + stubSpawn((argv) => + argv.includes("-tzf") + ? { exitCode: 0, stdout: "archgate\n" } + : { exitCode: 2 } + ); - const message = await rejectionMessage( + const message = await rejectionWithoutLeak( downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) ); - expect(message).toContain("Failed to extract archive (tar exit code"); + expect(message).toBe("Failed to extract archive (tar exit code 2)"); }); test("reports the PowerShell exit code when zip extraction fails", async () => { mockArchiveDownload(new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8])); - // A corrupt archive does not produce a failing exit code here: - // `Expand-Archive`'s error is non-terminating, so `powershell -Command` - // still exits 0. Stubbing the spawn is what reaches the failure branch, - // and it reaches it on every runner rather than only on Windows. - const spawnSpy = spyOn(Bun, "spawn"); - // oxlint-disable-next-line typescript/no-unsafe-type-assertion - spawnSpy.mockImplementation((() => ({ - stdout: "", - stderr: "Expand-Archive : Central Directory corrupt.", - exited: Promise.resolve(3), - })) as unknown as typeof Bun.spawn); + // Stubbing the spawn reaches the failure branch on every runner rather + // than only on Windows, the sole platform shipping `.zip` releases. + stubSpawn(3); - const message = await rejectionMessage( + const message = await rejectionWithoutLeak( downloadReleaseBinary("v1.0.0", ZIP_ARTIFACT) ); expect(message).toBe("Failed to extract archive (PowerShell exit code 3)"); }); + test("stops PowerShell on a non-terminating Expand-Archive error", async () => { + mockArchiveDownload(new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8])); + const calls = stubSpawn(0); + + await rejectionWithoutLeak(downloadReleaseBinary("v1.0.0", ZIP_ARTIFACT)); + + // Without `-ErrorAction Stop`, Expand-Archive reports a corrupt archive as + // a non-terminating error and `powershell -Command` still exits 0. + expect(calls[0]).toContain("powershell"); + expect(calls[0].at(-1)).toContain("-ErrorAction Stop"); + }); + + test("rejects when extraction reports success but produces no binary", async () => { + mockArchiveDownload(new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8])); + stubSpawn(0); + + const message = await rejectionWithoutLeak( + downloadReleaseBinary("v1.0.0", ZIP_ARTIFACT) + ); + + expect(message).toBe( + "Extraction produced no archgate.exe — the downloaded archive is corrupt or incomplete" + ); + }); + test("continues past checksum verification when the request fails", async () => { let callCount = 0; // oxlint-disable-next-line typescript/no-unsafe-type-assertion @@ -211,12 +345,12 @@ describe("downloadReleaseBinary archive handling", () => { throw new Error("checksum host unreachable"); }) as unknown as typeof fetch; - const message = await rejectionMessage( + const message = await rejectionWithoutLeak( downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) ); - // The transport failure is swallowed; the run proceeds to extraction. + // The transport failure is swallowed; the run proceeds to archive handling. expect(message).not.toContain("checksum host unreachable"); - expect(message).toContain("Failed to extract archive"); + expect(message).toContain("tar exit code"); }); }); diff --git a/tests/helpers/binary-upgrade.test.ts b/tests/helpers/binary-upgrade.test.ts index e7b11968..692a2545 100644 --- a/tests/helpers/binary-upgrade.test.ts +++ b/tests/helpers/binary-upgrade.test.ts @@ -380,11 +380,17 @@ describe("downloadReleaseBinary", () => { ext: ".zip" as const, binaryName: "archgate.exe", }; + let extractDir: string | undefined; try { - const binaryPath = await downloadReleaseBinary("v1.0.0", artifact); - expect(binaryPath).toContain("archgate.exe"); - expect(existsSync(binaryPath)).toBe(true); + const result = await downloadReleaseBinary("v1.0.0", artifact); + extractDir = result.tmpDir; + expect(result.binaryPath).toContain("archgate.exe"); + expect(existsSync(result.binaryPath)).toBe(true); } finally { + // The extraction directory is this call's, not the fixture's tmpDir. + if (extractDir !== undefined) { + rmSync(extractDir, { recursive: true, force: true }); + } try { rmSync(tmpDir, { recursive: true, force: true }); } catch {