From e93b1cce6a5e30c169cef693596935abfabda4eb Mon Sep 17 00:00:00 2001 From: Naved Date: Wed, 29 Jul 2026 21:15:01 -0700 Subject: [PATCH 01/10] refactor: extract managed binary installation infrastructure Refs #1055 --- src/eslint-suppressions.json | 5 - .../__tests__/semble-downloader.spec.ts | 42 +- .../code-index/semble/semble-downloader.ts | 412 +++--------------- .../managed-binary/__tests__/archive.spec.ts | 93 ++++ .../managed-binary/__tests__/download.spec.ts | 161 +++++++ .../managed-binary/__tests__/install.spec.ts | 88 ++++ src/services/managed-binary/archive.ts | 105 +++++ src/services/managed-binary/download.ts | 149 +++++++ src/services/managed-binary/install.ts | 131 ++++++ 9 files changed, 804 insertions(+), 382 deletions(-) create mode 100644 src/services/managed-binary/__tests__/archive.spec.ts create mode 100644 src/services/managed-binary/__tests__/download.spec.ts create mode 100644 src/services/managed-binary/__tests__/install.spec.ts create mode 100644 src/services/managed-binary/archive.ts create mode 100644 src/services/managed-binary/download.ts create mode 100644 src/services/managed-binary/install.ts diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 608e190d04..4141e6a259 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1499,11 +1499,6 @@ "count": 3 } }, - "services/code-index/semble/semble-downloader.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 1 - } - }, "services/code-index/shared/__tests__/validation-helpers.spec.ts": { "@typescript-eslint/no-explicit-any": { "count": 4 diff --git a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts index 7a3eee9a70..8261bfd307 100644 --- a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts +++ b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts @@ -263,12 +263,14 @@ describe("semble-downloader", () => { ) // Version file should be written expect(fs.writeFile).toHaveBeenCalledWith( - path.join("/storage", "semble", ".semble-version"), + path.join("/storage", "semble.new", ".semble-version"), "v0.4.1", "utf-8", ) // Archive should be cleaned up (version-prefixed local cache path) - expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "v0.4.1-semble-linux-x64-fast.tar.gz")) + expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.1-semble-linux-x64-fast.tar.gz"), { + force: true, + }) } finally { if (originalPlatform) Object.defineProperty(process, "platform", originalPlatform) if (originalArch) Object.defineProperty(process, "arch", originalArch) @@ -325,7 +327,9 @@ describe("semble-downloader", () => { try { await expect(downloadSemble("/storage")).rejects.toThrow("Failed to download semble") - expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "v0.4.1-semble-linux-arm64-fast.tar.gz")) + expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.1-semble-linux-arm64-fast.tar.gz"), { + force: true, + }) // Should clean up staging directory, not the original expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "semble.new"), { recursive: true, @@ -589,7 +593,7 @@ describe("semble-downloader", () => { }) // Archive cleanup fails but should not throw (only archive removal after extraction) - ;(fs.unlink as any).mockRejectedValue(new Error("unlink cleanup failed")) + ;(fs.rm as any).mockRejectedValueOnce(new Error("archive cleanup failed")) try { const result = await downloadSemble("/storage") @@ -641,7 +645,7 @@ describe("semble-downloader", () => { expect(https.get).toHaveBeenCalledWith(expect.stringContaining("v0.4.1"), expect.any(Function)) // Should write the new version file expect(fs.writeFile).toHaveBeenCalledWith( - path.join("/storage", "semble", ".semble-version"), + path.join("/storage", "semble.new", ".semble-version"), "v0.4.1", "utf-8", ) @@ -700,19 +704,23 @@ describe("semble-downloader", () => { ) // The stale archive is removed before the fresh download to guarantee // a clean package is verified against the new checksum. - expect(fs.unlink).toHaveBeenCalledWith(versionedArchive) + expect(fs.rm).toHaveBeenCalledWith(versionedArchive, { force: true }) // The prior-version archive (v0.4.0-*) is swept by cleanupStaleArchives // after a successful install, so a version upgrade doesn't accumulate // orphaned packages on disk. - expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz")) + expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz"), { + force: true, + }) // The legacy unversioned archive (pre-v0.4.0 cache layout) is also // swept, covering the v0.3.1 → v0.4.1 upgrade path. - expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz")) + expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz"), { + force: true, + }) // Unrelated files in the storage dir must not be touched. expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt")) // The new version file is recorded expect(fs.writeFile).toHaveBeenCalledWith( - path.join("/storage", "semble", ".semble-version"), + path.join("/storage", "semble.new", ".semble-version"), "v0.4.1", "utf-8", ) @@ -789,7 +797,7 @@ describe("semble-downloader", () => { ) // Should write version file again expect(fs.writeFile).toHaveBeenCalledWith( - path.join("/storage", "semble", ".semble-version"), + path.join("/storage", "semble.new", ".semble-version"), "v0.4.1", "utf-8", ) @@ -831,7 +839,7 @@ describe("semble-downloader", () => { ) // Should write version file expect(fs.writeFile).toHaveBeenCalledWith( - path.join("/storage", "semble", ".semble-version"), + path.join("/storage", "semble.new", ".semble-version"), "v0.4.1", "utf-8", ) @@ -903,8 +911,12 @@ describe("semble-downloader", () => { const currentArchive = path.join("/storage", "v0.4.1-semble-linux-x64-fast.tar.gz") // Stale versioned + legacy unversioned archives are swept - expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz")) - expect(fs.unlink).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz")) + expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "v0.4.0-semble-linux-x64-fast.tar.gz"), { + force: true, + }) + expect(fs.rm).toHaveBeenCalledWith(path.join("/storage", "semble-linux-x64-fast.tar.gz"), { + force: true, + }) // The current archive is never swept by cleanupStaleArchives (it is // excluded by the currentArchivePath guard). It is unlinked only by // the pre-download partial-archive cleanup and the post-install @@ -913,8 +925,8 @@ describe("semble-downloader", () => { // Sanity: the current archive path is never passed to the stale sweep. // It is unlinked exactly twice (pre-download cleanup + post-install // archive cleanup), never via cleanupStaleArchives. - const currentUnlinks = (fs.unlink as any).mock.calls.filter((c: any[]) => c[0] === currentArchive) - expect(currentUnlinks.length).toBe(2) + const currentRemovals = (fs.rm as any).mock.calls.filter((c: any[]) => c[0] === currentArchive) + expect(currentRemovals.length).toBe(2) } finally { if (originalPlatform) Object.defineProperty(process, "platform", originalPlatform) if (originalArch) Object.defineProperty(process, "arch", originalArch) diff --git a/src/services/code-index/semble/semble-downloader.ts b/src/services/code-index/semble/semble-downloader.ts index fc8a8e2a33..836e1946e1 100644 --- a/src/services/code-index/semble/semble-downloader.ts +++ b/src/services/code-index/semble/semble-downloader.ts @@ -1,10 +1,9 @@ import * as fs from "fs/promises" import * as path from "path" -import * as https from "https" -import { createWriteStream } from "fs" -import { createHash } from "crypto" -import { createReadStream } from "fs" -import { spawn } from "child_process" + +import { extractTarGzArchive, extractZipArchive } from "../../managed-binary/archive" +import { downloadBinaryFile, verifySha256Checksum } from "../../managed-binary/download" +import { ensureManagedBinaryInstalled, getManagedBinaryPaths } from "../../managed-binary/install" /** * Supported platform/arch combinations for the semble standalone executable. @@ -47,19 +46,14 @@ export const SEMBLE_SHA256: Record = { * Throws if the checksum does not match. */ export async function verifyChecksum(filePath: string, expected: string): Promise { - const hash = createHash("sha256") - await new Promise((resolve, reject) => { - const stream = createReadStream(filePath) - stream.on("data", (chunk) => hash.update(chunk)) - stream.on("end", resolve) - stream.on("error", reject) - }) - const actual = hash.digest("hex") - if (actual !== expected) { - throw new Error( - `Checksum mismatch for ${path.basename(filePath)}: expected ${expected.slice(0, 12)}…, got ${actual.slice(0, 12)}…`, - ) - } + await verifySha256Checksum( + filePath, + expected, + (actual) => + new Error( + `Checksum mismatch for ${path.basename(filePath)}: expected ${expected.slice(0, 12)}…, got ${actual.slice(0, 12)}…`, + ), + ) } /** @@ -87,64 +81,6 @@ function getArchiveInfo(platform?: string, arch?: string): { archive: string; bi return SEMBLE_ARCHIVES[`${p}-${a}`] } -/** - * Reads the locally installed version from the version metadata file. - * Returns undefined if no version file exists (first install or legacy). - */ -async function getInstalledVersion(storageDir: string): Promise { - try { - const versionPath = path.join(storageDir, "semble", VERSION_FILE) - const version = (await fs.readFile(versionPath, "utf-8")).trim() - return version || undefined - } catch { - return undefined - } -} - -/** - * Writes the version metadata file after a successful download. - */ -async function writeInstalledVersion(storageDir: string, version: string): Promise { - const versionPath = path.join(storageDir, "semble", VERSION_FILE) - await fs.writeFile(versionPath, version, "utf-8") -} - -/** - * Best-effort removal of archive files left over from previous semble versions. - * - * Because the local archive cache path is version-prefixed (see `downloadSemble`), - * upgrading SEMBLE_VERSION leaves the prior version's archive orphaned on disk. - * This sweeps those stale packages so a version upgrade doesn't accumulate them. - * - * Matches both the version-prefixed cache names (`${version}-${archiveName}`, - * used since v0.4.0) and the legacy unversioned cache name (`${archiveName}`, - * used before v0.4.0), so a v0.3.1 → v0.4.1 upgrade also clears the legacy file. - * The current archive path is always preserved. - * - * Errors are swallowed since this is purely cosmetic cleanup. - */ -async function cleanupStaleArchives( - storageDir: string, - archiveName: string, - currentArchivePath: string, -): Promise { - try { - const entries = await fs.readdir(storageDir) - const suffix = `-${archiveName}` - await Promise.all( - entries - .filter( - (name) => - (name === archiveName || name.endsWith(suffix)) && - path.join(storageDir, name) !== currentArchivePath, - ) - .map((name) => fs.unlink(path.join(storageDir, name)).catch(() => {})), - ) - } catch { - // ignore — storage dir may not be listable yet - } -} - /** * Downloads and extracts the semble archive for the current platform. * @@ -164,130 +100,48 @@ export async function downloadSemble(storageDir: string): Promise + downloadBinaryFile(url, archivePath, { + name: "Semble", + trustedDomains: TRUSTED_DOWNLOAD_DOMAINS, + timeoutMs: 120_000, + }), + verifyArchive: (archivePath) => verifyChecksum(archivePath, expectedChecksum), + extractArchive: async (archivePath, stagingDir) => { + if (info.archive.endsWith(".tar.gz")) { + await extractTarGzArchive(archivePath, stagingDir) + } else if (info.archive.endsWith(".zip")) { + await extractZipArchive(archivePath, stagingDir) + } + }, + }) + + console.log(`[SembleDownloader] Successfully installed semble ${SEMBLE_VERSION} to ${paths.binaryPath}`) + return result } /** @@ -309,174 +163,8 @@ export async function getSembleBinaryPath(storageDir: string): Promise { - return new Promise((resolve, reject) => { - const args = ["-xzf", archivePath, "-C", destDir, "--no-same-owner"] - // GNU tar: --no-overwrite-dir adds defense-in-depth against ../relative traversal. - // macOS bsdtar strips absolute paths by default. - if (process.platform === "linux") { - args.push("--no-overwrite-dir") - } - const child = spawn("tar", args, { - shell: false, - stdio: ["ignore", "pipe", "pipe"], - }) - - let stderr = "" - child.stderr?.on("data", (data: Buffer) => { - stderr += data.toString() - }) - - child.on("error", (err) => reject(err)) - child.on("close", (code) => { - if (code === 0) { - resolve() - } else { - reject(new Error(`tar extraction failed (code ${code}): ${stderr.trim()}`)) - } - }) - }) -} - -/** - * Escapes a string for use inside a PowerShell single-quoted literal. - * In PowerShell, the only special character in a single-quoted string is the - * apostrophe itself, which is escaped by doubling it. - */ -function escapePowerShellLiteral(value: string): string { - return value.replace(/'/g, "''") -} - -/** - * Extracts a .zip archive into the destination directory. - * Uses PowerShell on Windows, unzip on other platforms. - */ -function extractZip(archivePath: string, destDir: string): Promise { - return new Promise((resolve, reject) => { - let child - - if (process.platform === "win32") { - child = spawn( - "powershell", - [ - "-NoProfile", - "-Command", - `Expand-Archive -Path '${escapePowerShellLiteral(archivePath)}' -DestinationPath '${escapePowerShellLiteral(destDir)}' -Force`, - ], - { shell: false, stdio: ["ignore", "pipe", "pipe"] }, - ) - } else { - child = spawn("unzip", ["-o", archivePath, "-d", destDir], { - shell: false, - stdio: ["ignore", "pipe", "pipe"], - }) - } - - let stderr = "" - child.stderr?.on("data", (data: Buffer) => { - stderr += data.toString() - }) - - child.on("error", (err) => reject(err)) - child.on("close", (code) => { - if (code === 0) { - resolve() - } else { - reject(new Error(`zip extraction failed (code ${code}): ${stderr.trim()}`)) - } - }) - }) -} - /** * Trusted domains for following redirects during semble binary download. * GitHub releases redirect to objects.githubusercontent.com for the actual download. */ const TRUSTED_DOWNLOAD_DOMAINS = ["github.com", "objects.githubusercontent.com", "release-assets.githubusercontent.com"] - -/** - * Validates that a URL belongs to a trusted domain. - * Uses domain-boundary aware matching to prevent suffix-based bypasses - * (e.g. "evilgithub.com" does NOT match "github.com"). - */ -function isTrustedDownloadUrl(url: string): boolean { - try { - const parsed = new URL(url) - const h = parsed.hostname - return parsed.protocol === "https:" && TRUSTED_DOWNLOAD_DOMAINS.some((d) => h === d || h.endsWith("." + d)) - } catch { - return false - } -} - -/** - * Downloads a file from the given URL to the destination path. - * Follows redirects (GitHub releases use 302 redirects to CDN). - * Only follows redirects to trusted domains to prevent redirect-based attacks. - */ -function downloadFile(url: string, destPath: string, maxRedirects = 5): Promise { - return new Promise((resolve, reject) => { - if (maxRedirects <= 0) { - reject(new Error("Too many redirects")) - return - } - - const request = https.get(url, (response) => { - // Follow redirects - if ( - response.statusCode && - response.statusCode >= 300 && - response.statusCode < 400 && - response.headers.location - ) { - response.destroy() - const redirectUrl = response.headers.location - if (!isTrustedDownloadUrl(redirectUrl)) { - reject( - new Error( - `Redirect to untrusted domain blocked: ${redirectUrl}. Only ${TRUSTED_DOWNLOAD_DOMAINS.join(", ")} are allowed.`, - ), - ) - return - } - downloadFile(redirectUrl, destPath, maxRedirects - 1) - .then(resolve) - .catch(reject) - return - } - - if (response.statusCode !== 200) { - response.destroy() - reject(new Error(`HTTP ${response.statusCode}: Failed to download ${url}`)) - return - } - - const file = createWriteStream(destPath) - response.pipe(file) - - file.on("finish", () => { - file.close() - resolve() - }) - - file.on("error", (err) => { - file.close() - reject(err) - }) - }) - - request.on("error", reject) - request.on("timeout", () => { - request.destroy() - reject(new Error("Download timed out")) - }) - - // 2 minute timeout for download - request.setTimeout(120_000) - }) -} diff --git a/src/services/managed-binary/__tests__/archive.spec.ts b/src/services/managed-binary/__tests__/archive.spec.ts new file mode 100644 index 0000000000..b100c5adc4 --- /dev/null +++ b/src/services/managed-binary/__tests__/archive.spec.ts @@ -0,0 +1,93 @@ +import { EventEmitter } from "events" +import { PassThrough } from "stream" + +import { spawn } from "child_process" + +import { + escapePowerShellLiteral, + extractSingleFileTarXzArchive, + extractSingleFileZipArchive, + extractTarGzArchive, + runProcess, +} from "../archive" + +vi.mock("child_process", () => ({ spawn: vi.fn() })) + +const mockSpawn = vi.mocked(spawn) + +function createChild() { + return Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) +} + +describe("managed binary archive utilities", () => { + beforeEach(() => mockSpawn.mockReset()) + + it("runs processes without a shell and returns their output", async () => { + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const processResult = runProcess("tool", ["--version"]) + child.stdout.write("1.2.3") + child.emit("close", 0) + + await expect(processResult).resolves.toEqual({ stdout: "1.2.3", stderr: "" }) + expect(mockSpawn).toHaveBeenCalledWith("tool", ["--version"], { + shell: false, + stdio: ["ignore", "pipe", "pipe"], + }) + }) + + it("escapes PowerShell single-quoted literals", () => { + expect(escapePowerShellLiteral("C:\\it's\\archive.zip")).toBe("C:\\it''s\\archive.zip") + }) + + it("extracts tar.gz archives with hardened flags", async () => { + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const extraction = extractTarGzArchive("/tmp/archive.tar.gz", "/tmp/output") + child.emit("close", 0) + await extraction + + expect(mockSpawn).toHaveBeenCalledWith( + "tar", + expect.arrayContaining(["-xzf", "/tmp/archive.tar.gz", "-C", "/tmp/output", "--no-same-owner"]), + expect.objectContaining({ shell: false }), + ) + }) + + it("validates a single-file tar.xz layout before extraction", async () => { + const listing = createChild() + const extraction = createChild() + mockSpawn.mockReturnValueOnce(listing as unknown as ReturnType) + mockSpawn.mockReturnValueOnce(extraction as unknown as ReturnType) + const result = extractSingleFileTarXzArchive("/tmp/archive.tar.xz", "/tmp/output", "binary", "Tool") + listing.stdout.write("./binary\n") + listing.emit("close", 0) + await new Promise((resolve) => setImmediate(resolve)) + extraction.emit("close", 0) + await result + + expect(mockSpawn).toHaveBeenNthCalledWith( + 2, + "tar", + ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "binary"], + expect.any(Object), + ) + }) + + it("builds a single-entry-validated PowerShell ZIP extraction", async () => { + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const extraction = extractSingleFileZipArchive("C:\\archive.zip", "C:\\output", "binary.exe", "Tool") + child.emit("close", 0) + await extraction + + const script = mockSpawn.mock.calls[0][1][3] + expect(script).toContain("$entries.Count -ne 1") + expect(script).toContain("binary.exe") + expect(script).toContain("Tool archive has an unexpected layout") + }) +}) diff --git a/src/services/managed-binary/__tests__/download.spec.ts b/src/services/managed-binary/__tests__/download.spec.ts new file mode 100644 index 0000000000..4c4ea195e5 --- /dev/null +++ b/src/services/managed-binary/__tests__/download.spec.ts @@ -0,0 +1,161 @@ +import { EventEmitter } from "events" +import { createReadStream, createWriteStream } from "fs" +import { get } from "https" +import type { IncomingMessage, RequestOptions } from "http" + +import { + assertSizeWithinLimit, + downloadBinaryFile, + isTrustedHttpsUrl, + resolveTrustedRedirect, + verifySha256Checksum, +} from "../download" + +vi.mock("crypto", () => ({ + createHash: vi.fn(() => ({ + update: vi.fn(), + digest: vi.fn(() => "actual-checksum"), + })), +})) + +vi.mock("fs", () => ({ + createReadStream: vi.fn(), + createWriteStream: vi.fn(), +})) + +vi.mock("https", () => ({ get: vi.fn() })) + +const trustedDomains = ["github.com", "objects.githubusercontent.com"] +const mockGet = vi.mocked(get) +const mockCreateReadStream = vi.mocked(createReadStream) +const mockCreateWriteStream = vi.mocked(createWriteStream) + +function createRequest(): EventEmitter & { setTimeout: ReturnType; destroy: ReturnType } { + return Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) +} + +function createResponse(statusCode: number, headers: Record = {}) { + return Object.assign(new EventEmitter(), { + statusCode, + headers, + destroy: vi.fn(), + pipe: vi.fn(), + }) +} + +describe("managed binary downloads", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("validates HTTPS URLs against hostname boundaries", () => { + expect(isTrustedHttpsUrl("https://github.com/release", trustedDomains)).toBe(true) + expect(isTrustedHttpsUrl("https://cdn.objects.githubusercontent.com/release", trustedDomains)).toBe(true) + expect(isTrustedHttpsUrl("http://github.com/release", trustedDomains)).toBe(false) + expect(isTrustedHttpsUrl("https://evilgithub.com/release", trustedDomains)).toBe(false) + expect(isTrustedHttpsUrl("not a URL", trustedDomains)).toBe(false) + }) + + it("resolves relative redirects and rejects unsafe or exhausted redirects", () => { + const options = { name: "Example", trustedDomains } + expect(resolveTrustedRedirect("https://github.com/release", "/asset", 5, options)).toBe( + "https://github.com/asset", + ) + expect(() => + resolveTrustedRedirect("https://github.com/release", "https://example.com/asset", 5, options), + ).toThrow("Example download redirected to an untrusted host") + expect(() => resolveTrustedRedirect("https://github.com/release", "/asset", 0, options)).toThrow( + "Too many Example download redirects", + ) + }) + + it("enforces configurable archive size limits", () => { + expect(() => assertSizeWithinLimit(10, 10, "Example")).not.toThrow() + expect(() => assertSizeWithinLimit(11, 10, "Example")).toThrow( + "Example archive exceeds the download size limit", + ) + }) + + it("reports the actual SHA-256 value through a caller-defined mismatch error", async () => { + const input = new EventEmitter() + mockCreateReadStream.mockReturnValue(input as ReturnType) + const verification = verifySha256Checksum( + "/tmp/archive", + "expected-checksum", + (actual) => new Error(`checksum mismatch: ${actual}`), + ) + input.emit("data", Buffer.from("archive")) + input.emit("end") + await expect(verification).rejects.toThrow("checksum mismatch: actual-checksum") + }) + + it("follows a trusted redirect and applies destination security options", async () => { + const requestOne = createRequest() + const requestTwo = createRequest() + const redirect = createResponse(302, { location: "/asset" }) + const success = createResponse(200, { "content-length": "7" }) + const output = Object.assign(new EventEmitter(), { close: vi.fn() }) + mockCreateWriteStream.mockReturnValue(output as unknown as ReturnType) + + mockGet + .mockImplementationOnce((_url, optionsOrCallback, optionalCallback) => { + const callback = + typeof optionsOrCallback === "function" + ? optionsOrCallback + : (optionalCallback as ((response: IncomingMessage) => void) | undefined) + setImmediate(() => callback?.(redirect as unknown as IncomingMessage)) + return requestOne as unknown as ReturnType + }) + .mockImplementationOnce((_url, optionsOrCallback, optionalCallback) => { + const callback = + typeof optionsOrCallback === "function" + ? optionsOrCallback + : (optionalCallback as ((response: IncomingMessage) => void) | undefined) + setImmediate(() => callback?.(success as unknown as IncomingMessage)) + return requestTwo as unknown as ReturnType + }) + + const download = downloadBinaryFile("https://github.com/release", "/tmp/archive", { + name: "Example", + trustedDomains, + timeoutMs: 1_000, + maxBytes: 10, + exclusiveDestination: true, + }) + await new Promise((resolve) => setImmediate(resolve)) + await new Promise((resolve) => setImmediate(resolve)) + output.emit("finish") + await download + + expect(mockGet).toHaveBeenNthCalledWith(2, "https://github.com/asset", expect.any(Function)) + expect(mockCreateWriteStream).toHaveBeenCalledWith("/tmp/archive", { flags: "wx", mode: 0o600 }) + expect(requestOne.setTimeout).toHaveBeenCalledWith(1_000, expect.any(Function)) + expect(requestTwo.setTimeout).toHaveBeenCalledWith(1_000, expect.any(Function)) + }) + + it("rejects an oversized declared response before opening the destination", async () => { + const request = createRequest() + const response = createResponse(200, { "content-length": "11" }) + mockGet.mockImplementation( + ( + _url: string | URL, + optionsOrCallback: RequestOptions | ((response: IncomingMessage) => void), + optionalCallback?: (response: IncomingMessage) => void, + ) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => callback?.(response as unknown as IncomingMessage)) + return request as unknown as ReturnType + }, + ) + + await expect( + downloadBinaryFile("https://github.com/release", "/tmp/archive", { + name: "Example", + trustedDomains, + timeoutMs: 1_000, + maxBytes: 10, + }), + ).rejects.toThrow("Example archive exceeds the download size limit") + expect(mockCreateWriteStream).not.toHaveBeenCalled() + }) +}) diff --git a/src/services/managed-binary/__tests__/install.spec.ts b/src/services/managed-binary/__tests__/install.spec.ts new file mode 100644 index 0000000000..0041706470 --- /dev/null +++ b/src/services/managed-binary/__tests__/install.spec.ts @@ -0,0 +1,88 @@ +import { access, chmod, mkdir, mkdtemp, readFile, rm, writeFile } from "fs/promises" +import { tmpdir } from "os" +import * as path from "path" + +import { ensureManagedBinaryInstalled, getManagedBinaryPaths, type ManagedBinaryInstallOptions } from "../install" + +describe("managed binary installation", () => { + let tempDir: string + + beforeEach(async () => { + tempDir = await mkdtemp(path.join(tmpdir(), "managed-binary-")) + }) + + afterEach(async () => { + await rm(tempDir, { recursive: true, force: true }) + }) + + function createOptions(overrides: Partial = {}): ManagedBinaryInstallOptions { + return { + storageDir: tempDir, + id: "example", + version: "v1.2.3", + versionFile: ".example-version", + archiveName: "example.tar.gz", + binaryName: "example", + download: vi.fn(), + verifyArchive: vi.fn(), + extractArchive: vi.fn(), + ...overrides, + } + } + + it("derives one consistent mutable installation layout", () => { + expect(getManagedBinaryPaths(createOptions())).toEqual({ + installRoot: path.join(tempDir, "example"), + binaryPath: path.join(tempDir, "example", "example"), + versionPath: path.join(tempDir, "example", ".example-version"), + stagingDir: path.join(tempDir, "example.new"), + stagedBinaryPath: path.join(tempDir, "example.new", "example"), + archivePath: path.join(tempDir, "v1.2.3-example.tar.gz"), + }) + }) + + it("reuses a current executable without invoking update callbacks", async () => { + const options = createOptions() + const paths = getManagedBinaryPaths(options) + await mkdir(paths.installRoot, { recursive: true }) + await writeFile(paths.binaryPath, "current") + await writeFile(paths.versionPath, options.version) + if (process.platform !== "win32") await chmod(paths.binaryPath, 0o600) + + await expect(ensureManagedBinaryInstalled(options)).resolves.toBe(paths.binaryPath) + expect(options.download).not.toHaveBeenCalled() + }) + + it("deduplicates concurrent installations", () => { + const options = createOptions({ download: () => new Promise(() => {}) }) + expect(ensureManagedBinaryInstalled(options)).toBe(ensureManagedBinaryInstalled(options)) + }) + + it("coordinates update, metadata promotion, and cleanup", async () => { + const calls: string[] = [] + const options = createOptions({ + download: async (archivePath) => { + calls.push("download") + await writeFile(archivePath, "archive") + }, + verifyArchive: async () => { + calls.push("verify") + }, + extractArchive: async (_archivePath, stagingDir) => { + calls.push("extract") + await writeFile(path.join(stagingDir, "example"), "binary") + }, + validateBinary: async () => { + calls.push("validate") + }, + }) + const paths = getManagedBinaryPaths(options) + + await expect(ensureManagedBinaryInstalled(options)).resolves.toBe(paths.binaryPath) + expect(calls).toEqual(["download", "verify", "extract", "validate"]) + expect(await readFile(paths.binaryPath, "utf8")).toBe("binary") + expect(await readFile(paths.versionPath, "utf8")).toBe(options.version) + await expect(access(paths.archivePath)).rejects.toThrow() + await expect(access(paths.stagingDir)).rejects.toThrow() + }) +}) diff --git a/src/services/managed-binary/archive.ts b/src/services/managed-binary/archive.ts new file mode 100644 index 0000000000..5c3f26a7fa --- /dev/null +++ b/src/services/managed-binary/archive.ts @@ -0,0 +1,105 @@ +import { spawn } from "child_process" +import * as path from "path" + +export interface ProcessResult { + stdout: string + stderr: string +} + +export function runProcess(executable: string, args: string[], timeoutMs = 30_000): Promise { + return new Promise((resolve, reject) => { + const child = spawn(executable, args, { shell: false, stdio: ["ignore", "pipe", "pipe"] }) + let stdout = "" + let stderr = "" + const timer = setTimeout(() => { + child.kill("SIGKILL") + reject(new Error(`${path.basename(executable)} timed out`)) + }, timeoutMs) + child.stdout?.on("data", (chunk: Buffer) => (stdout += chunk.toString())) + child.stderr?.on("data", (chunk: Buffer) => (stderr += chunk.toString())) + child.on("error", (error) => { + clearTimeout(timer) + reject(error) + }) + child.on("close", (code) => { + clearTimeout(timer) + if (code === 0) { + resolve({ stdout, stderr }) + } else { + reject(new Error(stderr.trim() || `Process exited with code ${code}`)) + } + }) + }) +} + +export function escapePowerShellLiteral(value: string): string { + return value.replace(/'/g, "''") +} + +export async function extractTarGzArchive(archivePath: string, destination: string): Promise { + const args = ["-xzf", archivePath, "-C", destination, "--no-same-owner"] + if (process.platform === "linux") { + args.push("--no-overwrite-dir") + } + await runProcess("tar", args) +} + +export async function extractTarXzArchive(archivePath: string, destination: string): Promise { + const args = ["-xJf", archivePath, "-C", destination, "--no-same-owner"] + if (process.platform === "linux") { + args.push("--no-overwrite-dir") + } + await runProcess("tar", args) +} + +export async function extractZipArchive(archivePath: string, destination: string): Promise { + if (process.platform === "win32") { + await runProcess("powershell", [ + "-NoProfile", + "-Command", + `Expand-Archive -Path '${escapePowerShellLiteral(archivePath)}' -DestinationPath '${escapePowerShellLiteral(destination)}' -Force`, + ]) + return + } + + await runProcess("unzip", ["-o", archivePath, "-d", destination]) +} + +export async function extractSingleFileZipArchive( + archivePath: string, + destination: string, + expectedFile: string, + archiveName: string, +): Promise { + const outputPath = path.join(destination, expectedFile) + const script = [ + "$ErrorActionPreference = 'Stop'", + "Add-Type -AssemblyName System.IO.Compression.FileSystem", + `$archive = [System.IO.Compression.ZipFile]::OpenRead('${escapePowerShellLiteral(archivePath)}')`, + "try {", + " $entries = @($archive.Entries | Where-Object { -not [string]::IsNullOrEmpty($_.Name) })", + ` if ($entries.Count -ne 1 -or $entries[0].FullName -ne '${escapePowerShellLiteral(expectedFile)}') { throw '${escapePowerShellLiteral(archiveName)} archive has an unexpected layout' }`, + ` [System.IO.Compression.ZipFileExtensions]::ExtractToFile($entries[0], '${escapePowerShellLiteral(outputPath)}', $false)`, + "} finally { $archive.Dispose() }", + ].join("; ") + + await runProcess("powershell", ["-NoProfile", "-NonInteractive", "-Command", script]) +} + +export async function extractSingleFileTarXzArchive( + archivePath: string, + destination: string, + expectedFile: string, + archiveName: string, +): Promise { + const listing = await runProcess("tar", ["-tJf", archivePath]) + const entries = listing.stdout + .split(/\r?\n/) + .map((entry) => entry.trim().replace(/^\.\//, "")) + .filter(Boolean) + if (entries.length !== 1 || entries[0] !== expectedFile) { + throw new Error(`${archiveName} archive has an unexpected layout`) + } + + await runProcess("tar", ["-xJf", archivePath, "-C", destination, expectedFile]) +} diff --git a/src/services/managed-binary/download.ts b/src/services/managed-binary/download.ts new file mode 100644 index 0000000000..c17eaed909 --- /dev/null +++ b/src/services/managed-binary/download.ts @@ -0,0 +1,149 @@ +import { createHash } from "crypto" +import { createReadStream, createWriteStream } from "fs" +import * as https from "https" + +export interface BinaryDownloadOptions { + name: string + trustedDomains: readonly string[] + timeoutMs: number + maxBytes?: number + maxRedirects?: number + exclusiveDestination?: boolean +} + +export function isTrustedHttpsUrl(url: string, trustedDomains: readonly string[]): boolean { + try { + const parsed = new URL(url) + return ( + parsed.protocol === "https:" && + trustedDomains.some((domain) => parsed.hostname === domain || parsed.hostname.endsWith(`.${domain}`)) + ) + } catch { + return false + } +} + +export function resolveTrustedRedirect( + url: string, + location: string | undefined, + redirectsRemaining: number, + options: Pick, +): string { + if (redirectsRemaining <= 0 || !location) { + throw new Error(`Too many ${options.name} download redirects`) + } + + const nextUrl = new URL(location, url).toString() + if (!isTrustedHttpsUrl(nextUrl, options.trustedDomains)) { + throw new Error(`${options.name} download redirected to an untrusted host (untrusted domain)`) + } + + return nextUrl +} + +export function assertSizeWithinLimit(size: number, maxBytes: number, name: string): void { + if (size > maxBytes) { + throw new Error(`${name} archive exceeds the download size limit`) + } +} + +export async function verifySha256Checksum( + filePath: string, + expected: string, + createMismatchError: (actual: string) => Error, +): Promise { + const hash = createHash("sha256") + await new Promise((resolve, reject) => { + const input = createReadStream(filePath) + input.on("data", (chunk) => hash.update(chunk)) + input.on("end", resolve) + input.on("error", reject) + }) + + const actual = hash.digest("hex") + if (actual !== expected) { + throw createMismatchError(actual) + } +} + +export function downloadBinaryFile(url: string, destination: string, options: BinaryDownloadOptions): Promise { + return downloadBinaryFileWithRedirects(url, destination, options, options.maxRedirects ?? 5) +} + +function downloadBinaryFileWithRedirects( + url: string, + destination: string, + options: BinaryDownloadOptions, + redirectsRemaining: number, +): Promise { + return new Promise((resolve, reject) => { + if (!isTrustedHttpsUrl(url, options.trustedDomains)) { + reject(new Error(`${options.name} download redirected to an untrusted host (untrusted domain)`)) + return + } + + const request = https.get(url, (response) => { + const status = response.statusCode ?? 0 + if ([301, 302, 303, 307, 308].includes(status)) { + response.destroy() + let nextUrl: string + try { + nextUrl = resolveTrustedRedirect(url, response.headers.location, redirectsRemaining, options) + } catch (error) { + reject(error) + return + } + downloadBinaryFileWithRedirects(nextUrl, destination, options, redirectsRemaining - 1).then( + resolve, + reject, + ) + return + } + + if (status !== 200) { + response.destroy() + reject(new Error(`${options.name} download failed with HTTP ${status}`)) + return + } + + const declaredSize = Number(response.headers["content-length"] ?? 0) + if (options.maxBytes !== undefined) { + try { + assertSizeWithinLimit(declaredSize, options.maxBytes, options.name) + } catch (error) { + response.destroy() + reject(error) + return + } + } + + let received = 0 + const output = createWriteStream( + destination, + options.exclusiveDestination ? { flags: "wx", mode: 0o600 } : undefined, + ) + response.on("data", (chunk: Buffer) => { + received += chunk.length + if (options.maxBytes !== undefined) { + try { + assertSizeWithinLimit(received, options.maxBytes, options.name) + } catch (error) { + response.destroy() + request.destroy(error as Error) + reject(error) + } + } + }) + response.on("error", reject) + response.pipe(output) + output.on("finish", () => { + output.close() + resolve() + }) + output.on("error", reject) + }) + + request.setTimeout(options.timeoutMs, () => request.destroy(new Error(`${options.name} download timed out`))) + request.on("error", reject) + }) +} diff --git a/src/services/managed-binary/install.ts b/src/services/managed-binary/install.ts new file mode 100644 index 0000000000..5257cfdf9f --- /dev/null +++ b/src/services/managed-binary/install.ts @@ -0,0 +1,131 @@ +import * as fs from "fs/promises" +import * as path from "path" + +const installationPromises = new Map>() + +export interface ManagedBinaryInstallOptions { + storageDir: string + id: string + version: string + versionFile: string + archiveName: string + binaryName: string + download: (archivePath: string) => Promise + verifyArchive: (archivePath: string) => Promise + extractArchive: (archivePath: string, stagingDir: string) => Promise + validateBinary?: (stagedBinaryPath: string) => Promise + errorPrefix?: string +} + +export interface ManagedBinaryPaths { + installRoot: string + binaryPath: string + versionPath: string + stagingDir: string + stagedBinaryPath: string + archivePath: string +} + +export function getManagedBinaryPaths( + options: Pick< + ManagedBinaryInstallOptions, + "storageDir" | "id" | "version" | "versionFile" | "archiveName" | "binaryName" + >, +): ManagedBinaryPaths { + const installRoot = path.join(options.storageDir, options.id) + const stagingDir = path.join(options.storageDir, `${options.id}.new`) + return { + installRoot, + binaryPath: path.join(installRoot, options.binaryName), + versionPath: path.join(installRoot, options.versionFile), + stagingDir, + stagedBinaryPath: path.join(stagingDir, options.binaryName), + archivePath: path.join(options.storageDir, `${options.version}-${options.archiveName}`), + } +} + +async function readInstalledVersion(versionPath: string): Promise { + try { + const version = (await fs.readFile(versionPath, "utf8")).trim() + return version || undefined + } catch { + return undefined + } +} + +async function makeExecutable(binaryPath: string): Promise { + await fs.access(binaryPath) + if (process.platform !== "win32") { + await fs.chmod(binaryPath, 0o755) + } +} + +async function cleanupStaleArchives(options: ManagedBinaryInstallOptions, currentArchivePath: string): Promise { + try { + const entries = await fs.readdir(options.storageDir) + const suffix = `-${options.archiveName}` + await Promise.all( + entries + .filter( + (name) => + (name === options.archiveName || name.endsWith(suffix)) && + path.join(options.storageDir, name) !== currentArchivePath, + ) + .map((name) => fs.rm(path.join(options.storageDir, name), { force: true }).catch(() => {})), + ) + } catch { + // Archive cleanup is cosmetic and must not invalidate a successful installation. + } +} + +async function installManagedBinary(options: ManagedBinaryInstallOptions): Promise { + const paths = getManagedBinaryPaths(options) + await fs.mkdir(options.storageDir, { recursive: true }) + const installedVersion = await readInstalledVersion(paths.versionPath) + if (installedVersion === options.version) { + try { + await makeExecutable(paths.binaryPath) + return paths.binaryPath + } catch { + // The installation is absent or incomplete, so rebuild it below. + } + } + + await fs.rm(paths.archivePath, { force: true }).catch(() => {}) + await fs.rm(paths.stagingDir, { recursive: true, force: true }).catch(() => {}) + await fs.mkdir(paths.stagingDir, { recursive: true }) + + try { + await options.download(paths.archivePath) + await options.verifyArchive(paths.archivePath) + await options.extractArchive(paths.archivePath, paths.stagingDir) + await makeExecutable(paths.stagedBinaryPath) + await options.validateBinary?.(paths.stagedBinaryPath) + await fs.writeFile(path.join(paths.stagingDir, options.versionFile), options.version, "utf-8") + await fs.rm(paths.installRoot, { recursive: true, force: true }) + await fs.rename(paths.stagingDir, paths.installRoot) + await cleanupStaleArchives(options, paths.archivePath) + return paths.binaryPath + } catch (error) { + if (!options.errorPrefix) { + throw error + } + const message = error instanceof Error ? error.message : String(error) + throw new Error(`${options.errorPrefix}: ${message}`) + } finally { + await fs.rm(paths.archivePath, { force: true }).catch(() => {}) + await fs.rm(paths.stagingDir, { recursive: true, force: true }).catch(() => {}) + } +} + +export function ensureManagedBinaryInstalled(options: ManagedBinaryInstallOptions): Promise { + const key = path.join(options.storageDir, options.id) + const existing = installationPromises.get(key) + if (existing) { + return existing + } + + const installation = installManagedBinary(options).finally(() => installationPromises.delete(key)) + installationPromises.set(key, installation) + return installation +} From 24d527e885c7e9be24e70159f5e1dc7aa82253dc Mon Sep 17 00:00:00 2001 From: Naved Date: Sat, 1 Aug 2026 10:15:04 -0700 Subject: [PATCH 02/10] fix: address managed binary review feedback --- .../__tests__/semble-downloader.spec.ts | 6 +- .../code-index/semble/semble-downloader.ts | 12 +--- .../managed-binary/__tests__/archive.spec.ts | 63 ++++++++++++++++--- .../managed-binary/__tests__/download.spec.ts | 42 +++++++++++++ .../managed-binary/__tests__/install.spec.ts | 56 ++++++++++++++++- src/services/managed-binary/archive.ts | 38 +++++++---- src/services/managed-binary/download.ts | 11 +++- src/services/managed-binary/install.ts | 26 ++++++-- 8 files changed, 211 insertions(+), 43 deletions(-) diff --git a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts index 8261bfd307..7ef0df9184 100644 --- a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts +++ b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts @@ -32,6 +32,10 @@ vi.mock("fs/promises", () => ({ readdir: vi.fn().mockResolvedValue([]), })) +vi.mock("proper-lockfile", () => ({ + lock: vi.fn().mockResolvedValue(vi.fn().mockResolvedValue(undefined)), +})) + // Mock fs (createWriteStream and createReadStream for checksum verification) const mockWriteStream = { on: vi.fn(), @@ -717,7 +721,7 @@ describe("semble-downloader", () => { force: true, }) // Unrelated files in the storage dir must not be touched. - expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt")) + expect(fs.rm).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt"), expect.anything()) // The new version file is recorded expect(fs.writeFile).toHaveBeenCalledWith( path.join("/storage", "semble.new", ".semble-version"), diff --git a/src/services/code-index/semble/semble-downloader.ts b/src/services/code-index/semble/semble-downloader.ts index 836e1946e1..d0697298f5 100644 --- a/src/services/code-index/semble/semble-downloader.ts +++ b/src/services/code-index/semble/semble-downloader.ts @@ -3,7 +3,7 @@ import * as path from "path" import { extractTarGzArchive, extractZipArchive } from "../../managed-binary/archive" import { downloadBinaryFile, verifySha256Checksum } from "../../managed-binary/download" -import { ensureManagedBinaryInstalled, getManagedBinaryPaths } from "../../managed-binary/install" +import { ensureManagedBinaryInstalled } from "../../managed-binary/install" /** * Supported platform/arch combinations for the semble standalone executable. @@ -101,14 +101,6 @@ export async function downloadSemble(storageDir: string): Promise { }) }) - it("escapes PowerShell single-quoted literals", () => { - expect(escapePowerShellLiteral("C:\\it's\\archive.zip")).toBe("C:\\it''s\\archive.zip") + it("kills a process that exceeds its timeout", async () => { + vi.useFakeTimers() + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const processResult = runProcess("tool", [], 100) + const assertion = expect(processResult).rejects.toThrow("tool timed out") + + await vi.advanceTimersByTimeAsync(100) + await assertion + expect(child.kill).toHaveBeenCalledWith("SIGKILL") + vi.useRealTimers() }) it("extracts tar.gz archives with hardened flags", async () => { @@ -58,6 +69,43 @@ describe("managed binary archive utilities", () => { ) }) + it("extracts tar.xz archives with hardened flags", async () => { + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const extraction = extractTarXzArchive("/tmp/archive.tar.xz", "/tmp/output") + child.emit("close", 0) + await extraction + + const expectedArgs = ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "--no-same-owner"] + if (process.platform === "linux") expectedArgs.push("--no-overwrite-dir") + expect(mockSpawn).toHaveBeenCalledWith("tar", expectedArgs, { + shell: false, + stdio: ["ignore", "pipe", "pipe"], + }) + }) + + it("extracts ZIP archives with platform-safe process arguments", async () => { + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const extraction = extractZipArchive("/tmp/archive.zip", "/tmp/output") + child.emit("close", 0) + await extraction + + if (process.platform === "win32") { + expect(mockSpawn).toHaveBeenCalledWith( + "powershell", + ["-NoProfile", "-NonInteractive", "-Command", expect.any(String), "/tmp/archive.zip", "/tmp/output"], + expect.objectContaining({ shell: false }), + ) + } else { + expect(mockSpawn).toHaveBeenCalledWith( + "unzip", + ["-o", "/tmp/archive.zip", "-d", "/tmp/output"], + expect.objectContaining({ shell: false }), + ) + } + }) + it("validates a single-file tar.xz layout before extraction", async () => { const listing = createChild() const extraction = createChild() @@ -73,7 +121,7 @@ describe("managed binary archive utilities", () => { expect(mockSpawn).toHaveBeenNthCalledWith( 2, "tar", - ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "binary"], + ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "./binary"], expect.any(Object), ) }) @@ -85,9 +133,10 @@ describe("managed binary archive utilities", () => { child.emit("close", 0) await extraction - const script = mockSpawn.mock.calls[0][1][3] + const args = mockSpawn.mock.calls[0][1] + const script = args[3] expect(script).toContain("$entries.Count -ne 1") - expect(script).toContain("binary.exe") - expect(script).toContain("Tool archive has an unexpected layout") + expect(script).not.toContain("C:\\archive.zip") + expect(args.slice(4)).toEqual(["C:\\archive.zip", path.join("C:\\output", "binary.exe"), "binary.exe", "Tool"]) }) }) diff --git a/src/services/managed-binary/__tests__/download.spec.ts b/src/services/managed-binary/__tests__/download.spec.ts index 4c4ea195e5..2fa4fc353a 100644 --- a/src/services/managed-binary/__tests__/download.spec.ts +++ b/src/services/managed-binary/__tests__/download.spec.ts @@ -40,6 +40,7 @@ function createResponse(statusCode: number, headers: Record = {} headers, destroy: vi.fn(), pipe: vi.fn(), + unpipe: vi.fn(), }) } @@ -67,6 +68,20 @@ describe("managed binary downloads", () => { expect(() => resolveTrustedRedirect("https://github.com/release", "/asset", 0, options)).toThrow( "Too many Example download redirects", ) + expect(() => resolveTrustedRedirect("https://github.com/release", undefined, 5, options)).toThrow( + "Example download redirect is missing a Location header", + ) + }) + + it("distinguishes an untrusted initial URL from an unsafe redirect", async () => { + await expect( + downloadBinaryFile("http://github.com/release", "/tmp/archive", { + name: "Example", + trustedDomains, + timeoutMs: 1_000, + }), + ).rejects.toThrow("Example download URL is not a trusted HTTPS host") + expect(mockGet).not.toHaveBeenCalled() }) it("enforces configurable archive size limits", () => { @@ -158,4 +173,31 @@ describe("managed binary downloads", () => { ).rejects.toThrow("Example archive exceeds the download size limit") expect(mockCreateWriteStream).not.toHaveBeenCalled() }) + + it("unpipes and destroys the destination when streamed bytes exceed the limit", async () => { + const request = createRequest() + const response = createResponse(200) + const output = Object.assign(new EventEmitter(), { close: vi.fn(), destroy: vi.fn() }) + mockCreateWriteStream.mockReturnValue(output as unknown as ReturnType) + mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => callback?.(response as unknown as IncomingMessage)) + return request as unknown as ReturnType + }) + + const download = downloadBinaryFile("https://github.com/release", "/tmp/archive", { + name: "Example", + trustedDomains, + timeoutMs: 1_000, + maxBytes: 10, + }) + await new Promise((resolve) => setImmediate(resolve)) + response.emit("data", Buffer.alloc(11)) + + await expect(download).rejects.toThrow("Example archive exceeds the download size limit") + expect(response.unpipe).toHaveBeenCalledWith(output) + expect(output.destroy).toHaveBeenCalledOnce() + expect(response.destroy).toHaveBeenCalledOnce() + expect(request.destroy).toHaveBeenCalledOnce() + }) }) diff --git a/src/services/managed-binary/__tests__/install.spec.ts b/src/services/managed-binary/__tests__/install.spec.ts index 0041706470..6650cc9718 100644 --- a/src/services/managed-binary/__tests__/install.spec.ts +++ b/src/services/managed-binary/__tests__/install.spec.ts @@ -23,6 +23,7 @@ describe("managed binary installation", () => { versionFile: ".example-version", archiveName: "example.tar.gz", binaryName: "example", + errorPrefix: "Failed to install example", download: vi.fn(), verifyArchive: vi.fn(), extractArchive: vi.fn(), @@ -53,9 +54,22 @@ describe("managed binary installation", () => { expect(options.download).not.toHaveBeenCalled() }) - it("deduplicates concurrent installations", () => { - const options = createOptions({ download: () => new Promise(() => {}) }) - expect(ensureManagedBinaryInstalled(options)).toBe(ensureManagedBinaryInstalled(options)) + it("deduplicates concurrent installations", async () => { + let finishDownload: (() => void) | undefined + const download = vi.fn(() => new Promise((resolve) => (finishDownload = resolve))) + const options = createOptions({ + download, + extractArchive: async (_archivePath, stagingDir) => { + await writeFile(path.join(stagingDir, "example"), "binary") + }, + }) + const first = ensureManagedBinaryInstalled(options) + const second = ensureManagedBinaryInstalled(options) + expect(first).toBe(second) + await vi.waitFor(() => expect(download).toHaveBeenCalledOnce()) + finishDownload?.() + await Promise.all([first, second]) + expect(download).toHaveBeenCalledOnce() }) it("coordinates update, metadata promotion, and cleanup", async () => { @@ -84,5 +98,41 @@ describe("managed binary installation", () => { expect(await readFile(paths.versionPath, "utf8")).toBe(options.version) await expect(access(paths.archivePath)).rejects.toThrow() await expect(access(paths.stagingDir)).rejects.toThrow() + await expect(access(path.join(tempDir, ".example.install.lock"))).rejects.toThrow() + }) + + it("cleans up partial artifacts when downloading fails", async () => { + const options = createOptions({ + download: async (archivePath) => { + await writeFile(archivePath, "partial") + throw new Error("network failure") + }, + }) + const paths = getManagedBinaryPaths(options) + + await expect(ensureManagedBinaryInstalled(options)).rejects.toThrow( + "Failed to install example: network failure", + ) + await expect(access(paths.archivePath)).rejects.toThrow() + await expect(access(paths.stagingDir)).rejects.toThrow() + await expect(access(paths.binaryPath)).rejects.toThrow() + }) + + it("removes stale versioned archives without touching unrelated files", async () => { + const options = createOptions({ + download: async (archivePath) => writeFile(archivePath, "archive"), + extractArchive: async (_archivePath, stagingDir) => { + await writeFile(path.join(stagingDir, "example"), "binary") + }, + }) + const staleArchive = path.join(tempDir, "v1.2.2-example.tar.gz") + const unrelated = path.join(tempDir, "notes.txt") + await writeFile(staleArchive, "stale") + await writeFile(unrelated, "keep") + + await ensureManagedBinaryInstalled(options) + + await expect(access(staleArchive)).rejects.toThrow() + await expect(readFile(unrelated, "utf8")).resolves.toBe("keep") }) }) diff --git a/src/services/managed-binary/archive.ts b/src/services/managed-binary/archive.ts index 5c3f26a7fa..868118c0f7 100644 --- a/src/services/managed-binary/archive.ts +++ b/src/services/managed-binary/archive.ts @@ -32,10 +32,6 @@ export function runProcess(executable: string, args: string[], timeoutMs = 30_00 }) } -export function escapePowerShellLiteral(value: string): string { - return value.replace(/'/g, "''") -} - export async function extractTarGzArchive(archivePath: string, destination: string): Promise { const args = ["-xzf", archivePath, "-C", destination, "--no-same-owner"] if (process.platform === "linux") { @@ -56,8 +52,11 @@ export async function extractZipArchive(archivePath: string, destination: string if (process.platform === "win32") { await runProcess("powershell", [ "-NoProfile", + "-NonInteractive", "-Command", - `Expand-Archive -Path '${escapePowerShellLiteral(archivePath)}' -DestinationPath '${escapePowerShellLiteral(destination)}' -Force`, + "$archivePath = $args[0]; $destination = $args[1]; Expand-Archive -LiteralPath $archivePath -DestinationPath $destination -Force", + archivePath, + destination, ]) return } @@ -71,19 +70,31 @@ export async function extractSingleFileZipArchive( expectedFile: string, archiveName: string, ): Promise { - const outputPath = path.join(destination, expectedFile) const script = [ "$ErrorActionPreference = 'Stop'", + "$archivePath = $args[0]", + "$outputPath = $args[1]", + "$expectedFile = $args[2]", + "$archiveName = $args[3]", "Add-Type -AssemblyName System.IO.Compression.FileSystem", - `$archive = [System.IO.Compression.ZipFile]::OpenRead('${escapePowerShellLiteral(archivePath)}')`, + "$archive = [System.IO.Compression.ZipFile]::OpenRead($archivePath)", "try {", " $entries = @($archive.Entries | Where-Object { -not [string]::IsNullOrEmpty($_.Name) })", - ` if ($entries.Count -ne 1 -or $entries[0].FullName -ne '${escapePowerShellLiteral(expectedFile)}') { throw '${escapePowerShellLiteral(archiveName)} archive has an unexpected layout' }`, - ` [System.IO.Compression.ZipFileExtensions]::ExtractToFile($entries[0], '${escapePowerShellLiteral(outputPath)}', $false)`, + ' if ($entries.Count -ne 1 -or $entries[0].FullName -ne $expectedFile) { throw "$archiveName archive has an unexpected layout" }', + " [System.IO.Compression.ZipFileExtensions]::ExtractToFile($entries[0], $outputPath, $false)", "} finally { $archive.Dispose() }", ].join("; ") - await runProcess("powershell", ["-NoProfile", "-NonInteractive", "-Command", script]) + await runProcess("powershell", [ + "-NoProfile", + "-NonInteractive", + "-Command", + script, + archivePath, + path.join(destination, expectedFile), + expectedFile, + archiveName, + ]) } export async function extractSingleFileTarXzArchive( @@ -95,11 +106,12 @@ export async function extractSingleFileTarXzArchive( const listing = await runProcess("tar", ["-tJf", archivePath]) const entries = listing.stdout .split(/\r?\n/) - .map((entry) => entry.trim().replace(/^\.\//, "")) + .map((entry) => entry.trim()) .filter(Boolean) - if (entries.length !== 1 || entries[0] !== expectedFile) { + const archiveEntry = entries[0] + if (entries.length !== 1 || archiveEntry.replace(/^\.\//, "") !== expectedFile) { throw new Error(`${archiveName} archive has an unexpected layout`) } - await runProcess("tar", ["-xJf", archivePath, "-C", destination, expectedFile]) + await runProcess("tar", ["-xJf", archivePath, "-C", destination, archiveEntry]) } diff --git a/src/services/managed-binary/download.ts b/src/services/managed-binary/download.ts index c17eaed909..567cfa91d3 100644 --- a/src/services/managed-binary/download.ts +++ b/src/services/managed-binary/download.ts @@ -29,9 +29,12 @@ export function resolveTrustedRedirect( redirectsRemaining: number, options: Pick, ): string { - if (redirectsRemaining <= 0 || !location) { + if (redirectsRemaining <= 0) { throw new Error(`Too many ${options.name} download redirects`) } + if (!location) { + throw new Error(`${options.name} download redirect is missing a Location header`) + } const nextUrl = new URL(location, url).toString() if (!isTrustedHttpsUrl(nextUrl, options.trustedDomains)) { @@ -78,7 +81,7 @@ function downloadBinaryFileWithRedirects( ): Promise { return new Promise((resolve, reject) => { if (!isTrustedHttpsUrl(url, options.trustedDomains)) { - reject(new Error(`${options.name} download redirected to an untrusted host (untrusted domain)`)) + reject(new Error(`${options.name} download URL is not a trusted HTTPS host (untrusted domain)`)) return } @@ -128,8 +131,10 @@ function downloadBinaryFileWithRedirects( try { assertSizeWithinLimit(received, options.maxBytes, options.name) } catch (error) { + response.unpipe(output) + output.destroy() response.destroy() - request.destroy(error as Error) + request.destroy() reject(error) } } diff --git a/src/services/managed-binary/install.ts b/src/services/managed-binary/install.ts index 5257cfdf9f..77143a58ea 100644 --- a/src/services/managed-binary/install.ts +++ b/src/services/managed-binary/install.ts @@ -1,5 +1,6 @@ import * as fs from "fs/promises" import * as path from "path" +import * as lockfile from "proper-lockfile" const installationPromises = new Map>() @@ -14,7 +15,7 @@ export interface ManagedBinaryInstallOptions { verifyArchive: (archivePath: string) => Promise extractArchive: (archivePath: string, stagingDir: string) => Promise validateBinary?: (stagedBinaryPath: string) => Promise - errorPrefix?: string + errorPrefix: string } export interface ManagedBinaryPaths { @@ -107,17 +108,30 @@ async function installManagedBinary(options: ManagedBinaryInstallOptions): Promi await cleanupStaleArchives(options, paths.archivePath) return paths.binaryPath } catch (error) { - if (!options.errorPrefix) { - throw error - } const message = error instanceof Error ? error.message : String(error) - throw new Error(`${options.errorPrefix}: ${message}`) + throw new Error(`${options.errorPrefix}: ${message}`, { cause: error }) } finally { await fs.rm(paths.archivePath, { force: true }).catch(() => {}) await fs.rm(paths.stagingDir, { recursive: true, force: true }).catch(() => {}) } } +async function installManagedBinaryWithLock(options: ManagedBinaryInstallOptions): Promise { + await fs.mkdir(options.storageDir, { recursive: true }) + const lockTarget = path.join(options.storageDir, `.${options.id}.install`) + const release = await lockfile.lock(lockTarget, { + realpath: false, + stale: 5 * 60_000, + update: 30_000, + retries: { retries: 10, factor: 1.5, minTimeout: 100, maxTimeout: 1_000 }, + }) + try { + return await installManagedBinary(options) + } finally { + await release() + } +} + export function ensureManagedBinaryInstalled(options: ManagedBinaryInstallOptions): Promise { const key = path.join(options.storageDir, options.id) const existing = installationPromises.get(key) @@ -125,7 +139,7 @@ export function ensureManagedBinaryInstalled(options: ManagedBinaryInstallOption return existing } - const installation = installManagedBinary(options).finally(() => installationPromises.delete(key)) + const installation = installManagedBinaryWithLock(options).finally(() => installationPromises.delete(key)) installationPromises.set(key, installation) return installation } From 346074876251364bb9cb761af1d4453b9617fb89 Mon Sep 17 00:00:00 2001 From: Naved Date: Wed, 29 Jul 2026 21:15:39 -0700 Subject: [PATCH 03/10] feat: add destructive command guard binary service Refs #1056 --- .../__tests__/manager.spec.ts | 249 ++++++++++++++++++ .../__tests__/runner.spec.ts | 126 +++++++++ .../destructive-command-guard/constants.ts | 41 +++ .../destructive-command-guard/manager.ts | 98 +++++++ .../destructive-command-guard/runner.ts | 84 ++++++ 5 files changed, 598 insertions(+) create mode 100644 src/services/destructive-command-guard/__tests__/manager.spec.ts create mode 100644 src/services/destructive-command-guard/__tests__/runner.spec.ts create mode 100644 src/services/destructive-command-guard/constants.ts create mode 100644 src/services/destructive-command-guard/manager.ts create mode 100644 src/services/destructive-command-guard/runner.ts diff --git a/src/services/destructive-command-guard/__tests__/manager.spec.ts b/src/services/destructive-command-guard/__tests__/manager.spec.ts new file mode 100644 index 0000000000..32831e749f --- /dev/null +++ b/src/services/destructive-command-guard/__tests__/manager.spec.ts @@ -0,0 +1,249 @@ +import { createHash } from "crypto" +import { EventEmitter } from "events" +import { access, chmod, mkdtemp, mkdir, readFile, rm, stat, writeFile } from "fs/promises" +import { tmpdir } from "os" +import path from "path" +import { PassThrough } from "stream" + +import { spawn } from "child_process" +import { get } from "https" +import type { IncomingMessage, RequestOptions } from "http" + +import { DCG_ARCHIVES, DCG_VERSION } from "../constants" +import { + downloadFile, + extractSingleBinary, + getDcgArchiveInfo, + getDcgBinaryPath, + isDcgSupportedPlatform, + isTrustedDownloadUrl, + resolveTrustedRedirect, + ensureDcgInstalled, + verifyChecksum, +} from "../manager" + +vi.mock("child_process", () => ({ spawn: vi.fn() })) +vi.mock("https", () => ({ get: vi.fn() })) + +const mockSpawn = vi.mocked(spawn) +const mockGet = vi.mocked(get) + +describe("Destructive Command Guard manager", () => { + let tempDir: string + + beforeEach(async () => { + tempDir = await mkdtemp(path.join(tmpdir(), "dcg-manager-")) + mockSpawn.mockReset() + mockGet.mockReset() + }) + + afterEach(async () => { + await rm(tempDir, { recursive: true, force: true }) + }) + + it("maps all supported platform and architecture combinations", () => { + expect(Object.keys(DCG_ARCHIVES).sort()).toEqual(["darwin-arm64", "linux-arm64", "linux-x64", "win32-x64"]) + expect(getDcgArchiveInfo("darwin", "arm64")?.archive).toBe("dcg-aarch64-apple-darwin.tar.xz") + expect(getDcgArchiveInfo("win32", "x64")?.binary).toBe("dcg.exe") + }) + + it("rejects unsupported platforms", () => { + expect(isDcgSupportedPlatform("freebsd", "x64")).toBe(false) + expect(getDcgBinaryPath("/storage", "freebsd", "x64")).toBeUndefined() + }) + + it("returns the managed binary path", () => { + expect(getDcgBinaryPath("/storage", "linux", "x64")).toBe( + path.join("/storage", "destructive-command-guard", "dcg"), + ) + }) + + it("accepts only HTTPS URLs on trusted host boundaries", () => { + expect(isTrustedDownloadUrl("https://github.com/release")).toBe(true) + expect(isTrustedDownloadUrl("https://cdn.objects.githubusercontent.com/release")).toBe(true) + expect(isTrustedDownloadUrl("http://github.com/release")).toBe(false) + expect(isTrustedDownloadUrl("https://evilgithub.com/release")).toBe(false) + expect(isTrustedDownloadUrl("not a URL")).toBe(false) + }) + + it("rejects untrusted download URLs before opening a destination", async () => { + await expect(downloadFile("https://example.com/dcg", path.join(tempDir, "archive"))).rejects.toThrow( + "DCG download redirected to an untrusted host", + ) + }) + + it("allows trusted relative redirects and rejects unsafe or exhausted redirects", () => { + expect(resolveTrustedRedirect("https://github.com/release", "/asset", 5)).toBe("https://github.com/asset") + expect(() => resolveTrustedRedirect("https://github.com/release", "https://example.com/asset", 5)).toThrow( + "DCG download redirected to an untrusted host", + ) + expect(() => resolveTrustedRedirect("https://github.com/release", "/asset", 0)).toThrow( + "Too many DCG download redirects", + ) + expect(() => resolveTrustedRedirect("https://github.com/release", undefined, 5)).toThrow( + "Too many DCG download redirects", + ) + }) + + it("verifies matching checksums and rejects mismatches", async () => { + const filePath = path.join(tempDir, "archive") + const contents = Buffer.from("verified archive") + await writeFile(filePath, contents) + const checksum = createHash("sha256").update(contents).digest("hex") + + await expect(verifyChecksum(filePath, checksum)).resolves.toBeUndefined() + await expect(verifyChecksum(filePath, "0".repeat(64))).rejects.toThrow( + "DCG archive checksum verification failed", + ) + }) + + it("uses the platform ZIP extractor", async () => { + const child = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) + // The production code uses only the event and stream subset supplied by this test double. + mockSpawn.mockReturnValue(child as unknown as ReturnType) + + const extraction = extractSingleBinary("C:\\dcg.zip", "C:\\staging", DCG_ARCHIVES["win32-x64"]) + child.emit("close", 0) + await extraction + + const expectedExecutable = process.platform === "win32" ? "powershell" : "unzip" + const expectedArgs = + process.platform === "win32" + ? ["-NoProfile", "-Command", "Expand-Archive -Path 'C:\\dcg.zip' -DestinationPath 'C:\\staging' -Force"] + : ["-o", "C:\\dcg.zip", "-d", "C:\\staging"] + + expect(mockSpawn).toHaveBeenCalledWith(expectedExecutable, expectedArgs, { + shell: false, + stdio: ["ignore", "pipe", "pipe"], + }) + }) + + it("extracts tar archives without imposing a single-file layout", async () => { + const child = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) + // The production code uses only the event and stream subset supplied by this test double. + mockSpawn.mockReturnValue(child as unknown as ReturnType) + + const extraction = extractSingleBinary("/tmp/dcg.tar.xz", tempDir, DCG_ARCHIVES["linux-x64"]) + child.emit("close", 0) + + await expect(extraction).resolves.toBeUndefined() + expect(mockSpawn).toHaveBeenCalledTimes(1) + expect(mockSpawn).toHaveBeenCalledWith( + "tar", + expect.arrayContaining(["-xJf", "/tmp/dcg.tar.xz", "-C", tempDir, "--no-same-owner"]), + expect.objectContaining({ shell: false }), + ) + }) + + it("surfaces process failures during extraction", async () => { + const child = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) + mockSpawn.mockReturnValue(child as unknown as ReturnType) + + const extraction = extractSingleBinary("/tmp/dcg.tar.xz", tempDir, DCG_ARCHIVES["linux-x64"]) + child.stderr.write("invalid archive") + child.emit("close", 2) + + await expect(extraction).rejects.toThrow("invalid archive") + }) + + it("reuses an existing managed binary and restores its executable permissions", async () => { + const binaryPath = getDcgBinaryPath(tempDir) + expect(binaryPath).toBeDefined() + await mkdir(path.dirname(binaryPath!), { recursive: true }) + await writeFile(binaryPath!, "existing binary") + await writeFile(path.join(path.dirname(binaryPath!), ".dcg-version"), DCG_VERSION) + if (process.platform !== "win32") { + await chmod(binaryPath!, 0o600) + } + + await expect(ensureDcgInstalled(tempDir)).resolves.toBe(binaryPath) + expect(mockSpawn).not.toHaveBeenCalled() + if (process.platform !== "win32") { + expect((await stat(binaryPath!)).mode & 0o111).toBe(0o111) + } + }) + + it("downloads, verifies, extracts, and deduplicates a new installation", async () => { + const info = getDcgArchiveInfo() + expect(info).toBeDefined() + if (!info || info.archive.endsWith(".zip")) return + + const archive = Buffer.from("test archive") + const originalChecksum = info.sha256 + Object.defineProperty(info, "sha256", { + value: createHash("sha256").update(archive).digest("hex"), + configurable: true, + }) + const response = Object.assign(new PassThrough(), { + statusCode: 200, + headers: { "content-length": String(archive.length) }, + }) + const request = Object.assign(new EventEmitter(), { + setTimeout: vi.fn(), + destroy: vi.fn(), + }) + mockGet.mockImplementation( + ( + _url: string | URL, + optionsOrCallback: RequestOptions | ((response: IncomingMessage) => void), + optionalCallback?: (response: IncomingMessage) => void, + ) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => { + // The downloader uses only the response stream/status subset supplied here. + callback?.(response as unknown as IncomingMessage) + response.end(archive) + }) + // The downloader uses only timeout/error handling from ClientRequest. + return request as unknown as ReturnType + }, + ) + + mockSpawn.mockImplementation((executable, args) => { + const child = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) + setImmediate(async () => { + if (executable === "tar") { + const stagingDir = args[args.indexOf("-C") + 1] + await writeFile(path.join(stagingDir, info.binary), "executable") + } + child.emit("close", 0) + }) + // The process runner uses only the event and stream subset supplied here. + return child as unknown as ReturnType + }) + + try { + const firstInstallation = ensureDcgInstalled(tempDir) + const concurrentInstallation = ensureDcgInstalled(tempDir) + expect(concurrentInstallation).toBe(firstInstallation) + + const binaryPath = await firstInstallation + if (!binaryPath) throw new Error("Expected DCG to be supported in this test") + expect(await readFile(binaryPath, "utf8")).toBe("executable") + expect(mockGet).toHaveBeenCalledTimes(1) + expect(mockSpawn).toHaveBeenCalledTimes(1) + await expect(access(path.join(tempDir, `${DCG_VERSION}-${info.archive}`))).rejects.toThrow() + expect(await readFile(path.join(tempDir, "destructive-command-guard", ".dcg-version"), "utf8")).toBe( + DCG_VERSION, + ) + } finally { + Object.defineProperty(info, "sha256", { value: originalChecksum, configurable: true }) + } + }) +}) diff --git a/src/services/destructive-command-guard/__tests__/runner.spec.ts b/src/services/destructive-command-guard/__tests__/runner.spec.ts new file mode 100644 index 0000000000..47eb355dc8 --- /dev/null +++ b/src/services/destructive-command-guard/__tests__/runner.spec.ts @@ -0,0 +1,126 @@ +import { EventEmitter } from "events" +import { PassThrough } from "stream" + +import { spawn } from "child_process" + +import { DCG_MAX_OUTPUT_BYTES } from "../constants" +import { runDcg } from "../runner" + +vi.mock("child_process", () => ({ spawn: vi.fn() })) + +type MockChild = EventEmitter & { + stdout: PassThrough + stderr: PassThrough + kill: ReturnType +} + +const mockSpawn = vi.mocked(spawn) + +const useChild = (child: MockChild): void => { + // runDcg uses only the event, stream, and kill subset supplied by this test double. + mockSpawn.mockReturnValue(child as unknown as ReturnType) +} + +function createChild(): MockChild { + return Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) +} + +function emitResult(child: MockChild, payload: unknown, code: number): void { + child.stdout.write(JSON.stringify(payload)) + child.emit("close", code, null) +} + +describe("runDcg", () => { + beforeEach(() => { + vi.useRealTimers() + mockSpawn.mockReset() + }) + + afterEach(() => vi.useRealTimers()) + + it.each([ + [{ schema_version: 1, decision: "allow" }, 0, { decision: "allow" }], + [ + { schema_version: 2, decision: "deny", reason: "unsafe", rule_id: "delete" }, + 1, + { decision: "deny", reason: "unsafe", ruleId: "delete" }, + ], + [ + { schema_version: 2, decision: "deny", pack_id: "core", pattern_name: "delete" }, + 1, + { decision: "deny", ruleId: "core:delete" }, + ], + ])("accepts valid DCG result %#", async (payload, code, expected) => { + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + emitResult(child, payload, code) + + await expect(result).resolves.toEqual(expected) + }) + + it.each([ + ["not json", 0, "DCG returned invalid JSON"], + [JSON.stringify({ schema_version: 3, decision: "allow" }), 0, "DCG returned an unsupported response schema"], + [JSON.stringify({ schema_version: 1, decision: "deny" }), 0, "DCG decision did not match its exit status"], + ])("rejects invalid output %#", async (output, code, message) => { + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + child.stdout.write(output) + child.emit("close", code, null) + + await expect(result).rejects.toThrow(message) + }) + + it("rejects non-DCG exit statuses with stderr", async () => { + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + child.stderr.write("failure details") + child.emit("close", 2, null) + + await expect(result).rejects.toThrow("DCG evaluation failed: failure details") + }) + + it("rejects process startup errors", async () => { + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + child.emit("error", new Error("ENOENT")) + + await expect(result).rejects.toThrow("Unable to start DCG: ENOENT") + }) + + it("rejects excessive output and kills the process", async () => { + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + child.stdout.write(Buffer.alloc(DCG_MAX_OUTPUT_BYTES + 1)) + + await expect(result).rejects.toThrow("DCG produced too much output") + expect(child.kill).toHaveBeenCalledWith("SIGKILL") + }) + + it("times out and kills the process", async () => { + vi.useFakeTimers() + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + const rejection = expect(result).rejects.toThrow("DCG evaluation timed out") + await vi.runAllTimersAsync() + + await rejection + expect(child.kill).toHaveBeenCalledWith("SIGKILL") + }) +}) diff --git a/src/services/destructive-command-guard/constants.ts b/src/services/destructive-command-guard/constants.ts new file mode 100644 index 0000000000..74ce76ada4 --- /dev/null +++ b/src/services/destructive-command-guard/constants.ts @@ -0,0 +1,41 @@ +export const DCG_VERSION = "v0.7.7" + +export type DcgArchiveInfo = Readonly<{ + archive: string + binary: "dcg" | "dcg.exe" + sha256: string +}> + +export const DCG_ARCHIVES: Readonly> = { + "darwin-arm64": { + archive: "dcg-aarch64-apple-darwin.tar.xz", + binary: "dcg", + sha256: "a63cf82bd3584055112d5ec7a4ab3d7e0619a9f806a53930c27aa0e6297484de", + }, + "linux-arm64": { + archive: "dcg-aarch64-unknown-linux-gnu.tar.xz", + binary: "dcg", + sha256: "abb0d94f23ab50f9edc16f8ca6939ff8eec23e1831d3ad7a28d9f03252c3306d", + }, + "linux-x64": { + archive: "dcg-x86_64-unknown-linux-musl.tar.xz", + binary: "dcg", + sha256: "472b130a9b235edc57e6cb7566641da5fef905e9dbefd3a46f9ad1e33205fa04", + }, + "win32-x64": { + archive: "dcg-x86_64-pc-windows-msvc.zip", + binary: "dcg.exe", + sha256: "435127410eabc53e772be4f5c668a875b45fbaf806654b577c2d975bd0e38964", + }, +} as const + +export const DCG_DOWNLOAD_BASE_URL = `https://github.com/Dicklesworthstone/destructive_command_guard/releases/download/${DCG_VERSION}` + +export const DCG_RUN_TIMEOUT_MS = 3_000 +export const DCG_MAX_OUTPUT_BYTES = 256 * 1024 + +export const DCG_TRUSTED_DOWNLOAD_DOMAINS = [ + "github.com", + "objects.githubusercontent.com", + "release-assets.githubusercontent.com", +] as const diff --git a/src/services/destructive-command-guard/manager.ts b/src/services/destructive-command-guard/manager.ts new file mode 100644 index 0000000000..61325954bb --- /dev/null +++ b/src/services/destructive-command-guard/manager.ts @@ -0,0 +1,98 @@ +import * as path from "path" + +import { extractTarXzArchive, extractZipArchive } from "../managed-binary/archive" +import { + downloadBinaryFile, + isTrustedHttpsUrl, + resolveTrustedRedirect as resolveManagedBinaryRedirect, + verifySha256Checksum, +} from "../managed-binary/download" +import { ensureManagedBinaryInstalled, getManagedBinaryPaths } from "../managed-binary/install" + +import { + DCG_ARCHIVES, + DCG_DOWNLOAD_BASE_URL, + DCG_TRUSTED_DOWNLOAD_DOMAINS, + DCG_VERSION, + type DcgArchiveInfo, +} from "./constants" + +const VERSION_FILE = ".dcg-version" + +export function getDcgArchiveInfo(platform = process.platform, arch = process.arch): DcgArchiveInfo | undefined { + return DCG_ARCHIVES[`${platform}-${arch}`] +} + +export function isDcgSupportedPlatform(platform = process.platform, arch = process.arch): boolean { + return getDcgArchiveInfo(platform, arch) !== undefined +} + +export function getDcgBinaryPath( + storageDir: string, + platform = process.platform, + arch = process.arch, +): string | undefined { + const info = getDcgArchiveInfo(platform, arch) + return info ? path.join(storageDir, "destructive-command-guard", info.binary) : undefined +} + +export function isTrustedDownloadUrl(url: string): boolean { + return isTrustedHttpsUrl(url, DCG_TRUSTED_DOWNLOAD_DOMAINS) +} + +export function resolveTrustedRedirect(url: string, location: string | undefined, redirectsRemaining: number): string { + return resolveManagedBinaryRedirect(url, location, redirectsRemaining, { + name: "DCG", + trustedDomains: DCG_TRUSTED_DOWNLOAD_DOMAINS, + }) +} + +export function downloadFile(url: string, destination: string, maxRedirects = 5): Promise { + return downloadBinaryFile(url, destination, { + name: "DCG", + trustedDomains: DCG_TRUSTED_DOWNLOAD_DOMAINS, + timeoutMs: 120_000, + maxRedirects, + }) +} + +export async function verifyChecksum(filePath: string, expected: string): Promise { + await verifySha256Checksum(filePath, expected, () => new Error("DCG archive checksum verification failed")) +} + +export async function extractSingleBinary( + archivePath: string, + stagingDir: string, + info: DcgArchiveInfo, +): Promise { + if (info.archive.endsWith(".zip")) { + await extractZipArchive(archivePath, stagingDir) + return + } + + await extractTarXzArchive(archivePath, stagingDir) +} + +function installDcg(storageDir: string): Promise { + const info = getDcgArchiveInfo() + if (!info) { + return Promise.resolve(undefined) + } + + return ensureManagedBinaryInstalled({ + storageDir, + id: "destructive-command-guard", + version: DCG_VERSION, + versionFile: VERSION_FILE, + archiveName: info.archive, + binaryName: info.binary, + errorPrefix: "Failed to download DCG", + download: (archivePath) => downloadFile(`${DCG_DOWNLOAD_BASE_URL}/${info.archive}`, archivePath), + verifyArchive: (archivePath) => verifyChecksum(archivePath, info.sha256), + extractArchive: (archivePath, stagingDir) => extractSingleBinary(archivePath, stagingDir, info), + }) +} + +export function ensureDcgInstalled(storageDir: string): Promise { + return installDcg(storageDir) +} diff --git a/src/services/destructive-command-guard/runner.ts b/src/services/destructive-command-guard/runner.ts new file mode 100644 index 0000000000..22c0ac58e2 --- /dev/null +++ b/src/services/destructive-command-guard/runner.ts @@ -0,0 +1,84 @@ +import { spawn } from "child_process" + +import { DCG_MAX_OUTPUT_BYTES, DCG_RUN_TIMEOUT_MS } from "./constants" + +export type DcgDecision = { decision: "allow" } | { decision: "deny"; reason?: string; ruleId?: string } + +type DcgJsonOutput = { + schema_version?: number | string + decision?: string + reason?: string + rule_id?: string + pattern_name?: string + pack_id?: string +} + +export function runDcg(binaryPath: string, command: string, cwd: string): Promise { + return new Promise((resolve, reject) => { + const child = spawn(binaryPath, ["test", "--format", "json", "--no-color", command], { + cwd, + shell: false, + stdio: ["ignore", "pipe", "pipe"], + env: { ...process.env, NO_COLOR: "1" }, + }) + let stdout: Buffer = Buffer.alloc(0) + let stderr: Buffer = Buffer.alloc(0) + let settled = false + const fail = (error: Error) => { + if (settled) return + settled = true + clearTimeout(timer) + child.kill("SIGKILL") + reject(error) + } + const append = (current: Buffer, chunk: Buffer): Buffer => { + if (current.length + chunk.length > DCG_MAX_OUTPUT_BYTES) { + fail(new Error("DCG produced too much output")) + return current + } + return Buffer.concat([current, chunk]) + } + const timer = setTimeout(() => fail(new Error("DCG evaluation timed out")), DCG_RUN_TIMEOUT_MS) + child.stdout?.on("data", (chunk: Buffer) => (stdout = append(stdout, chunk))) + child.stderr?.on("data", (chunk: Buffer) => (stderr = append(stderr, chunk))) + child.on("error", (error) => fail(new Error(`Unable to start DCG: ${error.message}`))) + child.on("close", (code, signal) => { + if (settled) return + settled = true + clearTimeout(timer) + if (signal || (code !== 0 && code !== 1)) { + reject(new Error(`DCG evaluation failed${stderr.length ? `: ${stderr.toString().trim()}` : ""}`)) + return + } + + let payload: DcgJsonOutput + try { + payload = JSON.parse(stdout.toString("utf8")) as DcgJsonOutput + } catch { + reject(new Error("DCG returned invalid JSON")) + return + } + + const schemaVersion = Number(payload.schema_version) + if (![1, 2].includes(schemaVersion)) { + reject(new Error("DCG returned an unsupported response schema")) + return + } + if (payload.decision === "allow" && code === 0) { + resolve({ decision: "allow" }) + } else if (payload.decision === "deny" && code === 1) { + resolve({ + decision: "deny", + reason: payload.reason, + ruleId: + payload.rule_id ?? + (payload.pack_id && payload.pattern_name + ? `${payload.pack_id}:${payload.pattern_name}` + : undefined), + }) + } else { + reject(new Error("DCG decision did not match its exit status")) + } + }) + }) +} From e1a0c3943af6ba575a38d8fc3765e47672630554 Mon Sep 17 00:00:00 2001 From: Naved Date: Sat, 1 Aug 2026 10:16:56 -0700 Subject: [PATCH 04/10] test: strengthen DCG binary service coverage --- .../__tests__/manager.spec.ts | 21 ++++++++++++------- .../__tests__/runner.spec.ts | 5 +++++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/services/destructive-command-guard/__tests__/manager.spec.ts b/src/services/destructive-command-guard/__tests__/manager.spec.ts index 32831e749f..45543a8129 100644 --- a/src/services/destructive-command-guard/__tests__/manager.spec.ts +++ b/src/services/destructive-command-guard/__tests__/manager.spec.ts @@ -68,7 +68,7 @@ describe("Destructive Command Guard manager", () => { it("rejects untrusted download URLs before opening a destination", async () => { await expect(downloadFile("https://example.com/dcg", path.join(tempDir, "archive"))).rejects.toThrow( - "DCG download redirected to an untrusted host", + "DCG download URL is not a trusted HTTPS host", ) }) @@ -81,7 +81,7 @@ describe("Destructive Command Guard manager", () => { "Too many DCG download redirects", ) expect(() => resolveTrustedRedirect("https://github.com/release", undefined, 5)).toThrow( - "Too many DCG download redirects", + "DCG download redirect is missing a Location header", ) }) @@ -113,7 +113,14 @@ describe("Destructive Command Guard manager", () => { const expectedExecutable = process.platform === "win32" ? "powershell" : "unzip" const expectedArgs = process.platform === "win32" - ? ["-NoProfile", "-Command", "Expand-Archive -Path 'C:\\dcg.zip' -DestinationPath 'C:\\staging' -Force"] + ? [ + "-NoProfile", + "-NonInteractive", + "-Command", + "$archivePath = $args[0]; $destination = $args[1]; Expand-Archive -LiteralPath $archivePath -DestinationPath $destination -Force", + "C:\\dcg.zip", + "C:\\staging", + ] : ["-o", "C:\\dcg.zip", "-d", "C:\\staging"] expect(mockSpawn).toHaveBeenCalledWith(expectedExecutable, expectedArgs, { @@ -136,11 +143,9 @@ describe("Destructive Command Guard manager", () => { await expect(extraction).resolves.toBeUndefined() expect(mockSpawn).toHaveBeenCalledTimes(1) - expect(mockSpawn).toHaveBeenCalledWith( - "tar", - expect.arrayContaining(["-xJf", "/tmp/dcg.tar.xz", "-C", tempDir, "--no-same-owner"]), - expect.objectContaining({ shell: false }), - ) + const expectedArgs = ["-xJf", "/tmp/dcg.tar.xz", "-C", tempDir, "--no-same-owner"] + if (process.platform === "linux") expectedArgs.push("--no-overwrite-dir") + expect(mockSpawn).toHaveBeenCalledWith("tar", expectedArgs, { shell: false, stdio: ["ignore", "pipe", "pipe"] }) }) it("surfaces process failures during extraction", async () => { diff --git a/src/services/destructive-command-guard/__tests__/runner.spec.ts b/src/services/destructive-command-guard/__tests__/runner.spec.ts index 47eb355dc8..7be215f2cc 100644 --- a/src/services/destructive-command-guard/__tests__/runner.spec.ts +++ b/src/services/destructive-command-guard/__tests__/runner.spec.ts @@ -62,6 +62,11 @@ describe("runDcg", () => { emitResult(child, payload, code) await expect(result).resolves.toEqual(expected) + expect(mockSpawn).toHaveBeenCalledWith( + "/dcg", + expect.any(Array), + expect.objectContaining({ cwd: "/workspace" }), + ) }) it.each([ From bae4ff7fb170df438999145e1916e0d7fa7eff5f Mon Sep 17 00:00:00 2001 From: Naved Date: Sat, 1 Aug 2026 17:54:52 -0700 Subject: [PATCH 05/10] fix: address DCG service review feedback --- .../__tests__/manager.spec.ts | 129 +++++++++++++++++- .../__tests__/runner.spec.ts | 35 ++++- .../destructive-command-guard/manager.ts | 11 +- .../destructive-command-guard/runner.ts | 41 ++++-- 4 files changed, 198 insertions(+), 18 deletions(-) diff --git a/src/services/destructive-command-guard/__tests__/manager.spec.ts b/src/services/destructive-command-guard/__tests__/manager.spec.ts index 45543a8129..b49ef8aebe 100644 --- a/src/services/destructive-command-guard/__tests__/manager.spec.ts +++ b/src/services/destructive-command-guard/__tests__/manager.spec.ts @@ -63,6 +63,7 @@ describe("Destructive Command Guard manager", () => { expect(isTrustedDownloadUrl("https://cdn.objects.githubusercontent.com/release")).toBe(true) expect(isTrustedDownloadUrl("http://github.com/release")).toBe(false) expect(isTrustedDownloadUrl("https://evilgithub.com/release")).toBe(false) + expect(isTrustedDownloadUrl("https://github.com.evil.com/release")).toBe(false) expect(isTrustedDownloadUrl("not a URL")).toBe(false) }) @@ -72,6 +73,61 @@ describe("Destructive Command Guard manager", () => { ) }) + it("rejects non-successful HTTP responses", async () => { + const response = Object.assign(new PassThrough(), { statusCode: 503, headers: {}, destroy: vi.fn() }) + const request = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) + mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => callback?.(response as unknown as IncomingMessage)) + return request as unknown as ReturnType + }) + + await expect(downloadFile("https://github.com/release", path.join(tempDir, "archive"))).rejects.toThrow( + "DCG download failed with HTTP 503", + ) + }) + + it("rejects request errors", async () => { + const request = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) + mockGet.mockReturnValue(request as unknown as ReturnType) + + const download = downloadFile("https://github.com/release", path.join(tempDir, "archive")) + request.emit("error", new Error("socket failed")) + + await expect(download).rejects.toThrow("socket failed") + }) + + it("times out stalled requests", async () => { + const request = Object.assign(new EventEmitter(), { + setTimeout: vi.fn((_timeout: number, callback: () => void) => setImmediate(callback)), + destroy: vi.fn((error: Error) => request.emit("error", error)), + }) + mockGet.mockReturnValue(request as unknown as ReturnType) + + await expect(downloadFile("https://github.com/release", path.join(tempDir, "archive"))).rejects.toThrow( + "DCG download timed out", + ) + expect(request.setTimeout).toHaveBeenCalledWith(120_000, expect.any(Function)) + }) + + it("rejects archives larger than 50 MiB", async () => { + const response = Object.assign(new PassThrough(), { + statusCode: 200, + headers: { "content-length": String(50 * 1024 * 1024 + 1) }, + destroy: vi.fn(), + }) + const request = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) + mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => callback?.(response as unknown as IncomingMessage)) + return request as unknown as ReturnType + }) + + await expect(downloadFile("https://github.com/release", path.join(tempDir, "archive"))).rejects.toThrow( + "DCG archive exceeds the download size limit", + ) + }) + it("allows trusted relative redirects and rejects unsafe or exhausted redirects", () => { expect(resolveTrustedRedirect("https://github.com/release", "/asset", 5)).toBe("https://github.com/asset") expect(() => resolveTrustedRedirect("https://github.com/release", "https://example.com/asset", 5)).toThrow( @@ -92,9 +148,7 @@ describe("Destructive Command Guard manager", () => { const checksum = createHash("sha256").update(contents).digest("hex") await expect(verifyChecksum(filePath, checksum)).resolves.toBeUndefined() - await expect(verifyChecksum(filePath, "0".repeat(64))).rejects.toThrow( - "DCG archive checksum verification failed", - ) + await expect(verifyChecksum(filePath, "0".repeat(64))).rejects.toThrow(`got ${checksum}`) }) it("uses the platform ZIP extractor", async () => { @@ -180,6 +234,22 @@ describe("Destructive Command Guard manager", () => { } }) + it("warns when the current platform is unsupported", async () => { + const platformKey = `${process.platform}-${process.arch}` + const info = DCG_ARCHIVES[platformKey] + if (!info) return + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + Reflect.deleteProperty(DCG_ARCHIVES, platformKey) + + try { + await expect(ensureDcgInstalled(tempDir)).resolves.toBeUndefined() + expect(warnSpy).toHaveBeenCalledWith(`[DCG] Unsupported platform: ${platformKey}`) + } finally { + Reflect.set(DCG_ARCHIVES, platformKey, info) + warnSpy.mockRestore() + } + }) + it("downloads, verifies, extracts, and deduplicates a new installation", async () => { const info = getDcgArchiveInfo() expect(info).toBeDefined() @@ -251,4 +321,57 @@ describe("Destructive Command Guard manager", () => { Object.defineProperty(info, "sha256", { value: originalChecksum, configurable: true }) } }) + + it("downloads, verifies, extracts, and installs a ZIP archive", async () => { + const info = getDcgArchiveInfo() + if (!info?.archive.endsWith(".zip")) return + + const archive = Buffer.from("test ZIP archive") + const originalChecksum = info.sha256 + Object.defineProperty(info, "sha256", { + value: createHash("sha256").update(archive).digest("hex"), + configurable: true, + }) + const response = Object.assign(new PassThrough(), { + statusCode: 200, + headers: { "content-length": String(archive.length) }, + }) + const request = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) + mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => { + callback?.(response as unknown as IncomingMessage) + response.end(archive) + }) + return request as unknown as ReturnType + }) + mockSpawn.mockImplementation((_executable, args) => { + const child = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) + setImmediate(async () => { + const destinationIndex = args.indexOf( + "$archivePath = $args[0]; $destination = $args[1]; Expand-Archive -LiteralPath $archivePath -DestinationPath $destination -Force", + ) + const stagingDir = args[destinationIndex + 2] + await writeFile(path.join(stagingDir, info.binary), "ZIP executable") + child.emit("close", 0) + }) + return child as unknown as ReturnType + }) + + try { + const binaryPath = await ensureDcgInstalled(tempDir) + if (!binaryPath) throw new Error("Expected DCG to be supported in this test") + expect(await readFile(binaryPath, "utf8")).toBe("ZIP executable") + expect(await readFile(path.join(tempDir, "destructive-command-guard", ".dcg-version"), "utf8")).toBe( + DCG_VERSION, + ) + await expect(access(path.join(tempDir, `${DCG_VERSION}-${info.archive}`))).rejects.toThrow() + } finally { + Object.defineProperty(info, "sha256", { value: originalChecksum, configurable: true }) + } + }) }) diff --git a/src/services/destructive-command-guard/__tests__/runner.spec.ts b/src/services/destructive-command-guard/__tests__/runner.spec.ts index 7be215f2cc..d738de9b7a 100644 --- a/src/services/destructive-command-guard/__tests__/runner.spec.ts +++ b/src/services/destructive-command-guard/__tests__/runner.spec.ts @@ -35,12 +35,18 @@ function emitResult(child: MockChild, payload: unknown, code: number): void { } describe("runDcg", () => { + let warnSpy: ReturnType + beforeEach(() => { vi.useRealTimers() mockSpawn.mockReset() + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) }) - afterEach(() => vi.useRealTimers()) + afterEach(() => { + vi.useRealTimers() + warnSpy.mockRestore() + }) it.each([ [{ schema_version: 1, decision: "allow" }, 0, { decision: "allow" }], @@ -67,6 +73,30 @@ describe("runDcg", () => { expect.any(Array), expect.objectContaining({ cwd: "/workspace" }), ) + if (expected.decision === "deny") { + const reason = "reason" in expected ? expected.reason : undefined + expect(warnSpy).toHaveBeenCalledWith("[DCG] Command denied", reason ?? "No reason provided") + } + }) + + it("passes only the environment variables DCG requires", async () => { + const child = createChild() + useChild(child) + const originalToken = process.env.GITHUB_TOKEN + process.env.GITHUB_TOKEN = "secret" + + try { + const result = runDcg("/dcg", "echo test", "/workspace") + emitResult(child, { schema_version: 1, decision: "allow" }, 0) + await result + + const options = mockSpawn.mock.calls[0][2] + expect(options?.env).toMatchObject({ NO_COLOR: "1" }) + expect(options?.env).not.toHaveProperty("GITHUB_TOKEN") + } finally { + if (originalToken === undefined) delete process.env.GITHUB_TOKEN + else process.env.GITHUB_TOKEN = originalToken + } }) it.each([ @@ -82,6 +112,7 @@ describe("runDcg", () => { child.emit("close", code, null) await expect(result).rejects.toThrow(message) + expect(warnSpy).toHaveBeenCalledWith("[DCG]", message) }) it("rejects non-DCG exit statuses with stderr", async () => { @@ -93,6 +124,7 @@ describe("runDcg", () => { child.emit("close", 2, null) await expect(result).rejects.toThrow("DCG evaluation failed: failure details") + expect(warnSpy).toHaveBeenCalledWith("[DCG]", "DCG evaluation failed: failure details") }) it("rejects process startup errors", async () => { @@ -103,6 +135,7 @@ describe("runDcg", () => { child.emit("error", new Error("ENOENT")) await expect(result).rejects.toThrow("Unable to start DCG: ENOENT") + expect(warnSpy).toHaveBeenCalledWith("[DCG]", "Unable to start DCG: ENOENT") }) it("rejects excessive output and kills the process", async () => { diff --git a/src/services/destructive-command-guard/manager.ts b/src/services/destructive-command-guard/manager.ts index 61325954bb..84338713b4 100644 --- a/src/services/destructive-command-guard/manager.ts +++ b/src/services/destructive-command-guard/manager.ts @@ -7,7 +7,7 @@ import { resolveTrustedRedirect as resolveManagedBinaryRedirect, verifySha256Checksum, } from "../managed-binary/download" -import { ensureManagedBinaryInstalled, getManagedBinaryPaths } from "../managed-binary/install" +import { ensureManagedBinaryInstalled } from "../managed-binary/install" import { DCG_ARCHIVES, @@ -18,6 +18,7 @@ import { } from "./constants" const VERSION_FILE = ".dcg-version" +const MAX_ARCHIVE_BYTES = 50 * 1024 * 1024 export function getDcgArchiveInfo(platform = process.platform, arch = process.arch): DcgArchiveInfo | undefined { return DCG_ARCHIVES[`${platform}-${arch}`] @@ -53,11 +54,16 @@ export function downloadFile(url: string, destination: string, maxRedirects = 5) trustedDomains: DCG_TRUSTED_DOWNLOAD_DOMAINS, timeoutMs: 120_000, maxRedirects, + maxBytes: MAX_ARCHIVE_BYTES, }) } export async function verifyChecksum(filePath: string, expected: string): Promise { - await verifySha256Checksum(filePath, expected, () => new Error("DCG archive checksum verification failed")) + await verifySha256Checksum( + filePath, + expected, + (actual) => new Error(`DCG archive checksum verification failed (got ${actual})`), + ) } export async function extractSingleBinary( @@ -76,6 +82,7 @@ export async function extractSingleBinary( function installDcg(storageDir: string): Promise { const info = getDcgArchiveInfo() if (!info) { + console.warn(`[DCG] Unsupported platform: ${process.platform}-${process.arch}`) return Promise.resolve(undefined) } diff --git a/src/services/destructive-command-guard/runner.ts b/src/services/destructive-command-guard/runner.ts index 22c0ac58e2..83d5b95c7b 100644 --- a/src/services/destructive-command-guard/runner.ts +++ b/src/services/destructive-command-guard/runner.ts @@ -13,25 +13,39 @@ type DcgJsonOutput = { pack_id?: string } +const DCG_ENV_KEYS = ["HOME", "PATH", "TEMP", "TMP", "USERPROFILE", "SystemRoot", "WINDIR"] as const + +function getDcgEnvironment(): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { NO_COLOR: "1" } + for (const key of DCG_ENV_KEYS) { + if (process.env[key] !== undefined) env[key] = process.env[key] + } + return env +} + export function runDcg(binaryPath: string, command: string, cwd: string): Promise { return new Promise((resolve, reject) => { const child = spawn(binaryPath, ["test", "--format", "json", "--no-color", command], { cwd, shell: false, stdio: ["ignore", "pipe", "pipe"], - env: { ...process.env, NO_COLOR: "1" }, + env: getDcgEnvironment(), }) let stdout: Buffer = Buffer.alloc(0) let stderr: Buffer = Buffer.alloc(0) let settled = false - const fail = (error: Error) => { + const fail = (error: Error, killChild = true) => { if (settled) return settled = true clearTimeout(timer) - child.kill("SIGKILL") + if (killChild) child.kill("SIGKILL") + console.warn("[DCG]", error.message) reject(error) } - const append = (current: Buffer, chunk: Buffer): Buffer => { + const appendOutputOrFail = ( + current: Buffer, + chunk: Buffer, + ): Buffer => { if (current.length + chunk.length > DCG_MAX_OUTPUT_BYTES) { fail(new Error("DCG produced too much output")) return current @@ -39,15 +53,13 @@ export function runDcg(binaryPath: string, command: string, cwd: string): Promis return Buffer.concat([current, chunk]) } const timer = setTimeout(() => fail(new Error("DCG evaluation timed out")), DCG_RUN_TIMEOUT_MS) - child.stdout?.on("data", (chunk: Buffer) => (stdout = append(stdout, chunk))) - child.stderr?.on("data", (chunk: Buffer) => (stderr = append(stderr, chunk))) + child.stdout?.on("data", (chunk: Buffer) => (stdout = appendOutputOrFail(stdout, chunk))) + child.stderr?.on("data", (chunk: Buffer) => (stderr = appendOutputOrFail(stderr, chunk))) child.on("error", (error) => fail(new Error(`Unable to start DCG: ${error.message}`))) child.on("close", (code, signal) => { if (settled) return - settled = true - clearTimeout(timer) if (signal || (code !== 0 && code !== 1)) { - reject(new Error(`DCG evaluation failed${stderr.length ? `: ${stderr.toString().trim()}` : ""}`)) + fail(new Error(`DCG evaluation failed${stderr.length ? `: ${stderr.toString().trim()}` : ""}`), false) return } @@ -55,18 +67,23 @@ export function runDcg(binaryPath: string, command: string, cwd: string): Promis try { payload = JSON.parse(stdout.toString("utf8")) as DcgJsonOutput } catch { - reject(new Error("DCG returned invalid JSON")) + fail(new Error("DCG returned invalid JSON"), false) return } const schemaVersion = Number(payload.schema_version) if (![1, 2].includes(schemaVersion)) { - reject(new Error("DCG returned an unsupported response schema")) + fail(new Error("DCG returned an unsupported response schema"), false) return } if (payload.decision === "allow" && code === 0) { + settled = true + clearTimeout(timer) resolve({ decision: "allow" }) } else if (payload.decision === "deny" && code === 1) { + settled = true + clearTimeout(timer) + console.warn("[DCG] Command denied", payload.reason ?? "No reason provided") resolve({ decision: "deny", reason: payload.reason, @@ -77,7 +94,7 @@ export function runDcg(binaryPath: string, command: string, cwd: string): Promis : undefined), }) } else { - reject(new Error("DCG decision did not match its exit status")) + fail(new Error("DCG decision did not match its exit status"), false) } }) }) From 28d81f9541a156092874a600bff80c94941ebeee Mon Sep 17 00:00:00 2001 From: Naved Merchant <14171946+navedmerchant@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:41:45 +0000 Subject: [PATCH 06/10] fix: address managed binary review feedback --- .../code-index/semble/semble-downloader.ts | 4 ++ .../managed-binary/__tests__/archive.spec.ts | 50 +++++++++++++++---- .../managed-binary/__tests__/download.spec.ts | 10 ++++ .../managed-binary/__tests__/install.spec.ts | 5 +- src/services/managed-binary/archive.ts | 13 +++-- src/services/managed-binary/download.ts | 17 ++++--- src/services/managed-binary/install.ts | 3 ++ 7 files changed, 80 insertions(+), 22 deletions(-) diff --git a/src/services/code-index/semble/semble-downloader.ts b/src/services/code-index/semble/semble-downloader.ts index d0697298f5..5f68ffe58c 100644 --- a/src/services/code-index/semble/semble-downloader.ts +++ b/src/services/code-index/semble/semble-downloader.ts @@ -26,6 +26,7 @@ const SEMBLE_ARCHIVES: Record = { export const SEMBLE_VERSION = "v0.4.1" const DOWNLOAD_BASE_URL = `https://github.com/Zoo-Code-Org/sembleexec/releases/download/${SEMBLE_VERSION}` const VERSION_FILE = ".semble-version" +const MAX_ARCHIVE_BYTES = 50 * 1024 * 1024 /** * SHA-256 checksums for each platform archive at SEMBLE_VERSION. @@ -121,6 +122,7 @@ export async function downloadSemble(storageDir: string): Promise verifyChecksum(archivePath, expectedChecksum), extractArchive: async (archivePath, stagingDir) => { @@ -128,6 +130,8 @@ export async function downloadSemble(storageDir: string): Promise { it("kills a process that exceeds its timeout", async () => { vi.useFakeTimers() - const child = createChild() - mockSpawn.mockReturnValue(child as unknown as ReturnType) - const processResult = runProcess("tool", [], 100) - const assertion = expect(processResult).rejects.toThrow("tool timed out") - - await vi.advanceTimersByTimeAsync(100) - await assertion - expect(child.kill).toHaveBeenCalledWith("SIGKILL") - vi.useRealTimers() + try { + const child = createChild() + mockSpawn.mockReturnValue(child as unknown as ReturnType) + const processResult = runProcess("tool", [], 100) + const assertion = expect(processResult).rejects.toThrow("tool timed out") + + await vi.advanceTimersByTimeAsync(100) + await assertion + expect(child.kill).toHaveBeenCalledWith("SIGKILL") + } finally { + vi.useRealTimers() + } }) it("extracts tar.gz archives with hardened flags", async () => { @@ -112,7 +115,7 @@ describe("managed binary archive utilities", () => { mockSpawn.mockReturnValueOnce(listing as unknown as ReturnType) mockSpawn.mockReturnValueOnce(extraction as unknown as ReturnType) const result = extractSingleFileTarXzArchive("/tmp/archive.tar.xz", "/tmp/output", "binary", "Tool") - listing.stdout.write("./binary\n") + listing.stdout.write("-rwxr-xr-x user/group 1 2026-01-01 00:00 ./binary\n") listing.emit("close", 0) await new Promise((resolve) => setImmediate(resolve)) extraction.emit("close", 0) @@ -121,11 +124,36 @@ describe("managed binary archive utilities", () => { expect(mockSpawn).toHaveBeenNthCalledWith( 2, "tar", - ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "./binary"], + [ + "-xJf", + "/tmp/archive.tar.xz", + "-C", + "/tmp/output", + "--no-same-owner", + ...(process.platform === "linux" ? ["--no-overwrite-dir"] : []), + "./binary", + ], expect.any(Object), ) }) + it.each([ + ["-rwxr-xr-x user/group 1 2026-01-01 00:00 ./other\n", "an unexpected filename"], + [ + "-rwxr-xr-x user/group 1 2026-01-01 00:00 ./binary\n-rwxr-xr-x user/group 1 2026-01-01 00:00 ./other\n", + "multiple entries", + ], + ["lrwxrwxrwx user/group 0 2026-01-01 00:00 ./binary\n", "a non-regular entry"], + ])("rejects a tar.xz archive with %s", async (listingOutput) => { + const listing = createChild() + mockSpawn.mockReturnValue(listing as unknown as ReturnType) + const result = extractSingleFileTarXzArchive("/tmp/archive.tar.xz", "/tmp/output", "binary", "Tool") + listing.stdout.write(listingOutput) + listing.emit("close", 0) + + await expect(result).rejects.toThrow("Tool archive has an unexpected layout") + }) + it("builds a single-entry-validated PowerShell ZIP extraction", async () => { const child = createChild() mockSpawn.mockReturnValue(child as unknown as ReturnType) diff --git a/src/services/managed-binary/__tests__/download.spec.ts b/src/services/managed-binary/__tests__/download.spec.ts index 2fa4fc353a..a7639068f8 100644 --- a/src/services/managed-binary/__tests__/download.spec.ts +++ b/src/services/managed-binary/__tests__/download.spec.ts @@ -104,6 +104,16 @@ describe("managed binary downloads", () => { await expect(verification).rejects.toThrow("checksum mismatch: actual-checksum") }) + it("accepts a matching SHA-256 checksum", async () => { + const input = new EventEmitter() + mockCreateReadStream.mockReturnValue(input as ReturnType) + const verification = verifySha256Checksum("/tmp/archive", "actual-checksum", () => new Error("should not fail")) + input.emit("data", Buffer.from("archive")) + input.emit("end") + + await expect(verification).resolves.toBeUndefined() + }) + it("follows a trusted redirect and applies destination security options", async () => { const requestOne = createRequest() const requestTwo = createRequest() diff --git a/src/services/managed-binary/__tests__/install.spec.ts b/src/services/managed-binary/__tests__/install.spec.ts index 6650cc9718..bf903264f8 100644 --- a/src/services/managed-binary/__tests__/install.spec.ts +++ b/src/services/managed-binary/__tests__/install.spec.ts @@ -68,7 +68,10 @@ describe("managed binary installation", () => { expect(first).toBe(second) await vi.waitFor(() => expect(download).toHaveBeenCalledOnce()) finishDownload?.() - await Promise.all([first, second]) + await expect(Promise.all([first, second])).resolves.toEqual([ + getManagedBinaryPaths(options).binaryPath, + getManagedBinaryPaths(options).binaryPath, + ]) expect(download).toHaveBeenCalledOnce() }) diff --git a/src/services/managed-binary/archive.ts b/src/services/managed-binary/archive.ts index 868118c0f7..6963609922 100644 --- a/src/services/managed-binary/archive.ts +++ b/src/services/managed-binary/archive.ts @@ -70,6 +70,7 @@ export async function extractSingleFileZipArchive( expectedFile: string, archiveName: string, ): Promise { + // This deliberately uses PowerShell because it is only called for Windows release archives. const script = [ "$ErrorActionPreference = 'Stop'", "$archivePath = $args[0]", @@ -103,15 +104,21 @@ export async function extractSingleFileTarXzArchive( expectedFile: string, archiveName: string, ): Promise { - const listing = await runProcess("tar", ["-tJf", archivePath]) + const listing = await runProcess("tar", ["-tvJf", archivePath]) const entries = listing.stdout .split(/\r?\n/) .map((entry) => entry.trim()) .filter(Boolean) const archiveEntry = entries[0] - if (entries.length !== 1 || archiveEntry.replace(/^\.\//, "") !== expectedFile) { + const entryName = archiveEntry?.split(/\s+/).at(-1) + if (entries.length !== 1 || !archiveEntry.startsWith("-") || entryName?.replace(/^\.\//, "") !== expectedFile) { throw new Error(`${archiveName} archive has an unexpected layout`) } - await runProcess("tar", ["-xJf", archivePath, "-C", destination, archiveEntry]) + const args = ["-xJf", archivePath, "-C", destination, "--no-same-owner"] + if (process.platform === "linux") { + args.push("--no-overwrite-dir") + } + args.push(entryName) + await runProcess("tar", args) } diff --git a/src/services/managed-binary/download.ts b/src/services/managed-binary/download.ts index 567cfa91d3..f958460068 100644 --- a/src/services/managed-binary/download.ts +++ b/src/services/managed-binary/download.ts @@ -125,27 +125,30 @@ function downloadBinaryFileWithRedirects( destination, options.exclusiveDestination ? { flags: "wx", mode: 0o600 } : undefined, ) + const abort = (error: Error) => { + response.unpipe(output) + output.destroy() + response.destroy() + request.destroy() + reject(error) + } response.on("data", (chunk: Buffer) => { received += chunk.length if (options.maxBytes !== undefined) { try { assertSizeWithinLimit(received, options.maxBytes, options.name) } catch (error) { - response.unpipe(output) - output.destroy() - response.destroy() - request.destroy() - reject(error) + abort(error instanceof Error ? error : new Error(String(error))) } } }) - response.on("error", reject) + response.on("error", abort) response.pipe(output) output.on("finish", () => { output.close() resolve() }) - output.on("error", reject) + output.on("error", abort) }) request.setTimeout(options.timeoutMs, () => request.destroy(new Error(`${options.name} download timed out`))) diff --git a/src/services/managed-binary/install.ts b/src/services/managed-binary/install.ts index 77143a58ea..4a6a548b0b 100644 --- a/src/services/managed-binary/install.ts +++ b/src/services/managed-binary/install.ts @@ -124,6 +124,9 @@ async function installManagedBinaryWithLock(options: ManagedBinaryInstallOptions stale: 5 * 60_000, update: 30_000, retries: { retries: 10, factor: 1.5, minTimeout: 100, maxTimeout: 1_000 }, + onCompromised: (error) => { + throw error + }, }) try { return await installManagedBinary(options) From 09314e3dc76c36e45c4ad307b26e9f299aeac809 Mon Sep 17 00:00:00 2001 From: Naved Merchant <14171946+navedmerchant@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:45:00 +0000 Subject: [PATCH 07/10] test: cover managed binary cleanup boundaries --- .../__tests__/semble-downloader.spec.ts | 2 +- .../managed-binary/__tests__/archive.spec.ts | 29 +++++++++++++------ src/services/managed-binary/archive.ts | 5 +++- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts index 7ef0df9184..ea0f089ba6 100644 --- a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts +++ b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts @@ -925,7 +925,7 @@ describe("semble-downloader", () => { // excluded by the currentArchivePath guard). It is unlinked only by // the pre-download partial-archive cleanup and the post-install // archive cleanup steps. unrelated.txt is never touched. - expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated.txt")) + expect(fs.rm).not.toHaveBeenCalledWith(path.join("/storage", "unrelated.txt"), expect.anything()) // Sanity: the current archive path is never passed to the stale sweep. // It is unlinked exactly twice (pre-download cleanup + post-install // archive cleanup), never via cleanupStaleArchives. diff --git a/src/services/managed-binary/__tests__/archive.spec.ts b/src/services/managed-binary/__tests__/archive.spec.ts index 9095463ca5..cc06e85fd4 100644 --- a/src/services/managed-binary/__tests__/archive.spec.ts +++ b/src/services/managed-binary/__tests__/archive.spec.ts @@ -157,14 +157,25 @@ describe("managed binary archive utilities", () => { it("builds a single-entry-validated PowerShell ZIP extraction", async () => { const child = createChild() mockSpawn.mockReturnValue(child as unknown as ReturnType) - const extraction = extractSingleFileZipArchive("C:\\archive.zip", "C:\\output", "binary.exe", "Tool") - child.emit("close", 0) - await extraction - - const args = mockSpawn.mock.calls[0][1] - const script = args[3] - expect(script).toContain("$entries.Count -ne 1") - expect(script).not.toContain("C:\\archive.zip") - expect(args.slice(4)).toEqual(["C:\\archive.zip", path.join("C:\\output", "binary.exe"), "binary.exe", "Tool"]) + const originalPlatform = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { value: "win32", configurable: true }) + try { + const extraction = extractSingleFileZipArchive("C:\\archive.zip", "C:\\output", "binary.exe", "Tool") + child.emit("close", 0) + await extraction + + const args = mockSpawn.mock.calls[0][1] + const script = args[3] + expect(script).toContain("$entries.Count -ne 1") + expect(script).not.toContain("C:\\archive.zip") + expect(args.slice(4)).toEqual([ + "C:\\archive.zip", + path.join("C:\\output", "binary.exe"), + "binary.exe", + "Tool", + ]) + } finally { + if (originalPlatform) Object.defineProperty(process, "platform", originalPlatform) + } }) }) diff --git a/src/services/managed-binary/archive.ts b/src/services/managed-binary/archive.ts index 6963609922..a3076b3777 100644 --- a/src/services/managed-binary/archive.ts +++ b/src/services/managed-binary/archive.ts @@ -70,7 +70,10 @@ export async function extractSingleFileZipArchive( expectedFile: string, archiveName: string, ): Promise { - // This deliberately uses PowerShell because it is only called for Windows release archives. + if (process.platform !== "win32") { + throw new Error("Single-file ZIP extraction is only supported on Windows") + } + const script = [ "$ErrorActionPreference = 'Stop'", "$archivePath = $args[0]", From 5f3854a568a7e10d6f3bb06dc4fa1d679bd8246e Mon Sep 17 00:00:00 2001 From: Naved Merchant <14171946+navedmerchant@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:46:19 +0000 Subject: [PATCH 08/10] fix: address DCG binary service feedback --- .../__tests__/manager.spec.ts | 106 ++++++++++-------- .../__tests__/runner.spec.ts | 6 +- .../destructive-command-guard/runner.ts | 2 +- 3 files changed, 64 insertions(+), 50 deletions(-) diff --git a/src/services/destructive-command-guard/__tests__/manager.spec.ts b/src/services/destructive-command-guard/__tests__/manager.spec.ts index b49ef8aebe..15bed8b511 100644 --- a/src/services/destructive-command-guard/__tests__/manager.spec.ts +++ b/src/services/destructive-command-guard/__tests__/manager.spec.ts @@ -130,6 +130,13 @@ describe("Destructive Command Guard manager", () => { it("allows trusted relative redirects and rejects unsafe or exhausted redirects", () => { expect(resolveTrustedRedirect("https://github.com/release", "/asset", 5)).toBe("https://github.com/asset") + expect( + resolveTrustedRedirect( + "https://github.com/release", + "https://release-assets.githubusercontent.com/asset", + 5, + ), + ).toBe("https://release-assets.githubusercontent.com/asset") expect(() => resolveTrustedRedirect("https://github.com/release", "https://example.com/asset", 5)).toThrow( "DCG download redirected to an untrusted host", ) @@ -322,56 +329,59 @@ describe("Destructive Command Guard manager", () => { } }) - it("downloads, verifies, extracts, and installs a ZIP archive", async () => { - const info = getDcgArchiveInfo() - if (!info?.archive.endsWith(".zip")) return - - const archive = Buffer.from("test ZIP archive") - const originalChecksum = info.sha256 - Object.defineProperty(info, "sha256", { - value: createHash("sha256").update(archive).digest("hex"), - configurable: true, - }) - const response = Object.assign(new PassThrough(), { - statusCode: 200, - headers: { "content-length": String(archive.length) }, - }) - const request = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) - mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => { - const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback - setImmediate(() => { - callback?.(response as unknown as IncomingMessage) - response.end(archive) + it.skipIf(!getDcgArchiveInfo()?.archive.endsWith(".zip"))( + "downloads, verifies, extracts, and installs a ZIP archive", + async () => { + const info = getDcgArchiveInfo() + if (!info) throw new Error("Expected a ZIP archive in this test") + + const archive = Buffer.from("test ZIP archive") + const originalChecksum = info.sha256 + Object.defineProperty(info, "sha256", { + value: createHash("sha256").update(archive).digest("hex"), + configurable: true, }) - return request as unknown as ReturnType - }) - mockSpawn.mockImplementation((_executable, args) => { - const child = Object.assign(new EventEmitter(), { - stdout: new PassThrough(), - stderr: new PassThrough(), - kill: vi.fn(), + const response = Object.assign(new PassThrough(), { + statusCode: 200, + headers: { "content-length": String(archive.length) }, }) - setImmediate(async () => { - const destinationIndex = args.indexOf( - "$archivePath = $args[0]; $destination = $args[1]; Expand-Archive -LiteralPath $archivePath -DestinationPath $destination -Force", - ) - const stagingDir = args[destinationIndex + 2] - await writeFile(path.join(stagingDir, info.binary), "ZIP executable") - child.emit("close", 0) + const request = Object.assign(new EventEmitter(), { setTimeout: vi.fn(), destroy: vi.fn() }) + mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => { + const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback + setImmediate(() => { + callback?.(response as unknown as IncomingMessage) + response.end(archive) + }) + return request as unknown as ReturnType + }) + mockSpawn.mockImplementation((_executable, args) => { + const child = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + }) + setImmediate(async () => { + const destinationIndex = args.indexOf( + "$archivePath = $args[0]; $destination = $args[1]; Expand-Archive -LiteralPath $archivePath -DestinationPath $destination -Force", + ) + const stagingDir = args[destinationIndex + 2] + await writeFile(path.join(stagingDir, info.binary), "ZIP executable") + child.emit("close", 0) + }) + return child as unknown as ReturnType }) - return child as unknown as ReturnType - }) - try { - const binaryPath = await ensureDcgInstalled(tempDir) - if (!binaryPath) throw new Error("Expected DCG to be supported in this test") - expect(await readFile(binaryPath, "utf8")).toBe("ZIP executable") - expect(await readFile(path.join(tempDir, "destructive-command-guard", ".dcg-version"), "utf8")).toBe( - DCG_VERSION, - ) - await expect(access(path.join(tempDir, `${DCG_VERSION}-${info.archive}`))).rejects.toThrow() - } finally { - Object.defineProperty(info, "sha256", { value: originalChecksum, configurable: true }) - } - }) + try { + const binaryPath = await ensureDcgInstalled(tempDir) + if (!binaryPath) throw new Error("Expected DCG to be supported in this test") + expect(await readFile(binaryPath, "utf8")).toBe("ZIP executable") + expect(await readFile(path.join(tempDir, "destructive-command-guard", ".dcg-version"), "utf8")).toBe( + DCG_VERSION, + ) + await expect(access(path.join(tempDir, `${DCG_VERSION}-${info.archive}`))).rejects.toThrow() + } finally { + Object.defineProperty(info, "sha256", { value: originalChecksum, configurable: true }) + } + }, + ) }) diff --git a/src/services/destructive-command-guard/__tests__/runner.spec.ts b/src/services/destructive-command-guard/__tests__/runner.spec.ts index d738de9b7a..3dcfd805e2 100644 --- a/src/services/destructive-command-guard/__tests__/runner.spec.ts +++ b/src/services/destructive-command-guard/__tests__/runner.spec.ts @@ -83,7 +83,9 @@ describe("runDcg", () => { const child = createChild() useChild(child) const originalToken = process.env.GITHUB_TOKEN + const originalTmpdir = process.env.TMPDIR process.env.GITHUB_TOKEN = "secret" + process.env.TMPDIR = "/sandbox/tmp" try { const result = runDcg("/dcg", "echo test", "/workspace") @@ -91,11 +93,13 @@ describe("runDcg", () => { await result const options = mockSpawn.mock.calls[0][2] - expect(options?.env).toMatchObject({ NO_COLOR: "1" }) + expect(options?.env).toMatchObject({ NO_COLOR: "1", TMPDIR: "/sandbox/tmp" }) expect(options?.env).not.toHaveProperty("GITHUB_TOKEN") } finally { if (originalToken === undefined) delete process.env.GITHUB_TOKEN else process.env.GITHUB_TOKEN = originalToken + if (originalTmpdir === undefined) delete process.env.TMPDIR + else process.env.TMPDIR = originalTmpdir } }) diff --git a/src/services/destructive-command-guard/runner.ts b/src/services/destructive-command-guard/runner.ts index 83d5b95c7b..3e3b3cab45 100644 --- a/src/services/destructive-command-guard/runner.ts +++ b/src/services/destructive-command-guard/runner.ts @@ -13,7 +13,7 @@ type DcgJsonOutput = { pack_id?: string } -const DCG_ENV_KEYS = ["HOME", "PATH", "TEMP", "TMP", "USERPROFILE", "SystemRoot", "WINDIR"] as const +const DCG_ENV_KEYS = ["HOME", "PATH", "TEMP", "TMPDIR", "TMP", "USERPROFILE", "SystemRoot", "WINDIR"] as const function getDcgEnvironment(): NodeJS.ProcessEnv { const env: NodeJS.ProcessEnv = { NO_COLOR: "1" } From 1f1af77872b8a89c0744a0bf626186e63bb57779 Mon Sep 17 00:00:00 2001 From: Naved Merchant <14171946+navedmerchant@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:58:43 +0000 Subject: [PATCH 09/10] fix: finalize managed binary download handling --- src/services/managed-binary/__tests__/download.spec.ts | 7 +++++++ src/services/managed-binary/archive.ts | 10 +++++++++- src/services/managed-binary/download.ts | 6 ++---- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/services/managed-binary/__tests__/download.spec.ts b/src/services/managed-binary/__tests__/download.spec.ts index a7639068f8..32543ce8ed 100644 --- a/src/services/managed-binary/__tests__/download.spec.ts +++ b/src/services/managed-binary/__tests__/download.spec.ts @@ -149,7 +149,14 @@ describe("managed binary downloads", () => { }) await new Promise((resolve) => setImmediate(resolve)) await new Promise((resolve) => setImmediate(resolve)) + let resolved = false + void download.then(() => { + resolved = true + }) output.emit("finish") + await Promise.resolve() + expect(resolved).toBe(false) + output.emit("close") await download expect(mockGet).toHaveBeenNthCalledWith(2, "https://github.com/asset", expect.any(Function)) diff --git a/src/services/managed-binary/archive.ts b/src/services/managed-binary/archive.ts index a3076b3777..674fe93ebb 100644 --- a/src/services/managed-binary/archive.ts +++ b/src/services/managed-binary/archive.ts @@ -112,9 +112,17 @@ export async function extractSingleFileTarXzArchive( .split(/\r?\n/) .map((entry) => entry.trim()) .filter(Boolean) + if (entries.length !== 1) { + throw new Error(`${archiveName} archive has an unexpected layout`) + } const archiveEntry = entries[0] const entryName = archiveEntry?.split(/\s+/).at(-1) - if (entries.length !== 1 || !archiveEntry.startsWith("-") || entryName?.replace(/^\.\//, "") !== expectedFile) { + if ( + !archiveEntry || + !entryName || + !archiveEntry.startsWith("-") || + entryName.replace(/^\.\//, "") !== expectedFile + ) { throw new Error(`${archiveName} archive has an unexpected layout`) } diff --git a/src/services/managed-binary/download.ts b/src/services/managed-binary/download.ts index f958460068..78c21da1ca 100644 --- a/src/services/managed-binary/download.ts +++ b/src/services/managed-binary/download.ts @@ -144,10 +144,8 @@ function downloadBinaryFileWithRedirects( }) response.on("error", abort) response.pipe(output) - output.on("finish", () => { - output.close() - resolve() - }) + output.on("finish", () => output.close()) + output.on("close", resolve) output.on("error", abort) }) From ae26434cdc39d3204c3ac5a1f09b7c776d72ae23 Mon Sep 17 00:00:00 2001 From: Naved Merchant <14171946+navedmerchant@users.noreply.github.com> Date: Sun, 2 Aug 2026 20:07:41 +0000 Subject: [PATCH 10/10] test: mirror download stream close events --- .../__tests__/semble-downloader.spec.ts | 71 +++---------------- 1 file changed, 8 insertions(+), 63 deletions(-) diff --git a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts index ea0f089ba6..7cf7599c89 100644 --- a/src/services/code-index/semble/__tests__/semble-downloader.spec.ts +++ b/src/services/code-index/semble/__tests__/semble-downloader.spec.ts @@ -37,10 +37,15 @@ vi.mock("proper-lockfile", () => ({ })) // Mock fs (createWriteStream and createReadStream for checksum verification) +let closeHandler: (() => void) | undefined const mockWriteStream = { on: vi.fn(), close: vi.fn(), } +const onWriteStreamEvent = (event: string, callback: () => void) => { + if (event === "finish") setImmediate(callback) + if (event === "close") closeHandler = callback +} vi.mock("fs", () => ({ createWriteStream: vi.fn(() => mockWriteStream), createReadStream: vi.fn(() => { @@ -107,8 +112,9 @@ describe("SEMBLE_SHA256 checksum fixture", () => { describe("semble-downloader", () => { beforeEach(() => { vi.clearAllMocks() - mockWriteStream.on = vi.fn() - mockWriteStream.close = vi.fn() + closeHandler = undefined + mockWriteStream.on = vi.fn(onWriteStreamEvent) + mockWriteStream.close = vi.fn(() => closeHandler?.()) // Restore the default https.get mock so tests that override it don't leak ;(https.get as any).mockImplementation((_url: string, callback: (res: any) => void) => { @@ -227,13 +233,6 @@ describe("semble-downloader", () => { // No version file exists ;(fs.readFile as any).mockRejectedValue(new Error("ENOENT")) - // Simulate successful download: pipe is called, then "finish" fires - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -382,12 +381,6 @@ describe("semble-downloader", () => { }) // Simulate successful download on the second response - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -550,12 +543,6 @@ describe("semble-downloader", () => { ;(fs.readFile as any).mockRejectedValue(new Error("ENOENT")) // Simulate successful download - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -590,12 +577,6 @@ describe("semble-downloader", () => { ;(fs.readFile as any).mockRejectedValue(new Error("ENOENT")) // Simulate successful download - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - // Archive cleanup fails but should not throw (only archive removal after extraction) ;(fs.rm as any).mockRejectedValueOnce(new Error("archive cleanup failed")) @@ -625,12 +606,6 @@ describe("semble-downloader", () => { ;(fs.access as any).mockResolvedValue(undefined) // Simulate successful download - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -681,12 +656,6 @@ describe("semble-downloader", () => { ;(fs.access as any).mockResolvedValue(undefined) // Simulate successful download - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -781,12 +750,6 @@ describe("semble-downloader", () => { }) // Simulate successful download - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -824,12 +787,6 @@ describe("semble-downloader", () => { ;(fs.access as any).mockResolvedValue(undefined) // Simulate successful download - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -868,12 +825,6 @@ describe("semble-downloader", () => { // readdir rejects — exercises the catch block in cleanupStaleArchives ;(fs.readdir as any).mockRejectedValue(new Error("EACCES")) - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { const result = await downloadSemble("/storage") @@ -904,12 +855,6 @@ describe("semble-downloader", () => { "unrelated.txt", ]) - mockWriteStream.on.mockImplementation((event: string, cb: () => void) => { - if (event === "finish") { - setImmediate(cb) - } - }) - try { await downloadSemble("/storage")