-
Notifications
You must be signed in to change notification settings - Fork 940
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
Fix exposing of .env variables in Create React App #560
base: master
Are you sure you want to change the base?
Conversation
1 similar comment
I'm 👎 on this. It's adding very specific checks where this is really a shortcoming in electron. Is this even still an issue? I know this is kind of an old PR. |
Yup - definitely still an issue. It's not an electron issue -- the code referenced above ends up in CRA bundle that's consumed by the browser, thereby exposing the entirety of CRA app's various I'd be happy to submit an update if there is interest. |
Hi, sorry it took so long to get back. Could you rebase please? I'm okay with adding this check. |
merge from upstream
@@ -14,7 +14,7 @@ module.exports = function (config) { | |||
// Test results reporter to use | |||
// possible values: 'dots', 'progress' | |||
// available reporters: https://npmjs.org/browse/keyword/karma-reporter | |||
reporters: ['progress'], | |||
reporters: ['mocha'], |
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.
So we can see that browser env was selected.
package.json
Outdated
@@ -25,9 +25,10 @@ | |||
"license": "MIT", | |||
"scripts": { | |||
"lint": "xo", | |||
"test": "npm run test:node && npm run test:browser", | |||
"test": "npm --node run test:node && npm run test:browser && npm --electron run test:electron", |
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.
--node
and --electron
get passed to test as process.env.npm_config_node
and process.env.npm_config_electron
.
} catch (error) { | ||
// Swallow | ||
// XXX (@Qix-) should we be logging these? | ||
} | ||
|
||
// If debug isn't set in LS, and we're in Electron, try to load $DEBUG | ||
if (!r && typeof process !== 'undefined' && 'env' in process) { |
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.
This is the critical part that was exposing process.env
in the browser. It's been pulled out into the electron-specific flow.
package.json
Outdated
"test:browser": "karma start --single-run", | ||
"test:node": "istanbul cover --dir coverage/node -x test.js node_modules/mocha/bin/_mocha", | ||
"test:electron": "istanbul cover --dir coverage/electron -x test.js node_modules/mocha/bin/_mocha", | ||
"posttest": "istanbul report --include coverage/**/coverage.json", |
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.
combines coverage for node + electron.
holy 🐟 -- let's hope I don't have to do that again! |
Ping! Would hate to see this become stale again. |
Create React Apps have the nifty feature of getting environment variables either from the shell or
.env
files. These are then resolved and baked into the JS bundle by webpack.When using debug, the entirety of your
.env
file gets exposed in the bundle, e.g.:This PR fixes that situation by moving the env access to a separate file that isn't accessed when in browser mode.
Also should resolve #467 (comment).