Visitar URL original
Incorrect pathname in version 1.5.0 · Issue #199 · unshiftio/url-parse · GitHub
Skip to content

Incorrect pathname in version 1.5.0 #199

Closed
Closed
Incorrect pathname in version 1.5.0#199

Description

@gdelacruzfdez
gdelacruzfdez
opened on Feb 18, 2021

Good morning,
Since the last update the pathname is not correctly parsed.

If I run this example from http://localhost:3000/PROD/trends:

new URL('/dataApi/PROD/ws')

Version 1.5.0 of the library returns this:

{
   "auth":"",
   "hash":"",
   "host":"localhost:3000",
   "hostname":"localhost",
   "href":"http://localhost:3000/PROD/dataApi/PROD/ws",
   "origin":"http://localhost:3000",
   "password":"",
   "pathname":"/PROD/dataApi/PROD/ws",
   "port":"3000",
   "protocol":"http:",
   "query":"",
   "slashes":true,
   "username":""
}

While version 1.4.7 was returning this:

{
   "auth":"",
   "hash":"",
   "host":"localhost:3000",
   "hostname":"localhost",
   "href":"http://localhost:3000/dataApi/PROD/ws",
   "origin":"http://localhost:3000",
   "password":"",
   "pathname":"//dataApi/PROD/ws",
   "port":"3000",
   "protocol":"http:",
   "query":"",
   "slashes":true,
   "username":""
}

AFAIK, the pathname of the page you are in shouldn't be taken into account to parse the URL.

This bug is breaking sockjs-client library.

Activity

  1. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member

    Looking into this as we speak, that seems super wierd 😞

  2. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member

    It's interesting that when I test:

    var url = '/dataApi/PROD/ws'
      , parsed = parse(url, 'http://localhost:3000/PROD');

    console.log(parsed);
    assume(parsed.pathname).equals('/dataApi/PROD/ws');

    it passes the assertion just fine and outputs:

    { slashes: true,
  protocol: 'http:',
  hash: '',
  query: '',
  pathname: '/dataApi/PROD/ws',
  auth: '',
  host: 'localhost:3000',
  port: '3000',
  hostname: 'localhost',
  password: '',
  username: '',
  origin: 'http://localhost:3000',
  href: 'http://localhost:3000/dataApi/PROD/ws' }

    Which is what we would expect.

  3. gdelacruzfdez commented on Feb 18, 2021

    @gdelacruzfdez
    gdelacruzfdez
    on Feb 18, 2021
    Author

    Actually I put a wrong url. The url from which I try is http://localhost:3000/PROD/trends

  4. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member

    http://localhost:3000/PROD/trends

    Ah, now it's indeed breaking. That so weird :/

  5. lpinca commented on Feb 18, 2021

    @lpinca
    lpinca
    on Feb 18, 2021
    Member

    The problem is caused by the new protocolre regular expression. data.rest is no longer /dataApi/PROD/ws but dataApi/PROD/ws.

  6. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member

    The problem is caused by the new protocolre regular expression. data.rest is no longer /dataApi/PROD/ws but dataApi/PROD/ws.

    That's what I thought as well, but unfortunately that regexp change is necessary to catch all the weird slash combo's that people can use within URL's. I thought I fixed all edge-cases but somehow this one slipped through. Even our massive resolve test didn't get triggered by this.

  7. JSeligsohn commented on Feb 18, 2021

    @JSeligsohn
    JSeligsohn
    on Feb 18, 2021

    Just jumping in here for a minute, as I see it's an active conversation right now.
    Looks like in v1.5.0, pathname no longer includes the initial / from the url. Is that intentional? A breaking change from v1.4.7?

    e.g.
    url: https://google.com/yippee
    1.4.7 -> path = /yippee
    1.5.0 -> path = yippee

  8. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member

    Just jumping in here for a minute, as I see it's an active conversation right now.
    Looks like in v1.5.0, pathname no longer includes the initial / from the url. Is that intentional? A breaking change from v1.4.7?

    e.g.
    url: https://google.com/yippee
    1.4.7 -> path = /yippee
    1.5.0 -> path = yippee

    > url('http://google.com/yippy')
{ slashes: true,
  protocol: 'http:',
  hash: '',
  query: '',
  pathname: '/yippy',
  auth: '',
  host: 'google.com',
  port: '',
  hostname: 'google.com',
  password: '',
  username: '',
  origin: 'http://google.com',
  href: 'http://google.com/yippy' }

    Cant reproduce that, please open a new ticket with steps to reproduce

  9. lpinca commented on Feb 18, 2021

    @lpinca
    lpinca
    on Feb 18, 2021
    Member
    diff --git a/index.js b/index.js
index e54575a..b35a9df 100644
--- a/index.js
+++ b/index.js
@@ -123,7 +123,7 @@ function extractProtocol(address) {
   return {
     protocol: protocol,
     slashes: slashes,
-    rest: match[3]
+    rest: match[2] && match[2].length === 1 ? '/' + match[3] : match[3]
   };
 }

    This seems to fix the issue but I'm not sure if it has side effects and we also need a regression test.

  10. JSeligsohn commented on Feb 18, 2021

    @JSeligsohn
    JSeligsohn
    on Feb 18, 2021

    Just jumping in here for a minute, as I see it's an active conversation right now.
    Looks like in v1.5.0, pathname no longer includes the initial / from the url. Is that intentional? A breaking change from v1.4.7?
    e.g.
    url: https://google.com/yippee
    1.4.7 -> path = /yippee
    1.5.0 -> path = yippee

    > url('http://google.com/yippy')
{ slashes: true,
  protocol: 'http:',
  hash: '',
  query: '',
  pathname: '/yippy',
  auth: '',
  host: 'google.com',
  port: '',
  hostname: 'google.com',
  password: '',
  username: '',
  origin: 'http://google.com',
  href: 'http://google.com/yippy' }

    Cant reproduce that, please open a new ticket with steps to reproduce

    Yes, will do. Thanks. Please see here: Issue #200

  11. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member
    diff --git a/index.js b/index.js
index e54575a..b35a9df 100644
--- a/index.js
+++ b/index.js
@@ -123,7 +123,7 @@ function extractProtocol(address) {
   return {
     protocol: protocol,
     slashes: slashes,
-    rest: match[3]
+    rest: match[2] && match[2].length === 1 ? '/' + match[3] : match[3]
   };
 }

    This seems to fix the issue but I'm not sure if it has side effects and we also need a regression test.

    I landed on a similar fix:

    function extractProtocol(address) {
  address = trimLeft(address);

  var match = protocolre.exec(address)
    , protocol = match[1] ? match[1].toLowerCase() : ''
    , slashes = !!(match[2] && match[2].length >= 2)
    , rest = match[2] && !protocol ? match[2] + match[3] : match[3];

  return {
    protocol: protocol,
    slashes: slashes,
    rest: rest
  };
}

    But it's currently breaking 2 tests.

  12. 3rd-Eden
    added 2 commits that reference this issue on Feb 18, 2021

    [fix] Fixes relative path resolving #199 #200

    9819b59

    [fix] Fixes relative path resolving #199 #200 (#201)

    750d8e8

  13. 3rd-Eden commented on Feb 18, 2021

    @3rd-Eden
    3rd-Eden
    on Feb 18, 2021
    Member

    Fixed in 1.5.1, thanks for reporting <3

  14. 3rd-Eden
    closed this as completedon Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions