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

 */
 functionisURL(self){
-returnBoolean(self?.href&&self.protocol&&self.auth===undefined);
+returnBoolean(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([email protected]) ⚠ - commit 333fa8eaa787 is authored by [email protected] -------------------------------------------------------------------------------- ✔ 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([email protected]) ⚠ - commit 333fa8eaa787 is authored by [email protected] -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

Fixes: #48921 PR-URL: #48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

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#48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

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#48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

Fixes: #48921 PR-URL: #48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

Fixes: nodejs#48921 PR-URL: nodejs#48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

Fixes: #48921 PR-URL: #48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

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#48928Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

Fixes: #48921 PR-URL: #48928 Backport-PR-URL: #50105Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

Fixes: nodejs/node#48921 PR-URL: nodejs/node#48928 Backport-PR-URL: nodejs/node#50105Fixes: getsentry/sentry-javascript#8552Fixes: request/request#3458 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

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

Uh oh!

url: fix isURL detection by checking path#48928

url: fix isURL detection by checking path#48928

Uh oh!

Conversation

Zhangdroid commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

ZhangdroidJul 26, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 28, 2023

Uh oh!

Zhangdroid commented Jul 28, 2023

Uh oh!

nodejs-github-bot commented Jul 29, 2023

Uh oh!

debadree25 commented Jul 29, 2023

Uh oh!

debadree25 commented Jul 29, 2023

Uh oh!

Apollon77 commented Jul 31, 2023

Uh oh!

aduh95 commented Aug 8, 2023

Uh oh!

ruyadorno commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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

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

ZhangdroidJul 26, 2023•
edited
Loading