url: fix `isURL` detection by checking `path` #48928

Zhangdroid · 2023-07-26T04:01:57Z

Fixes: #48921
Fixes: getsentry/sentry-javascript#8552
Fixes: request/request#3458

nodejs-github-bot · 2023-07-26T04:02:01Z

Review requested:

@nodejs/url

Zhangdroid · 2023-07-26T04:08:48Z

lib/internal/url.js

 * WHATWG URL instance.
 * @param {*} self
 * @returns {self is URL}
 */
 function isURL(self) {
-  return Boolean(self?.href && self.protocol && self.auth === undefined);
+  return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined);


TBH, I don't think this is the ideal way to fix the issue #48921 , but since path is an attribute in legacy URL object instance, and another issue has been fixed in same way: #47886 I think this fix maybe fine. If you have a better way, please let me know.

anonrig

Well, this is sort of like a chicken in the egg problem. We can't properly check for URL and we can't properly check for url. The only thing we can do is check their behavior/definition. I'm fine with merging this but, I'm afraid there will be some other edge case that we didn't cover.

nodejs-github-bot · 2023-07-26T14:09:06Z

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

nodejs-github-bot · 2023-07-26T22:07:24Z

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

nodejs-github-bot · 2023-07-28T04:14:43Z

Commit Queue failed

- Loading data for nodejs/node/pull/48928
✔  Done loading data for nodejs/node/pull/48928
----------------------------------- PR info ------------------------------------
Title      url: fix `isURL` detection by checking `path` (#48928)
Author     Zhuo Zhang  (@Zhangdroid, first-time contributor)
Branch     Zhangdroid:zhuo/fix-is-url -> nodejs:main
Labels     whatwg-url, author ready, needs-ci
Commits    1
 - url: fix `isURL` detection by checking `path`
Committers 1
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/48928
Fixes: https://github.com/nodejs/node/issues/48921
Fixes: https://github.com/getsentry/sentry-javascript/issues/8552
Fixes: https://github.com/request/request/issues/3458
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Luigi Pinca 
Reviewed-By: Matthew Aitken 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48928
Fixes: https://github.com/nodejs/node/issues/48921
Fixes: https://github.com/getsentry/sentry-javascript/issues/8552
Fixes: https://github.com/request/request/issues/3458
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Luigi Pinca 
Reviewed-By: Matthew Aitken 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 26 Jul 2023 04:01:57 GMT
   ✔  Approvals: 3
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/48928#pullrequestreview-1547810927
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/48928#pullrequestreview-1548415285
   ✔  - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/48928#pullrequestreview-1551214741
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-07-26T22:07:24Z: https://ci.nodejs.org/job/node-test-pull-request/52947/
- Querying data for job/node-test-pull-request/52947/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @Zhangdroid(i@zhuo.dev)
   ⚠  - commit 333fa8eaa787 is authored by contrary.zhuo@gmail.com
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

Zhangdroid · 2023-07-28T09:19:46Z

Hi @anonrig, I saw the bot tried to merge this PR but failed, it might because I use two emails, can you help to merge this?

nodejs-github-bot · 2023-07-29T17:57:24Z

Commit Queue failed

- Loading data for nodejs/node/pull/48928
✔  Done loading data for nodejs/node/pull/48928
----------------------------------- PR info ------------------------------------
Title      url: fix `isURL` detection by checking `path` (#48928)
Author     Zhuo Zhang  (@Zhangdroid, first-time contributor)
Branch     Zhangdroid:zhuo/fix-is-url -> nodejs:main
Labels     whatwg-url, author ready, needs-ci
Commits    1
 - url: fix `isURL` detection by checking `path`
Committers 1
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/48928
Fixes: https://github.com/nodejs/node/issues/48921
Fixes: https://github.com/getsentry/sentry-javascript/issues/8552
Fixes: https://github.com/request/request/issues/3458
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Luigi Pinca 
Reviewed-By: Matthew Aitken 
Reviewed-By: Debadree Chatterjee 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48928
Fixes: https://github.com/nodejs/node/issues/48921
Fixes: https://github.com/getsentry/sentry-javascript/issues/8552
Fixes: https://github.com/request/request/issues/3458
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Luigi Pinca 
Reviewed-By: Matthew Aitken 
Reviewed-By: Debadree Chatterjee 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 26 Jul 2023 04:01:57 GMT
   ✔  Approvals: 4
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/48928#pullrequestreview-1547810927
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/48928#pullrequestreview-1548415285
   ✔  - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/48928#pullrequestreview-1551214741
   ✔  - Debadree Chatterjee (@debadree25): https://github.com/nodejs/node/pull/48928#pullrequestreview-1553347070
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-07-28T04:14:43Z: https://ci.nodejs.org/job/node-test-pull-request/52947/
- Querying data for job/node-test-pull-request/52947/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @Zhangdroid(i@zhuo.dev)
   ⚠  - commit 333fa8eaa787 is authored by contrary.zhuo@gmail.com
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

Fixes: #48921 PR-URL: #48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

debadree25 · 2023-07-29T18:14:30Z

Landed in d7ed3a4

debadree25 · 2023-07-29T18:15:55Z

Thank you for your contribution 🥳 @Zhangdroid

Apollon77 · 2023-07-31T06:28:00Z

I known in which node-js version this fix will be included?

Fixes: nodejs#48921 PR-URL: nodejs#48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

aduh95 · 2023-08-08T10:27:29Z

I known in which node-js version this fix will be included?

As usual for semver-patch changes, it should be included in the next (non-security) release of the Current Line (which is 20.x at the time of writing), and once it has been on a Current Line for at least two weeks, it should be backported to the next non-security release of the Active LTS line (which is 18.x at the time of writing).

Fixes: nodejs#48921 PR-URL: nodejs#48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

Fixes: #48921 PR-URL: #48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

Fixes: nodejs#48921 PR-URL: nodejs#48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

Fixes: #48921 PR-URL: #48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

ruyadorno · 2023-09-08T02:45:06Z

This commit is not landing cleanly on v18.x-staging and will need manual backport if we want this to land on v18.x.

Fixes: nodejs#48921 PR-URL: nodejs#48928 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

Fixes: #48921 PR-URL: #48928 Backport-PR-URL: #50105 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

Fixes: nodejs/node#48921 PR-URL: nodejs/node#48928 Backport-PR-URL: nodejs/node#50105 Fixes: getsentry/sentry-javascript#8552 Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

url: fix isURL detection by checking path

333fa8e

Fixes: #48921

nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jul 26, 2023

Zhangdroid commented Jul 26, 2023

View reviewed changes

anonrig requested a review from aduh95 July 26, 2023 13:59

anonrig approved these changes Jul 26, 2023

View reviewed changes

anonrig added request-ci Add this label to start a Jenkins CI on a PR. lts-watch-v18.x and removed lts-watch-v18.x labels Jul 26, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2023

lpinca approved these changes Jul 26, 2023

View reviewed changes

github-actions bot mentioned this pull request Jul 27, 2023

CI Reliability 2023-07-27 nodejs/reliability#625

Open

18 tasks

anonrig 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 Jul 27, 2023

github-actions bot mentioned this pull request Jul 28, 2023

CI Reliability 2023-07-28 nodejs/reliability#626

Open

19 tasks

KhafraDev approved these changes Jul 28, 2023

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 28, 2023

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 28, 2023

github-actions bot mentioned this pull request Jul 29, 2023

CI Reliability 2023-07-29 nodejs/reliability#627

Open

23 tasks

debadree25 approved these changes Jul 29, 2023

View reviewed changes

debadree25 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 29, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 29, 2023

debadree25 closed this Jul 29, 2023

debadree25 removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 29, 2023

This was referenced Jul 30, 2023

CI Reliability 2023-07-30 nodejs/reliability#628

Open

CI Reliability 2023-07-31 nodejs/reliability#629

Open

This was referenced Aug 1, 2023

CI Reliability 2023-08-01 nodejs/reliability#630

Open

CI Reliability 2023-08-02 nodejs/reliability#631

Open

lforst mentioned this pull request Aug 4, 2023

request library fails with Sentry enabled on Node 20 getsentry/sentry-javascript#8552

Closed

3 tasks

UlisesGascon mentioned this pull request Aug 15, 2023

v20.6.0 proposal #49185

Merged

aduh95 mentioned this pull request Aug 18, 2023

v18.18.0 proposal #49220

Merged

ruyadorno added the backport-requested-v18.x label Sep 8, 2023

aduh95 mentioned this pull request Oct 9, 2023

[v18.x backport] url test #50105

Closed

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: fix `isURL` detection by checking `path` #48928

url: fix `isURL` detection by checking `path` #48928

Zhangdroid commented Jul 26, 2023

nodejs-github-bot commented Jul 26, 2023

Zhangdroid Jul 26, 2023 •

edited

Loading

anonrig left a comment

nodejs-github-bot commented Jul 26, 2023

nodejs-github-bot commented Jul 26, 2023

nodejs-github-bot commented Jul 28, 2023

Zhangdroid commented Jul 28, 2023

nodejs-github-bot commented Jul 29, 2023

debadree25 commented Jul 29, 2023

debadree25 commented Jul 29, 2023

Apollon77 commented Jul 31, 2023

aduh95 commented Aug 8, 2023

ruyadorno commented Sep 8, 2023

url: fix isURL detection by checking path #48928

url: fix isURL detection by checking path #48928

Conversation

Zhangdroid commented Jul 26, 2023

nodejs-github-bot commented Jul 26, 2023

Zhangdroid Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

anonrig left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 26, 2023

nodejs-github-bot commented Jul 26, 2023

nodejs-github-bot commented Jul 28, 2023

Zhangdroid commented Jul 28, 2023

nodejs-github-bot commented Jul 29, 2023

debadree25 commented Jul 29, 2023

debadree25 commented Jul 29, 2023

Apollon77 commented Jul 31, 2023

aduh95 commented Aug 8, 2023

ruyadorno commented Sep 8, 2023

url: fix `isURL` detection by checking `path` #48928

url: fix `isURL` detection by checking `path` #48928

Zhangdroid Jul 26, 2023 •

edited

Loading