diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js index 92e54fbab493..f3ed889e8c19 100644 --- a/lib/internal/process/permission.js +++ b/lib/internal/process/permission.js @@ -53,7 +53,9 @@ module.exports = ObjectFreeze({ isAuditMode() { if (_audit === undefined) { const { getOptionValue } = require('internal/options'); - _audit = getOptionValue('--permission-audit'); + // --permission takes precedence over --permission-audit. + _audit = getOptionValue('--permission-audit') && + !getOptionValue('--permission'); } return _audit; }, diff --git a/src/env.cc b/src/env.cc index 33e2121d45f9..0dd9372d4385 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1111,7 +1111,9 @@ Environment::Environment(IsolateData* isolate_data, if (options_->permission || options_->permission_audit) { permission()->EnablePermissions(); static const std::array args = {std::string("*")}; - if (options_->permission_audit) { + // --permission takes precedence over --permission-audit. + const bool audit_only = options_->permission_audit && !options_->permission; + if (audit_only) { permission()->EnableWarningOnly(); } // The process shouldn't be able to neither @@ -1121,7 +1123,7 @@ Environment::Environment(IsolateData* isolate_data, // In audit mode addon loading must stay enabled: the denial is // published through the diagnostics channel by the permission // check in DLOpen() instead of being rejected upfront. - if (!options_->permission_audit) { + if (!audit_only) { options_->allow_native_addons = false; } permission()->Apply(this, args, permission::PermissionScope::kAddon); diff --git a/src/node.cc b/src/node.cc index f7a19747bed6..b66e0cdfd0da 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1416,7 +1416,8 @@ InitializeOncePerProcessInternal( permission::ParseEnvAllowList(env_options->allow_env); const bool allow_all = std::find(allow.begin(), allow.end(), "*") != allow.end(); - if (env_options->permission_audit && !allow_all) { + if (env_options->permission_audit && !env_options->permission && + !allow_all) { // In audit mode nothing is removed, but accesses to the variables that // enforcing the permission model would remove are published. permission::RecordAuditedEnvironmentVariables(allow); diff --git a/test/parallel/test-permission-audit-fs-does-not-deny.js b/test/parallel/test-permission-audit-fs-does-not-deny.js index 13284802839d..d031fc323c81 100644 --- a/test/parallel/test-permission-audit-fs-does-not-deny.js +++ b/test/parallel/test-permission-audit-fs-does-not-deny.js @@ -57,3 +57,31 @@ test('permission-audit logs fs denial without throwing', () => { test('permission-audit logs fs denial without throwing (eval)', () => { runAudit('eval'); }); + +// --permission takes precedence over --permission-audit, so the read is denied. +function runEnforced(args, extraEnv) { + const { status, stdout, stderr } = spawnSync( + process.execPath, + [...args, '-e', ` + try { + require('node:fs').readFileSync(${JSON.stringify(blockedFile)}); + console.log('RESULT NO_THROW'); + } catch (e) { + console.log('RESULT THREW ' + e.code); + } + `], + { encoding: 'utf8', env: { ...process.env, ...extraEnv } }, + ); + assert.strictEqual(status, 0, stderr); + assert.ok(stdout.split('\n').includes('RESULT THREW ERR_ACCESS_DENIED'), + stdout); +} + +test('--permission takes precedence over --permission-audit', () => { + runEnforced(['--permission', '--permission-audit']); +}); + +test('--permission takes precedence over --permission-audit in NODE_OPTIONS', () => { + runEnforced(['--permission'], { NODE_OPTIONS: '--permission-audit' }); + runEnforced(['--permission-audit'], { NODE_OPTIONS: '--permission' }); +}); diff --git a/test/parallel/test-permission-env-scrub.js b/test/parallel/test-permission-env-scrub.js index c5d86a87f7a7..26a6e71fedfa 100644 --- a/test/parallel/test-permission-env-scrub.js +++ b/test/parallel/test-permission-env-scrub.js @@ -125,6 +125,16 @@ function run(...flags) { assert.strictEqual(result.hasAll, false); } +// --permission takes precedence over --permission-audit. +{ + const result = run( + '--permission', '--permission-audit', '--allow-env=PERMISSION_ENV_ALLOWED'); + assert.deepStrictEqual(result.keys, ['PERMISSION_ENV_ALLOWED', 'TZ']); + assert.strictEqual(result.has.PERMISSION_ENV_SECRET, false); + assert.strictEqual(result.has.PERMISSION_ENV_ALLOWED, true); + assert.strictEqual(result.hasAll, false); +} + // --allow-env requires the permission model. spawnSyncAndExit( process.execPath,