-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
permission: propagate permission model flags on spawn #58853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,8 @@ const { | |
|
|
||
| const MAX_BUFFER = 1024 * 1024; | ||
|
|
||
| const permission = require('internal/process/permission'); | ||
|
|
||
| const isZOS = process.platform === 'os390'; | ||
| let addAbortListener; | ||
|
|
||
|
|
@@ -536,6 +538,31 @@ function copyProcessEnvToEnv(env, name, optionEnv) { | |
| } | ||
| } | ||
|
|
||
| let permissionModelFlagsToCopy; | ||
|
|
||
| function getPermissionModelFlagsToCopy() { | ||
| if (permissionModelFlagsToCopy === undefined) { | ||
| permissionModelFlagsToCopy = [...permission.availableFlags(), '--permission']; | ||
| } | ||
| return permissionModelFlagsToCopy; | ||
| } | ||
|
|
||
| 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)) { | ||
| return; | ||
| } | ||
|
|
||
| const flagsToCopy = getPermissionModelFlagsToCopy(); | ||
| for (const arg of process.execArgv) { | ||
| for (const flag of flagsToCopy) { | ||
| if (arg.startsWith(flag)) { | ||
| env[key] = `${env[key] ? env[key] + ' ' + arg : arg}`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a safe operation? I mean, if I do something like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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())In any case, escaping characters is the user's responsibility, so I wouldn't worry much about it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm... I thought "\ " was not allowed, but it seems it is. |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let emittedDEP0190Already = false; | ||
| function normalizeSpawnArguments(file, args, options) { | ||
| validateString(file, 'file'); | ||
|
|
@@ -652,7 +679,8 @@ function normalizeSpawnArguments(file, args, options) { | |
| ArrayPrototypeUnshift(args, file); | ||
| } | ||
|
|
||
| const env = options.env || process.env; | ||
| // Shallow copy to guarantee changes won't impact process.env | ||
| const env = options.env || { ...process.env }; | ||
| const envPairs = []; | ||
|
|
||
| // process.env.NODE_V8_COVERAGE always propagates, making it possible to | ||
|
|
@@ -672,6 +700,10 @@ function normalizeSpawnArguments(file, args, options) { | |
| copyProcessEnvToEnv(env, '_EDC_SUSV3', options.env); | ||
| } | ||
|
|
||
| if (permission.isEnabled()) { | ||
| copyPermissionModelFlagsToEnv(env, 'NODE_OPTIONS', args); | ||
| } | ||
|
|
||
| let envKeys = []; | ||
| // Prototype values are intentionally included. | ||
| for (const key in env) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,4 +33,15 @@ module.exports = ObjectFreeze({ | |
|
|
||
| return permission.has(scope, reference); | ||
| }, | ||
| availableFlags() { | ||
| return [ | ||
| '--allow-fs-read', | ||
| '--allow-fs-write', | ||
| '--allow-addons', | ||
| '--allow-child-process', | ||
| '--allow-net', | ||
| '--allow-wasi', | ||
| '--allow-worker', | ||
|
Comment on lines
+38
to
+44
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: If sequence doesn't matter, I think it would be better (grokability) to alphabetise them. |
||
| ]; | ||
| }, | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| // Flags: --permission --allow-child-process --allow-fs-read=* --allow-worker | ||
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| const { isMainThread } = require('worker_threads'); | ||
|
|
||
| if (!isMainThread) { | ||
| common.skip('This test only works on a main thread'); | ||
| } | ||
|
|
||
| const assert = require('assert'); | ||
| const childProcess = require('child_process'); | ||
|
|
||
| { | ||
| assert.ok(process.permission.has('child')); | ||
| } | ||
|
|
||
| { | ||
| assert.strictEqual(process.env.NODE_OPTIONS, undefined); | ||
| } | ||
|
|
||
| { | ||
| const { status, stdout } = childProcess.spawnSync(process.execPath, | ||
| [ | ||
| '-e', | ||
| ` | ||
| console.log(process.permission.has("fs.write")); | ||
| console.log(process.permission.has("fs.read")); | ||
| console.log(process.permission.has("child")); | ||
| console.log(process.permission.has("net")); | ||
| console.log(process.permission.has("worker")); | ||
| `, | ||
| ] | ||
| ); | ||
| const [fsWrite, fsRead, child, net, worker] = stdout.toString().split('\n'); | ||
| assert.strictEqual(status, 0); | ||
| assert.strictEqual(fsWrite, 'false'); | ||
| assert.strictEqual(fsRead, 'true'); | ||
| assert.strictEqual(child, 'true'); | ||
| assert.strictEqual(net, 'false'); | ||
| assert.strictEqual(worker, 'true'); | ||
| } | ||
|
|
||
| // It should not override when --permission is passed | ||
| { | ||
| const { status, stdout } = childProcess.spawnSync( | ||
| process.execPath, | ||
| [ | ||
| '--permission', | ||
| '--allow-fs-write=*', | ||
| '-e', | ||
| ` | ||
| console.log(process.permission.has("fs.write")); | ||
| console.log(process.permission.has("fs.read")); | ||
| console.log(process.permission.has("child")); | ||
| console.log(process.permission.has("net")); | ||
| console.log(process.permission.has("worker")); | ||
| `, | ||
| ] | ||
| ); | ||
| const [fsWrite, fsRead, child, net, worker] = stdout.toString().split('\n'); | ||
| assert.strictEqual(status, 0); | ||
| assert.strictEqual(fsWrite, 'true'); | ||
| assert.strictEqual(fsRead, 'false'); | ||
| assert.strictEqual(child, 'false'); | ||
| assert.strictEqual(net, 'false'); | ||
| assert.strictEqual(worker, 'false'); | ||
| } | ||
|
|
||
| // It should not override when NODE_OPTIONS with --permission is passed | ||
| { | ||
| const { status, stdout } = childProcess.spawnSync( | ||
| process.execPath, | ||
| [ | ||
| '-e', | ||
| ` | ||
| console.log(process.permission.has("fs.write")); | ||
| console.log(process.permission.has("fs.read")); | ||
| console.log(process.permission.has("child")); | ||
| console.log(process.permission.has("net")); | ||
| console.log(process.permission.has("worker")); | ||
| `, | ||
| ], | ||
| { | ||
| env: { | ||
| ...process.env, | ||
| 'NODE_OPTIONS': '--permission --allow-fs-write=*', | ||
| } | ||
| } | ||
| ); | ||
| const [fsWrite, fsRead, child, net, worker] = stdout.toString().split('\n'); | ||
| assert.strictEqual(status, 0); | ||
| assert.strictEqual(fsWrite, 'true'); | ||
| assert.strictEqual(fsRead, 'false'); | ||
| assert.strictEqual(child, 'false'); | ||
| assert.strictEqual(net, 'false'); | ||
| assert.strictEqual(worker, 'false'); | ||
| } | ||
|
|
||
| { | ||
| assert.strictEqual(process.env.NODE_OPTIONS, undefined); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this expression a bit with...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? We need to compare the result with
!== -1and also handle the case when the key isundefined. Leaving it asenv?.[key]?.indexOf('--permission')might return0, which is truthy in this case, so I don't see how usingenv?.[key]?.indexOf(...)would simplify this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible with the dark arts:
Not saying you should do this, but you can.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are correct
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is
~undefinedreturns -1 which is truthy (just like when it is being found), so ~ doesnt work well in this case. I had the same thought