nodejs
url: runtime deprecate url.parse
We documentation-only deprecated URL.parse on v18, almost 2 years ago. Without a runtime deprecation people will continue to use it and be exposed to security flaws. This is a nudge on the direction for a possible EOL in 3-5 years?
cc @nodejs/tsc
I don't think this is used for new code and there are a lot of unmaintained but perfectly working and safe modules that will be affected by this for no good reason.
safe modules that will be affected by this for no good reason.
Even in the deprecation note says that it's not recommended and safe to use it. How can it be safe? url.parse() can result in unwanted/unexpected outputs.
It is perfectly safe when used on trusted and well defined inputs. For example, there is nothing wrong with url.parse() when used to parse URLs returned by a trusted server like the URLs in the Location and Link headers.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 88.41%. Comparing base (
7ae193d) to head (76c9793). Report is 13 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #55017 +/- ##
========================================
Coverage 88.40% 88.41%
========================================
Files 653 653
Lines 187600 187484 -116
Branches 36117 36091 -26
========================================
- Hits 165854 165755 -99
+ Misses 14974 14967 -7
+ Partials 6772 6762 -10
| Files with missing lines | Coverage Ξ | |
|---|---|---|
| lib/url.js | 100.00% <100.00%> (ΓΈ) |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
perfectly working and safe modules that will be affected by this for no good reason.
I am neutral here, but can you elaborate what you mean by 'affected'?
For example, the mongodb module uses url.parse. It means that potentially all Node.js applications using it will print a warning, annoying many users who are not the ones that should care about it.
I think we should be doing what we have done for Buffer, emitting a warning only if the code is not inside node_modules.
I agree with Matteo here. This will be super disruptive. Let's limit the warning only to modules not inside node_modules.
I think we should be doing what we have done for Buffer, emitting a warning only if the code is not inside node_modules.
I updated the code as recommended. Thank you for the reviews!
gyp ERR! configure error
gyp ERR! stack TypeError: isInsideNodeModules is not a function
gyp ERR! stack at urlParse (node:url:126:27)
gyp ERR! stack at Object.urlResolve [as resolve] (node:url:717:10)
Related test failure:
=== release test-url-parse-invalid-input ===
Path: parallel/test-url-parse-invalid-input
Error: --- stderr ---
(node:174506) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
(Use `node --trace-deprecation ...` to show where the warning was created)
node:assert:90
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ '`url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.'
- 'The URL https://evil.com:.example.com/ is invalid. Future versions of Node.js will throw an error.'
at Object.<anonymous> (/home/runner/work/node/node/test/common/index.js:684:14)
at Object.DeprecationWarning (/home/runner/work/node/node/test/common/index.js:491:15)
at process.<anonymous> (/home/runner/work/node/node/test/common/index.js:708:33)
at process.emit (node:events:519:35)
at doEmitWarning (node:internal/process/warning:85:11)
at process.processTicksAndRejections (node:internal/process/task_queues:89:21) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: '`url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.',
expected: 'The URL https://evil.com:.example.com/ is invalid. Future versions of Node.js will throw an error.',
operator: 'strictEqual'
}
Node.js v24.0.0-pre
Command: out/Release/node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /home/runner/work/node/node/test/parallel/test-url-parse-invalid-input.js
===
=== 1 tests failed
===
Commit Queue failed
- Loading data for nodejs/node/pull/55017 β Done loading data for nodejs/node/pull/55017 ----------------------------------- PR info ------------------------------------ Title url: runtime deprecate url.parse (#55017) β Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch anonrig:runtime-deprecate-url-parse -> nodejs:main Labels url, semver-major, deprecations, needs-ci Commits 2 - url: runtime deprecate url.parse - fix test Committers 1 - Yagiz Nizipli <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55017 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55017 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- βΉ This PR was created on Thu, 19 Sep 2024 19:40:10 GMT β Approvals: 3 β - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55017#pullrequestreview-2378667703 β - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/55017#pullrequestreview-2379246574 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55017#pullrequestreview-2380649916 β Last GitHub CI successful βΉ Last Full PR CI on 2024-10-20T16:41:32Z: https://ci.nodejs.org/job/node-test-pull-request/63220/ - Querying data for job/node-test-pull-request/63220/ β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD β origin/main is now up-to-date - Downloading patch for 55017 From https://github.com/nodejs/node * branch refs/pull/55017/merge -> FETCH_HEAD β Fetched commits as c124cfb4facb..76c97932e55b -------------------------------------------------------------------------------- [main dff1620ae5] url: runtime deprecate url.parse Author: Yagiz Nizipli <[email protected]> Date: Thu Sep 19 15:38:25 2024 -0400 3 files changed, 8 insertions(+), 4 deletions(-) [main fb8ddd6cdc] fix test Author: Yagiz Nizipli <[email protected]> Date: Sat Oct 19 12:54:22 2024 -0400 1 file changed, 5 insertions(+), 5 deletions(-) β Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- url: runtime deprecate url.parsehttps://github.com/nodejs/node/actions/runs/11428689062PR-URL: https://github.com/nodejs/node/pull/55017 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: James M Snell <[email protected]>
[detached HEAD dc82eebff2] url: runtime deprecate url.parse Author: Yagiz Nizipli <[email protected]> Date: Thu Sep 19 15:38:25 2024 -0400 3 files changed, 8 insertions(+), 4 deletions(-) Rebasing (3/4) Rebasing (4/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fix test
PR-URL: https://github.com/nodejs/node/pull/55017 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: James M Snell <[email protected]>
[detached HEAD d8affe05b8] fix test Author: Yagiz Nizipli <[email protected]> Date: Sat Oct 19 12:54:22 2024 -0400 1 file changed, 5 insertions(+), 5 deletions(-) Successfully rebased and updated refs/heads/main.
βΉ Add
commit-queue-squashlabel to land the PR as one commit, orcommit-queue-rebaseto land as separate commits.
Nit: it's not immediately obvious what this API should be replaced with. Personally, the "WHATWG URL API" wasn't explicit enough, and I had to lookup this PR and search on google to understand that the recommendation is simple new URL() instead of url.parse().
Maybe it could be nice to mention URL.parse() or new URL() in the message, and/or link to this page?
https://nodejs.org/dist/latest/docs/api/url.html#the-whatwg-url-api
I think it would also save time to the community if some common migration cases were addressed.
For example url.parse("/xyz").pathname works, but we need URL.parse("/xyz","https://example.com").pathname with the WHATWG API
@slorber Agreed, this has been really painful for years with no clear path forward.
Here was my workaround from 2020 and that still seems to be the best solution:
- https://github.com/nodejs/web-server-frameworks/issues/71#issuecomment-702903372
Thoughts on documenting that?
