`isURL` cannot accurately determine if it is a URL object #48921

Zhangdroid · 2023-07-25T13:52:34Z

Version

20.5.0

Platform

Darwin 22.2.0

Subsystem

No response

What steps will reproduce the bug?

When using request library and @sentry/nodejs, the https/http will send the request with empty path - https://api.io/testing will become https://api.io.

The reason is request library added an href field to request args, and Sentry added a protocol field:

https://github.com/getsentry/sentry-javascript/blob/beec5be5cf90fc13d5c0000419422f51d2d75d1c/packages/node/src/integrations/utils/http.ts#L188-L192
https://github.com/request/request/blob/3c0cddc7c8eb60b470e9519da85896ed7ee0081e/request.js#L729

And when href and protocol both exist, isURL will think it's an URL object and reset path to pathname+search, and the above is clearly not a URL object and does not have pathname

node/lib/internal/url.js

Lines 761 to 763 in 36c72c8

    
           function isURL(self) { 
        
             return Boolean(self?.href && self.protocol && self.auth === undefined); 
        
           }

node/lib/internal/url.js

Lines 1332 to 1336 in 36c72c8

    
             search: search, 
        
             pathname: pathname, 
        
             path: `${pathname || ''}${search || ''}`, 
        
             href: url.href, 
        
           };

Having the following code, if the protocol and href in options, the https will set path to empty to request https://64bfcd8c0d8e251fd1117729.mockapi.io, and without protocol or href, it will request https://64bfcd8c0d8e251fd1117729.mockapi.io/test as expected.

const https = require('https');
const href = 'https://64bfcd8c0d8e251fd1117729.mockapi.io/test';
const options = {
  method: 'GET',
  host: '64bfcd8c0d8e251fd1117729.mockapi.io',
  href,
  protocol: 'https:',
  path: '/test',
};

const req = https.request(options);

let rawData = '';
req.on('response', (res) => {
  res.on('data', (chunk) => {
    rawData += chunk;
  });
  res.on('end', () => {
    try {
      const parsedData = JSON.parse(rawData);
      console.log(parsedData);
    } catch (e) {
      console.error('error', e.message);
    }
  });
});

req.on('error', (e) => {
  console.log(e);
});

req.end();

And when run node test.js with protocol and href, it will output

error Unexpected token '<', "<html>
<h"... is not valid JSON

And if delete protocol or href, it will get the correct response []

How often does it reproduce? Is there a required condition?

Everytime.

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

isURL cannot accurately determine if it is a URL object

Additional information

Of course, the request library might should not add the href field since it is not the correct args for http(s).request, but it's not maintained and there are still a lot of users, it should be fixed.

IMO, the root cause is isURL cannot accurately determine if it is a URL object, I think it can be fixed by

Change isURL to

 function isURL(self) { 
   return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined); 
 }

Or respect user's path by changing urlToHttpOptions - move ...url to the bottom of options instead of top.

  const options = {
    __proto__: null,
    protocol: url.protocol,
    hostname: hostname && StringPrototypeStartsWith(hostname, '[') ?
      StringPrototypeSlice(hostname, 1, -1) :
      hostname,
    hash: url.hash,
    search: search,
    pathname: pathname,
    path: `${pathname || ''}${search || ''}`,
    href: url.href,
    ...url, // In case the url object was extended by the user.
  };

The text was updated successfully, but these errors were encountered:

anonrig · 2023-07-26T01:24:31Z

Can you open a pull request with your proposed changes?

Apollon77 · 2023-07-31T06:27:32Z

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

nodejs/node#48921

debadree25 · 2023-08-01T19:40:53Z

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

In the next version of node 20 for sure and in node 18 too after this backported

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>

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>

Zhangdroid mentioned this issue Jul 25, 2023

network issues with node 20+ request/request#3458

Open

pkerschbaum mentioned this issue Jul 25, 2023

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

Closed

3 tasks

Zhangdroid mentioned this issue Jul 26, 2023

url: fix isURL detection by checking path #48928

Closed

mydea mentioned this issue Jul 26, 2023

Incompatibility with loopback-connector-rest getsentry/sentry-javascript#8549

Closed

3 tasks

debadree25 closed this as completed in d7ed3a4 Jul 29, 2023

mifi added a commit to transloadit/uppy that referenced this issue Jul 31, 2023

lock node-js version on ci

20cc9c2

nodejs/node#48921

mifi mentioned this issue Jul 31, 2023

lock node-js version on ci transloadit/uppy#4606

Merged

mifi added a commit to transloadit/uppy that referenced this issue Jul 31, 2023

lock node-js version on ci (#4606)

2e8a78d

nodejs/node#48921

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`isURL` cannot accurately determine if it is a URL object #48921

`isURL` cannot accurately determine if it is a URL object #48921

Zhangdroid commented Jul 25, 2023 •

edited

Loading

anonrig commented Jul 26, 2023

Apollon77 commented Jul 31, 2023

debadree25 commented Aug 1, 2023

isURL cannot accurately determine if it is a URL object #48921

isURL cannot accurately determine if it is a URL object #48921

Comments

Zhangdroid commented Jul 25, 2023 • edited Loading

Version

Platform

Subsystem

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior? Why is that the expected behavior?

What do you see instead?

Additional information

anonrig commented Jul 26, 2023

Apollon77 commented Jul 31, 2023

debadree25 commented Aug 1, 2023

`isURL` cannot accurately determine if it is a URL object #48921

`isURL` cannot accurately determine if it is a URL object #48921

Zhangdroid commented Jul 25, 2023 •

edited

Loading