fix: version cached binary filename to detect stale/mismatched drivers (#733)

mccmrunal · web-flow · commit 6e24754e4d0a · 2026-07-02T06:43:57.000-07:00
diff --git 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<ReturnType<typeof retryFetch>>, cacheDir: string) {
+async function downloadZip(res: Awaited<ReturnType<typeof retryFetch>>, 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
@@ -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<typeof vi.fn>
+
+    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 () => {
