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.

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

Security: all env vars in production bundle #10021

Closed
Fetten opened this issue Nov 19, 2018 · 9 comments · Fixed by #10030
Closed

Security: all env vars in production bundle #10021

Fetten opened this issue Nov 19, 2018 · 9 comments · Fixed by #10030
Labels
type: bug An issue or pull request relating to a bug in Gatsby

Comments

@Fetten
Copy link
Contributor

Fetten commented Nov 19, 2018

Description

While inspecting production bundles of a gatsby site, I saw that all environment variables used during the build process are included in a bundle js file. This can also include sensitive data (e.g WordPress htaccess passwords) when used via a .env file.

Steps to reproduce

Expected result

Only env vars starting with GATSBY_ (and maybe others necessary for the build - which ones?) should be included in production bundles, as stated on https://www.gatsbyjs.org/docs/environment-variables/

Possible solution

I would guess that the problem is this line in the webpack.config.js:

return Object.assign(envObject, gatsbyVarObject)

Maybe we could white-list necessary env vars (including those starting with GATSBY_ ) and ignore all others. Would love to hear your thoughts on this.

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

const envObject = Object.keys(parsed).reduce((acc, key) => {
acc[key] = JSON.stringify(parsed[key])
return acc
}, {})

should use if (key.match(/^GATSBY_/)) { condition before adding :/

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

Hmmm, maybe that's not that simple

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

Update from me - I think real problem is actually here

"process.env": processEnv(stage, `development`),

with this we replace process.env with object containing all env vars, so if some component uses process.env.SOME_ENV_VAR this will be replaced with Object({NODE_ENV:"production",...RestOfEnvVars}).SOME_ENV_VAR (not sure if in all cases or just in some)

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

Just looking at https://webpack.js.org/plugins/environment-plugin/ (which is just helper around webpack.DefinePlugin) - I think we should do it same way

@pieh pieh added the type: bug An issue or pull request relating to a bug in Gatsby label Nov 19, 2018
@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

Only env vars starting with GATSBY_ (and maybe others necessary for the build - which ones?) should be included in production bundles, as stated on gatsbyjs.org/docs/environment-variables

Actually I just read this and

For Project Env Vars that you want to access in client-side browser JavaScript, you can define an environment config file, .env.development and/or .env.production, in your root folder. Depending on your active environment, the correct one will be found and its values embedded as environment variables in the browser JavaScript.

In addition to these Project Environment Variables defined in .env.* files, you could also define OS Env Vars. OS Env Vars which are prefixed with GATSBY_ will become available in browser JavaScript.

So this actually say that all vars from .env files will be available and OS env vars prefixed with GATSBY_ will be available. I will do some testing to see if can not leak to preserve this - if we can't then we will need to change both docs and code

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

@Fetten Just to check with you - in your app-*.js when env vars were leaked - was this something like

Object({ NODE_ENV: "production", ...otherEnvVars }).NOT_EXISTING_ENV_VAR ?

that's what I could reproduce and have fix for ready

@Fetten
Copy link
Contributor Author

Fetten commented Nov 19, 2018

Thank you for the quick investigation, Michal!

The lines in app.js look like this:

var O = void 0 !== e && Object({
    WORDPRESS_BASE_URL: "example.com",
    WORDPRESS_BASE_PROTOCOL: "https",
    WORDPRESS_HTACCESS_USER: "secretuser",
    WORDPRESS_HTACCESS_PASSWORD: "secretpassword",
    GATSBY_SOME_OTHER_SERVICE_URL: "https://some.example.com",
    ANOTHER_SERVICE_TOKEN: "1234567890",
    NODE_ENV: "production",
    PUBLIC_DIR: "path/to/build/dir/public",
    BUILD_STAGE: "build-javascript",
    GATSBY_BUILD_STAGE: "build-javascript"
}).SC_ATTR

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

Yeah, so similar situation - something is using process.env.SC_ATTR but it's not defined - so yeah, my PR should fix that

@Fetten
Copy link
Contributor Author

Fetten commented Nov 19, 2018

Yes, you are absolutely right. SC_ATTR seems to come from styled-components but isn't defined.
Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants