url: fix `isURL` detection by checking `path` by Zhangdroid · Pull Request #48928 · nodejs/node · GitHub

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

Review requested:

@nodejs/url

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.

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.

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

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

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

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?

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>

Landed in d7ed3a4

Thank you for your contribution 🥳 @Zhangdroid

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

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>

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 outstanding review comments, 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

                */
               function isURL(self) {
-                return Boolean(self?.href && self.protocol && self.auth === undefined);
+                return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined);

url: fix `isURL` detection by checking `path` by Zhangdroid · Pull Request #48928 · nodejs/node · GitHub

Uh oh!

Uh oh!

url: fix isURL detection by checking path - #48928

url: fix isURL detection by checking path#48928Zhangdroid wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom Zhangdroid:zhuo/fix-is-urlCopy head branch name to clipboard

Conversation

Zhangdroid commented Jul 26, 2023

Uh oh!

nodejs-github-bot commented Jul 26, 2023

Uh oh!

Zhangdroid Jul 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
Zhangdroid wants to merge 1 commit into
nodejs:mainfrom
Zhangdroid:zhuo/fix-is-url

Zhangdroid Jul 26, 2023 •
edited
Loading