Skip to content

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

Description

@heskew

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:

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:

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:componentsComponents / applications subsystemarea:securitySecurity, TLS/certs, authentication, authorizationbugSomething isn't working

    Type

    Fields

    Priority

    P1

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions