Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

url resolve(href, '.') sometimes leaves off trailing slash #8992

Closed
blatx opened this issue Jan 7, 2015 · 5 comments
Closed

url resolve(href, '.') sometimes leaves off trailing slash #8992

blatx opened this issue Jan 7, 2015 · 5 comments
Assignees
@misterdjules

Comments

@blatx
Copy link

blatx commented Jan 7, 2015

The following script:

var url = require('url');
console.log(url.resolve('http://www.example.com/path/to/edit', 'view'));
console.log(url.resolve('/path/to/edit', 'view'));
console.log(url.resolve('http://www.example.com/path/to/edit', '.'));
console.log(url.resolve('/path/to/edit', '.'));

produces the following output:

http://www.example.com/path/to/view
/path/to/view
http://www.example.com/path/to/
/path/to

which is: resolving '.' produces a result that contains a trailing slash if the base url is absolute including host, but leaves it off if there is no host name (AKA "relative to the domain root").

These are very different urls both for a webserver and as a base url for a browser.

On every browser I have tested, using the following html

<html>
<head>
<base href="/path/to/edit" />
</head>
<body>
<a href="." onclick="alert(this.href); return false">test</a>
</body>
</html>

the href of the test link also always includes the slash.

Tested with v0.10.35
@amir-s
Copy link

amir-s commented Jan 8, 2015

I'm not sure if this is a bug or not.
In the source code (https://github.com/joyent/node/blob/master/lib/url.js#L473) it is mentioned: "urlParse appends trailing / to urls like http://www.example.com"

@blatx
Copy link
Author

blatx commented Jan 8, 2015

"http://www.example.com" and "http://www.example.com/" represent the exact same thing: the root in the domain. This is something else.

I found the following comment and code in the same file you pointed to:

  // if a url ENDs in . or .., then it must get a trailing slash.
  // however, if it ends in anything else non-slashy,
  // then it must NOT get a trailing slash.
  var last = srcPath.slice(-1)[0];
  var hasTrailingSlash = (
      (result.host || relative.host) && (last === '.' || last === '..') ||
      last === '');

I'm sure the "bug" is in that test for the host in (result.host || relative.host). I see no good reason at all to let that behaviour depend on whether a host as provided, or not. There is no well-known browser in the world that behaves that way, and compatibility with browser behaviour is supposed to be a very high priority for this module.

"." and ".." are directory specs, "/path/to/" is a directory spec, but "/path/to" is not.

Hence, the trailing slash should always be there.

@amir-s
Copy link

amir-s commented Jan 10, 2015

You are right.

Actually url.resolve('/blah', '.') produces an empty string too, which should produce /.

I managed to solve the problem on my machine. I'll send a pull request.

amir-s added a commit to amir-s/node that referenced this issue Jan 11, 2015
@amir-s
url: reslove urls with . and ..
1db28ef 
'.' and '..' are directory specs and resloving urls with or without
the hostname with '.' and '..' should add a trailing slash to the
end of the url.

Fix nodejs#8992
amir-s added a commit to amir-s/node that referenced this issue Jan 11, 2015
@amir-s
url: reslove urls with . and ..
3f99a92 
'.' and '..' are directory specs and resolving urls with or without
the hostname with '.' and '..' should add a trailing slash to the
end of the url.

Fix nodejs#8992
@amir-s amir-s mentioned this issue Jan 11, 2015
Closed
trevnorris pushed a commit that referenced this issue Feb 9, 2015
@amir-s @trevnorris
url: reslove urls with . and ..
ad06848 
'.' and '..' are directory specs and resolving urls with or without the
hostname with '.' and '..' should add a trailing slash to the end of the
url.

Fixes: #8992
PR-URL: #9010
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link

Fixed by ad06848.

cjihrig pushed a commit to nodejs/node that referenced this issue Feb 13, 2015
@amir-s @cjihrig
url: reslove urls with . and ..
faa687b 
'.' and '..' are directory specs and resolving urls with or
without the hostname with '.' and '..' should add a trailing
slash to the end of the url.

Fixes: nodejs/node-v0.x-archive#8992
PR-URL: #278
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
misterdjules pushed a commit to misterdjules/node that referenced this issue Mar 17, 2015
@amir-s
url: resolve urls with . and ..
e2f21b9 
'.' and '..' are directory specs and resolving urls with or without the
hostname with '.' and '..' should add a trailing slash to the end of the
url.

Fixes: nodejs#8992
PR-URL: nodejs#9010
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
misterdjules pushed a commit to misterdjules/node that referenced this issue Mar 17, 2015
@amir-s
url: resolve urls with . and ..
6ab3d43 
'.' and '..' are directory specs and resolving urls with or without the
hostname with '.' and '..' should add a trailing slash to the end of the
url.

Fixes nodejs#8992.
@misterdjules misterdjules mentioned this issue Mar 17, 2015
Closed
@misterdjules misterdjules self-assigned this Mar 17, 2015
misterdjules pushed a commit that referenced this issue Mar 17, 2015
@amir-s
url: resolve urls with . and ..
9b534e2 
'.' and '..' are directory specs and resolving urls with or without the
hostname with '.' and '..' should add a trailing slash to the end of the
url.

Fixes #8992.

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #9427
@misterdjules
Copy link

Just for the record, this actually landed for good in 9b534e2.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@misterdjules misterdjules

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@trevnorris @misterdjules @amir-s @blatx