Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.5k
Always allow requests with IP-address as host in checkHost()#1007
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This patch will allow any requests made using an IP-address to always pass thecheckHost-test.IP-addresses are not susceptible to a dns-rebind like attack so it would makesense to not block them to make local-network development possible withoutneeding to disable the host-checks entirely.fixeswebpack#931
jsf-clabot commentedJul 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
codecovbot commentedJul 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #1007 +/- ##==========================================+ Coverage 72.13% 72.25% +0.11%========================================== Files 4 4 Lines 463 465 +2 Branches 139 140 +1 ==========================================+ Hits 334 336 +2 Misses 129 129
Continue to review full report at Codecov.
|
first-time contributor here. Please let me know if there are any docs I need to validate/update or anything else I should think of. |
lib/Server.js Outdated
| if(hostname==="127.0.0.1"||hostname==="localhost")returntrue; | ||
| // always allow requests with explicit IP-address | ||
| if(/^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.|$)){4}$/.test(hostname))returntrue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we add a few more testcases, for both the valid and invalid IP cases?
Also, I wonder if a simplified regex that just checked that each octet was numeric would be simpler to read/faster/still as secure?RFC3696 section 2 says that top level domain names aren't allowed to be all-numeric, so eg 999.999.999.999 still won't be treated as a domain, so can't be used for DNS rebinding.
For example:
// Requests to explicit IP-addresses can't be exploited by DNS rebinding.// For simplicity the regex matches numeric ranges that aren't valid IPs, but// this is still secure since top level domain names can never be all-numeric:// https://tools.ietf.org/html/rfc3696#section-2if(/^([0-9]{1,3}(\.|$)){4}$/.test(hostname)) return true;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like the simplicity and probably would want to do that. We don't validate other aspects of IP adress as well (reserved, multicast, etc) and I can't think of a way to have an invalid or unrouteable IP-address in the browser anyway.
lib/Server.js Outdated
| if(hostname==="127.0.0.1"||hostname==="localhost")returntrue; | ||
| // always allow requests with explicit IP-address | ||
| if(/^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.|$)){4}$/.test(hostname))returntrue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to seehttps://www.npmjs.com/package/ip andip.isV4Format used here, rather than a regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought about that as well, but didn't want to introduce any new dependencies for something that could be put in a simple (ok, thats arguable) regex. Is there any policy regarding adding dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the dependencies were being packaged for a production environment, I'd agree with you. Since this is strictly a development tool, tried and true dependencies to perform utility functions are acceptable. You'll see that pattern throughout the commit history. (for example; loglevel over a custom implementation).
As per@shellscape's comment, switch to the[ip](https://npmjs.com/package/ip)-module to do validation ofip-address-format.
Thanks! 🎊 |
| consthostname=idx>=0 ?hostHeader.substr(0,idx) :hostHeader; | ||
| // always allow requests with explicit IP-address | ||
| if(ip.isV4Format(hostname))returntrue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't it allow IPv6-adresses as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that's true, good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should I do another PR for this? Did anyone ever use IPv6 in such a context?
Usinghttp://[::1]:1234/ is absolutely possible, but that will already break in the lines before as a colon no longer works to split out the port-number from the host-header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It may be worthwhile to solve. I haven't heard of anyone using IPv6 in that way, but the call out by@trygveaa is valid. Totally up to you if you'd like to create a followup PR
What kind of change does this PR introduce?
enhancement
Did you add or update the
examples/?no.
Summary
This patch will allow any requests made using an IP-address to always pass the
checkHost-test.
IP-addresses are not susceptible to a dns-rebind like attack so it would make
sense to not block them to make local-network development possible without
needing to disable the host-checks entirely.
Does this PR introduce a breaking change?
no
Other information
fixes#931