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
Closed

doc: correct unsafe URL example in http docs #52536

Changes from all commits
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
4 changes: 2 additions & 2 deletions doc/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
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
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
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
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
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
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

@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
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