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

fix: avoid leaking defined env vars when trying to access not defined env vars #10030

Merged
merged 3 commits into from
Nov 19, 2018

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Nov 19, 2018

fixes #10021

With current approach when process.env.NOT_EXISTING_ENV_VAR is used then bundle will contain:
Object({ NODE_ENV: "production", ...otherEnvVars }).NOT_EXISTING_ENV_VAR - with this approach it will produce: {}.NOT_EXISTING_ENV_VAR, while still being able to access all defined vars.

This is important because not only user code can be responsible for leaks - all imported npm packages can cause this as well.

Limitation - users won't be able to iterate over process.env object but this was exactly what was causing potential leak.

@pieh pieh requested a review from a team as a code owner November 19, 2018 17:01
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - I think this looks great. Nice work @pieh 💪

@pieh pieh merged commit 8061e3b into gatsbyjs:master Nov 19, 2018
@pieh pieh deleted the dont-leak-env-vars branch November 19, 2018 20:36
@elskwid
Copy link

elskwid commented Nov 30, 2018

@pieh thank you for the security fix. I don't know the best place to make mention of this but the change will seriously impact anyone that is destructuring environment variables.

const { FOO, BAR } = process.env

Will result in undefined values for both.

@brimtown
Copy link
Contributor

Is there a reason that this was not communicated as a breaking change? It's surprising to have an app break after a patch release 🙁 .

@jonniebigodes
Copy link

@brimtown this was brought to the attention of the team and from the flow of commits and the issue associated to it, needed to be patched with some urgency to prevent further damage to the people using gatsby.

@DSchau
Copy link
Contributor

DSchau commented Dec 11, 2018

@brimtown you're definitely right, though. This probably should have been communicated and pushed out as something beyond a patch release. Minimally, we should have marked it as a breaking change so it shows up in CHANGELOG.md.

We're sorry for the confusion this has caused, and we'll attempt to improve the documentation and communication on these types of changes going forward!

@brimtown
Copy link
Contributor

Thanks for the response. Appreciate the hard work yall are doing!

@elskwid
Copy link

elskwid commented Dec 11, 2018

@DSchau what @brimtown said. 😄 Thanks!

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
… env vars (gatsbyjs#10030)

* fix: avoid leaking defined env vars when trying to access not defined env vars

* test: add some testing for env var usage and leaking

* add some comments to .env.production - it normally shouldn't be commited to repo
@pieh pieh restored the dont-leak-env-vars branch May 17, 2020 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security: all env vars in production bundle
5 participants