Skip to content

doc: correct unsafe URL example in http docs #52536

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.

Sign up for GitHub

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

Closed
thisalihassan wants to merge 2 commits into from
+2 −2
Closed

doc: correct unsafe URL example in http docs #52536

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf1ce59
doc: correct unsafe URL example in http docs
thisalihassan Apr 15, 2024
95be674
doc: resolved feedback
thisalihassan Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions doc/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ const proxy = createServer((req, res) => {
});
proxy.on('connect', (req, clientSocket, head) => {
// Connect to an origin server
const { port, hostname } = new URL(`http://${req.url}`);
const { port, hostname } = new URL(`http://${req.headers.host}${req.url}`);
Comment thread
thisalihassan marked this conversation as resolved.
Outdated
const serverSocket = connect(port || 80, hostname, () => {
clientSocket.write('HTTP/1.1 200 Connection Established\r\n' +
'Proxy-agent: Node.js-Proxy\r\n' +
Expand Down Expand Up @@ -543,7 +543,7 @@ const proxy = http.createServer((req, res) => {
});
proxy.on('connect', (req, clientSocket, head) => {
// Connect to an origin server
const { port, hostname } = new URL(`http://${req.url}`);
const { port, hostname } = new URL(`http://${req.headers.host}${req.url}`);
Comment thread
thisalihassan marked this conversation as resolved.
Outdated
const serverSocket = net.connect(port || 80, hostname, () => {
clientSocket.write('HTTP/1.1 200 Connection Established\r\n' +
'Proxy-agent: Node.js-Proxy\r\n' +
Expand Down Expand Up @@ -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}`);
Copy link
Copy Markdown
Member

@lpinca lpinca Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not safer than the original, see #52494 (comment)

Copy link
Copy Markdown
Member

@avivkeller avivkeller Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I reviewed without thoroughly analysis. I second @redyetidev idea.

Copy link
Copy Markdown
Contributor

@mlegenhausen mlegenhausen Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@avivkeller avivkeller Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@mlegenhausen mlegenhausen Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

@getalihassan getalihassan Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redyetidev I agree, I can close this one and @mlegenhausen can open a seperate PR to fix this issue

Copy link
Copy Markdown
Contributor

@mlegenhausen mlegenhausen Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done #52555

```

When `request.url` is `'/status?name=ryan'` and `request.headers.host` is
`'localhost:3000'`:

```console
$ node
> new URL(request.url, `http://${request.headers.host}`)
> new URL(`http://${req.headers.host}${req.url}`)
URL {
href: 'http://localhost:3000/status?name=ryan',
origin: 'http://localhost:3000',
Expand Down
⚡