url: runtime deprecate url.parse

[anonrig]

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

[nodejs-github-bot]

Review requested:

• @nodejs/url

[lpinca]

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.

[anonrig]

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.

[lpinca]

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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%> (ø)

... and 42 files with indirect coverage changes

[lemire]

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'?

[targos]

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.

[mcollina]

I think we should be doing what we have done for Buffer, emitting a warning only if the code is not inside node_modules.

[jasnell]

I agree with Matteo here. This will be super disruptive. Let's limit the warning only to modules not inside node_modules.

[anonrig]

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!

[jasnell]

Looks like there are some tests that need updating

[mcollina]

lgtm

but CI is failing

[targos]

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)

[mcollina]

still lgtm

[aduh95]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63202/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63220/

[nodejs-github-bot]

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.parse
PR-URL: #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: #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-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/11428689062

[nodejs-github-bot]

Landed in 11fbdd8