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

url: improve port validation #45012

Merged
merged 3 commits into from
Oct 17, 2022
Merged

url: improve port validation #45012

merged 3 commits into from
Oct 17, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 15, 2022

If a port is not a number, throw rather than treating the : that delineates the port as part of the path. This is consistent with WHATWG URL and also mitigates hostname-spoofing.

Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. labels Oct 15, 2022
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Oct 15, 2022

As with #45011, this could be considered a semver-major breaking change, but we may want to consider this is a bugfix instead. Out of caution, though, we should run CITGM. We should also definitely make sure that git+ssh URLs continue to work (or continue to not work) in npm before and after this change.

lib/url.js Outdated Show resolved Hide resolved
lib/url.js Outdated Show resolved Hide resolved
If a port is not a number, throw rather than treating the `:` that
delineates the port as part of the path. This is consistent with WHATWG
URL and also mitigates hostname-spoofing.

Concerns about hostname-spoofing were raised and presented in excellent
detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Oct 15, 2022

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3019/ (It will 404 until job 3018 finishes, though.)

@Trott
Copy link
Member Author

Trott commented Oct 15, 2022

CITGM results look good to me

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 15, 2022
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2022
@nodejs-github-bot nodejs-github-bot merged commit 5f7730e into nodejs:main Oct 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 5f7730e

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
If a port is not a number, throw rather than treating the `:` that
delineates the port as part of the path. This is consistent with WHATWG
URL and also mitigates hostname-spoofing.

Concerns about hostname-spoofing were raised and presented in excellent
detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).

PR-URL: #45012
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
If a port is not a number, throw rather than treating the `:` that
delineates the port as part of the path. This is consistent with WHATWG
URL and also mitigates hostname-spoofing.

Concerns about hostname-spoofing were raised and presented in excellent
detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).

PR-URL: #45012
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@merceyz
Copy link
Member

merceyz commented Nov 17, 2022

As with #45011, this could be considered a semver-major breaking change, but we may want to consider this is a bugfix instead. Out of caution, though, we should run CITGM. We should also definitely make sure that git+ssh URLs continue to work (or continue to not work) in npm before and after this change.

This ended up breaking git+ssh URLs in Yarn.

@Trott Trott deleted the toss-2 branch November 19, 2022 13:12
@Trott Trott added baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Nov 19, 2022
Trott added a commit to Trott/io.js that referenced this pull request Nov 19, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: nodejs#45514
Refs: nodejs#45012
@richardlau richardlau removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 19, 2022
@richardlau
Copy link
Member

Given the ecosystem breakage I can't see how this can land in LTS so I've removed the baking-for-lts label.

nodejs-github-bot pushed a commit that referenced this pull request Nov 19, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: #45514
Refs: #45012
PR-URL: #45517
Fixes: #45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: #45514
Refs: #45012
PR-URL: #45517
Fixes: #45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: nodejs#45514
Refs: nodejs#45012
PR-URL: nodejs#45517
Fixes: nodejs#45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants