permission: keep parent allowlist when Worker execArgv is empty

[yunshingng]

Description

Under --permission, creating a Worker with execArgv: [] could drop the parent's filesystem allowlist compared to a default Worker.

This change re-attaches parent Permission Model flags when execArgv is provided explicitly (including an empty array).

Test plan

• test/parallel/test-permission-worker-empty-execargv.js
• CI

cc @RafaelGSS

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg

[RafaelGSS]

Thanks for the PR! However, I'm not sure this is something we should fix. The permission model doesn't inherit to worker threads by design.

--allow-child-process: if you grant it, you are trusting that code. We've been closing worker "bypass" reports as documented limitations for that reason.

Modifying execArgv is also a legit use case (giving the worker different flags than the parent), and this PR would remove that - we could argue that's a semver-major.

Even if we wanted inheritance, I don't think this approach works. Flags from NODE_OPTIONS don't show up in process.execArgv, so they wouldn't be copied at all. Repeated flags like --allow-fs-read=/a --allow-fs-read=/b only copy the first value (bug on this implementation)

So it gives the impression of inheritance without actually guaranteeing it, which IMO is worse than the current documented behavior. Doing this properly would mean enforcing it on the C++ side (intersection with the parent options) and it would likely be semver-major.

[yunshingng]

Thanks for the review!— agreed this should be treated as semver-major if we change the
behavior.

The intent of the current diff is not a patch-level fix and not a security
advisory fix. It’s a deliberate model change: when the parent has the
Permission Model enabled, a Worker’s permission-related grants must not
exceed the parent (C++ intersection after option parse), including the
execArgv: [] case.

That avoids the incomplete JS execArgv copying approach (NODE_OPTIONS,
repeated --allow-*, false sense of inheritance).

Please treat / label this PR as semver-major. I’m happy to adjust the
implementation or tests for the major-line process you prefer.

[yunshingng]

Some bug fixed , mostly done

[yunshingng]

re: "doesn't inherit to a worker thread" — that's actually inconsistent with what the worker_threads doc says. It describes execArgv itself as "By default, options are inherited from the parent thread," and separately says workers pick up the parent's CLI flags automatically as long as you don't touch execArgv. --allow-fs-read etc are CLI flags, so a default Worker inheriting them is literally the documented behavior on that page. not arguing to change the design here, just that the two docs contradict each other right now regardless of what we land on.

separately: omit execArgv and you keep the parent's allowlist, pass execArgv: [] and you lose it. that's backwards — the more explicit/careful-looking code ends up less safe than doing nothing. feels worth fixing on its own, independent of the bigger inheritance question.

also curious — did this specific execArgv: [] vs omitted case come up in the past "documented limitation" closures, or were those about full inheritance? if it's the latter I don't think that precedent covers this one as-is.

[yunshingng]

To keep this focused: I’m not trying to win a docs debate.

The case I think is worth a decision on its own is only:

• omit execArgv → parent allowlist applies
• execArgv: [] → Permission Model grants can be dropped

That’s surprising under --permission. Full “always inherit everything”
can stay out of scope.

I’m treating this PR as a semver-major C++ ceiling for that footgun and
am happy to keep iterating on the implementation if that direction is
acceptable.

[yunshingng]

@RafaelGSS , All updates and test fixes are complete and pushed!

[codecov]

Codecov Report

❌ Patch coverage is 58.26087% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.09%. Comparing base (cf30b2e) to head (e1839d5).
⚠️ Report is 164 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_worker.cc	58.26%	19 Missing and 29 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65359      +/-   ##
==========================================
- Coverage   90.11%   90.09%   -0.02%     
==========================================
  Files         752      752              
  Lines      251861   252412     +551     
  Branches    47365    47468     +103     
==========================================
+ Hits       226955   227421     +466     
- Misses      16238    16280      +42     
- Partials     8668     8711      +43     

Files with missing lines	Coverage Δ
src/node_worker.cc	79.34% <58.26%> (-2.48%)	⬇️

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

[RafaelGSS]

Thanks for reworking this. The C++ intersection is the right shape, and it resolves my earlier points: because it clamps the parsed options instead of copying process.execArgv, NODE_OPTIONS-derived flags and repeated --allow-fs-read / -write

My remaining concern isn't the implementation, it's whether we want this at all. Today the model explicitly does not inherit to workers, and we've been closing worker "bypass" reports as documented limitations on that basis.

This PR reverses that contract and rewrites the docs to match, so it's a deliberate semver-major behavior change rather than a bug fix.

I'm still not confident we want this behavior to change, since it also changes how we triage existing and future reports in this area.

If we do go ahead, one implementation note: ApplyParentPermissionCeiling, IntersectPermissionGrants, and IsPermissionCliToken each list the permission dimensions by hand.

They match today, but a future --allow-* that misses one of them would let an xplicit-execArgv worker exceed the parent

[yunshingng]

Thanks for the review. Technical status first, then a direct answer to the triage concern, since I think that's the real crux of what's still unresolved.

Technical: all prior points are addressed — permission dimensions now come from a single macro table instead of three hand-maintained lists, the path-resolution mismatch (validating a resolved path but storing the raw one) is fixed, the ["*", ] asymmetry is fixed and covered by a new regression test, and space-form flag parsing no longer mishandles paths starting with -.

On triage: I think what you're getting at is bigger than "is this specific fix correct" — it's that approving this sets a precedent for how similar reports get judged going forward, and that's harder to walk back than the code itself. That's a fair thing to be cautious about, especially since Permission Model is Stability: 2 (stable) — this is a semver-major change on a stable surface, not something with room to casually adjust later.

What I'd propose is making that precedent as narrow as possible, in writing, so it doesn't become an open-ended shift in how "bypass" reports get evaluated:

Workers not auto-inheriting permission grants remains a documented design decision, closed as such — except when two calling conventions that should be equivalent (omitting execArgv vs. passing an empty or non-permission-affecting execArgv) produce different grants. That specific inconsistency is a bug, not a documented limitation.

General "workers don't inherit" reports would keep closing exactly as they do today. Only this one specific asymmetry gets carved out, and it's a fixed rule rather than something re-evaluated case by case. I can add this to CONTRIBUTING or wherever it belongs before merge, so the boundary is explicit rather than left to interpretation next time a similar report comes in.