fix: address DCG service review feedback

navedmerchant · navedmerchant · commit bae4ff7fb170 · 2026-08-01T17:54:52.000-07:00
diff --git 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<typeof get>
+		})
+
+		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<typeof get>)
+
+		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<typeof get>)
+
+		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<typeof get>
+		})
+
+		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<typeof get>
+		})
+		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<typeof spawn>
+		})
+
+		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
@@ -35,12 +35,18 @@ function emitResult(child: MockChild, payload: unknown, code: number): void {
 }
 
 describe("runDcg", () => {
+	let warnSpy: ReturnType<typeof vi.spyOn>
+
 	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
@@ -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<void> {
-	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<string | undefined> {
 	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
@@ -13,60 +13,77 @@ 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<DcgDecision> {
 	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<ArrayBufferLike> = Buffer.alloc(0)
 		let stderr: Buffer<ArrayBufferLike> = 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<ArrayBufferLike>, chunk: Buffer<ArrayBufferLike>): Buffer<ArrayBufferLike> => {
+		const appendOutputOrFail = (
+			current: Buffer<ArrayBufferLike>,
+			chunk: Buffer<ArrayBufferLike>,
+		): Buffer<ArrayBufferLike> => {
 			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.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
 			}
 
 			let payload: DcgJsonOutput
 			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)
 			}
 		})
 	})
