doc: correct unsafe URL example in http docs #52536
Conversation
The previous documentation example for converting request.url to an URL object was unsafe, as it could allow a server crash through malformed URL inputs and potentially enable host header attacks. This commit revises the example to use string concatenation, mitigating both the crash and security risks by ensuring the host part of the URL remains controlled and predictable. Fixes: nodejs#52494
|
Review requested:
|
nodejs-github-bot
added
doc
| @@ -2886,15 +2886,15 @@ Accept: text/plain | |||
| To parse the URL into its parts: | |||
|
|
|||
| ```js | |||
| new URL(request.url, `http://${request.headers.host}`); | |||
| new URL(`http://${req.headers.host}${req.url}`); | |||
There was a problem hiding this comment.
This is not safer than the original, see #52494 (comment)
There was a problem hiding this comment.
@lpinca is correct. Relying on user headers for any part of the url parsing can lead to the malicious injection of a URL.
I believe the smartest idea would be to just include a note in the documentation that this setup is only an example and shouldn't be used in production for security reasons
There was a problem hiding this comment.
Sorry, I reviewed without thoroughly analysis. I second @RedYetiDev idea.
There was a problem hiding this comment.
Nice to see a PR improving the docs. Maybe the best idea to fix the problem with the Host header is simply not to use it? Maybe something like this is better?
new URL(`http://localhost${request.url}`)
new URL(`http://${process.env.HOST ?? 'localhost'}${request.url}`)I believe the smartest idea would be to just include a note in the documentation that this setup is only an example and shouldn't be used in production for security reasons
I actually read the nodeJS docs as a best practice 🙈 If it is not documented here who should come up with the best possible approach?
There was a problem hiding this comment.
If we were to go with your approach, we wouldn't need a note because it wouldn't present a security issue anymore :-).
If the team agrees with your suggestion, you should open a PR for it. With all the work you put into finding and fixing this issue, you should be the one to request it!
WDYT @lpinca and @ShogunPanda?
There was a problem hiding this comment.
If the team agrees with your suggestion, you should open a PR for it.
Sure can do. Waiting for an agree- or disagreement 🙈
With all the work you put into finding and fixing this issue, you should be the one to request it!
Team afford. Kudos to @astlouisf and @samhh
There was a problem hiding this comment.
@RedYetiDev I agree, I can close this one and @mlegenhausen can open a seperate PR to fix this issue
The previous documentation example for converting request.url to an URL object was unsafe, as it could allow a server crash through malformed URL inputs and potentially enable host header attacks. This commit revises the example to use string concatenation, mitigating both the crash and security risks by ensuring the host part of the URL remains controlled and predictable.
Fixes: #52494