HarperFast / HarperFast/harper
install.command bypasses --ignore-scripts, so allowInstallScripts: false is not binding
- 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
Assessment
This issue has not been assessed yet.