diff --git a/README.md b/README.md index 710ee73..b9ff016 100644 --- a/README.md +++ b/README.md @@ -155,7 +155,7 @@ await fn('🙌') // => true await teardown() ``` -If the code tries to require a package not in the allowed list, an `UntrustedDependencyError` is thrown **before** any npm install happens: +If the code tries to require a package not in the allowed list, a `DependencyUnallowedError` is thrown **before** any npm install happens: ```js const [fn, teardown] = isolatedFunction( @@ -169,7 +169,7 @@ const [fn, teardown] = isolatedFunction( ) await fn() -// => UntrustedDependencyError: Dependency 'malicious-package' is not in the allowed list +// => DependencyUnallowedError: Dependency 'malicious-package' is not in the allowed list ``` > **Security Note**: Even with the sandbox, arbitrary package installation is dangerous because npm packages can execute code during installation via `preinstall`/`postinstall` scripts. The `--ignore-scripts` flag is used to mitigate this, but providing an `allow.dependencies` whitelist is the recommended approach for running untrusted code. diff --git a/package.json b/package.json index 0ef0af4..069d9e1 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,8 @@ "types": "src/index.d.ts", "main": "src/index.js", "exports": { - ".": "./src/index.js" + ".": "./src/index.js", + "./errors": "./src/errors.js" }, "author": { "email": "hello@microlink.io", diff --git a/src/compile/index.js b/src/compile/index.js index e1260f4..f22e0b8 100644 --- a/src/compile/index.js +++ b/src/compile/index.js @@ -41,4 +41,3 @@ module.exports = async (snippet, { tmpdir = tmpdirDefault, allow = {} } = {}) => module.exports.detectDependencies = detectDependencies module.exports.transformDependencies = transformDependencies -module.exports.UntrustedDependencyError = installDependencies.UntrustedDependencyError diff --git a/src/compile/install-dependencies.js b/src/compile/install-dependencies.js index 4133686..42d9f59 100644 --- a/src/compile/install-dependencies.js +++ b/src/compile/install-dependencies.js @@ -3,6 +3,8 @@ const { execSync } = require('child_process') const $ = require('tinyspawn') +const { DependencyNameError, DependencyUnallowedError } = require('../errors') + const install = (() => { try { execSync('which pnpm', { stdio: ['pipe', 'pipe', 'ignore'] }) @@ -14,14 +16,6 @@ const install = (() => { } })() -class UntrustedDependencyError extends Error { - constructor (dependency) { - super(`Dependency '${dependency}' is not in the allowed list`) - this.name = 'UntrustedDependencyError' - this.dependency = dependency - } -} - const extractPackageName = dependency => { if (dependency.startsWith('@')) { const slashIndex = dependency.indexOf('/') @@ -41,12 +35,19 @@ const extractPackageName = dependency => { } const validateDependencies = (dependencies, allowed) => { + // Always check for command injection, regardless of allow list + for (const dependency of dependencies) { + if (dependency.includes(' ')) { + throw new DependencyNameError(dependency) + } + } + if (!allowed) return for (const dependency of dependencies) { const packageName = extractPackageName(dependency) if (!allowed.includes(packageName)) { - throw new UntrustedDependencyError(packageName) + throw new DependencyUnallowedError(packageName) } } } @@ -55,5 +56,3 @@ module.exports = async ({ dependencies, cwd, allow = {} }) => { validateDependencies(dependencies, allow.dependencies) return $(`${install} ${dependencies.join(' ')}`, { cwd, env: { ...process.env, CI: true } }) } - -module.exports.UntrustedDependencyError = UntrustedDependencyError diff --git a/src/errors.js b/src/errors.js new file mode 100644 index 0000000..5a4b039 --- /dev/null +++ b/src/errors.js @@ -0,0 +1,32 @@ +'use strict' + +class IsolatedFunctionError extends Error { + constructor (message) { + super(message) + this.name = 'IsolatedFunctionError' + } +} + +class DependencyNameError extends IsolatedFunctionError { + constructor (dependency) { + super(`Dependency '${dependency}' is not a valid npm package name`) + this.name = 'DependencyNameError' + this.code = 'EDEPENDENCYNAME' + this.dependency = dependency + } +} + +class DependencyUnallowedError extends IsolatedFunctionError { + constructor (dependency) { + super(`Dependency '${dependency}' is not in the allowed list`) + this.name = 'DependencyUnallowedError' + this.code = 'EDEPENDENCYUNALLOWED' + this.dependency = dependency + } +} + +module.exports = { + IsolatedFunctionError, + DependencyNameError, + DependencyUnallowedError +} diff --git a/test/compile/install-dependencies.js b/test/compile/install-dependencies.js index 2f267d2..9d1c569 100644 --- a/test/compile/install-dependencies.js +++ b/test/compile/install-dependencies.js @@ -2,7 +2,7 @@ const test = require('ava') -const { UntrustedDependencyError } = require('../../src/compile') +const { DependencyNameError, DependencyUnallowedError } = require('../../src/errors') const isolatedFunction = require('../..') const run = promise => Promise.resolve(promise).then(({ value }) => value) @@ -33,7 +33,7 @@ test('allow.dependencies › blocks untrusted dependencies', async t => { const error = await t.throwsAsync(fn('🙌')) - t.true(error instanceof UntrustedDependencyError) + t.true(error instanceof DependencyUnallowedError) t.is(error.message, "Dependency 'is-standard-emoji' is not in the allowed list") t.is(error.dependency, 'is-standard-emoji') }) @@ -63,7 +63,7 @@ test('allow.dependencies › blocks untrusted scoped packages', async t => { const error = await t.throwsAsync(fn()) - t.true(error instanceof UntrustedDependencyError) + t.true(error instanceof DependencyUnallowedError) t.is(error.message, "Dependency '@kikobeats/time-span' is not in the allowed list") t.is(error.dependency, '@kikobeats/time-span') }) @@ -104,7 +104,35 @@ test('allow.dependencies › handles multiple dependencies', async t => { ) const error = await t.throwsAsync(fn()) - t.true(error instanceof UntrustedDependencyError) + t.true(error instanceof DependencyUnallowedError) t.is(error.dependency, 'is-number') } }) + +test('allow.dependencies › blocks invalid package names with spaces', async t => { + const [fn] = isolatedFunction( + () => { + const _ = require('lodash@latest express') + return typeof _ + }, + { allow: { dependencies: ['lodash'] } } + ) + + const error = await t.throwsAsync(fn()) + + t.true(error instanceof DependencyNameError) + t.is(error.dependency, 'lodash@latest express') + t.true(error.message.includes('not a valid npm package name')) +}) + +test('allow.dependencies › blocks invalid package names even without allow list', async t => { + const [fn] = isolatedFunction(() => { + const _ = require('lodash@latest express') + return typeof _ + }) + + const error = await t.throwsAsync(fn()) + + t.true(error instanceof DependencyNameError) + t.is(error.dependency, 'lodash@latest express') +}) diff --git a/test/error.js b/test/error.js index a182cf9..ea41e52 100644 --- a/test/error.js +++ b/test/error.js @@ -153,7 +153,7 @@ test('handle untrusted dependencies', async t => { const error = await t.throwsAsync(fn()) - t.is(error.name, 'UntrustedDependencyError') + t.is(error.name, 'DependencyUnallowedError') t.is(error.message, "Dependency 'malicious-package' is not in the allowed list") t.is(error.dependency, 'malicious-package') })