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

Revert "url: improve port validation" #45517

Merged
merged 1 commit into from Nov 19, 2022
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented 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 has been around for a long time and we can wait for a major release to introduce the fix 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

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
@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 Nov 19, 2022
@Trott
Copy link
Member Author

Trott commented Nov 19, 2022

@ljharb @merceyz

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2022
@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added 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
@richardlau
Copy link
Member

Since this is a revert, setting the same don't-land labels as #45012.

@ljharb
Copy link
Member

ljharb commented Nov 19, 2022

Thanks!

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 19, 2022
@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 19, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @Trott. Please 👍 to approve.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45517
✔  Done loading data for nodejs/node/pull/45517
----------------------------------- PR info ------------------------------------
Title      Revert "url: improve port validation" (#45517)
Author     Rich Trott  (@Trott)
Branch     Trott:revert-it -> nodejs:main
Labels     url, fast-track, author ready, needs-ci, dont-land-on-v14.x, dont-land-on-v16.x, dont-land-on-v18.x
Commits    1
 - Revert "url: improve port validation"
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/45517
Fixes: https://github.com/nodejs/node/issues/45514
Refs: https://github.com/nodejs/node/pull/45012
Reviewed-By: James M Snell 
Reviewed-By: Richard Lau 
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45517
Fixes: https://github.com/nodejs/node/issues/45514
Refs: https://github.com/nodejs/node/pull/45012
Reviewed-By: James M Snell 
Reviewed-By: Richard Lau 
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 19 Nov 2022 14:23:45 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/45517#pullrequestreview-1187082412
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/45517#pullrequestreview-1187082570
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/45517#pullrequestreview-1187100493
   ℹ  This PR is being fast-tracked
   ✖  The fast-track request requires at least two collaborators' approvals (👍).
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-11-19T15:29:52Z: https://ci.nodejs.org/job/node-test-pull-request/48018/
- Querying data for job/node-test-pull-request/48018/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3504637731

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 19, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2022
@nodejs-github-bot nodejs-github-bot merged commit bd965aa into nodejs:main Nov 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in bd965aa

@Trott Trott deleted the revert-it branch November 19, 2022 20:07
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>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
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. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. fast-track PRs that do not need to wait for 48 hours to land. 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.

node 19.1 breaks url.parse
7 participants