HarperFast / HarperFast/harper

install.command bypasses --ignore-scripts, so allowInstallScripts: false is not binding

Open
#1,978 0 comments 0 reactions 1 assignee Claimed by @kriszyp View on GitHub
area:components area:security bug
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

## Summary

`install.command` has three separable problems. The middle one is a security control that silently does not apply.

## 1. `--ignore-scripts` is bypassed, so `allowInstallScripts: false` is not binding

The default install path (`components/Application.ts:424`) and the `devEngines.packageManager` path (`:377`) both add `--ignore-scripts` unless `install.allowInstallScripts` is true. The custom-command path (`:309-337`) runs the user's string verbatim and never adds it.

So when `install.command` is set, package install scripts run regardless of `allowInstallScripts: false`. A component config (or API request) that explicitly disables install scripts does not get what it asked for. This is the part I'd treat as a defect independent of everything else here.

Same divergence exists in the deprecated `install_node_modules` operation (`utility/npmUtilities.ts:45`), whose args are `['install', '--force', '--omit=dev', '--json']` — no `--ignore-scripts` either.

## 2. Naive argument splitting

`components/Application.ts:310`:

```js
const [command, ...args] = application.install.command.split(' ');
```

Splitting on a single space breaks any quoted argument or path containing spaces. Straightforward bug.

## 3. Policy inconsistency — asking for a ruling, not necessarily a change

`install_command` is an **operations API parameter** on both `deploy_component` and `add_component` (`components/operations.js`), executed through a shell, with no allowlist.

Meanwhile the component-runtime process globals *are* allowlisted against `applications_allowedSpawnCommands` (`security/jsLoader.ts:987`) — and unevenly, since `fork` is registered with `alwaysAllow`:

```ts
exec: createSpawn(child_process.exec),
execFile: createSpawn(child_process.execFile),
fork: createSpawn(child_process.fork, true), // this is launching node, so deemed safe
spawn: createSpawn(child_process.spawn),
```

So within one process there are three different policies for starting a subprocess: allowlisted (`spawn`/`exec`/`execFile`), unconditionally allowed (`fork`), and unbounded shell via config or API (`install.command`). That may all be intentional under a threat model where application developers are administrators — but right now it reads as four independent historical decisions rather than one policy. Worth an explicit decision recorded somewhere, even if the answer is "this is fine."

## Constraint on any fix

`shell: true` in `nonInteractiveSpawn` (`:719`) is **load-bearing**. `applications_packageManagerPrefix` works by string-prepending the prefix to the command (`prefix + ' ' + 'npm'`) and relying on the shell to split it; `jsLoader.ts:987` uses the matching `command.split(' ')[0]` convention. Removing the shell breaks the prefix feature.

That also means adding an argv-array form alongside the string creates two paths with different semantics — an argv form would not go through a shell, so the prefix would silently not apply to it. If argv is added, it should either apply the prefix as `argv[0]` explicitly or be validated as mutually exclusive with the prefix. Shipping both undecided is worse than either alone.

## Migration

`install.command` is an operations API parameter, so removing it is a breaking API change and presumably off the table for 5.x. The realistic path is to keep it, fix (1) and (2), and give the lifecycle enough first-class knobs that it stops being the default answer for routine adjustments.

---

🤖 Filed by Claude on behalf of @heskew

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.