fix: prevent host header injection by sanitizing userinfo in Host header - #7317
fix: prevent host header injection by sanitizing userinfo in Host header#7317bjohansebas wants to merge 1 commit into
Conversation
20be0d9 to
f556b0b
Compare
|
cc: @expressjs/express-collaborators @expressjs/security-wg |
There was a problem hiding this comment.
req.host and req.hostname are documented as strings in code, DefinitelyTyped and on the website. Changing them to This is wrong, because when the client does not send a string? might break some assumptions in user code.Host header, the properties are undefined.
Demo
HTTP/0.9 and HTTP/1.0 don't require (or even support) the Host header, so the demo shows real requests that are accepted by Node.js. HTTP/1.1 without Host is rejected by Node.js.
const { createConnection } = require("node:net")
const express = require("express");
const app = express();
app.use((req, res) => {
const { host, hostname } = req;
console.log(host, hostname);
// => undefined undefined
res.json({ host, hostname });
// => {}
});
const IP = "127.0.0.1"
const server = app.listen(0, IP, e => {
if (e) return;
const { port } = server.address();
const socket = createConnection(port, () => {
socket.once("end", () => server.close());
socket.on("data", (d) => console.log(d.toString("utf8")));
socket.end(`GET / HTTP/1.0\r\nUser-Agent: raw bytes\r\n\r\n`);
// or HTTP/0.9
// socket.end(`GET / HTTP/0.9\r\n\r\n`);
});
});$ node index.js
undefined undefined
HTTP/1.1 200 OK
X-Powered-By: Express
Content-Type: application/json; charset=utf-8
Content-Length: 2
ETag: W/"2-vyGp6PvFo4RvsFtPoIWeCReyIC8"
Date: Mon, 03 Aug 2026 19:39:04 GMT
Connection: close
{}History
JSDoc for req.host was added together with req.host in 2b90cd7 in 2012 and released in Express 3.0.0beta2. Since at least 2013 and Express 3.2.4 it has been known that req.host may return undefined (06ead58).
Website did not have information about types until expressjs/expressjs.com#2389.
DefinitelyTyped has used string as the type of req.host since introducing type definitions for Express 3.0 in 2012 (DefinitelyTyped/DefinitelyTyped@c02fecf#diff-b40c3775054ff9d4caf7eeecfeb581188302f77086dd8e3dcd748e801229ccc7R130).
|
Context for reviewers: the problem this PR is meant to solve is that our E.g. A few things to note about master:
I think there are two questions hereWhat should we do with malformed input when resolving A few options jump out at me:
Which getter gets a change? host or hostname? This PR takes the approach of doing sanitization at the That change means the "split at first colon" hostname getter can't have the same bug it does today. My thoughtsI need to wrap up here and step away lol, Im now too close to this. I think that if we have a bug here at all, it's that the Ultimately I don't think there's a need for that particular getter to morph into a full URL parser, there be dragons yonder way. But I will say it is really surprising the way that it behaves today. I'm less inclined to ship a change which affects Host headers are always user controlled, but also have a well defined "valid" grammar, which gives us space to make an assertion about deriving a value from a Host header. What should the hostname of an invalid Host header be? How can you reliably compute something from a bad input?
|
| // A Host header must not contain userinfo (RFC 9110 section 7.2). Drop any | ||
| // "user@" / "user:pass@" prefix so a crafted value like | ||
| // "evil.com:x@good.com" cannot inject an attacker-controlled host via | ||
| // req.hostname; the real host is the part after the last "@". | ||
| var atIndex = val.lastIndexOf('@'); | ||
| if (atIndex !== -1) val = val.slice(atIndex + 1); | ||
|
|
||
| if (!val) return; | ||
|
|
||
| // The optional port must be numeric (RFC 3986 section 3.2.3). Reject | ||
| // malformed authorities, e.g. an encoded "@" smuggled in as the port | ||
| // ("evil.com:x%40good.com"), which would otherwise leak a fake hostname. | ||
| var portOffset = val[0] === '[' ? val.indexOf(']') + 1 : 0; | ||
| var portIndex = val.indexOf(':', portOffset); | ||
| if (portIndex !== -1 && !isValidPort(val.slice(portIndex + 1))) return; |
There was a problem hiding this comment.
These outcomes are inconsistent with each other aren't they?
Or more specifically, is there spec based logic im missing which would lead to sanitizing userinfo, but rejecting invalid port info? Both are technically invalid Host header values right? So why not set both to undefined?
These are questions, not necessarily suggested changes to be clear.
Is it that the non numeric port value is an invalid URI? Whereas a URI with userinfo is just an invalid Host header, not necessarily a bad URI?
req.hostname/req.host: master (unpatched) vs our versionreq.hostnamereq.hostnamereq.hostreq.hostexample.com:3000(control)example.comexample.comexample.com:3000example.com:3000example.com:notaportexample.comundefinedexample.com:notaportundefined[::1]:notaport[::1]undefined[::1]:notaportundefinedevil.com:fake@legitimate.com:3000evil.comlegitimate.comevil.com:fake@legitimate.com:3000legitimate.com:3000user@example.comuser@example.comexample.comuser@example.comexample.comuser:pass@example.com:8080userexample.comuser:pass@example.com:8080example.com:8080user@[::1]:8080user@[[::1]user@[::1]:8080[::1]:8080a@b@example.coma@b@example.comexample.coma@b@example.comexample.comevil.com:x%40legitimate.comevil.comundefinedevil.com:x%40legitimate.comundefineduser@user@undefineduser@undefinedX-Forwarded-Host: evil.com:fake@legitimate.com(trust proxy)evil.comlegitimate.comevil.com:fake@legitimate.comlegitimate.comref: https://github.com/expressjs/express/security/advisories/GHSA-rvrq-qj4c-w73v (Note that it was closed because it did not meet our criteria for being considered a vulnerability and will instead be treated as a bug)