Skip to content

Commit 24d527e

Browse files
navedmerchantnavedmerchant
committed
fix: address managed binary review feedback
1 parent e93b1cc commit 24d527e

8 files changed

Lines changed: 211 additions & 43 deletions

File tree

‎src/services/code-index/semble/__tests__/semble-downloader.spec.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ vi.mock("fs/promises", () => ({
3232
readdir: vi.fn().mockResolvedValue([]),
3333
}))
3434

35+
vi.mock("proper-lockfile", () => ({
36+
lock: vi.fn().mockResolvedValue(vi.fn().mockResolvedValue(undefined)),
37+
}))
38+
3539
// Mock fs (createWriteStream and createReadStream for checksum verification)
3640
const mockWriteStream = {
3741
on: vi.fn(),
@@ -717,7 +721,7 @@ describe("semble-downloader", () => {
717721
force: true,
718722
})
719723
// Unrelated files in the storage dir must not be touched.
720-
expect(fs.unlink).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt"))
724+
expect(fs.rm).not.toHaveBeenCalledWith(path.join("/storage", "unrelated-file.txt"), expect.anything())
721725
// The new version file is recorded
722726
expect(fs.writeFile).toHaveBeenCalledWith(
723727
path.join("/storage", "semble.new", ".semble-version"),

‎src/services/code-index/semble/semble-downloader.ts‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import * as path from "path"
33

44
import { extractTarGzArchive, extractZipArchive } from "../../managed-binary/archive"
55
import { downloadBinaryFile, verifySha256Checksum } from "../../managed-binary/download"
6-
import { ensureManagedBinaryInstalled, getManagedBinaryPaths } from "../../managed-binary/install"
6+
import { ensureManagedBinaryInstalled } from "../../managed-binary/install"
77

88
/**
99
* Supported platform/arch combinations for the semble standalone executable.
@@ -101,14 +101,6 @@ export async function downloadSemble(storageDir: string): Promise<string | undef
101101
}
102102

103103
const url = `${DOWNLOAD_BASE_URL}/${info.archive}`
104-
const paths = getManagedBinaryPaths({
105-
storageDir,
106-
id: "semble",
107-
version: SEMBLE_VERSION,
108-
versionFile: VERSION_FILE,
109-
archiveName: info.archive,
110-
binaryName: info.binary,
111-
})
112104
console.log(`[SembleDownloader] Downloading semble ${SEMBLE_VERSION} from ${url}`)
113105

114106
const platformKey = `${process.platform}-${process.arch}`
@@ -140,7 +132,7 @@ export async function downloadSemble(storageDir: string): Promise<string | undef
140132
},
141133
})
142134

143-
console.log(`[SembleDownloader] Successfully installed semble ${SEMBLE_VERSION} to ${paths.binaryPath}`)
135+
console.log(`[SembleDownloader] Successfully installed semble ${SEMBLE_VERSION} to ${result}`)
144136
return result
145137
}
146138

‎src/services/managed-binary/__tests__/archive.spec.ts‎

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
import { EventEmitter } from "events"
2+
import * as path from "path"
23
import { PassThrough } from "stream"
34

45
import { spawn } from "child_process"
56

67
import {
7-
escapePowerShellLiteral,
88
extractSingleFileTarXzArchive,
99
extractSingleFileZipArchive,
1010
extractTarGzArchive,
11+
extractTarXzArchive,
12+
extractZipArchive,
1113
runProcess,
1214
} from "../archive"
1315

@@ -40,8 +42,17 @@ describe("managed binary archive utilities", () => {
4042
})
4143
})
4244

43-
it("escapes PowerShell single-quoted literals", () => {
44-
expect(escapePowerShellLiteral("C:\\it's\\archive.zip")).toBe("C:\\it''s\\archive.zip")
45+
it("kills a process that exceeds its timeout", async () => {
46+
vi.useFakeTimers()
47+
const child = createChild()
48+
mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>)
49+
const processResult = runProcess("tool", [], 100)
50+
const assertion = expect(processResult).rejects.toThrow("tool timed out")
51+
52+
await vi.advanceTimersByTimeAsync(100)
53+
await assertion
54+
expect(child.kill).toHaveBeenCalledWith("SIGKILL")
55+
vi.useRealTimers()
4556
})
4657

4758
it("extracts tar.gz archives with hardened flags", async () => {
@@ -58,6 +69,43 @@ describe("managed binary archive utilities", () => {
5869
)
5970
})
6071

72+
it("extracts tar.xz archives with hardened flags", async () => {
73+
const child = createChild()
74+
mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>)
75+
const extraction = extractTarXzArchive("/tmp/archive.tar.xz", "/tmp/output")
76+
child.emit("close", 0)
77+
await extraction
78+
79+
const expectedArgs = ["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "--no-same-owner"]
80+
if (process.platform === "linux") expectedArgs.push("--no-overwrite-dir")
81+
expect(mockSpawn).toHaveBeenCalledWith("tar", expectedArgs, {
82+
shell: false,
83+
stdio: ["ignore", "pipe", "pipe"],
84+
})
85+
})
86+
87+
it("extracts ZIP archives with platform-safe process arguments", async () => {
88+
const child = createChild()
89+
mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>)
90+
const extraction = extractZipArchive("/tmp/archive.zip", "/tmp/output")
91+
child.emit("close", 0)
92+
await extraction
93+
94+
if (process.platform === "win32") {
95+
expect(mockSpawn).toHaveBeenCalledWith(
96+
"powershell",
97+
["-NoProfile", "-NonInteractive", "-Command", expect.any(String), "/tmp/archive.zip", "/tmp/output"],
98+
expect.objectContaining({ shell: false }),
99+
)
100+
} else {
101+
expect(mockSpawn).toHaveBeenCalledWith(
102+
"unzip",
103+
["-o", "/tmp/archive.zip", "-d", "/tmp/output"],
104+
expect.objectContaining({ shell: false }),
105+
)
106+
}
107+
})
108+
61109
it("validates a single-file tar.xz layout before extraction", async () => {
62110
const listing = createChild()
63111
const extraction = createChild()
@@ -73,7 +121,7 @@ describe("managed binary archive utilities", () => {
73121
expect(mockSpawn).toHaveBeenNthCalledWith(
74122
2,
75123
"tar",
76-
["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "binary"],
124+
["-xJf", "/tmp/archive.tar.xz", "-C", "/tmp/output", "./binary"],
77125
expect.any(Object),
78126
)
79127
})
@@ -85,9 +133,10 @@ describe("managed binary archive utilities", () => {
85133
child.emit("close", 0)
86134
await extraction
87135

88-
const script = mockSpawn.mock.calls[0][1][3]
136+
const args = mockSpawn.mock.calls[0][1]
137+
const script = args[3]
89138
expect(script).toContain("$entries.Count -ne 1")
90-
expect(script).toContain("binary.exe")
91-
expect(script).toContain("Tool archive has an unexpected layout")
139+
expect(script).not.toContain("C:\\archive.zip")
140+
expect(args.slice(4)).toEqual(["C:\\archive.zip", path.join("C:\\output", "binary.exe"), "binary.exe", "Tool"])
92141
})
93142
})

‎src/services/managed-binary/__tests__/download.spec.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ function createResponse(statusCode: number, headers: Record<string, string> = {}
4040
headers,
4141
destroy: vi.fn(),
4242
pipe: vi.fn(),
43+
unpipe: vi.fn(),
4344
})
4445
}
4546

@@ -67,6 +68,20 @@ describe("managed binary downloads", () => {
6768
expect(() => resolveTrustedRedirect("https://github.com/release", "/asset", 0, options)).toThrow(
6869
"Too many Example download redirects",
6970
)
71+
expect(() => resolveTrustedRedirect("https://github.com/release", undefined, 5, options)).toThrow(
72+
"Example download redirect is missing a Location header",
73+
)
74+
})
75+
76+
it("distinguishes an untrusted initial URL from an unsafe redirect", async () => {
77+
await expect(
78+
downloadBinaryFile("http://github.com/release", "/tmp/archive", {
79+
name: "Example",
80+
trustedDomains,
81+
timeoutMs: 1_000,
82+
}),
83+
).rejects.toThrow("Example download URL is not a trusted HTTPS host")
84+
expect(mockGet).not.toHaveBeenCalled()
7085
})
7186

7287
it("enforces configurable archive size limits", () => {
@@ -158,4 +173,31 @@ describe("managed binary downloads", () => {
158173
).rejects.toThrow("Example archive exceeds the download size limit")
159174
expect(mockCreateWriteStream).not.toHaveBeenCalled()
160175
})
176+
177+
it("unpipes and destroys the destination when streamed bytes exceed the limit", async () => {
178+
const request = createRequest()
179+
const response = createResponse(200)
180+
const output = Object.assign(new EventEmitter(), { close: vi.fn(), destroy: vi.fn() })
181+
mockCreateWriteStream.mockReturnValue(output as unknown as ReturnType<typeof createWriteStream>)
182+
mockGet.mockImplementation((_url, optionsOrCallback, optionalCallback) => {
183+
const callback = typeof optionsOrCallback === "function" ? optionsOrCallback : optionalCallback
184+
setImmediate(() => callback?.(response as unknown as IncomingMessage))
185+
return request as unknown as ReturnType<typeof get>
186+
})
187+
188+
const download = downloadBinaryFile("https://github.com/release", "/tmp/archive", {
189+
name: "Example",
190+
trustedDomains,
191+
timeoutMs: 1_000,
192+
maxBytes: 10,
193+
})
194+
await new Promise<void>((resolve) => setImmediate(resolve))
195+
response.emit("data", Buffer.alloc(11))
196+
197+
await expect(download).rejects.toThrow("Example archive exceeds the download size limit")
198+
expect(response.unpipe).toHaveBeenCalledWith(output)
199+
expect(output.destroy).toHaveBeenCalledOnce()
200+
expect(response.destroy).toHaveBeenCalledOnce()
201+
expect(request.destroy).toHaveBeenCalledOnce()
202+
})
161203
})

‎src/services/managed-binary/__tests__/install.spec.ts‎

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ describe("managed binary installation", () => {
2323
versionFile: ".example-version",
2424
archiveName: "example.tar.gz",
2525
binaryName: "example",
26+
errorPrefix: "Failed to install example",
2627
download: vi.fn(),
2728
verifyArchive: vi.fn(),
2829
extractArchive: vi.fn(),
@@ -53,9 +54,22 @@ describe("managed binary installation", () => {
5354
expect(options.download).not.toHaveBeenCalled()
5455
})
5556

56-
it("deduplicates concurrent installations", () => {
57-
const options = createOptions({ download: () => new Promise<void>(() => {}) })
58-
expect(ensureManagedBinaryInstalled(options)).toBe(ensureManagedBinaryInstalled(options))
57+
it("deduplicates concurrent installations", async () => {
58+
let finishDownload: (() => void) | undefined
59+
const download = vi.fn(() => new Promise<void>((resolve) => (finishDownload = resolve)))
60+
const options = createOptions({
61+
download,
62+
extractArchive: async (_archivePath, stagingDir) => {
63+
await writeFile(path.join(stagingDir, "example"), "binary")
64+
},
65+
})
66+
const first = ensureManagedBinaryInstalled(options)
67+
const second = ensureManagedBinaryInstalled(options)
68+
expect(first).toBe(second)
69+
await vi.waitFor(() => expect(download).toHaveBeenCalledOnce())
70+
finishDownload?.()
71+
await Promise.all([first, second])
72+
expect(download).toHaveBeenCalledOnce()
5973
})
6074

6175
it("coordinates update, metadata promotion, and cleanup", async () => {
@@ -84,5 +98,41 @@ describe("managed binary installation", () => {
8498
expect(await readFile(paths.versionPath, "utf8")).toBe(options.version)
8599
await expect(access(paths.archivePath)).rejects.toThrow()
86100
await expect(access(paths.stagingDir)).rejects.toThrow()
101+
await expect(access(path.join(tempDir, ".example.install.lock"))).rejects.toThrow()
102+
})
103+
104+
it("cleans up partial artifacts when downloading fails", async () => {
105+
const options = createOptions({
106+
download: async (archivePath) => {
107+
await writeFile(archivePath, "partial")
108+
throw new Error("network failure")
109+
},
110+
})
111+
const paths = getManagedBinaryPaths(options)
112+
113+
await expect(ensureManagedBinaryInstalled(options)).rejects.toThrow(
114+
"Failed to install example: network failure",
115+
)
116+
await expect(access(paths.archivePath)).rejects.toThrow()
117+
await expect(access(paths.stagingDir)).rejects.toThrow()
118+
await expect(access(paths.binaryPath)).rejects.toThrow()
119+
})
120+
121+
it("removes stale versioned archives without touching unrelated files", async () => {
122+
const options = createOptions({
123+
download: async (archivePath) => writeFile(archivePath, "archive"),
124+
extractArchive: async (_archivePath, stagingDir) => {
125+
await writeFile(path.join(stagingDir, "example"), "binary")
126+
},
127+
})
128+
const staleArchive = path.join(tempDir, "v1.2.2-example.tar.gz")
129+
const unrelated = path.join(tempDir, "notes.txt")
130+
await writeFile(staleArchive, "stale")
131+
await writeFile(unrelated, "keep")
132+
133+
await ensureManagedBinaryInstalled(options)
134+
135+
await expect(access(staleArchive)).rejects.toThrow()
136+
await expect(readFile(unrelated, "utf8")).resolves.toBe("keep")
87137
})
88138
})

‎src/services/managed-binary/archive.ts‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,6 @@ export function runProcess(executable: string, args: string[], timeoutMs = 30_00
3232
})
3333
}
3434

35-
export function escapePowerShellLiteral(value: string): string {
36-
return value.replace(/'/g, "''")
37-
}
38-
3935
export async function extractTarGzArchive(archivePath: string, destination: string): Promise<void> {
4036
const args = ["-xzf", archivePath, "-C", destination, "--no-same-owner"]
4137
if (process.platform === "linux") {
@@ -56,8 +52,11 @@ export async function extractZipArchive(archivePath: string, destination: string
5652
if (process.platform === "win32") {
5753
await runProcess("powershell", [
5854
"-NoProfile",
55+
"-NonInteractive",
5956
"-Command",
60-
`Expand-Archive -Path '${escapePowerShellLiteral(archivePath)}' -DestinationPath '${escapePowerShellLiteral(destination)}' -Force`,
57+
"$archivePath = $args[0]; $destination = $args[1]; Expand-Archive -LiteralPath $archivePath -DestinationPath $destination -Force",
58+
archivePath,
59+
destination,
6160
])
6261
return
6362
}
@@ -71,19 +70,31 @@ export async function extractSingleFileZipArchive(
7170
expectedFile: string,
7271
archiveName: string,
7372
): Promise<void> {
74-
const outputPath = path.join(destination, expectedFile)
7573
const script = [
7674
"$ErrorActionPreference = 'Stop'",
75+
"$archivePath = $args[0]",
76+
"$outputPath = $args[1]",
77+
"$expectedFile = $args[2]",
78+
"$archiveName = $args[3]",
7779
"Add-Type -AssemblyName System.IO.Compression.FileSystem",
78-
`$archive = [System.IO.Compression.ZipFile]::OpenRead('${escapePowerShellLiteral(archivePath)}')`,
80+
"$archive = [System.IO.Compression.ZipFile]::OpenRead($archivePath)",
7981
"try {",
8082
" $entries = @($archive.Entries | Where-Object { -not [string]::IsNullOrEmpty($_.Name) })",
81-
` if ($entries.Count -ne 1 -or $entries[0].FullName -ne '${escapePowerShellLiteral(expectedFile)}') { throw '${escapePowerShellLiteral(archiveName)} archive has an unexpected layout' }`,
82-
` [System.IO.Compression.ZipFileExtensions]::ExtractToFile($entries[0], '${escapePowerShellLiteral(outputPath)}', $false)`,
83+
' if ($entries.Count -ne 1 -or $entries[0].FullName -ne $expectedFile) { throw "$archiveName archive has an unexpected layout" }',
84+
" [System.IO.Compression.ZipFileExtensions]::ExtractToFile($entries[0], $outputPath, $false)",
8385
"} finally { $archive.Dispose() }",
8486
].join("; ")
8587

86-
await runProcess("powershell", ["-NoProfile", "-NonInteractive", "-Command", script])
88+
await runProcess("powershell", [
89+
"-NoProfile",
90+
"-NonInteractive",
91+
"-Command",
92+
script,
93+
archivePath,
94+
path.join(destination, expectedFile),
95+
expectedFile,
96+
archiveName,
97+
])
8798
}
8899

89100
export async function extractSingleFileTarXzArchive(
@@ -95,11 +106,12 @@ export async function extractSingleFileTarXzArchive(
95106
const listing = await runProcess("tar", ["-tJf", archivePath])
96107
const entries = listing.stdout
97108
.split(/\r?\n/)
98-
.map((entry) => entry.trim().replace(/^\.\//, ""))
109+
.map((entry) => entry.trim())
99110
.filter(Boolean)
100-
if (entries.length !== 1 || entries[0] !== expectedFile) {
111+
const archiveEntry = entries[0]
112+
if (entries.length !== 1 || archiveEntry.replace(/^\.\//, "") !== expectedFile) {
101113
throw new Error(`${archiveName} archive has an unexpected layout`)
102114
}
103115

104-
await runProcess("tar", ["-xJf", archivePath, "-C", destination, expectedFile])
116+
await runProcess("tar", ["-xJf", archivePath, "-C", destination, archiveEntry])
105117
}

0 commit comments

Comments
 (0)