From bf78f9b9af7bd98f37b5c7fb5854eb554215219a Mon Sep 17 00:00:00 2001 From: Adrian Estrada Date: Thu, 9 Jul 2026 16:42:00 -0500 Subject: [PATCH] src,permission: do not throw on denied access in audit mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The THROW_IF_INSUFFICIENT_PERMISSIONS and ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS macros called ThrowAccessDenied/AsyncThrowAccessDenied unconditionally and only guarded the `return` with `warning_only()`. ERR_ACCESS_DENIED_IF_INSUFFICIENT_PERMISSIONS had no `warning_only()` guard at all — it always set the access-denied error and returned. As a result, running with `--permission-audit` still produced ERR_ACCESS_DENIED on any denied operation (fs, net, child_process, worker, addon, ffi, inspector, wasi), defeating the audit-only purpose of the flag. Guard the denied-error path behind `!warning_only()` in all three macros. In audit mode, the diagnostics-channel message is published (already done in Permission::is_scope_granted) and execution continues; in enforce mode (`--permission`), behavior is unchanged — the error is raised and the call returns. The tests cover both the direct (top-level) call and an `eval()`-wrapped call: the direct call exercises the normal script path, and the `eval()`-wrapped call exercises the V8 script-context boundary (the diagnostics subscriber is registered in the outer module context while the denied operation runs inside an eval'd string). Refs: https://github.com/nodejs/node/commit/9ddd1a9c27c253f46d587a8c906ccd83417b4606 Signed-off-by: Adrian Estrada --- src/permission/permission.h | 33 +++++---- .../test-permission-audit-fs-does-not-deny.js | 58 ++++++++++++++++ ...test-permission-audit-net-does-not-deny.js | 69 +++++++++++++++++++ 3 files changed, 147 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-permission-audit-fs-does-not-deny.js create mode 100644 test/parallel/test-permission-audit-net-does-not-deny.js diff --git a/src/permission/permission.h b/src/permission/permission.h index 84e3ea67ed5e04..9b6b1dceaa173e 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -37,9 +37,11 @@ namespace permission { const auto resource__ = (resource); \ if (!env__->permission()->is_granted(env__, perm__, resource__)) \ [[unlikely]] { \ - node::permission::Permission::ThrowAccessDenied( \ - env__, perm__, resource__); \ - if (!env__->permission()->warning_only()) return __VA_ARGS__; \ + if (!env__->permission()->warning_only()) { \ + node::permission::Permission::ThrowAccessDenied( \ + env__, perm__, resource__); \ + return __VA_ARGS__; \ + } \ } \ } while (0) @@ -51,9 +53,11 @@ namespace permission { const auto resource__ = (resource); \ if (!env__->permission()->is_granted(env__, perm__, resource__)) \ [[unlikely]] { \ - node::permission::Permission::AsyncThrowAccessDenied( \ - env__, (wrap), perm__, resource__); \ - if (!env__->permission()->warning_only()) return __VA_ARGS__; \ + if (!env__->permission()->warning_only()) { \ + node::permission::Permission::AsyncThrowAccessDenied( \ + env__, (wrap), perm__, resource__); \ + return __VA_ARGS__; \ + } \ } \ } while (0) @@ -65,14 +69,17 @@ namespace permission { const auto resource__ = (resource); \ if (!env__->permission()->is_granted(env__, perm__, resource__)) \ [[unlikely]] { \ - Local err_access; \ - if (node::permission::CreateAccessDeniedError(env__, perm__, resource__) \ - .ToLocal(&err_access)) { \ - args.GetReturnValue().Set(err_access); \ - } else { \ - args.GetReturnValue().Set(UV_EACCES); \ + if (!env__->permission()->warning_only()) { \ + Local err_access; \ + if (node::permission::CreateAccessDeniedError(env__, perm__, \ + resource__) \ + .ToLocal(&err_access)) { \ + args.GetReturnValue().Set(err_access); \ + } else { \ + args.GetReturnValue().Set(UV_EACCES); \ + } \ + return __VA_ARGS__; \ } \ - return __VA_ARGS__; \ } \ } while (0) diff --git a/test/parallel/test-permission-audit-fs-does-not-deny.js b/test/parallel/test-permission-audit-fs-does-not-deny.js new file mode 100644 index 00000000000000..4fcc07b1952252 --- /dev/null +++ b/test/parallel/test-permission-audit-fs-does-not-deny.js @@ -0,0 +1,58 @@ +'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 { spawnSync } = require('child_process'); +const { test } = require('node:test'); +const fixtures = require('../common/fixtures'); + +const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md'); + +function runAudit(mode) { + const childScript = ` + const dc = require('node:diagnostics_channel'); + const msgs = []; + dc.subscribe('node:permission-model:fs', (m) => msgs.push({ + permission: m.permission, + resource: m.resource, + })); + try { + ${mode === 'eval' ? + `eval('require("node:fs").readFileSync(${JSON.stringify(blockedFile)})');` : + `require('node:fs').readFileSync(${JSON.stringify(blockedFile)});`} + console.log('RESULT NO_THROW'); + } catch (e) { + console.log('RESULT THREW ' + e.code); + } + console.log('AUDIT ' + JSON.stringify(msgs)); + `; + + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--permission-audit', '-e', childScript], + { encoding: 'utf8' }, + ); + assert.strictEqual(status, 0, stderr); + const lines = stdout.split('\n'); + assert.ok(lines.includes('RESULT NO_THROW'), stdout); + const auditLine = lines.find((l) => l.startsWith('AUDIT ')); + assert.ok(auditLine, stdout); + const msgs = JSON.parse(auditLine.replace('AUDIT ', '')); + assert.strictEqual(msgs.length, 1); + assert.strictEqual(msgs[0].permission, 'FileSystemRead'); + assert.ok(msgs[0].resource.endsWith('protected-file.md')); +} + +test('permission-audit logs fs denial without throwing', () => { + runAudit('direct'); +}); + +test('permission-audit logs fs denial without throwing (eval)', () => { + runAudit('eval'); +}); diff --git a/test/parallel/test-permission-audit-net-does-not-deny.js b/test/parallel/test-permission-audit-net-does-not-deny.js new file mode 100644 index 00000000000000..4fdaaf3494c95e --- /dev/null +++ b/test/parallel/test-permission-audit-net-does-not-deny.js @@ -0,0 +1,69 @@ +'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 { spawnSync } = require('child_process'); +const { test } = require('node:test'); +const net = require('net'); + +async function runAudit(mode) { + const server = net.createServer(); + + await new Promise((resolve, reject) => { + server.on('error', reject); + server.listen(0, '127.0.0.1', resolve); + }); + const { port } = server.address(); + const env = { ...process.env, PORT: String(port), HOST: '127.0.0.1' }; + + const childScript = ` + const dc = require('node:diagnostics_channel'); + const net = require('node:net'); + const msgs = []; + dc.subscribe('node:permission-model:net', (m) => msgs.push({ + permission: m.permission, + resource: m.resource, + })); + const s = ${mode === 'eval' ? + `eval('net.connect(Number(process.env.PORT), process.env.HOST)')` : + `net.connect(Number(process.env.PORT), process.env.HOST)`}; + s.on('connect', () => { s.destroy(); console.log('RESULT CONNECTED'); }); + s.on('error', (e) => { console.log('RESULT ERROR ' + e.code); }); + s.on('close', () => { + console.log('AUDIT ' + JSON.stringify(msgs)); + }); + `; + + try { + const { status, stdout, stderr } = spawnSync( + process.execPath, + ['--permission-audit', '-e', childScript], + { encoding: 'utf8', env }, + ); + assert.strictEqual(status, 0, stderr); + const lines = stdout.split('\n'); + assert.strictEqual(lines[0], 'RESULT CONNECTED', stdout); + const auditLine = lines.find((l) => l.startsWith('AUDIT ')); + assert.ok(auditLine, stdout); + const msgs = JSON.parse(auditLine.replace('AUDIT ', '')); + assert.strictEqual(msgs.length, 1); + assert.strictEqual(msgs[0].permission, 'Net'); + assert.ok(msgs[0].resource.includes('127.0.0.1')); + } finally { + server.close(); + } +} + +test('permission-audit logs net denial without blocking connect', async () => { + await runAudit('direct'); +}); + +test('permission-audit logs net denial without blocking connect (eval)', async () => { + await runAudit('eval'); +});