Skip to content

fix(npm): merge NODE_OPTIONS and drop quotes in shadow --node-options#1433

Open
John-David Dalton (jdalton) wants to merge 1 commit into
v1.xfrom
jdalton/fix-npm-node-options-1160-1036
Open

fix(npm): merge NODE_OPTIONS and drop quotes in shadow --node-options#1433
John-David Dalton (jdalton) wants to merge 1 commit into
v1.xfrom
jdalton/fix-npm-node-options-1160-1036

Conversation

@jdalton

@jdalton John-David Dalton (jdalton) commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

What

socket npm builds the --node-options value it hands to npm for lifecycle scripts. Two bugs lived on that single line in src/shadow/npm-base.mts:

`--node-options='${nodeOptionsArg ? nodeOptionsArg.slice(15) : ''}${cmdFlagsToString(permArgs)}'`

Fixes #1160 and #1036.

Bug 1 — socket npm run build crashes on Node >= 24 (#1160)

shadowNpmBase spawns npm without a shell, so the literal single quotes are not interpreted — they become part of the value npm assigns to NODE_OPTIONS for lifecycle scripts:

NODE_OPTIONS='--permission --allow-child-process --allow-fs-read=* ...'

Node tolerates the garbled tokens, but consumers that re-tokenize NODE_OPTIONS on whitespace and only honour " (e.g. Next.js's TypeScript build worker) drop '--permission (leading quote) while keeping the valid --allow-* flags. Node 24 then throws:

TypeError [ERR_MISSING_OPTION]: --permission is required

because the allow-list flags are orphaned. Removing the quotes keeps --permission a clean, standalone token. Quotes were never needed here (no shell).

Bug 2 — socket npm overrides user NODE_OPTIONS (#1036)

The value only merged a --node-options= npm argument, never the caller's existing process.env.NODE_OPTIONS. Since npm's --node-options config replaces the inherited NODE_OPTIONS for scripts, a globally-configured NODE_OPTIONS was silently dropped:

$ NODE_OPTIONS=test socket npm run echo   # echoes socket's flags, not "test"

We now merge, in order: process.env.NODE_OPTIONS, any --node-options= npm arg, then our permission flags.

How

Extracted the construction into a pure, exported buildNpmNodeOptionsArg(envNodeOptions, nodeOptionsArg, permArgs) helper so both behaviours are unit-testable without spawning, and swapped the call site to use it.

Result for NODE_OPTIONS=--max-old-space-size=4096 socket npm run build on Node 24

Before: --node-options='--permission --allow-child-process ...'--permission dropped downstream → crash, and --max-old-space-size lost.

After: --node-options=--max-old-space-size=4096 --permission --allow-child-process --allow-fs-read=* ... → all tokens clean, user value preserved.

Tests

New src/shadow/npm-base.test.mts (7 cases) covers:

Verification

  • pnpm exec vitest run src/shadow/npm-base.test.mts — 7/7 pass
  • pnpm run check:tsc — clean
  • oxlint on changed files — 0 errors (only pre-existing no-named-as-default-member warnings on unchanged lines)

Notes

Targets v1.x (the branch where the shadow-npm wrapper lives; the issues reproduce on v1.1.78). On main, socket npm hands off to Socket Firewall and no longer builds this argument, so main is unaffected.


Note

Medium Risk
Changes NODE_OPTIONS passed into npm lifecycle scripts (build/run), which can affect all downstream script processes, but the logic is isolated, well-tested, and targets known production bugs.

Overview
Fixes how socket npm builds the --node-options flag for npm lifecycle scripts by replacing inline string templating with buildNpmNodeOptionsArg.

The value is no longer wrapped in single quotes, so --permission stays a real token when tools re-split NODE_OPTIONS on whitespace (fixes Node ≥ 24 ERR_MISSING_OPTION on builds like Next.js, #1160). The helper also merges process.env.NODE_OPTIONS, any existing npm --node-options=… arg, and Socket’s permission flags in that order, because npm’s flag replaces inherited NODE_OPTIONS for scripts (#1036).

Adds npm-base.test.mts with seven unit tests for quoting, merge order, prefix stripping, and empty inputs.

Reviewed by Cursor Bugbot for commit 3f272fb. Configure here.

The shadow npm wrapper built npm's `--node-options` value as:

    --node-options='<user value><perm flags>'

Two bugs on that one line:

1. Node >= 24 crash (#1160). spawn() runs npm without a shell, so the
   literal single quotes became part of the NODE_OPTIONS value npm set
   for lifecycle scripts. Consumers that re-tokenize NODE_OPTIONS on
   whitespace (e.g. Next.js) only understand `"`, not `'`, so they
   dropped `'--permission` (leading quote) while keeping the `--allow-*`
   flags. Node 24 then throws `ERR_MISSING_OPTION: --permission is
   required` because the allow-list flags are orphaned. Dropping the
   quotes keeps `--permission` a clean, standalone token.

2. NODE_OPTIONS override (#1036). The value only folded in a
   `--node-options=` npm arg, never the caller's existing
   `process.env.NODE_OPTIONS`, so npm replaced the user's global
   NODE_OPTIONS instead of extending it. We now merge, in order,
   process.env.NODE_OPTIONS, any `--node-options=` npm arg, and our
   permission flags.

Extracted the construction into a pure, exported `buildNpmNodeOptionsArg`
helper and unit-tested both fixes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant