diff --git a/server/loader/npm.ts b/server/loader/npm.ts index 91bfc2e..e137905 100644 --- a/server/loader/npm.ts +++ b/server/loader/npm.ts @@ -1,4 +1,4 @@ -import { exec } from 'node:child_process' +import { execFile } from 'node:child_process' import { rmSync } from 'node:fs' import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' @@ -12,7 +12,7 @@ import type { } from '../../common/types.ts' const DOWNLOADED_PACKAGES = new Map() -const execAsync = promisify(exec) +const execFileAsync = promisify(execFile) function isGitUrl(version: DependencyVersion): boolean { return ( @@ -89,13 +89,22 @@ export async function getNpmContent( let tarballBuffer = await downloadTarball(tarballUrl) let tempDir = await mkdtemp(join(tmpdir(), 'multiocular-')) - let tarballPath = join(tempDir, 'package.tgz') - await writeFile(tarballPath, Buffer.from(tarballBuffer)) - await execAsync( - `tar -xzf "${tarballPath}" --strip-components=1 -C "${tempDir}"` - ) - await rm(tarballPath) + try { + let tarballPath = join(tempDir, 'package.tgz') + await writeFile(tarballPath, Buffer.from(tarballBuffer)) + + await execFileAsync( + 'tar', + ['-xzf', 'package.tgz', '--strip-components=1'], + { cwd: tempDir } + ) + + await rm(tarballPath) + } catch (error) { + await rm(tempDir, { force: true, recursive: true }) + throw error + } DOWNLOADED_PACKAGES.set(spec, tempDir as FilePath) } diff --git a/server/test/tar.test.ts b/server/test/tar.test.ts new file mode 100644 index 0000000..ab181b9 --- /dev/null +++ b/server/test/tar.test.ts @@ -0,0 +1,101 @@ +import { deepEqual, equal, match, rejects } from 'node:assert/strict' +import { execSync } from 'node:child_process' +import { existsSync } from 'node:fs' +import { + chmod, + mkdtemp, + readdir, + readFile, + rm, + writeFile +} from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, test } from 'node:test' + +import type { + DependencyName, + DependencyVersion, + FilePath +} from '../../common/types.ts' +import { deleteTemporary, getNpmContent } from '../loader/npm.ts' + +let shimDir: string | undefined +let originalPath = process.env.PATH + +afterEach(async () => { + process.env.PATH = originalPath + deleteTemporary() + if (shimDir) { + await rm(shimDir, { force: true, recursive: true }) + shimDir = undefined + } +}) + +async function forbidAbsolutePathsInTar(): Promise { + let real = execSync('command -v tar').toString().trim() + shimDir = await mkdtemp(join(tmpdir(), 'multiocular-shim-')) + let shim = join(shimDir, 'tar') + await writeFile( + shim, + '#!/bin/sh\n' + + 'for arg in "$@"; do\n' + + ' case "$arg" in\n' + + ' /*)\n' + + ' echo "tar: absolute path $arg is not portable" >&2\n' + + ' exit 128\n' + + ' ;;\n' + + ' esac\n' + + 'done\n' + + `exec "${real}" "$@"\n` + ) + await chmod(shim, 0o755) + process.env.PATH = `${shimDir}:${originalPath}` +} + +async function makeTarFail(): Promise { + shimDir = await mkdtemp(join(tmpdir(), 'multiocular-shim-')) + let shim = join(shimDir, 'tar') + await writeFile( + shim, + '#!/bin/sh\necho "tar: simulated extraction failure" >&2\nexit 1\n' + ) + await chmod(shim, 0o755) + process.env.PATH = `${shimDir}:${originalPath}` +} + +test( + 'extracts tarballs without absolute paths in tar arguments', + { skip: process.platform === 'win32' }, + async () => { + await forbidAbsolutePathsInTar() + let dir = await getNpmContent( + '.' as FilePath, + 'nanoid' as DependencyName, + '5.1.5' as DependencyVersion + ) + match(await readFile(join(dir, 'package.json'), 'utf8'), /"name": "nanoid"/) + equal(existsSync(join(dir, 'package.tgz')), false) + } +) + +test( + 'removes temporary directory when tar extraction fails', + { skip: process.platform === 'win32' }, + async () => { + await makeTarFail() + let before = await readdir(tmpdir()) + await rejects( + getNpmContent( + '.' as FilePath, + 'nanoid' as DependencyName, + '5.1.5' as DependencyVersion + ) + ) + let after = await readdir(tmpdir()) + let leaked = after.filter( + name => name.startsWith('multiocular-') && !before.includes(name) + ) + deepEqual(leaked, []) + } +)