permission: add --allow-env flag for environment variable access control#62827
permission: add --allow-env flag for environment variable access control#62827nabeel378 wants to merge 9 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
nabeel378
force-pushed
the
feat/permission-allow-env-flag
branch
from
9c74582 to
f3544f8
Compare
Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
…r environment variables Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62827 +/- ##
==========================================
- Coverage 89.69% 89.53% -0.16%
==========================================
Files 706 708 +2
Lines 218222 219086 +864
Branches 41768 41914 +146
==========================================
+ Hits 195731 196166 +435
- Misses 14411 14796 +385
- Partials 8080 8124 +44
🚀 New features to boost your workflow:
|
Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
|
|
||
| > Stability: 1.1 - Active development | ||
|
|
||
| When using the [Permission Model][], access to environment variables is |
There was a problem hiding this comment.
This paragraph and the console block below it describe a deny-by-default model that the code does not implement
./out/Release/node --permission '--allow-fs-read=*' -p 'process.env.HOME'
/Users/thisalihassanThere was a problem hiding this comment.
Good catch! You're right, env vars are unrestricted by default with --permission. They only get restricted when --allow-env is explicitly passed. Updated both cli.md and process.md with the correct description and working examples.
| ``` | ||
|
|
||
| ```console | ||
| $ node --permission index.js |
There was a problem hiding this comment.
As written, this output is unreachable
./out/Release/node --permission test.js
/Users/thisalihassan| The `process.env` property returns an object containing the user environment. | ||
| See environ(7). | ||
|
|
||
| When the [Permission Model][] is enabled, access to environment variables is |
There was a problem hiding this comment.
Same issue as cli.md
The documentation incorrectly described env var restriction as deny-by-default when --permission is used. In reality, env vars are unrestricted by default and only become restricted when --allow-env is explicitly passed. Update cli.md and process.md to accurately reflect this behavior. Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
|
I don't think this PR as it stands closes #62424, the issue asked for a deny-by-default model with throw-on-read, matching Deno and consistent with the rest of the permission model which doesn't defend against the threat the issue was explicitly motivated by The PR description is no longer accurate according to the current opt-in model |
I missed that. Switching to deny-by-default to stay consistent with the other permission flags and update desc proper. |
RafaelGSS
requested review from
RafaelGSS
and removed request for
thisalihassan
Signed-off-by: nabeel378 <mohammadnabeeljameel@gmail.com>
4 participants
Adds --allow-env permission flag to control access to environment variables when the permission model is enabled (--permission).
Supported usage:
When --permission is enabled without --allow-env, reading a restricted variable silently returns undefined, writing or deleting throws ERR_ACCESS_DENIED, and enumerating (Object.keys(process.env)) returns only allowed variables.
Fixes: #62424