Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix exposing of .env variables in Create React App #560

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix exposing of .env variables in Create React App #560

wants to merge 13 commits into from

Conversation

moos
Copy link

@moos moos commented Apr 18, 2018

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.:

    function u() {
      var e;
      try {
        e = t.storage.debug
      } catch (e) {
      }
      return !e && "undefined" !== typeof r && "env" in r && (e = Object({
        NODE_ENV: "production",
        PUBLIC_URL: "",
        REACT_APP_FOO: "1"
      }).DEBUG), e
    }

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).
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 75.0% when pulling 3d42235 on moos:fix-cra-env into 22f9932 on visionmedia:master.
1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 75.0% when pulling 3d42235 on moos:fix-cra-env into 22f9932 on visionmedia:master.
@coveralls
Copy link

coveralls commented Apr 18, 2018 •
edited
Loading

Coverage Status

Coverage increased (+2.2%) to 89.862% when pulling 86c9684 on moos:fix-cra-env into 5c7c61d on visionmedia:master.
@Qix-
Copy link
Member

Qix- commented Jun 20, 2018

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.
@Qix- Qix- added feature This proposes or provides a feature or enhancement awaiting-response This issue or pull request is awaiting a user's response change-patch This proposes or provides a change that requires a patch release labels Jun 20, 2018
@moos
Copy link
Author

moos commented Aug 30, 2018

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 .env files to the web site visitor -- clearly, an unintended (and hard-to-detect, unless you analyze the bundle -- I bet most app devs don't do that!) side-effect of .env + debug.

I'd be happy to submit an update if there is interest.
@Qix-
Copy link
Member

Qix- commented Dec 13, 2018

Hi, sorry it took so long to get back. Could you rebase please? I'm okay with adding this check.
@Qix- Qix- added this to the 5.x milestone Dec 19, 2018
moos
moos commented Feb 2, 2019
View reviewed changes
karma.conf.js
@@ -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'],
Copy link
Author

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.
moos
moos commented Feb 2, 2019
View reviewed changes
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",
Copy link
Author

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.
moos
moos commented Feb 2, 2019
View reviewed changes
src/browser.js
} 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) {
Copy link
Author

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.
moos
moos commented Feb 3, 2019
View reviewed changes
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",
Copy link
Author

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.
@moos
Copy link
Author

moos commented Feb 3, 2019

holy 🐟 -- let's hope I don't have to do that again!
@moos
Copy link
Author

moos commented Feb 27, 2019

Ping! Would hate to see this become stale again.
@snyk-bot snyk-bot mentioned this pull request Jul 17, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Jun 1, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Jun 22, 2022
Open
@LadyK-21 LadyK-21 mentioned this pull request Jun 23, 2022
Open
@ryan-ally ryan-ally mentioned this pull request Jun 23, 2022
Open
@LadyK-21 LadyK-21 mentioned this pull request Oct 6, 2022
Open
This was referenced Oct 13, 2022
Closed
Open
@ryan-ally ryan-ally mentioned this pull request Nov 27, 2023
Open
This was referenced Nov 28, 2023
Open
Open
@ryan-ally ryan-ally mentioned this pull request May 22, 2024
Open
@LadyK-21 LadyK-21 mentioned this pull request May 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response This issue or pull request is awaiting a user's response change-patch This proposes or provides a change that requires a patch release feature This proposes or provides a feature or enhancement
3 participants
@moos @coveralls @Qix-