Skip to content
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

doc: correct unsafe URL example in http docs #52555

Merged
merged 2 commits into from
Apr 21, 2024

Conversation

mlegenhausen
Copy link
Contributor

Co-authored-by: @astlouisf
Co-authored-by: @samhh

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 over the usage of the baseUrl and removes the usage of the req.headers.host as the authority part of the url, mitigating both the crash and security risks by ensuring the host part of the URL remains controlled and predictable.

Fixes #52494
Successor of #52536

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Apr 16, 2024
@RedYetiDev
Copy link
Member

LGTM

@ShogunPanda
Copy link
Contributor

I kinda like it.
I would mention somewhere that process.env.HOST (and localhost probably) are just for that example and not best practice in general. I would try to communicate that the host part must be provided by the server in order to build a full URL.

@mlegenhausen
Copy link
Contributor Author

I would mention somewhere that process.env.HOST (and localhost probably) are just for that example and not best practice in general.

I will try to formulate something that doesn't get to complicated. For the classic copy/paste developer this at least contains no surprises. Everyone else beyond that will understand how to adapt this. Should I document something about the dangers of the Host header or is this not needed anymore?

@ShogunPanda
Copy link
Contributor

I would say keep it. When it's about security we are never verbose enough ;)

@mlegenhausen
Copy link
Contributor Author

How about 2e593a6#diff-d692ac4524379ec6a1201165e8ff8d3267c8130e07014e8221ebf7e6f80c6641R2913-R2915?

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 over the
usage of the `baseUrl` and removes the usage of the `req.headers.host`
as the authority part of the url, mitigating both the crash and security
risks by ensuring the host part of the URL remains controlled and
predictable.

Fixes nodejs#52494

Co-authored-by: @astlouisf
Co-authored-by: @samhh
@RedYetiDev
Copy link
Member

Everything seems good to me!

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM! Nice job!

@lpinca
Copy link
Member

lpinca commented Apr 17, 2024

@mlegenhausen can you please fix the lint issue?

@mlegenhausen
Copy link
Contributor Author

@lpinca fixed

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 21, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2024
@nodejs-github-bot nodejs-github-bot merged commit 461722d into nodejs:main Apr 21, 2024
20 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 461722d

aduh95 pushed a commit that referenced this pull request Apr 29, 2024
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 over the
usage of the `baseUrl` and removes the usage of the `req.headers.host`
as the authority part of the url, mitigating both the crash and security
risks by ensuring the host part of the URL remains controlled and
predictable.

Fixes #52494

Co-authored-by: @astlouisf
Co-authored-by: @samhh
PR-URL: #52555
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
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 over the
usage of the `baseUrl` and removes the usage of the `req.headers.host`
as the authority part of the url, mitigating both the crash and security
risks by ensuring the host part of the URL remains controlled and
predictable.

Fixes #52494

Co-authored-by: @astlouisf
Co-authored-by: @samhh
PR-URL: #52555
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
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 over the
usage of the `baseUrl` and removes the usage of the `req.headers.host`
as the authority part of the url, mitigating both the crash and security
risks by ensuring the host part of the URL remains controlled and
predictable.

Fixes #52494

Co-authored-by: @astlouisf
Co-authored-by: @samhh
PR-URL: #52555
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Unsafe example for converting a message.url to an URL
6 participants