-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix security issues reported by Dependabot for version 4 #5514
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
base: version-4
Are you sure you want to change the base?
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 |
---|---|---|
|
@@ -250,6 +250,8 @@ const encodeOverlaySettings = (setting) => | |
? encodeURIComponent(setting.toString()) | ||
: setting; | ||
|
||
const DEFAULT_ALLOWED_PROTOCOLS = /^(file|.+-extension):/i; | ||
|
||
class Server { | ||
/** | ||
* @param {Configuration | Compiler | MultiCompiler} options | ||
|
@@ -2011,10 +2013,11 @@ class Server { | |
*/ | ||
(req, res, next) => { | ||
if ( | ||
this.checkHeader( | ||
this.isValidHost( | ||
/** @type {{ [key: string]: string | undefined }} */ | ||
(req.headers), | ||
"host" | ||
"host", | ||
true | ||
) | ||
) { | ||
return next(); | ||
|
@@ -2208,6 +2211,40 @@ class Server { | |
this.options.onBeforeSetupMiddleware(this); | ||
} | ||
|
||
// Register setup cross origin request check for securityAdd commentMore actions | ||
middlewares.push({ | ||
name: "cross-origin-header-check", | ||
/** | ||
* @param {Request} req | ||
* @param {Response} res | ||
* @param {NextFunction} next | ||
* @returns {void} | ||
*/ | ||
middleware: (req, res, next) => { | ||
const headers = | ||
/** @type {{ [key: string]: string | undefined }} */ | ||
(req.headers); | ||
|
||
const headerName = headers[":authority"] ? ":authority" : "host"; | ||
|
||
if (this.isValidHost(headers, headerName, false)) { | ||
next(); | ||
return; | ||
} | ||
|
||
if ( | ||
headers["sec-fetch-mode"] === "no-cors" && | ||
headers["sec-fetch-site"] === "cross-site" | ||
) { | ||
res.statusCode = 403; | ||
res.end("Cross-Origin request blocked"); | ||
return; | ||
} | ||
|
||
next(); | ||
}, | ||
}); | ||
|
||
if (typeof this.options.headers !== "undefined") { | ||
middlewares.push({ | ||
name: "set-headers", | ||
|
@@ -2598,8 +2635,8 @@ class Server { | |
|
||
if ( | ||
!headers || | ||
!this.checkHeader(headers, "host") || | ||
!this.checkHeader(headers, "origin") | ||
!this.isValidHost(headers, "host", true) || | ||
!this.isValidHost(headers, "origin", false) | ||
) { | ||
this.sendMessage([client], "error", "Invalid Host/Origin header"); | ||
|
||
|
@@ -3055,75 +3092,93 @@ class Server { | |
* @private | ||
* @param {{ [key: string]: string | undefined }} headers | ||
* @param {string} headerToCheck | ||
* @param {boolean} validateHost | ||
* @returns {boolean} | ||
*/ | ||
checkHeader(headers, headerToCheck) { | ||
// allow user to opt out of this security check, at their own risk | ||
// by explicitly enabling allowedHosts | ||
isValidHost(headers, headerToCheck, validateHost = true) { | ||
if (this.options.allowedHosts === "all") { | ||
return true; | ||
} | ||
|
||
// get the Host header and extract hostname | ||
// we don't care about port not matching | ||
const hostHeader = headers[headerToCheck]; | ||
const header = headers[headerToCheck]; | ||
|
||
if (!hostHeader) { | ||
if (!header) { | ||
return false; | ||
} | ||
|
||
if (/^(file|.+-extension):/i.test(hostHeader)) { | ||
if (DEFAULT_ALLOWED_PROTOCOLS.test(header)) { | ||
return true; | ||
} | ||
|
||
// use the node url-parser to retrieve the hostname from the host-header. | ||
const hostname = url.parse( | ||
// if hostHeader doesn't have scheme, add // for parsing. | ||
/^(.+:)?\/\//.test(hostHeader) ? hostHeader : `//${hostHeader}`, | ||
// if header doesn't have scheme, add // for parsing. | ||
/^(.+:)?\/\//.test(header) ? header : `//${header}`, | ||
false, | ||
true | ||
).hostname; | ||
|
||
if (hostname === null) { | ||
return false; | ||
} | ||
|
||
if (this.isHostAllowed(hostname)) { | ||
return true; | ||
} | ||
|
||
// always allow requests with explicit IPv4 or IPv6-address. | ||
// A note on IPv6 addresses: | ||
// hostHeader will always contain the brackets denoting | ||
// header 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. | ||
// For convenience, always allow localhost (hostname === 'localhost') | ||
// and its subdomains (hostname.endsWith(".localhost")). | ||
// allow hostname of listening address (hostname === this.options.host) | ||
const isValidHostname = | ||
(hostname !== null && ipaddr.IPv4.isValid(hostname)) || | ||
(hostname !== null && ipaddr.IPv6.isValid(hostname)) || | ||
hostname === "localhost" || | ||
(hostname !== null && hostname.endsWith(".localhost")) || | ||
hostname === this.options.host; | ||
|
||
if (isValidHostname) { | ||
return true; | ||
} | ||
const isValidHostname = validateHost | ||
? ipaddr.IPv4.isValid(hostname) || | ||
ipaddr.IPv6.isValid(hostname) || | ||
hostname === "localhost" || | ||
hostname.endsWith(".localhost") || | ||
hostname === this.options.host | ||
: true; | ||
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. I believe there is a bug in version 5 of Above function when called: 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. @kretajak it is not a bug, because 127.0.0.1 can be used for attack, you should manually set 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. Okey. I reverted it to |
||
|
||
return isValidHostname; | ||
} | ||
|
||
/** | ||
* @private | ||
* @param {string} value | ||
* @returns {boolean} | ||
*/ | ||
isHostAllowed(value) { | ||
const { allowedHosts } = this.options; | ||
|
||
// allow user to opt out of this security check, at their own risk | ||
// by explicitly enabling allowedHosts | ||
if (allowedHosts === "all") { | ||
return true; | ||
} | ||
|
||
// always allow localhost host, for convenience | ||
// allow if hostname is in allowedHosts | ||
// allow if value is in allowedHosts | ||
if (Array.isArray(allowedHosts) && allowedHosts.length > 0) { | ||
for (let hostIdx = 0; hostIdx < allowedHosts.length; hostIdx++) { | ||
const allowedHost = allowedHosts[hostIdx]; | ||
|
||
if (allowedHost === hostname) { | ||
for (const allowedHost of allowedHosts) { | ||
if (allowedHost === value) { | ||
return true; | ||
} | ||
|
||
// support "." as a subdomain wildcard | ||
// e.g. ".example.com" will allow "example.com", "www.example.com", "subdomain.example.com", etc | ||
if (allowedHost[0] === ".") { | ||
// "example.com" (hostname === allowedHost.substring(1)) | ||
// "*.example.com" (hostname.endsWith(allowedHost)) | ||
if (allowedHost.startsWith(".")) { | ||
// "example.com" (value === allowedHost.substring(1)) | ||
// "*.example.com" (value.endsWith(allowedHost)) | ||
if ( | ||
hostname === allowedHost.substring(1) || | ||
/** @type {string} */ (hostname).endsWith(allowedHost) | ||
value === allowedHost.substring(1) || | ||
/** @type {string} */ | ||
(value).endsWith(allowedHost) | ||
) { | ||
return true; | ||
} | ||
|
@@ -3135,17 +3190,17 @@ class Server { | |
if ( | ||
this.options.client && | ||
typeof ( | ||
/** @type {ClientConfiguration} */ (this.options.client).webSocketURL | ||
/** @type {ClientConfiguration} */ | ||
(this.options.client).webSocketURL | ||
) !== "undefined" | ||
) { | ||
return ( | ||
/** @type {WebSocketURL} */ | ||
(/** @type {ClientConfiguration} */ (this.options.client).webSocketURL) | ||
.hostname === hostname | ||
.hostname === value | ||
); | ||
} | ||
|
||
// disallow | ||
return false; | ||
} | ||
|
||
|
@@ -3166,6 +3221,64 @@ class Server { | |
} | ||
} | ||
|
||
/** | ||
* @private | ||
* @param {{ [key: string]: string | undefined }} headers | ||
* @returns {boolean} | ||
*/ | ||
isSameOrigin(headers) { | ||
if (this.options.allowedHosts === "all") { | ||
return true; | ||
} | ||
|
||
const originHeader = headers.origin; | ||
|
||
if (!originHeader) { | ||
return this.options.allowedHosts === "all"; | ||
} | ||
|
||
if (DEFAULT_ALLOWED_PROTOCOLS.test(originHeader)) { | ||
return true; | ||
} | ||
|
||
const origin = url.parse(originHeader, false, true).hostname; | ||
|
||
if (origin === null) { | ||
return false; | ||
} | ||
|
||
if (this.isHostAllowed(origin)) { | ||
return true; | ||
} | ||
|
||
const hostHeader = headers.host; | ||
|
||
if (!hostHeader) { | ||
return this.options.allowedHosts === "all"; | ||
} | ||
|
||
if (DEFAULT_ALLOWED_PROTOCOLS.test(hostHeader)) { | ||
return true; | ||
} | ||
|
||
const host = url.parse( | ||
// if hostHeader doesn't have scheme, add // for parsing. | ||
/^(.+:)?\/\//.test(hostHeader) ? hostHeader : `//${hostHeader}`, | ||
false, | ||
true | ||
).hostname; | ||
|
||
if (host === null) { | ||
return false; | ||
} | ||
|
||
if (this.isHostAllowed(host)) { | ||
return true; | ||
} | ||
|
||
return origin === host; | ||
} | ||
|
||
/** | ||
* @private | ||
* @param {Request} req | ||
|
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.
Recently we added some fixes here to skip check for
allowedHost
, please add it hereUh oh!
There was an error while loading. Please reload this page.
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 seems not that straightforward. To me bigger effort is needed to incorporate changes from 03d1214. This PR uses functions defined in previous commits not available in line 4.
If it's not a problem, I would consider merging this PR and creating separate PR for task you mentioned.
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.
@kretajak let's include 03d1214 here, otherwise in some cases it will be impossible to setup a new version and we can merge
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.
@kretajak let me know if you need help backporting that commit. I could try to add it on top of your existing PR if you don't have time to. We have at least 3 Docusaurus issues asking us to solve this security warning so happy to help and get this solved asap.
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'm currently trying to backport that commit. I'll inform you whether I was able to make it.
Uh oh!
There was an error while loading. Please reload this page.
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 have added new commit which should move us a bit closer. Seems like also changes from 6045b1e might be required as they are tightly connected to 03d1214.
@slorber feel free to continue the work.