permission: propagate permission model flags on spawn #58853

RafaelGSS · 2025-06-26T21:04:01Z

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 · 2025-06-26T21:04:05Z

Review requested:

@nodejs/security-wg

RafaelGSS · 2025-06-26T21:04:26Z

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

codecov · 2025-06-26T21:56:15Z

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 · 2025-06-27T14:21:31Z

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 · 2025-06-27T14:21:39Z

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

nodejs-github-bot · 2025-06-27T14:29:53Z

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

nodejs-github-bot · 2025-06-27T18:26:45Z

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

jasnell · 2025-06-27T18:38:35Z

lib/child_process.js

+
+function copyPermissionModelFlagsToEnv(env, key, args) {
+  // Do not override if permission was already passed to file
+  if (args.includes('--permission') || (env[key] && env[key].indexOf('--permission') !== -1)) {


You can simplify this expression a bit with...

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

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.

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.

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

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

test/parallel/test-permission-child-process-inherit-flags.js

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. Signed-off-by: RafaelGSS <[email protected]>

nodejs-github-bot · 2025-06-28T17:24:07Z

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

nodejs-github-bot · 2025-06-30T13:22:16Z

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

nodejs-github-bot · 2025-06-30T13:51:32Z

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

RafaelGSS · 2025-06-30T15:25:01Z

PTAL @nodejs/security-wg

JakobJingleheimer · 2025-06-30T21:42:12Z

lib/internal/process/permission.js

+      '--allow-fs-read',
+      '--allow-fs-write',
+      '--allow-addons',
+      '--allow-child-process',
+      '--allow-net',
+      '--allow-wasi',
+      '--allow-worker',


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

H4ad · 2025-06-30T22:57:17Z

lib/child_process.js

+  for (const arg of process.execArgv) {
+    for (const flag of flagsToCopy) {
+      if (arg.startsWith(flag)) {
+        env[key] = `${env[key] ? env[key] + ' ' + arg : arg}`;


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)'

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.

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

nodejs-github-bot · 2025-07-02T02:32:22Z

Landed in 8173d9d

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. Signed-off-by: RafaelGSS <[email protected]> PR-URL: #58853 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

Notable changes: crypto: * (SEMVER-MINOR) support outputLength option in crypto.hash for XOF functions (Aditi) #58121 doc: * (SEMVER-MINOR) add all watch-mode related flags to node.1 (Dario Piotrowicz) #58719 fs: * (SEMVER-MINOR) add disposable mkdtempSync (Kevin Gibbons) #58516 permission: * (SEMVER-MINOR) propagate permission model flags on spawn (Rafael Gonzaga) #58853 sqlite: * (SEMVER-MINOR) add support for readBigInts option in db connection level (Miguel Marcondes Filho) #58697 src,permission: * (SEMVER-MINOR) add support to permission.has(addon) (Rafael Gonzaga) #58951 watch: * (SEMVER-MINOR) add `--watch-kill-signal` flag (Dario Piotrowicz) #58719 PR-URL: #58993

RafaelGSS added the permission Issues and PRs related to the Permission Model label Jun 26, 2025

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jun 26, 2025

RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 26, 2025

RafaelGSS force-pushed the inherit-pm-to-child-process branch from a397ace to ecf6a03 Compare June 26, 2025 21:07

mcollina approved these changes Jun 26, 2025

View reviewed changes

RafaelGSS added request-ci Add this label to start a Jenkins CI on a PR. notable-change PRs with changes that should be highlighted in changelogs. labels Jun 27, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2025

RafaelGSS force-pushed the inherit-pm-to-child-process branch from ecf6a03 to 29b915a Compare June 27, 2025 18:06

jasnell reviewed Jun 27, 2025

View reviewed changes

RafaelGSS force-pushed the inherit-pm-to-child-process branch from 29b915a to 1f5354a Compare June 27, 2025 20:28

RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 30, 2025

JakobJingleheimer reviewed Jun 30, 2025

View reviewed changes

H4ad reviewed Jun 30, 2025

View reviewed changes

UlisesGascon approved these changes Jul 1, 2025

View reviewed changes

RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 2, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 2, 2025

nodejs-github-bot merged commit 8173d9d into nodejs:main Jul 2, 2025
60 checks passed

github-actions bot mentioned this pull request Jul 8, 2025

2025-07-09, Version 24.4.0 (Current) #58993

Merged

Uh oh!

permission: propagate permission model flags on spawn #58853

permission: propagate permission model flags on spawn #58853

Uh oh!

Conversation

RafaelGSS commented Jun 26, 2025

Uh oh!

nodejs-github-bot commented Jun 26, 2025

Uh oh!

RafaelGSS commented Jun 26, 2025

Uh oh!

codecov bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 27, 2025

Uh oh!

RafaelGSS commented Jun 27, 2025

Uh oh!

nodejs-github-bot commented Jun 27, 2025

Uh oh!

nodejs-github-bot commented Jun 27, 2025

Uh oh!

jasnell Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 28, 2025

Uh oh!

nodejs-github-bot commented Jun 30, 2025

Uh oh!

nodejs-github-bot commented Jun 30, 2025

Uh oh!

RafaelGSS commented Jun 30, 2025

Uh oh!

JakobJingleheimer Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

H4ad Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H4ad Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 2, 2025

Uh oh!

Uh oh!

codecov bot commented Jun 26, 2025 •

edited

Loading

RafaelGSS Jul 1, 2025 •

edited

Loading

marco-ippolito Jul 1, 2025 •

edited

Loading

RafaelGSS Jul 1, 2025 •

edited

Loading