permission: propagate permission model flags on spawn

[RafaelGSS]

Previously, only child_process.fork propagated the exec arguments (execvArgs) to the child process.
This commit adds support for spawn and spawnSync to propagate permission model flags — except when they are already provided explicitly via arguments or through NODE_OPTIONS.

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg

[RafaelGSS]

Since --allow-child-process is still on experimental phrase, it's fine to keep it as semver-minor.

[codecov]

Codecov Report

Attention: Patch coverage is 47.82609% with 24 lines in your changes missing coverage. Please review.

Project coverage is 90.07%. Comparing base (d08513d) to head (1f5354a).
Report is 50 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/child_process.js	27.27%	24 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58853      +/-   ##
==========================================
- Coverage   90.11%   90.07%   -0.04%     
==========================================
  Files         640      640              
  Lines      188363   188485     +122     
  Branches    36931    36965      +34     
==========================================
+ Hits       169739   169784      +45     
- Misses      11341    11390      +49     
- Partials     7283     7311      +28     

Files with missing lines	Coverage Δ
lib/internal/process/permission.js	76.59% <100.00%> (+7.15%)	⬆️
lib/internal/process/pre_execution.js	91.39% <100.00%> (-0.10%)	⬇️
lib/child_process.js	95.54% <27.27%> (-2.21%)	⬇️

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @RafaelGSS.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[RafaelGSS]

Adding notable-change label as it will impact current users of --allow-child-process

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67682/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67689/

[jasnell]

You can simplify this expression a bit with...

env?.[key]?.indexOf(...)

[RafaelGSS]

How? We need to compare the result with !== -1 and also handle the case when the key is undefined. Leaving it as env?.[key]?.indexOf('--permission') might return 0, which is truthy in this case, so I don't see how using env?.[key]?.indexOf(...) would simplify this.

[JakobJingleheimer]

I think it's possible with the dark arts:

if (!~env?.[key]?.indexOf('--permission'))

if (!~undefined) // no match
if (!~-1) // match
if (!~0) // no match
if (!~1) // no match

Not saying you should do this, but you can.

[RafaelGSS]

I don't think you are correct

!~({'foo': '--permission'})?.['foo'].indexOf('--permission')  // false
!~({'foo': '--permission'})?.['foo2']?.indexOf('--permission') // false
({'foo': '--permission'})?.['foo'].indexOf('--permission') // 0

[marco-ippolito]

the problem is ~undefined returns -1 which is truthy (just like when it is being found), so ~ doesnt work well in this case. I had the same thought

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67707/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67750/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67751/

[RafaelGSS]

PTAL @nodejs/security-wg

[JakobJingleheimer]

nit: If sequence doesn't matter, I think it would be better (grokability) to alphabetise them.

[H4ad]

Is this a safe operation? I mean, if I do something like node --permission --allow-fs-read="/tmp" --allow-fs-write=a\ --allow-fs-read="/home" -e 'console.log(process.execArgv)'

[RafaelGSS]

Yes. That works as expected. See:

const { spawnSync } = require('node:child_process')

const out = spawnSync(process.execPath, ['-p', '1 + 1'], { env: {
  'NODE_DEBUG_NATIVE': 'PERMISSION_MODEL'
} })

console.log(out.status)
console.log(out.stdout.toString())

0
Inserting /tmp/*
Inserting /home/*
Inserting /Users/rafaelgss/repos/os/node/a

In any case, escaping characters is the user's responsibility, so I wouldn't worry much about it.

[H4ad]

Hm... I thought "\ " was not allowed, but it seems it is.

[nodejs-github-bot]

Landed in 8173d9d