Skip to content

BG-9584: Require node production config when running express against prod#181

Merged
tylerlevine merged 5 commits into
masterfrom
BG-9584-express-fpd
Jan 11, 2019
Merged

BG-9584: Require node production config when running express against prod#181
tylerlevine merged 5 commits into
masterfrom
BG-9584-express-fpd

Conversation

@tylerlevine

@tylerlevine tylerlevine commented Jan 8, 2019

Copy link
Copy Markdown
Contributor

When node is running in development mode, it can output some debugging information which reveals sensitive data about the system running bitgo-express. It is currently possible to run express against the BitGo production environment, while the node environment is development (which is the default), thus potentially exposing customer system information.

By requiring that NODE_ENV is production when running against prod, express will not output this debugging information.

Note that this behavior is overridable by passing the --disableenvcheck flag when starting bitgo express, in case some customer wants to keep using the current behavior.

Signed-off-by: Tyler Levine tyler@bitgo.com

Tyler Levine added 2 commits January 8, 2019 10:18
BG-9584: Require node production config when running express against …
2bfee6b 
…prod

Signed-off-by: Tyler Levine <tyler@bitgo.com>
BG-9584: Fix and add tests
97f68ea 
Signed-off-by: Tyler Levine <tyler@bitgo.com>
cooncesean
cooncesean previously approved these changes Jan 8, 2019
View reviewed changes

@cooncesean cooncesean left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All seems reasonable. Clean code, tests, and documentation.

I have not pulled this down to see if I can bypass the NodeEnvironmentError without passing disableenvcheck=true, but it all looks 👍

@tylerlevine

tylerlevine commented Jan 8, 2019

Copy link
Copy Markdown
Contributor Author

I have a suspicion that many of our customers running bitgo-express are not running with NODE_ENV=production. If bitgo-express is exposed publicly, that would allow an attacker to glean some debugging information about the system which is running bitgo express.

This PR changes express to require NODE_ENV to be production when running bitgo-express against the prod environment. However, this will break customers who are not setting NODE_ENV correctly today.

I see a couple options:

  1. Ship this change as-is, and break customers who are using bitgo-express in an insecure way upon upgrade.
  2. Change from throwing an error and stopping everything, to just emitting a warning, and allowing bitgo-express to continue executing in an insecure fashion.

After some consideration, I believe option 1 is the best choice. However, we should reach out to customers proactively to let them know that there is a security update for bitgo-express which will require a change to the customer's runtime configuration.

@cooncesean

cooncesean commented Jan 10, 2019

Copy link
Copy Markdown
Contributor

Anything needed from me here?

@tylerlevine

tylerlevine commented Jan 10, 2019

Copy link
Copy Markdown
Contributor Author

Anything needed from me here?

Not at this time. I haven't heard back from Olivia re: customer messaging, so I think I will adopt solution 2 above as an interim solution, while we figure out how to message our customers.

Will update this diff to do that by EOD

@codecov

codecov Bot commented Jan 10, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #181 into master will increase coverage by 0.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   62.77%   62.78%   +0.01%     
==========================================
  Files          58       58              
  Lines        7360     7366       +6     
==========================================
+ Hits         4620     4625       +5     
- Misses       2740     2741       +1
Impacted Files Coverage Δ
src/errors.js 100% <100%> (ø) ⬆️
src/expressApp.js 83.33% <85.71%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34605db...b3d74d0. Read the comment docs.

Change to temporary warn-by-default policy for incorrect NODE_ENV
5491913 
Signed-off-by: Tyler Levine <tyler@bitgo.com>
@tylerlevine tylerlevine merged commit 8ea2da5 into master Jan 11, 2019
@tylerlevine tylerlevine mentioned this pull request Jan 15, 2019
Closed
@0xabrarhussain 0xabrarhussain deleted the BG-9584-express-fpd branch October 13, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dannydeezy dannydeezy Awaiting requested review from dannydeezy

1 more reviewer

@cooncesean cooncesean cooncesean approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tylerlevine @cooncesean