From 6db383b8ab75f697b4c1a9bcdcd5510fa5b0f171 Mon Sep 17 00:00:00 2001 From: Mrunal Chaudhari Date: Mon, 29 Jun 2026 22:43:26 +0530 Subject: [PATCH] fix: version cached binary filename to detect stale/mismatched drivers --- src/install.ts | 77 ++++++++++++++++-- tests/unit.test.ts | 199 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 266 insertions(+), 10 deletions(-) diff --git a/src/install.ts b/src/install.ts index 7a5c471..5ea978a 100644 --- a/src/install.ts +++ b/src/install.ts @@ -26,17 +26,35 @@ if (process.env.HTTPS_PROXY) { fetchOpts.agent = new HttpProxyAgent(process.env.HTTP_PROXY) } +// Only allow characters that are safe as a filename segment. +// Rejects path separators (/ \) and any traversal sequences. +const SAFE_VERSION_RE = /^[a-zA-Z0-9._-]+$/ + +export function getBinaryFilename (version: string) { + if (!SAFE_VERSION_RE.test(version)) { + throw new Error(`Invalid geckodriver version string: ${JSON.stringify(version)}`) + } + return `geckodriver-${version}` + (os.platform() === 'win32' ? '.exe' : '') +} + export async function download ( geckodriverVersion: string = process.env.GECKODRIVER_VERSION, cacheDir: string = process.env.GECKODRIVER_CACHE_DIR || os.tmpdir() ) { - const binaryFilePath = path.resolve(cacheDir, BINARY_FILE) - if (await hasAccess(binaryFilePath)) { - return binaryFilePath + /** + * If the version is already known, check the versioned cache path first. + * This is the hot path: zero network requests when the binary is cached. + */ + if (geckodriverVersion) { + const cachedPath = path.resolve(cacheDir, getBinaryFilename(geckodriverVersion)) + if (await hasAccess(cachedPath)) { + return cachedPath + } } /** - * get latest version of Geckodriver + * Version is unknown — fetch the latest release from Cargo.toml, then + * check the versioned cache before hitting the network for the binary. */ if (!geckodriverVersion) { const res = await retryFetch(GECKODRIVER_CARGO_YAML, fetchOpts) @@ -47,8 +65,15 @@ export async function download ( } geckodriverVersion = version.split(' = ').pop().slice(1, -1) log.info(`Detected Geckodriver v${geckodriverVersion} to be latest`) + + // the resolved latest may already be cached + const cachedPath = path.resolve(cacheDir, getBinaryFilename(geckodriverVersion)) + if (await hasAccess(cachedPath)) { + return cachedPath + } } + const binaryFilePath = path.resolve(cacheDir, getBinaryFilename(geckodriverVersion)) const url = getDownloadUrl(geckodriverVersion) log.info(`Downloading Geckodriver from ${url}`) const res = await retryFetch(url, fetchOpts) @@ -58,22 +83,56 @@ export async function download ( } await fsp.mkdir(cacheDir, { recursive: true }) - await (url.endsWith('.zip') - ? downloadZip(res, cacheDir) - : pipeline(res.body, zlib.createGunzip(), unpackTar(cacheDir))) + + // Extract into a unique per-operation staging directory so concurrent + // downloads (even of the same version, e.g. parallel test runners) never + // share intermediate files. The binary is moved into its final versioned + // location once extraction succeeds. + const stagingDir = await fsp.mkdtemp(path.join(cacheDir, 'geckodriver-')) + try { + await (url.endsWith('.zip') + ? downloadZip(res, stagingDir) + : pipeline(res.body, zlib.createGunzip(), unpackTar(stagingDir))) + + // archives always extract the binary with the generic name; rename to the + // versioned filename so future cache lookups resolve to the correct version + try { + await fsp.rename(path.resolve(stagingDir, BINARY_FILE), binaryFilePath) + } catch (err) { + // a concurrent download of the same version may have produced the + // final binary already; on Windows rename throws EEXIST/EPERM in + // that case. Treat it as success if the destination is accessible. + const code = (err as NodeJS.ErrnoException)?.code + if ((code === 'EEXIST' || code === 'EPERM') && await hasAccess(binaryFilePath)) { + return binaryFilePath + } + throw err + } + } finally { + await fsp.rm(stagingDir, { recursive: true, force: true }).catch(() => {}) + } await fsp.chmod(binaryFilePath, '755') return binaryFilePath } -async function downloadZip(res: Awaited>, cacheDir: string) { +async function downloadZip(res: Awaited>, stagingDir: string) { const zipBlob = await res.blob() const zip = new ZipReader(new BlobReader(zipBlob)) + const resolvedStagingDir = path.resolve(stagingDir) for (const entry of await zip.getEntries()) { - const unzippedFilePath = path.join(cacheDir, entry.filename) + const unzippedFilePath = path.join(stagingDir, entry.filename) if (entry.directory) { continue } + /** + * guard against Zip Slip: a malicious archive could contain entries + * with `../` or absolute paths that escape the staging directory + */ + const resolvedPath = path.resolve(unzippedFilePath) + if (resolvedPath !== resolvedStagingDir && !resolvedPath.startsWith(resolvedStagingDir + path.sep)) { + throw new Error(`Zip entry "${entry.filename}" resolves outside the staging directory`) + } const fileEntry = entry as FileEntry if (!await hasAccess(path.dirname(unzippedFilePath))) { await fsp.mkdir(path.dirname(unzippedFilePath), { recursive: true }) diff --git a/tests/unit.test.ts b/tests/unit.test.ts index 50f6be7..faa1f6b 100644 --- a/tests/unit.test.ts +++ b/tests/unit.test.ts @@ -1,8 +1,15 @@ import os from 'node:os' -import { vi, test, expect, afterEach } from 'vitest' +import path from 'node:path' +import fsp from 'node:fs/promises' +import { vi, test, expect, describe, beforeEach, afterEach } from 'vitest' import { getDownloadUrl, parseParams, retryFetch } from '../src/utils.js' +import { getBinaryFilename, download } from '../src/install.js' +// All vi.mock calls must be at module scope so Vitest hoists them before +// any imports — mocks inside test() bodies are not hoisted and the already- +// bound module closures (e.g. pipeline, unpackTar in install.js) would not +// see the mock. vi.mock('node:os', () => ({ default: { arch: vi.fn(), @@ -11,6 +18,43 @@ vi.mock('node:os', () => ({ } })) +vi.mock('node:fs/promises', () => ({ + default: { + access: vi.fn(), + mkdir: vi.fn().mockResolvedValue(undefined), + mkdtemp: vi.fn(), + rename: vi.fn().mockResolvedValue(undefined), + chmod: vi.fn().mockResolvedValue(undefined), + rm: vi.fn().mockResolvedValue(undefined), + }, + writeFile: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock('../src/utils.js', async (original) => { + const actual: any = await original() + return { + ...actual, + hasAccess: vi.fn(), + } +}) + +vi.mock('node:stream/promises', () => ({ + pipeline: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock('modern-tar/fs', () => ({ + unpackTar: vi.fn().mockReturnValue(vi.fn()), +})) + +const zipState = vi.hoisted(() => ({ entries: [] as any[] })) +vi.mock('@zip.js/zip.js', () => ({ + BlobReader: class { }, + BlobWriter: class { }, + ZipReader: class { + getEntries() { return Promise.resolve(zipState.entries) } + }, +})) + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ status: 400, text: () => Promise.resolve('foobar'), @@ -21,6 +65,23 @@ afterEach(() => { vi.mocked(globalThis.fetch).mockReset() }) +test('getBinaryFilename includes version in the filename', () => { + vi.mocked(os.platform).mockReturnValue('linux') + expect(getBinaryFilename('0.36.0')).toBe('geckodriver-0.36.0') + + vi.mocked(os.platform).mockReturnValue('darwin') + expect(getBinaryFilename('0.36.0')).toBe('geckodriver-0.36.0') + + vi.mocked(os.platform).mockReturnValue('win32') + expect(getBinaryFilename('0.36.0')).toBe('geckodriver-0.36.0.exe') +}) + +test('getBinaryFilename rejects version strings with path separators or traversal sequences', () => { + expect(() => getBinaryFilename('../../../evil')).toThrow('Invalid geckodriver version string') + expect(() => getBinaryFilename('0.36.0/etc/passwd')).toThrow('Invalid geckodriver version string') + expect(() => getBinaryFilename('0.36.0\\evil')).toThrow('Invalid geckodriver version string') +}) + test('getDownloadUrl', () => { vi.mocked(os.arch).mockReturnValue('arm') vi.mocked(os.platform).mockReturnValue('linux') @@ -42,6 +103,142 @@ test('getDownloadUrl', () => { expect(getDownloadUrl('0.33.0')).toMatchSnapshot() }) +describe('download caching behaviour', () => { + const CACHE_DIR = path.resolve(os.tmpdir(), 'test-cache') + let hasAccess: ReturnType + + beforeEach(async () => { + const utils = await import('../src/utils.js') + hasAccess = vi.mocked(utils.hasAccess) + + delete process.env.GECKODRIVER_VERSION + vi.mocked(os.platform).mockReturnValue('linux') + // default: cache miss + hasAccess.mockResolvedValue(false) + vi.mocked(fsp.rename).mockClear() + vi.mocked(fsp.chmod).mockClear() + vi.mocked(fsp.rm).mockClear() + vi.mocked(fsp.mkdtemp).mockClear() + }) + + test('returns cached versioned binary without any network request', async () => { + vi.mocked(hasAccess).mockResolvedValue(true) // cache hit + + const result = await download('0.36.0', CACHE_DIR) + + expect(result).toBe(path.resolve(CACHE_DIR, 'geckodriver-0.36.0')) + // zero network requests — the hot path must be purely local + expect(globalThis.fetch).not.toHaveBeenCalled() + }) + + test('does not fetch Cargo.toml when explicit version is cached', async () => { + vi.mocked(hasAccess).mockResolvedValue(true) + + await download('0.36.0', CACHE_DIR) + + const cargoFetched = vi.mocked(globalThis.fetch).mock.calls + .some(([url]) => String(url).includes('Cargo.toml')) + expect(cargoFetched).toBe(false) + }) + + test('resolves latest version from Cargo.toml then hits versioned cache (no binary download)', async () => { + vi.mocked(globalThis.fetch).mockResolvedValue({ + status: 200, + text: () => Promise.resolve('version = "0.36.0"\nother = "x"'), + } as any) + // after Cargo.toml resolves, the versioned binary is already cached + vi.mocked(hasAccess).mockResolvedValue(true) + + const result = await download(undefined, CACHE_DIR) + + expect(result).toBe(path.resolve(CACHE_DIR, 'geckodriver-0.36.0')) + // only Cargo.toml fetched — no binary download + expect(globalThis.fetch).toHaveBeenCalledTimes(1) + expect(vi.mocked(globalThis.fetch).mock.calls[0][0]).toContain('Cargo.toml') + }) + + test('extracts into a unique mkdtemp staging dir, then renames to the final versioned path', async () => { + const stagingDir = path.resolve(CACHE_DIR, 'geckodriver-AbC123') + vi.mocked(fsp.mkdtemp).mockResolvedValue(stagingDir) + vi.mocked(globalThis.fetch).mockResolvedValue({ + status: 200, + body: {}, + blob: vi.fn().mockResolvedValue(new Blob([])), + } as any) + + await download('0.36.0', CACHE_DIR).catch(() => {}) + + const finalPath = path.resolve(CACHE_DIR, 'geckodriver-0.36.0') + + // staging dir must be created via mkdtemp (unique per operation), not a + // deterministic path that parallel downloads of the same version share + expect(fsp.mkdtemp).toHaveBeenCalledWith(path.join(CACHE_DIR, 'geckodriver-')) + // rename must move the extracted binary out of that unique staging dir + expect(fsp.rename).toHaveBeenCalledWith( + path.resolve(stagingDir, 'geckodriver'), // extracted name inside staging dir + finalPath // versioned final destination + ) + // staging dir must be cleaned up + expect(fsp.rm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + }) + + test('concurrent downloads of the same version use isolated staging dirs', async () => { + // each mkdtemp call returns a distinct directory, mirroring the real OS + let counter = 0 + vi.mocked(fsp.mkdtemp).mockImplementation(async (prefix) => + `${prefix}${++counter}` + ) + vi.mocked(globalThis.fetch).mockResolvedValue({ + status: 200, + body: {}, + blob: vi.fn().mockResolvedValue(new Blob([])), + } as any) + + await Promise.all([ + download('0.36.0', CACHE_DIR).catch(() => {}), + download('0.36.0', CACHE_DIR).catch(() => {}), + ]) + + const stagingDirs = vi.mocked(fsp.rename).mock.calls.map(([src]) => path.dirname(String(src))) + expect(stagingDirs).toHaveLength(2) + // the two concurrent operations must not share a staging directory + expect(stagingDirs[0]).not.toBe(stagingDirs[1]) + }) + + test('treats EEXIST on the final rename as success when the binary is already present', async () => { + vi.mocked(fsp.mkdtemp).mockResolvedValue(path.resolve(CACHE_DIR, 'geckodriver-xyz')) + vi.mocked(globalThis.fetch).mockResolvedValue({ + status: 200, body: {}, blob: vi.fn().mockResolvedValue(new Blob([])), + } as any) + // cache miss on entry, but after the rename collision the binary exists + hasAccess.mockResolvedValueOnce(false).mockResolvedValue(true) + const renameErr: any = new Error('exists') + renameErr.code = 'EEXIST' + vi.mocked(fsp.rename).mockRejectedValueOnce(renameErr) + + const result = await download('0.36.0', CACHE_DIR) + + // a concurrent winner produced the binary — must resolve, not throw + expect(result).toBe(path.resolve(CACHE_DIR, 'geckodriver-0.36.0')) + }) + + test('rejects zip entries that escape the staging directory (Zip Slip)', async () => { + const stagingDir = path.resolve(CACHE_DIR, 'geckodriver-zip') + vi.mocked(os.platform).mockReturnValue('win32') // .zip download path + vi.mocked(os.arch).mockReturnValue('x64') + vi.mocked(fsp.mkdtemp).mockResolvedValue(stagingDir) + vi.mocked(globalThis.fetch).mockResolvedValue({ + status: 200, body: {}, blob: vi.fn().mockResolvedValue(new Blob([])), + } as any) + + zipState.entries = [ + { filename: '../../evil.exe', directory: false, getData: vi.fn() }, + ] + + await expect(download('0.36.0', CACHE_DIR)).rejects.toThrow('resolves outside the staging directory') + }) +}) + test('download with proxy support', async () => { // Ensure hasAccess always returns false so download always calls fetch vi.mock('../src/utils.js', async () => {