Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect pathname in version 1.5.0 #199

Closed
gdelacruzfdez opened this issue Feb 18, 2021 · 12 comments
Closed

Incorrect pathname in version 1.5.0 #199

gdelacruzfdez opened this issue Feb 18, 2021 · 12 comments

Comments

@gdelacruzfdez
Copy link

@gdelacruzfdez gdelacruzfdez commented 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.

@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented Feb 18, 2021

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

@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented Feb 18, 2021

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.

@gdelacruzfdez
Copy link
Author

@gdelacruzfdez gdelacruzfdez commented Feb 18, 2021

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

@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented Feb 18, 2021

http://localhost:3000/PROD/trends

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

@lpinca
Copy link
Member

@lpinca lpinca commented Feb 18, 2021

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

@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented Feb 18, 2021

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.

@JSeligsohn
Copy link

@JSeligsohn JSeligsohn commented 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

@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented 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

@lpinca
Copy link
Member

@lpinca lpinca commented Feb 18, 2021

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.

@JSeligsohn
Copy link

@JSeligsohn JSeligsohn commented 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

@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented Feb 18, 2021

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.

3rd-Eden added a commit that referenced this issue Feb 18, 2021
3rd-Eden added a commit that referenced this issue Feb 18, 2021
* [fix] Fixes relative path resolving #199 #200
* [test] Additional extractProtocol tests
@3rd-Eden
Copy link
Member

@3rd-Eden 3rd-Eden commented Feb 18, 2021

Fixed in 1.5.1, thanks for reporting <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants