Skip to content

permission: do not throw on denied access in audit mode#64398

Closed
edsadr wants to merge 1 commit into
nodejs:mainfrom
edsadr:permission-audit-no-throw
Closed

permission: do not throw on denied access in audit mode#64398
edsadr wants to merge 1 commit into
nodejs:mainfrom
edsadr:permission-audit-no-throw

Conversation

@edsadr

@edsadr edsadr commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

--permission-audit is intended to be an observe-only mode: it publishes
denial events to the node:permission-model:* diagnostics channels without
blocking the operation. Only --permission (enforce mode) should throw
ERR_ACCESS_DENIED.

The two C++ macros that gate every denied call site
(THROW_IF_INSUFFICIENT_PERMISSIONS for sync paths,
ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS for async paths) in
src/permission/permission.h were structured as:

Permission::ThrowAccessDenied(env__, perm__, resource__);   // runs ALWAYS
if (!warning_only()) return __VA_ARGS__;                     // only return is guarded

ThrowAccessDenied raises the V8 exception unconditionally — before the
warning_only() check. So in --permission-audit mode (where
warning_only_=true, set in src/env.cc) the exception is still thrown,
defeating the audit-only purpose of the flag. The diagnostics message is
published correctly (that happens inside Permission::is_scope_granted), but
then the API call wrongly throws instead of continuing.

This moves the throw/reject inside the !warning_only() branch for both macros.
In audit mode, the diagnostics-channel message is published and execution
continues; in enforce mode (--permission), behavior is unchanged.

Repro

echo hello > /tmp/aa.txt
cat > t.js <<'EOF'
const dc = require("node:diagnostics_channel");
dc.subscribe("node:permission-model:fs", (msg) => { console.log("LISTENED", msg); });
eval('require("fs").readFileSync("/tmp/aa.txt")');
EOF

node --permission-audit t.js   # before: LISTENED then ERR_ACCESS_DENIED throw
                               # after:  LISTENED only, exit 0
node --permission t.js         # unchanged: LISTENED then ERR_ACCESS_DENIED throw

Affected scopes

Every permission scope, both sync and async call sites — fs read/write, net,
child_process, worker, addon, ffi, inspector, wasi.

Refs: 9ddd1a9

The THROW_IF_INSUFFICIENT_PERMISSIONS and
ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS macros called
ThrowAccessDenied/AsyncThrowAccessDenied unconditionally, then only
guarded the `return` with `warning_only()`. As a result, running with
`--permission-audit` still threw ERR_ACCESS_DENIED on any denied
operation (fs, net, child_process, worker, addon, ffi, inspector,
wasi), defeating the audit-only purpose of the flag.

Move the throw/reject inside the `!warning_only()` branch so that, 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 thrown and
the call returns.

Refs: nodejs@9ddd1a9
Signed-off-by: Adrian Estrada <edsadr@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 9, 2026
@edsadr edsadr closed this Jul 9, 2026
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.25%. Comparing base (8fec65d) to head (d7ba2ac).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #64398   +/-   ##
=======================================
  Coverage   90.25%   90.25%           
=======================================
  Files         741      741           
  Lines      241207   241207           
  Branches    45430    45424    -6     
=======================================
+ Hits       217698   217704    +6     
+ Misses      15084    15068   -16     
- Partials     8425     8435   +10     
Files with missing lines Coverage Δ
src/permission/permission.h 100.00% <ø> (ø)

... and 34 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants