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?