diff --git a/.changeset/tidy-specifiers-guard.md b/.changeset/tidy-specifiers-guard.md new file mode 100644 index 000000000..f0eb0ab92 --- /dev/null +++ b/.changeset/tidy-specifiers-guard.md @@ -0,0 +1,6 @@ +--- +'@openfn/runtime': patch +'@openfn/cli': patch +--- + +Tighten guards against shell injection vectors. diff --git a/CLAUDE.md b/CLAUDE.md index c1744b67e..bbe60ab3a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,6 +128,8 @@ cd packages/cli && pnpm test:watch # Watch mode **Changesets**: Run `pnpm changeset` when submitting PRs. Releases publish automatically to npm on merge to main. +Changeset release notes must be a **single, short, high-level sentence** - vague is fine (e.g. `added new "compile-test" command to the CLI` or `Tighten guards against shell injection vectors`). Do not list files, functions, or implementation detail. For breaking changes only, you may add a short migration guide after the sentence. See the package `CHANGELOG.md` files for the house style. + The [.claude](.claude) folder contains detailed guides: - **[event-processor.md](.claude/event-processor.md)** - Worker event processing deep-dive (ordering, batching) — companion to `packages/ws-worker/CLAUDE.md` diff --git a/packages/cli/src/repo/handler.ts b/packages/cli/src/repo/handler.ts index dece45854..8b2ccc80a 100644 --- a/packages/cli/src/repo/handler.ts +++ b/packages/cli/src/repo/handler.ts @@ -5,6 +5,7 @@ import { install as rtInstall, loadRepoPkg, getNameAndVersion, + assertSafeSpecifier, } from '@openfn/runtime'; import type { Opts } from '../options'; import { defaultLogger, Logger } from '../util/logger'; @@ -41,6 +42,7 @@ export const removePackage = async ( } const aliasedName = `${name}_${version}`; + assertSafeSpecifier(aliasedName); logger.info(`Removing package ${aliasedName} from repo...`); try { @@ -68,6 +70,7 @@ export const clean = async (options: Opts, logger: Logger) => { options.force ); if (doIt) { + assertSafeSpecifier(options.repoDir); return new Promise((resolve) => { logger.info(`Cleaning repo at ${options.repoDir} `); exec(`npm exec rimraf ${options.repoDir}`, () => { diff --git a/packages/runtime/src/index.ts b/packages/runtime/src/index.ts index 916f79970..50410d139 100644 --- a/packages/runtime/src/index.ts +++ b/packages/runtime/src/index.ts @@ -13,4 +13,6 @@ export * from './modules/repo'; export * from './runtime-helpers'; +export { default as assertSafeSpecifier } from './util/assert-safe-specifier'; + export { registerEsmHook } from './modules/register-esm-hook'; diff --git a/packages/runtime/src/modules/repo.ts b/packages/runtime/src/modules/repo.ts index 1f39347e5..1ab794a28 100644 --- a/packages/runtime/src/modules/repo.ts +++ b/packages/runtime/src/modules/repo.ts @@ -2,6 +2,7 @@ import path from 'node:path'; import { readFile, writeFile, mkdir } from 'node:fs/promises'; import { defaultLogger, Logger } from '@openfn/logger'; import exec from '../util/exec'; +import assertSafeSpecifier from '../util/assert-safe-specifier'; import * as os from 'node:os'; const homeDir = os.homedir(); @@ -66,7 +67,9 @@ export const install = async ( const aliases = forInstalling.map(({ name, version }) => { const alias = `npm:${name}@${version}`; const aliasedName = `${name}_${version}`; - return `${aliasedName}@${alias}`; + const aliased = `${aliasedName}@${alias}`; + assertSafeSpecifier(aliased); + return aliased; }); log.info(`npm install ${npmInstallFlags.join(' ')} ${aliases.join(' ')}`); // TODO it would be nice to report something about what's going on under the hood here @@ -142,6 +145,7 @@ export const getAliasedName = (specifier: string, version?: string) => { }; export const getLatestVersion = async (specifier: string) => { + assertSafeSpecifier(specifier); const { stdout } = await exec(`npm view ${specifier} version`); return stdout.trim(); // TODO this works for now but isn't very robust }; diff --git a/packages/runtime/src/util/assert-safe-specifier.ts b/packages/runtime/src/util/assert-safe-specifier.ts new file mode 100644 index 000000000..81d14a968 --- /dev/null +++ b/packages/runtime/src/util/assert-safe-specifier.ts @@ -0,0 +1,14 @@ +// Guards against shell injection when a module specifier (or path) is +// interpolated into an npm command passed to child_process.exec + +// Whitespace splits arguments; the metacharacters below allow command +// chaining (; & |), substitution ($ ` ( )), redirection (< >) or escaping (\) +const UNSAFE_CHARS = /[\s;&|`$()<>\\]/; + +const assertSafeSpecifier = (specifier: string) => { + if (typeof specifier !== 'string' || UNSAFE_CHARS.test(specifier)) { + throw new Error(`Unsafe module specifier: ${specifier}`); + } +}; + +export default assertSafeSpecifier; diff --git a/packages/runtime/src/util/index.ts b/packages/runtime/src/util/index.ts index 1ad364095..b9af4bede 100644 --- a/packages/runtime/src/util/index.ts +++ b/packages/runtime/src/util/index.ts @@ -1,4 +1,5 @@ import assembleState from './assemble-state'; +import assertSafeSpecifier from './assert-safe-specifier'; import clone from './clone'; import defaultState from './default-state'; import exec from './exec'; @@ -9,6 +10,7 @@ import validatePlan from './validate-plan'; export { assembleState, + assertSafeSpecifier, clone, defaultState, exec, diff --git a/packages/runtime/test/util/assert-safe-specifier.test.ts b/packages/runtime/test/util/assert-safe-specifier.test.ts new file mode 100644 index 000000000..65bf807c9 --- /dev/null +++ b/packages/runtime/test/util/assert-safe-specifier.test.ts @@ -0,0 +1,60 @@ +import test from 'ava'; +import assertSafeSpecifier from '../../src/util/assert-safe-specifier'; + +test('allow a plain package name', (t) => { + t.notThrows(() => assertSafeSpecifier('lodash')); +}); + +test('allow a scoped package name', (t) => { + t.notThrows(() => assertSafeSpecifier('@openfn/language-http')); +}); + +test('allow a name and version specifier', (t) => { + t.notThrows(() => assertSafeSpecifier('@openfn/language-http@1.2.3')); +}); + +test('allow the aliased install form', (t) => { + t.notThrows(() => + assertSafeSpecifier('@openfn/language-http_1.2.3@npm:@openfn/language-http@1.2.3') + ); +}); + +test('allow a prerelease version', (t) => { + t.notThrows(() => assertSafeSpecifier('my-pkg@1.0.0-beta.1')); +}); + +test('allow a filesystem path', (t) => { + t.notThrows(() => assertSafeSpecifier('/home/user/openfn/repo/cli')); +}); + +// each of these should throw +const unsafe = [ + ['a space', 'lodash 4'], + ['a semicolon', 'lodash;rm -rf /'], + ['command chaining with &&', 'lodash&&whoami'], + ['a pipe', 'lodash|cat'], + ['a single ampersand', 'lodash&whoami'], + ['a backtick', 'lodash`whoami`'], + ['command substitution', 'lodash$(whoami)'], + ['a dollar sign', 'lodash$HOME'], + ['output redirection', 'lodash>/etc/passwd'], + ['input redirection', 'lodash { + test(`throw for ${label}`, (t) => { + t.throws(() => assertSafeSpecifier(specifier), { + message: /Unsafe module specifier/, + }); + }); +}); + +test('throw for a non-string specifier', (t) => { + // @ts-ignore deliberately passing the wrong type + t.throws(() => assertSafeSpecifier(undefined), { + message: /Unsafe module specifier/, + }); +});