-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat: warn if define['process.env']
contains path
key with a value
#19517
base: main
Are you sure you want to change the base?
feat: warn if define['process.env']
contains path
key with a value
#19517
Conversation
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 like this approach! I think the implementation looks good to me, but I wonder if it's better to always warn if "process.env"
is passed with any non-empty object 🤔 They could use "process.env.key"
keys if they want to replace a specific key-value.
I'd prefer to keep it as-is to reduce the false positives. |
Hey @sapphi-red, Good job, i like your approach too 🤝 |
@sapphi-red While I believe this is a step in the right direction but I do believe that changing the doc would be more beneficial, even with a warning some people will still ignore it or don't read it. The best way to save those people is to not have a problematic code that they can copy and paste. I believe that @qu35t-code suggestion provides a safer snippet that will not be problematic for users that don't read the documentation or the warning and would sill allow conscious developer to achieve what they want if required. |
@Techbrunch I replied at #19510 (comment) to keep this PR focused on the warning. |
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.
Leaving an approval for now and we can tweak the heuristic or warning again if needed.
Description
Added a warning if
define['process.env']
containspath: "some value"
.There are a lot of people passing
'process.env': process.env
(search). This shouldn't be done because it may expose all the env vars, which may contain access tokens.There were some PRs / issues that suggests to update the docs (#16686, #18441, #19510), but I don't think that can prevent people from doing it because the reason they did was because they didn't read the docs.
This PR adds a warning so that they can notice even without reading the docs.
The warning is output based on the following assumption:
Path
/PATH
/path
is declared on the machine and has a non-empty value: I believe this holds true in most casesprocess.env.path
is not expected to be replaced: in this case the added warning will be output even if they only havepath: "intended-value"
. But I don't think this will likely to happen and they can workaround by setting'process.env.path': 'value'
instead of'process.env': { path: 'value' }
.close #18441
close #19510