permission: keep parent allowlist when Worker execArgv is empty - #65359
permission: keep parent allowlist when Worker execArgv is empty#65359yunshingng wants to merge 5 commits into
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
There was a problem hiding this comment.
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.
fc2103a to
c7d385d
Compare
C++-side intersection after Worker option parse when the parent has the Permission Model enabled: - No JS process.execArgv copying (avoids NODE_OPTIONS / repeated-flag gaps) - If the worker did not configure permission flags (e.g. execArgv: []), effective grants become the parent grant set - If the worker configured permission flags, boolean and fs grants are intersected with the parent so the worker cannot exceed the parent - Non-permission execArgv differences remain possible May be semver-major relative to documented non-inheritance; for reviewer call. Signed-off-by: yunshingng <yunshingng25@gmail.com>
c7d385d to
bca44e6
Compare
- Normalize path boundary checks for allow-list intersection - Rewrite permission-related exec_argv to match clamped options - Preserve non-permission execArgv entries - Expand tests: --no-warnings, allow path success, repeated allows Signed-off-by: yunshingng <yunshingng25@gmail.com>
|
Thanks — agreed this should be treated as semver-major if we change the The intent of the current diff is not a patch-level fix and not a security That avoids the incomplete JS Please treat / label this PR as semver-major. I’m happy to adjust the |
Replace merged/broken helper text with a single clean implementation: complete WorkerConfiguredPermission, one path-intersection loop, one clamp call site, and exec_argv rewrite consistent with options. Signed-off-by: yunshingng <yunshingng25@gmail.com>
- Normalize allow-list paths with PathResolve before prefix checks - Case-insensitive path prefix matching on Windows - Strip space-separated --allow-fs-read/--allow-fs-write path tokens from exec_argv when rewriting permission flags Signed-off-by: yunshingng <yunshingng25@gmail.com>
Avoid treating longer distinct options that share a prefix (e.g. --allow-fs-read-extra) as permission args when stripping/rewriting exec_argv. Signed-off-by: yunshingng <yunshingng25@gmail.com>
3 participants
Description
Under
--permission, creating aWorkerwithexecArgv: []could drop the parent's filesystem allowlist compared to a defaultWorker.This change re-attaches parent Permission Model flags when
execArgvis provided explicitly (including an empty array).Test plan
test/parallel/test-permission-worker-empty-execargv.jscc @RafaelGSS