-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
permission: add initial environment permission #48077
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's great that enumerating properties of process.env
throws. (Then again, it's already bad for DX that asynchronous file system APIs can now also throw synchronous permission errors, in addition to existing asynchronous permission errors.)
On the other hand, hiding environment variables can be problematic, too, of course.
Deno chose a somewhat elegant approach by not making Deno.env
an object with enumerable environment variables. Deno.env.toObject()
requires a special "all" permission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned by @tniessen, since we don't have a standard way to have access to env properties besides enumerated properties. I think this is the best we can do.
Regarding synchronous throws in asynchronous API. We can adjust it, but, permission is not the only throw-sync check that runs in async APIs (CHECK_NULL).
Amazing work! I'll review it with attention later. Why is it a draft? what's missing?
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
}, | ||
} = internalBinding('util'); | ||
|
||
const childOrPrimary = 'NODE_UNIQUE_ID' in process[env_private_symbol] ? 'child' : 'primary'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to use ObjectHasOwn for this, since potentially anyone could add NODE_UNIQUE_ID
to the prototype chain.
const childOrPrimary = 'NODE_UNIQUE_ID' in process[env_private_symbol] ? 'child' : 'primary'; | |
const childOrPrimary = ObjectHasOwn(process[env_private_symbol], 'NODE_UNIQUE_ID') ? 'child' : 'primary'; |
I feel like this might need a bit more thinking of how to handle enumeration/cloning of the env object safely. There's lots of cases of the |
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
Sorry delay, I didn't have time to jump into this discussion.
I 100% agree. Honestly, after thinking for some time, I don't see a security reason to block the |
Well, the env could contain sensitive data such as AWS credentials so I do think restricting access could make sense. Though as has already been expressed, there are tools to do that externally. I think this may be a case of a solution looking for a problem though. I would suggest we hold off for now and reconsider later if users ask for it or a clear need comes up. New features can be added any time, but removing them is incredibly difficult. |
I'd like to respectfully step back from my original thought and express my agreement with everyone's opinion. I think it's appropriate to hold this request. Thank you for taking the time to think about this together. 🙏 |
Add initial environment permission support. This restricts permission to access environment variables through
process.env
by using--allow-env
flag.usages
Proposed basic usages are as follows:
--allow-env
: All environment variables are allowed.--allow-env=HOST
:HOST
is allowed.--allow-env=HOST,PORT
:HOST
andPORT
are allowed.--allow-env=DB_*
: Env vars starting withDB_
are allowed.--allow-env=DB_*,-DB_PASSWORD
: All env vars starting withDB_
exceptDB_PASSWORD
are allowed.--allow-env=*,-DB_PASSWORD
: All env vars exceptDB_PASSWORD
are allowed.process[env_private_symbol]
This is based on the idea of using a new privileged API for builtins to access the environment variables instead of
process.env
. It preserves current behaviors ofprocess.env
on the child processes and the worker threads by leveraging the existing native traps. This approach required manual changes to all internal uses of it.WIP:
-
.Refs: nodejs/security-wg#993
Signed-off-by: Daeyeon Jeong [email protected]