Skip to content

fix: disableHostCheck should be defaulted to true #890

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

Closed
wants to merge 2 commits into from
Closed

fix: disableHostCheck should be defaulted to true #890

wants to merge 2 commits into from

Conversation

cecilia-sanare
Copy link

@cecilia-sanare cecilia-sanare commented Apr 25, 2017

What kind of change does this PR introduce?

Fixes a breaking change in 2957853 by defaulting disableHostCheck to true.

Did you add or update the examples/?

No.

Summary

Previously webpack-dev-server would allow connections from any host 2957853 fixed this.

However it introduced a breaking change by enabling this functionality
by default.

Instead of breaking a current subset of users with this change, we should
disable it by default until the next major release of webpack-dev-server.

See #882 (comment).

Does this PR introduce a breaking change?

It introduces a breaking change for users dependent on disableHostCheck being false.

However this won't really prevent their servers from running,
instead it just won't verify the Host header.

What issues does this close?

resolves #882

@jsf-clabot
Copy link

jsf-clabot commented Apr 25, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 25, 2017

Codecov Report

Merging #890 into master will decrease coverage by 1.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
- Coverage   73.62%   72.01%   -1.61%     
==========================================
  Files           4        4              
  Lines         436      436              
  Branches      130      131       +1     
==========================================
- Hits          321      314       -7     
- Misses        115      122       +7
Impacted Files Coverage Δ
lib/Server.js 80.66% <100%> (-2.34%) ⬇️

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 662bc31...cca3c5e. Read the comment docs.

Previously `webpack-dev-server` would allow connections from
any host 2957853 fixed this.

However it introduced a breaking change by enabling this functionality
by default. We should disable it by default until the next major release
of `webpack-dev-server`.
@evan-scott-zocdoc
Copy link

Alternatively, I think it makes sense to autoenable this if setting the host to 0.0.0.0 e.g. bind to all IPs.

@cecilia-sanare
Copy link
Author

@evan-scott-zocdoc Totally agree, my primary focus for this PR was to revert the breaking change.

However at this point I'm sure a large enough number of people have worked
around it that implementing this wouldn't really make sense anymore.

@cecilia-sanare
Copy link
Author

Closing: due to the age of this PR merging it would likely break more users then it would help.

@evan-scott-zocdoc
Copy link

@sokra how to do you feel about the change described in #890 (comment)? I'd be happy to submit a PR.

@sokra
Copy link
Member

sokra commented Jun 1, 2017

@sokra how to do you feel about the change described in #890 (comment)? I'd be happy to submit a PR.

no. The default should be secure. Binding to 0.0.0.0 is insecure.

@evan-scott-zocdoc
Copy link

It's a dev server though... are you recommending people use this in production?

@sokra
Copy link
Member

sokra commented Jun 1, 2017

no, don't use it in production.

That's not what this attack is about. It's about attacking the dev-server from a website you visit while it's running.

See https://medium.com/webpack/webpack-dev-server-middleware-security-issues-1489d950874a

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.

--host 0.0.0.0 Not working
4 participants