diff --git a/src/shadow/npm-base.mts b/src/shadow/npm-base.mts index edd5cd3c30..0a9b9ac9a9 100644 --- a/src/shadow/npm-base.mts +++ b/src/shadow/npm-base.mts @@ -37,6 +37,42 @@ export type ShadowBinResult = { spawnPromise: SpawnResult } +// Length of the '--node-options=' prefix, used to slice the value out of an +// npm '--node-options=' argument. +const NODE_OPTIONS_FLAG_PREFIX_LENGTH = '--node-options='.length + +/** + * Build the '--node-options=' argument passed to npm so its lifecycle + * scripts run with our Node permission flags. + * + * npm assigns this value to NODE_OPTIONS for the scripts it runs, REPLACING any + * inherited NODE_OPTIONS. To avoid clobbering the user's configuration we merge, + * in order, the caller's existing process.env.NODE_OPTIONS and any + * '--node-options=' npm argument ahead of our permission flags (issue + * #1036). + * + * The value is intentionally NOT wrapped in quotes. shadowNpmBase spawns npm + * without a shell, so any quotes would be passed literally to npm and become + * part of the NODE_OPTIONS value. Consumers that re-tokenize NODE_OPTIONS on + * whitespace (e.g. Next.js) then strip the leading quote off '--permission', + * dropping it while keeping the '--allow-*' flags, which crashes Node >= 24 + * with ERR_MISSING_OPTION (issue #1160). + */ +export function buildNpmNodeOptionsArg( + envNodeOptions: string | undefined, + nodeOptionsArg: string | undefined, + permArgs: string[] | readonly string[], +): string { + const value = [ + envNodeOptions, + nodeOptionsArg ? nodeOptionsArg.slice(NODE_OPTIONS_FLAG_PREFIX_LENGTH) : '', + cmdFlagsToString(permArgs), + ] + .filter(Boolean) + .join(' ') + return `--node-options=${value}` +} + export default async function shadowNpmBase( binName: typeof NPM | typeof NPX, args: string[] | readonly string[] = process.argv.slice(2), @@ -113,7 +149,11 @@ export default async function shadowNpmBase( ...noAuditArgs, ...(useNodeOptions ? [ - `--node-options='${nodeOptionsArg ? nodeOptionsArg.slice(15) : ''}${cmdFlagsToString(permArgs)}'`, + buildNpmNodeOptionsArg( + process.env['NODE_OPTIONS'], + nodeOptionsArg, + permArgs, + ), ] : []), '--no-fund', diff --git a/src/shadow/npm-base.test.mts b/src/shadow/npm-base.test.mts new file mode 100644 index 0000000000..f528e2c4ae --- /dev/null +++ b/src/shadow/npm-base.test.mts @@ -0,0 +1,88 @@ +import { describe, expect, it } from 'vitest' + +import { buildNpmNodeOptionsArg } from './npm-base.mts' + +// The permission flags shadowNpmBase injects on Node versions that support +// --permission. Mirrors the array built in npm-base.mts. +const permArgs = [ + '--permission', + '--allow-child-process', + '--allow-fs-read=*', + '--allow-fs-write=/project/*', +] + +describe('buildNpmNodeOptionsArg', () => { + it('does not wrap the value in quotes (issue #1160)', () => { + // Literal quotes survive spawn() (no shell) and get re-tokenized by + // consumers like Next.js, which strips the leading quote off --permission + // and crashes Node >= 24 with ERR_MISSING_OPTION. + const result = buildNpmNodeOptionsArg(undefined, undefined, permArgs) + + expect(result).not.toContain("'") + expect(result).not.toContain('"') + expect(result).toBe( + '--node-options=--permission --allow-child-process --allow-fs-read=* --allow-fs-write=/project/*', + ) + // The NODE_OPTIONS value must keep --permission as a clean, standalone + // token so a whitespace re-tokenizer (Next.js) doesn't orphan the + // --allow-* flags. + const value = result.slice('--node-options='.length) + expect(value.split(' ')).toContain('--permission') + }) + + it('merges an inherited NODE_OPTIONS env value ahead of perm flags (issue #1036)', () => { + const result = buildNpmNodeOptionsArg( + '--max-old-space-size=4096', + undefined, + permArgs, + ) + + expect(result).toBe( + '--node-options=--max-old-space-size=4096 --permission --allow-child-process --allow-fs-read=* --allow-fs-write=/project/*', + ) + }) + + it('preserves the inherited NODE_OPTIONS even when there are no perm flags', () => { + const result = buildNpmNodeOptionsArg('--enable-source-maps', undefined, []) + + expect(result).toBe('--node-options=--enable-source-maps') + }) + + it('strips the --node-options= prefix from an npm --node-options arg', () => { + const result = buildNpmNodeOptionsArg( + undefined, + '--node-options=--trace-warnings', + permArgs, + ) + + expect(result).toBe( + '--node-options=--trace-warnings --permission --allow-child-process --allow-fs-read=* --allow-fs-write=/project/*', + ) + }) + + it('merges env NODE_OPTIONS, the npm --node-options arg, and perm flags in order', () => { + const result = buildNpmNodeOptionsArg( + '--enable-source-maps', + '--node-options=--trace-warnings', + permArgs, + ) + + expect(result).toBe( + '--node-options=--enable-source-maps --trace-warnings --permission --allow-child-process --allow-fs-read=* --allow-fs-write=/project/*', + ) + }) + + it('emits an empty value when nothing is provided', () => { + const result = buildNpmNodeOptionsArg(undefined, undefined, []) + + expect(result).toBe('--node-options=') + }) + + it('drops an empty-string env NODE_OPTIONS instead of adding a stray space', () => { + const result = buildNpmNodeOptionsArg('', undefined, permArgs) + + expect(result).toBe( + '--node-options=--permission --allow-child-process --allow-fs-read=* --allow-fs-write=/project/*', + ) + }) +})