Multi-variable destructuring breaks when working with webpack 3

[evisong]

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 webpack/webpack#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 webpack/webpack#5215 (comment) 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]

@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]

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:

• Successfully be able to read from process.env using destructors (const { TEST } = process.env)
• Successfully be able to read from process.env by explicit Reference (process.env.TEST)
• Should not leak sensitive data (only expose references variables explicitly)

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

• Successfully be able to read from process.env using destructors (const { TEST } = process.env)
• Successfully be able to read from process.env by explicit Reference (process.env.TEST)
• Should not leak sensitive data (only expose references variables explicitly)

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

• Successfully be able to read from process.env using destructors (const { TEST } = process.env)
• Successfully be able to read from process.env by explicit Reference (process.env.TEST)
• Should not leak sensitive data (only expose references variables explicitly)

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]

@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]

@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]

Happen to find that I was using dotenv-webpack@1.4.2 previously with webpack 2, and the 893d0d0 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]

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]

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]

@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]

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]

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

I just did a test on my mrsteele/dotenv-webpack-example@947cb5c 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]

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

[mrsteele]

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]

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.

• dotenv-webpack 1.6.0
• webpack 4.28.3

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

[kilgarenone]

I'm using

• dotenv-webpack 1.7.0
• webpack 4.31.0

Still can not do:

  const { BASE_URL, API_VERSION } = process.env;

But yeah the wordaround mentioned here works.

[DenisAvilov]

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

[yvesgurcan]

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]

I think we can close this

and focus on webpack 5

[piranna]

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

[mrsteele]

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]

I do not recommend destructuring

[piranna]

I do not recommend destructuring

@sibelius what do you propose?

[sibelius]

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]

Anyway to make the destructuring?

[cjfff]

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]

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