permission: prefer --permission over audit mode

When both --permission and --permission-audit are set, run in enforce
mode as documented, instead of only reporting denials and keeping the
environment variables.

Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: #67036
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>

Author: RafaelGSS

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;
   },

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

src/node.cc

@@ -1412,7 +1412,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);

test/parallel/test-permission-audit-fs-does-not-deny.js

@@ -57,3 +57,34 @@ 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', {
+  skip: process.config.variables.node_without_node_options &&
+    'missing NODE_OPTIONS support',
+}, () => {
+  runEnforced(['--permission'], { NODE_OPTIONS: '--permission-audit' });
+  runEnforced(['--permission-audit'], { NODE_OPTIONS: '--permission' });
+});

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,