Tighten exec commands to prevent injection attacks

.changeset/tidy-specifiers-guard.md

@@ -0,0 +1,6 @@
+---
+'@openfn/runtime': patch
+'@openfn/cli': patch
+---
+
+Tighten guards against shell injection vectors.

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`

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<void>((resolve) => {
         logger.info(`Cleaning repo at ${options.repoDir} `);
         exec(`npm exec rimraf ${options.repoDir}`, () => {

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';

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
 };

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;

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,

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</etc/passwd'],
+  ['an escape character', 'lodash\\x'],
+  ['a newline', 'lodash\nwhoami'],
+  ['a tab', 'lodash\twhoami'],
+];
+
+unsafe.forEach(([label, specifier]) => {
+  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/,
+  });
+});