Always allow requests with IP-address as host in checkHost() #1007
Conversation
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. fixes webpack#931
|
|
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
| @@ -444,6 +444,9 @@ Server.prototype.checkHost = function(headers) { | |||
| // always allow localhost host, for convience | |||
| if(hostname === "127.0.0.1" || hostname === "localhost") return true; | |||
|
|
|||
| // always allow requests with explicit IP-address | |||
| if(/^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.|$)){4}$/.test(hostname)) return true; | |||
There was a problem hiding this comment.
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-2
if(/^([0-9]{1,3}(\.|$)){4}$/.test(hostname)) return true;
There was a problem hiding this comment.
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
| @@ -444,6 +444,9 @@ Server.prototype.checkHost = function(headers) { | |||
| // always allow localhost host, for convience | |||
| if(hostname === "127.0.0.1" || hostname === "localhost") return true; | |||
|
|
|||
| // always allow requests with explicit IP-address | |||
| if(/^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.|$)){4}$/.test(hostname)) return true; | |||
There was a problem hiding this comment.
I'd like to see https://www.npmjs.com/package/ip and ip.isV4Format used here, rather than a regex.
There was a problem hiding this comment.
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.
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 of ip-address-format.
|
Thanks! 🎊 |
| @@ -441,8 +442,11 @@ Server.prototype.checkHost = function(headers) { | |||
| const idx = hostHeader.indexOf(":"); | |||
| const hostname = idx >= 0 ? hostHeader.substr(0, idx) : hostHeader; | |||
|
|
|||
| // always allow requests with explicit IP-address | |||
| if(ip.isV4Format(hostname)) return true; | |||
There was a problem hiding this comment.
Shouldn't it allow IPv6-adresses as well?
There was a problem hiding this comment.
Should I do another PR for this? Did anyone ever use IPv6 in such a context?
Using http://[::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.
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