Skip to content

Commit 54b061c

Browse files
committed
fix: harden managed install download and cleanup
Three robustness issues caught during session review: 1. HTTP download timeout: added 30-second timeout to https.get and a timeout event handler so the extension does not hang forever on a stalled connection. 2. Redirect depth limit: capped redirect following at 5 hops to prevent stack overflow on infinite redirect loops. 3. Staging cleanup on failure: the catch block in performManagedInstall now calls clearManagedInstallStaging so partially downloaded files do not accumulate across failed install attempts. Also switched checksum verification from readFile (loads entire archive into memory) to streaming SHA-256 via createReadStream + pipeline. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
1 parent d272511 commit 54b061c

1 file changed

Lines changed: 38 additions & 7 deletions

File tree

src/install/managed.ts

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { execFile } from "node:child_process";
22
import { createHash } from "node:crypto";
3-
import { createWriteStream } from "node:fs";
3+
import { createReadStream, createWriteStream } from "node:fs";
44
import * as https from "node:https";
55
import * as path from "node:path";
6+
import { pipeline } from "node:stream/promises";
67
import { promisify } from "node:util";
78
import { formatError } from "../util.js";
89

@@ -554,14 +555,16 @@ export async function performManagedInstall(inputs: PerformManagedInstallInputs)
554555
const extractArchive = inputs.extractArchive ?? extractManagedInstallArchive;
555556
const readContent = inputs.readFileContent ?? defaultReadFileContent;
556557

558+
let txPaths: ManagedInstallTransactionPaths | undefined;
559+
557560
try {
558561
report("fetching-version");
559562
const version = inputs.version
560563
? normalizeReleaseVersion(inputs.version)
561564
: await fetchVersion({ repo: inputs.repo });
562565

563566
const assets = buildManagedInstallReleaseAssets(version, target, inputs.repo);
564-
const txPaths = resolveManagedInstallTransactionPaths(inputs.installRoot, version, target);
567+
txPaths = resolveManagedInstallTransactionPaths(inputs.installRoot, version, target);
565568

566569
assertTrustedManagedInstallDownloadUrl(assets.archiveDownloadUrl, inputs.repo);
567570
assertTrustedManagedInstallDownloadUrl(assets.checksumDownloadUrl, inputs.repo);
@@ -580,8 +583,14 @@ export async function performManagedInstall(inputs: PerformManagedInstallInputs)
580583

581584
report("verifying");
582585
const checksumContent = await readContent(txPaths.stagedChecksumPath);
583-
const archiveContent = await (await import("node:fs/promises")).readFile(txPaths.stagedArchivePath);
584-
verifyManagedInstallArchiveChecksum(archiveContent, checksumContent, assets.archiveFileName);
586+
const expectedSha256 = resolveManagedInstallChecksum(checksumContent, assets.archiveFileName);
587+
const actualSha256 = await streamingSha256(txPaths.stagedArchivePath);
588+
if (expectedSha256 !== actualSha256) {
589+
throw new ManagedInstallVerificationError(
590+
"checksum-mismatch",
591+
`Checksum mismatch for ${assets.archiveFileName}. Expected ${expectedSha256}, got ${actualSha256}.`
592+
);
593+
}
585594

586595
report("extracting");
587596
await extractArchive({
@@ -610,6 +619,13 @@ export async function performManagedInstall(inputs: PerformManagedInstallInputs)
610619
failure,
611620
inputs.failurePersistence ?? { storageRoot: inputs.installRoot }
612621
);
622+
if (txPaths) {
623+
try {
624+
await clearManagedInstallStaging({ paths: txPaths });
625+
} catch {
626+
// Best-effort cleanup; the failure is already persisted
627+
}
628+
}
613629
throw error;
614630
}
615631
}
@@ -719,13 +735,17 @@ async function defaultFetchJson(url: string): Promise<{ tag_name: string }> {
719735
return response.json() as Promise<{ tag_name: string }>;
720736
}
721737

722-
async function defaultDownloadToFile(url: string, destPath: string): Promise<void> {
738+
async function defaultDownloadToFile(url: string, destPath: string, redirectsRemaining = 5): Promise<void> {
723739
return new Promise((resolve, reject) => {
724740
const file = createWriteStream(destPath);
725-
const request = https.get(url, { headers: { "User-Agent": "patchloom-vscode" } }, (response) => {
741+
const request = https.get(url, { headers: { "User-Agent": "patchloom-vscode" }, timeout: 30_000 }, (response) => {
726742
if (response.statusCode && response.statusCode >= 300 && response.statusCode < 400 && response.headers.location) {
727743
file.close();
728-
defaultDownloadToFile(response.headers.location, destPath).then(resolve, reject);
744+
if (redirectsRemaining <= 0) {
745+
reject(new Error(`Download failed: too many redirects for ${url}`));
746+
return;
747+
}
748+
defaultDownloadToFile(response.headers.location, destPath, redirectsRemaining - 1).then(resolve, reject);
729749
return;
730750
}
731751
if (response.statusCode && response.statusCode >= 400) {
@@ -739,6 +759,11 @@ async function defaultDownloadToFile(url: string, destPath: string): Promise<voi
739759
resolve();
740760
});
741761
});
762+
request.on("timeout", () => {
763+
request.destroy();
764+
file.close();
765+
reject(new Error(`Download timed out for ${url}`));
766+
});
742767
request.on("error", (error) => {
743768
file.close();
744769
reject(new Error(`Download failed for ${url}: ${formatError(error)}`));
@@ -757,3 +782,9 @@ async function defaultExecCommand(cmd: string, args: string[], cwd: string): Pro
757782
async function defaultReadFileContent(filePath: string): Promise<string> {
758783
return (await import("node:fs/promises")).readFile(filePath, "utf8");
759784
}
785+
786+
async function streamingSha256(filePath: string): Promise<string> {
787+
const hash = createHash("sha256");
788+
await pipeline(createReadStream(filePath), hash);
789+
return hash.digest("hex");
790+
}

0 commit comments

Comments
 (0)