search

mrsteele / dotenv-webpack

A secure webpack plugin that supports dotenv and other environment variables and only exposes what you choose and use.
MIT License
1.3k stars 75 forks source link

Multi-variable destructuring breaks when working with webpack 3 #70

Open evisong opened 7 years ago

evisong commented 7 years ago

Hi, team,

Found an interesting bug:

Issue

Using dotent-webpack@1.5.3 and webpack@3.3.0,

// .env
DB_HOST=127.0.0.1
DB_PASS=foobar
S3_API=mysecretkey

Use the vars in JS:

const { DB_HOST, DB_PASS } = process.env;

Would cause runtime error:

Uncaught (in promise) ReferenceError: process is not defined

Expected

It should replace both vars at compile time.

Possible Root Cause

I believe this issue is caused by https://github.com/webpack/webpack/issues/5215

So I changed the JS to:

const { DB_HOST } = process.env;
const { DB_PASS } = process.env;

It works well as expected.

I debugged the plugin and found that the underlying formatData (https://github.com/mrsteele/dotenv-webpack/blob/master/src/index.js#L63) transferred to webpack.DefinePlugin is:

{
  'process.env.DB_HOST': '"127.0.0.1"',
  'process.env.DB_PASS': '"foobar"',
  'process.env.S3_API': '"mysecretkey"'
}

However the format that recommended in https://github.com/webpack/webpack/issues/5215#issuecomment-314363436 is: 

new DefinePlugin({
  "process.env": {
    a: JSON.stringify("a"),
    b: JSON.stringify("b")
  }
})

So I guess a minor fix in dotenv-webpack would solve the issue.

However, I think it ought to be a webpack.DefinePlugin regression bug, since it the current formatData used to work well with webpack 2.

What do you think? Thanks.

mrsteele commented 7 years ago

@evisong Very interesting find.

A brief history of this plugin: We actually used to do what you have recommended (see PR #42), but by defining a single property as process.env it would bundle ALL environment variables which we decided against because of the fact that it could leak variables you didn't want to expose.

i.e.

env

NOTASECRET=123
SECRET=321

script.js

console.log(process.log.NOTASECRET)

bundle.js

console.log({"NOTASECRET":"123","SECRET":"321"}.NOTASECRET)

While there are many approaches that can solve this with different levels of effort, we opted to steer away from "white-listing" variables and such and just letting you expose what you choose in your code.

Let me do a little more investigating since webpack have changed the bundling mechanics since we last looked at this. Anyone else interested feel free to chime in as well.

mrsteele commented 7 years ago

I can confirm your suggestion would leak the data, however you are completely right in saying that the current approach does not support destructors due to the bundling methods.

I have added all of my test files to the dotenv-webpack-example repo you can review either below or view the source at https://github.com/mrsteele/dotenv-webpack-example.

Goals

Our goals are consistent between the test cases:

Consistent Test Files

The following files were consistent between the test cases.

env

TEST=Works!
TEST2=Yes!
TEST_SECRET=nope

script.js

const { TEST, TEST2 } = process.env

document.querySelector('body').innerHTML = `
  Structured: ${process.env.TEST}<br />
  Destructured: ${TEST}<br />
  <hr />
  Structured: ${process.env.TEST2}<br />
  Destructured: ${TEST2}<br />
`

@evisong's Suggested Approach

This approach is to pull all the variables to be defined from the environment into a single property process.env and pass that object into the DefinePlugin provided by Webpack.

Goals
bundle.js
...
var _process$env = Object({"TEST":"Works!","TEST2":"Yes!","TEST_SECRET":"nope"}),
    TEST = _process$env.TEST,
    TEST2 = _process$env.TEST2;

document.querySelector('body').innerHTML = '\n  Structured: ' + "Works!" + '<br />\n  Destructured: ' + TEST + '<br />\n  <hr />\n  Structured: ' + "Yes!" + '<br />\n  Destructured: ' + TEST2 + '<br />\n';
...

Current Approach

The current approach (latest in prod) defines a key-value pair object that prefixes process.env. to all variables so that the explicit variable references get bundled and the neglected variables are not included to prevent exposing unnecessary values.

Goals
bundle.js
...
var _process$env = process.env,
    TEST = "Works!",
    TEST2 = "Yes!";

document.querySelector('body').innerHTML = '\n  Structured: ' + "Works!" + '<br />\n  Destructured: ' + TEST + '<br />\n  <hr />\n  Structured: ' + "Yes!" + '<br />\n  Destructured: ' + TEST2 + '<br />\n';
...

I'm all ears for suggestions. Of course the suggested workaround would just be to define your variables explicitly since that currently works and doesn't pose a security threat for your application.

evisong commented 7 years ago

@mrsteele your comments is very detailed & professional, thanks, I also look forward to any others' feedback.

Btw, do you think it's a good idea to submit our use case to webpack? The key is that it used to work well with webpack 2.

mrsteele commented 7 years ago

@evisong thanks and I appreciate the feedback.

I think it may be worth mentioning to webpack. We have definitely done the research beforehand and we can use the previous ticket you mentioned above as a guide as well.

The time may be soon approaching where we have to circumvent DefinePlugin and write it ourselves, which I don't particularly look forward to but also recognize that day may be unavoidable.

evisong commented 7 years ago

Happen to find that I was using dotenv-webpack@1.4.2 previously with webpack 2, and the 893d0d0d security fix is introduced in 1.4.3. So this issue should also apply to all webpack versions, right?

The current workaround is working well: 

const { DB_HOST } = process.env;
const { DB_PASS } = process.env;
mrsteele commented 7 years ago

Workarounds are great, but I think the original issue still stands:

As a user, I need to be able to destruct any and all properties from an object.

33Fraise33 commented 6 years ago

This prevents me from doing things like: 

console.log(process.env[`${name}`])
const env = window._env_ ? window._env_ : process.env;
console.log(env.NODE_ENV)
mrsteele commented 6 years ago

@33Fraise33 I understand the frustration, unfortunately this is a complication in the way webpack.DefinePlugin works, you will see the test-cases I wrote above and pros and cons with switching some of the architecture.

I opted to keep things they way they are for the purposes of security and not accidentally leaking any information out that you don't wanna leak.

piranna commented 6 years ago

Not sure if webpack allows this, but could it be possible to do a two-pass scan? This way the first one could search for all the environment variables used in the object destructure, and later do the white-list of them...

mrsteele commented 5 years ago

Hey everyone, I wanted to ask if this was still a problem.

I just did a test on my https://github.com/mrsteele/dotenv-webpack-example/commit/947cb5cfda7a30bbfaab3cb1deac73a0d2730817 app to see if this was still a problem and it looks like the latest version of webpack has destructing working just fine.

Can anyone else confirm?

piranna commented 5 years ago

Maybe asking to webpack devs? I would like to know too...

mrsteele commented 5 years ago

It worked for me. It loaded just fine all the variables I referenced and didn't leak anything out so I imagine there is some magic transpiling/tree-shaking going on that resolved this on there end.

@piranna have you tried to upgrade webpack and babel?

cjolowicz commented 5 years ago

My bundle.js still contains var _process$env = process.env, leading to ReferenceError: process is not defined. The actual variables defined using object destructuring are replaced with their values, as expected.

Splitting the multi-variable destructuring into separate assignments avoids this problem, as suggested above.

kilgarenone commented 5 years ago

I'm using

Still can not do:

  const { BASE_URL, API_VERSION } = process.env;

But yeah the wordaround mentioned here works.

DenisAvilov commented 3 years ago

I'm using "webpack": "5.20.0" Don't works const { BASE_URL, API_VERSION } = process.env; // undefined undefined

yvesgurcan commented 3 years ago

I just stumbled on the same issue where the line below works:

const { DIRECTUS_ENDPOINT } = process.env;
// You can use `DIRECTUS_ENDPOINT` as expected.

Whereas the line below throws the process is undefined error:

const { DIRECTUS_ENDPOINT, DIRECTUS_ACCESS_TOKEN } = process.env;
// Triggers `process is undefined`

The workaround below does indeed work but doing this mostly defeats the purpose of destructuring:

const { DIRECTUS_ENDPOINT } = process.env;
const { DIRECTUS_ACCESS_TOKEN } = process.env;
// You can use `DIRECTUS_ENDPOINT` and `DIRECTUS_ACCESS_TOKEN` as expected.

Even worse, if you console.log process.env, things work as expected:

const { DIRECTUS_ENDPOINT, DIRECTUS_ACCESS_TOKEN } = process.env;
console.log(process.env);
// You can use `DIRECTUS_ENDPOINT` and `DIRECTUS_ACCESS_TOKEN` as expected.

This seems extremely counter intuitive and confusing from a developer's perspective and I'm sure this has and will continue to trip up developers because of understandable assumptions. If you want to drive a developer crazy, it looks like you found an effective method. If you can access process.env.DIRECTUS_ENDPOINT that way, you can infer that process.env is an object. It seems fair to then infer that destructuring process.env to get to more than one property would work like it would with any object in JavaScript. Not exposing process.env as an object like the Node runtime does seems non-standard. If you're not following the standards of the Node runtime, then maybe have the environment variables live in a global variable that does not have the same name as the one used by Node. This way, developers might be more careful about their assumptions (namely, not assumed that it works just like Node), even if not being able to destructure what looks like an object to get two properties is still extremely confusing, especially if using console.log fixes it.

sibelius commented 2 years ago

I think we can close this

and focus on webpack 5

piranna commented 2 years ago

Can there be any fix for this issue in Webpack 5?

mrsteele commented 2 years ago

I have noticed this works intermittently in webpack 5. I think some of the gatchas mentioned above are the reasonings. I do know with webpack 5 we have to be careful about the "target" property from webpack to make sure we are targeting a web environment when we build.

Since this plugin uses webpack.DefinePlugin under the hood, it would be on that system to resolve that issue.

Alternatively, we could look at using something other than process.env to write to, but you would lose some of the seamless integration aspect of this plugin.

new Dotenv({
  envVarName: 'process.env' // The default, but maybe we could configure it? Perhaps there is too much complexity for DefinePlugin to read from this variable...
})
sibelius commented 2 years ago

I do not recommend destructuring

piranna commented 2 years ago

I do not recommend destructuring

@sibelius what do you propose?

sibelius commented 2 years ago

create a config.ts file that has all your process.env like this

export const config = {
    API_URL: process.env.API_URL,
};

always consume process.env from that file, so you can destructure if needed

tutods commented 2 years ago

Anyway to make the destructuring?

cjfff commented 1 year ago

Anyway to make the destructuring?

I had used it in webpack 5+ but found it didn't work still.

I think they don't think the destructuring is a good idea at all. So...As you can see anyone give up for using the function anymore.

mgardunoH commented 7 months ago

Ve el siguiente link: https://medium.com/@desinaoluseun/using-env-to-store-environment-variables-in-angular-20c15c7c0e6a