From ffc40c755c78c94471d8a46c0519b8c0702bd375 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Thu, 6 Aug 2026 22:27:37 +0200 Subject: [PATCH 1/6] fix(upgrade): detect failed extraction and stop leaking temp directories Expand-Archive reports a corrupt archive as a non-terminating error, so `powershell -Command` exits 0 and extraction failure went unnoticed. `-ErrorAction Stop` promotes it to terminating. A post-extraction existence check covers both branches, so an archive that extracts to nothing is reported where it happens rather than as a missing binary later. The extraction directory is removed on every failure path, and the caller removes it once the binary is installed. The tar path-traversal guard's backslash normalization is live, not dead: GNU tar lists a member stored as `..\evil` with the backslash doubled, and normalizing that yields `..//evil`, which contains `../`. bsdtar lists it verbatim, which normalizes to `../evil`. Both trip the guard, and neither does without the normalization. The traversal table gains the case it was missing. Closes #554 Closes #555 Closes #556 Signed-off-by: Rhuan Barreto --- src/commands/upgrade.ts | 34 +++-- src/helpers/binary-upgrade.ts | 123 +++++++++++-------- tests/helpers/binary-upgrade-archive.test.ts | 110 ++++++++++++++--- 3 files changed, 189 insertions(+), 78 deletions(-) diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 033a1609..6f77cb6f 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,29 @@ async function upgradeBinary(tag: string): Promise { logDebug("Artifact:", artifact.name, "ext:", artifact.ext); const hint = getManualInstallHint(); + // downloadReleaseBinary hands back a path inside 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 newBinaryPath = await downloadReleaseBinary( + tag, + artifact, + onProgress + ); + extractDir = dirname(newBinaryPath); + finishDownloadProgress(); + logDebug("Downloaded binary to:", newBinaryPath); + logDebug("Replacing binary:", process.execPath); + replaceBinary(process.execPath, newBinaryPath); + } 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..a672d9cf 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, + existsSync, + mkdtempSync, + renameSync, + rmSync, + unlinkSync, +} from "node:fs"; import { unlink } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -218,62 +225,82 @@ 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); - - await Bun.write(archivePath, buffer); + 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 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` + ); + } + } - 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 proc = Bun.spawn(["tar", "-xzf", archivePath, "-C", tmpDir], { + stdout: "pipe", + stderr: "pipe", + }); + const exitCode = await proc.exited; + if (exitCode !== 0) { + throw new UserError( + `Failed to extract archive (tar exit code ${exitCode})` ); } - } - - const proc = Bun.spawn(["tar", "-xzf", archivePath, "-C", tmpDir], { - stdout: "pipe", - stderr: "pipe", - }); - const exitCode = await proc.exited; - 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 proc = Bun.spawn( + [ + "powershell", + "-NoProfile", + "-Command", + `Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpDir}' -Force -ErrorAction Stop`, + ], + { stdout: "pipe", stderr: "pipe" } ); + const exitCode = await proc.exited; + if (exitCode !== 0) { + throw new UserError( + `Failed to extract archive (PowerShell exit code ${exitCode})` + ); + } } - } 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) { + + const binaryPath = join(tmpDir, artifact.binaryName); + if (!existsSync(binaryPath)) { throw new UserError( - `Failed to extract archive (PowerShell exit code ${exitCode})` + `Extraction produced no ${artifact.binaryName} — the downloaded archive is corrupt or incomplete` ); } - } - return join(tmpDir, artifact.binaryName); + return binaryPath; + } catch (err) { + // The caller only receives a path on success, so it can only clean up the + // extraction directory then; every failure has to remove it here. + rmSync(tmpDir, { recursive: true, force: true }); + throw err; + } } // --------------------------------------------------------------------------- diff --git a/tests/helpers/binary-upgrade-archive.test.ts b/tests/helpers/binary-upgrade-archive.test.ts index 85b137b7..cfa6f482 100644 --- a/tests/helpers/binary-upgrade-archive.test.ts +++ b/tests/helpers/binary-upgrade-archive.test.ts @@ -9,7 +9,8 @@ import { beforeEach, afterEach, } from "bun:test"; -import { rmSync } from "node:fs"; +import { readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import { dirname } from "node:path"; import { @@ -105,6 +106,42 @@ 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; +} + +/** + * Replace `Bun.spawn` with a stub reporting `exitCode` and extracting nothing. + * + * @returns The argv of each spawn, populated as calls arrive. + */ +function stubSpawn(exitCode: number): string[][] { + const calls: string[][] = []; + const spawnSpy = spyOn(Bun, "spawn"); + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + spawnSpy.mockImplementation(((argv: string[]) => { + calls.push(argv); + return { stdout: "", stderr: "", exited: Promise.resolve(exitCode) }; + }) as unknown as typeof Bun.spawn); + return calls; +} + describe("downloadReleaseBinary archive handling", () => { let originalFetch: typeof fetch; @@ -118,10 +155,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 +173,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,6 +182,25 @@ 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 () => { @@ -166,7 +221,7 @@ describe("downloadReleaseBinary archive handling", () => { // and `tar -xzf` is what rejects it. mockArchiveDownload(new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8])); - const message = await rejectionMessage( + const message = await rejectionWithoutLeak( downloadReleaseBinary("v1.0.0", TAR_ARTIFACT) ); @@ -175,25 +230,42 @@ describe("downloadReleaseBinary archive handling", () => { 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 From 14ad0082ad9fb8e6cf68b7a105065ebb8e8c9381 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Thu, 6 Aug 2026 22:55:34 +0200 Subject: [PATCH 2/6] docs(memory): record probe-fixture and platform-skip verification traps Signed-off-by: Rhuan Barreto --- .claude/agent-memory/archgate-developer/MEMORY.md | 3 ++- .../archgate-developer/feedback_verify_agent_claims.md | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 81c8539d..6e5d2831 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -25,13 +25,14 @@ 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; a bad regex assertion survived a green local run this way. WSL Ubuntu has GNU tar and takes bun 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 directly (`node_modules` is portable enough for test runs). Fire-test there too — a skipped fire-test proves nothing. - **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..dd040ff6 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,9 @@ 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 carries no more authority than an external report — the analysis behind it was never executed, only reasoned. Reproduce the claim, then separately test the premise the fix rests on: a report can name a real defect while its stated mechanism is wrong, and a fix built on the stated mechanism then removes working code. 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. From d46bf3a79a3e6f0404b26ef3731731cfb5562249 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Thu, 6 Aug 2026 23:08:06 +0200 Subject: [PATCH 3/6] fix(upgrade): fail on an unreadable archive listing and drain both pipes An empty listing from a failed `tar -tzf` read as "no unsafe entries" and waved the archive past the path-traversal guard; a non-zero listing exit now aborts before extraction. Subprocess output goes through one helper that drains stdout and stderr concurrently with the exit code, the pattern ARCH-007 prescribes. The tar extraction-failure branch is now covered on every runner rather than only where `tar -tzf` can read a Windows path. Signed-off-by: Rhuan Barreto --- .../feedback_verify_agent_claims.md | 4 +- src/helpers/binary-upgrade.ts | 64 ++++++++++++------- tests/helpers/binary-upgrade-archive.test.ts | 50 ++++++++++++--- 3 files changed, 85 insertions(+), 33 deletions(-) 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 dd040ff6..2882288c 100644 --- a/.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md +++ b/.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md @@ -22,7 +22,9 @@ Four failure modes worth testing for by hand: **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 carries no more authority than an external report — the analysis behind it was never executed, only reasoned. Reproduce the claim, then separately test the premise the fix rests on: a report can name a real defect while its stated mechanism is wrong, and a fix built on the stated mechanism then removes working code. +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/helpers/binary-upgrade.ts b/src/helpers/binary-upgrade.ts index a672d9cf..b742c168 100644 --- a/src/helpers/binary-upgrade.ts +++ b/src/helpers/binary-upgrade.ts @@ -121,6 +121,24 @@ 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 }; +} + /** * Download and extract the release binary to a temp directory. * Returns the path to the extracted binary. @@ -236,14 +254,16 @@ export async function downloadReleaseBinary( // 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 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 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})` + ); + } + + for (const entry of list.stdout.split("\n").filter(Boolean)) { const normalized = entry.replaceAll("\\", "/").trim(); if ( normalized.startsWith("/") || @@ -256,11 +276,13 @@ export async function downloadReleaseBinary( } } - const proc = Bun.spawn(["tar", "-xzf", archivePath, "-C", tmpDir], { - stdout: "pipe", - stderr: "pipe", - }); - const exitCode = await proc.exited; + const { exitCode } = await runCapture([ + "tar", + "-xzf", + archivePath, + "-C", + tmpDir, + ]); if (exitCode !== 0) { throw new UserError( `Failed to extract archive (tar exit code ${exitCode})` @@ -270,16 +292,12 @@ export async function downloadReleaseBinary( // `-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 proc = Bun.spawn( - [ - "powershell", - "-NoProfile", - "-Command", - `Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpDir}' -Force -ErrorAction Stop`, - ], - { stdout: "pipe", stderr: "pipe" } - ); - const exitCode = await proc.exited; + 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})` diff --git a/tests/helpers/binary-upgrade-archive.test.ts b/tests/helpers/binary-upgrade-archive.test.ts index cfa6f482..2a96d07c 100644 --- a/tests/helpers/binary-upgrade-archive.test.ts +++ b/tests/helpers/binary-upgrade-archive.test.ts @@ -126,18 +126,33 @@ async function rejectionWithoutLeak(run: Promise): Promise { return message; } +interface SpawnReply { + exitCode: number; + stdout?: string; +} + /** - * Replace `Bun.spawn` with a stub reporting `exitCode` and extracting nothing. + * 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(exitCode: number): string[][] { +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); - return { stdout: "", stderr: "", exited: Promise.resolve(exitCode) }; + const { exitCode, stdout }: SpawnReply = respond(argv); + return { + stdout: stdout ?? "", + stderr: "", + exited: Promise.resolve(exitCode), + }; }) as unknown as typeof Bun.spawn); return calls; } @@ -216,16 +231,33 @@ describe("downloadReleaseBinary archive handling", () => { } ); + 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 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 () => { @@ -283,12 +315,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"); }); }); From 917e6f3b8a4970860d973aed5186850a5b5ff13f Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 7 Aug 2026 06:33:22 +0200 Subject: [PATCH 4/6] fix(upgrade): return the extraction directory instead of deriving it Deriving the directory to remove from `dirname(binaryPath)` deletes whichever directory the binary happens to sit in. downloadReleaseBinary now returns that directory alongside the binary, so the caller removes the one actually created rather than inferring it. The success-path cleanup gains an assertion against a real directory, which is what the mocked string path left uncovered. Signed-off-by: Rhuan Barreto --- src/commands/upgrade.ts | 13 ++++++------ src/helpers/binary-upgrade.ts | 22 +++++++++++++++----- tests/commands/upgrade-action.test.ts | 14 ++++++++++++- tests/commands/upgrade-dispatch.test.ts | 5 ++++- tests/helpers/binary-upgrade-archive.test.ts | 10 +++++---- tests/helpers/binary-upgrade.test.ts | 2 +- 6 files changed, 47 insertions(+), 19 deletions(-) diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 6f77cb6f..f32f6b86 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -278,23 +278,22 @@ async function upgradeBinary(tag: string): Promise { logDebug("Artifact:", artifact.name, "ext:", artifact.ext); const hint = getManualInstallHint(); - // downloadReleaseBinary hands back a path inside an extraction directory it - // cannot remove itself, so removing it once the binary is installed is the - // caller's job. + // 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 { try { const onProgress = createDownloadProgress(); - const newBinaryPath = await downloadReleaseBinary( + const { binaryPath, tmpDir } = await downloadReleaseBinary( tag, artifact, onProgress ); - extractDir = dirname(newBinaryPath); + extractDir = tmpDir; finishDownloadProgress(); - logDebug("Downloaded binary to:", newBinaryPath); + logDebug("Downloaded binary to:", binaryPath); logDebug("Replacing binary:", process.execPath); - replaceBinary(process.execPath, newBinaryPath); + replaceBinary(process.execPath, binaryPath); } finally { // Runs before the handler below, which ends the process via exitWith(). if (extractDir !== undefined) { diff --git a/src/helpers/binary-upgrade.ts b/src/helpers/binary-upgrade.ts index b742c168..00cc7138 100644 --- a/src/helpers/binary-upgrade.ts +++ b/src/helpers/binary-upgrade.ts @@ -139,19 +139,31 @@ async function runCapture( 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`; @@ -312,10 +324,10 @@ export async function downloadReleaseBinary( ); } - return binaryPath; + return { binaryPath, tmpDir }; } catch (err) { - // The caller only receives a path on success, so it can only clean up the - // extraction directory then; every failure has to remove it here. + // 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 2a96d07c..68cc9a1d 100644 --- a/tests/helpers/binary-upgrade-archive.test.ts +++ b/tests/helpers/binary-upgrade-archive.test.ts @@ -11,7 +11,6 @@ import { } from "bun:test"; import { readdirSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname } from "node:path"; import { type ArtifactInfo, @@ -221,12 +220,15 @@ describe("downloadReleaseBinary archive handling", () => { 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 }); } } ); diff --git a/tests/helpers/binary-upgrade.test.ts b/tests/helpers/binary-upgrade.test.ts index e7b11968..0a0dcbb5 100644 --- a/tests/helpers/binary-upgrade.test.ts +++ b/tests/helpers/binary-upgrade.test.ts @@ -381,7 +381,7 @@ describe("downloadReleaseBinary", () => { binaryName: "archgate.exe", }; try { - const binaryPath = await downloadReleaseBinary("v1.0.0", artifact); + const { binaryPath } = await downloadReleaseBinary("v1.0.0", artifact); expect(binaryPath).toContain("archgate.exe"); expect(existsSync(binaryPath)).toBe(true); } finally { From 63f9d8ce91e2768d4425fc5adfdada3b5f6ed229 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 7 Aug 2026 06:40:20 +0200 Subject: [PATCH 5/6] docs(memory): run WSL against every test file a change touches Signed-off-by: Rhuan Barreto --- .claude/agent-memory/archgate-developer/MEMORY.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 6e5d2831..195c9959 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -32,7 +32,8 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code - **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; a bad regex assertion survived a green local run this way. WSL Ubuntu has GNU tar and takes bun 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 directly (`node_modules` is portable enough for test runs). Fire-test there too — a skipped fire-test proves nothing. +- **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`. From 41602f9f0d3455282796e52d9d47aa558890dd44 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 7 Aug 2026 08:24:09 +0200 Subject: [PATCH 6/6] fix(upgrade): reject a binary that extracts as a symlink or directory existsSync follows symlinks and accepts directories, so an archive member that is either passed the post-extraction check. replaceBinary renames without following, which would leave ~/.archgate/bin/ holding a symlink aimed wherever the archive chose. lstat rejects both. The zip extraction test removes the directory the download created, which is not the fixture's own tmpDir the surrounding finally already handles. Signed-off-by: Rhuan Barreto --- src/helpers/binary-upgrade.ts | 14 ++++++-- tests/helpers/binary-upgrade-archive.test.ts | 34 ++++++++++++++++++-- tests/helpers/binary-upgrade.test.ts | 12 +++++-- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/src/helpers/binary-upgrade.ts b/src/helpers/binary-upgrade.ts index 00cc7138..4b76a625 100644 --- a/src/helpers/binary-upgrade.ts +++ b/src/helpers/binary-upgrade.ts @@ -3,7 +3,7 @@ import { createHash } from "node:crypto"; import { chmodSync, - existsSync, + lstatSync, mkdtempSync, renameSync, rmSync, @@ -318,11 +318,21 @@ export async function downloadReleaseBinary( } const binaryPath = join(tmpDir, artifact.binaryName); - if (!existsSync(binaryPath)) { + // 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( `Extraction produced no ${artifact.binaryName} — the downloaded archive is corrupt or incomplete` ); } + if (!stats.isFile()) { + throw new UserError( + `Extraction produced ${artifact.binaryName} as a ${stats.isSymbolicLink() ? "symbolic link" : "non-regular file"} — refusing to install it` + ); + } return { binaryPath, tmpDir }; } catch (err) { diff --git a/tests/helpers/binary-upgrade-archive.test.ts b/tests/helpers/binary-upgrade-archive.test.ts index 68cc9a1d..4c919536 100644 --- a/tests/helpers/binary-upgrade-archive.test.ts +++ b/tests/helpers/binary-upgrade-archive.test.ts @@ -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. @@ -233,6 +244,23 @@ describe("downloadReleaseBinary archive handling", () => { } ); + // 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. diff --git a/tests/helpers/binary-upgrade.test.ts b/tests/helpers/binary-upgrade.test.ts index 0a0dcbb5..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 {