-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fixes Origin validation for IP address. gives meaning to public and allowedHosts #1622
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -12,7 +12,6 @@ | |||||||
const fs = require('fs'); | ||||||||
const path = require('path'); | ||||||||
|
||||||||
const ip = require('ip'); | ||||||||
const tls = require('tls'); | ||||||||
const url = require('url'); | ||||||||
const http = require('http'); | ||||||||
|
@@ -647,6 +646,10 @@ Server.prototype.setContentHeaders = function (req, res, next) { | |||||||
}; | ||||||||
|
||||||||
Server.prototype.checkHost = function (headers, headerToCheck) { | ||||||||
/* This routine is also used to check the Origin header, whenever | ||||||||
* headerToCheck says so | ||||||||
*/ | ||||||||
|
||||||||
// allow user to opt-out this security check, at own risk | ||||||||
if (this.disableHostCheck) { | ||||||||
return true; | ||||||||
|
@@ -668,17 +671,9 @@ Server.prototype.checkHost = function (headers, headerToCheck) { | |||||||
false, | ||||||||
true | ||||||||
).hostname; | ||||||||
// always allow requests with explicit IPv4 or IPv6-address. | ||||||||
// A note on IPv6 addresses: | ||||||||
// hostHeader will always contain the brackets denoting | ||||||||
// an IPv6-address in URLs, | ||||||||
// these are removed from the hostname in url.parse(), | ||||||||
// so we have the pure IPv6-address in hostname. | ||||||||
if (ip.isV4Format(hostname) || ip.isV6Format(hostname)) { | ||||||||
return true; | ||||||||
} | ||||||||
|
||||||||
// always allow localhost host, for convience | ||||||||
if (hostname === 'localhost') { | ||||||||
if (hostname === 'localhost' || hostname === '127.0.0.1') { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need better check here, because some developers can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If they use a local IP, developers should specify webpack-dev-server/lib/Server.js Lines 714 to 716 in dd92f31
|
||||||||
return true; | ||||||||
} | ||||||||
// allow if hostname is in allowedHosts | ||||||||
|
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.
Why we remove this check, it can be breaking change for some developers?
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.
This check basically means no security (for both static server and sockjs server) (#1618). It is not breaking if people are following the docs 👍
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.
@carlosgeos developers are not following the docs 😄 Better avoid this removing, we can do this in next major release
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.
hahahah ok, then I'll close this PR