Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($location): don't remove multiple slashes from urls #15359

Closed
wants to merge 1 commit into from
Closed

fix($location): don't remove multiple slashes from urls #15359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions src/ng/location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,25 @@ function parseAbsoluteUrl(absoluteUrl, locationObj) {
locationObj.$$port = toInt(parsedUrl.port) || DEFAULT_PORTS[parsedUrl.protocol] || null;
}

function countLeadingSlashes(url) {
for (var i = 0; url.charAt(i) === '/'; i++) { /* noop */}
return i;
}

function parseAppUrl(relativeUrl, locationObj) {
var prefixed = (relativeUrl.charAt(0) !== '/');
if (prefixed) {
relativeUrl = '/' + relativeUrl;
}
var match = urlResolve(relativeUrl);
locationObj.$$path = decodeURIComponent(prefixed && match.pathname.charAt(0) === '/' ?
match.pathname.substring(1) : match.pathname);
function parseAppUrl(url, locationObj) {
var slashCount = countLeadingSlashes(url);
var slashes = url.substr(0, slashCount);

// We need to pass a url with a single leading slash to the `urlResolve` function
// so we strip off all the slashes and put just one special one back on
var match = urlResolve('/' + url.substr(slashCount));
// Then we put the slashes back on (removing the special one)
locationObj.$$path = decodeURIComponent(slashes + match.pathname.substr(1));
locationObj.$$search = parseKeyValue(match.search);
locationObj.$$hash = decodeURIComponent(match.hash);

// make sure path starts with '/';
if (locationObj.$$path && locationObj.$$path.charAt(0) !== '/') {
// fix up if we were passed a url with none or multiple leading slashes
if (locationObj.$$path && slashCount !== 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the extra leading / for slashCount > 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because it gets stripped off at some other point in the code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But don't we add it back 5 lines above with locationObj.$$path = decodeURIComponent(slashes + match.pathname.substr(1))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's complicated. Try it! Tests will fail :-)

locationObj.$$path = '/' + locationObj.$$path;
}
}
Expand Down
10 changes: 7 additions & 3 deletions test/ng/locationSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2453,9 +2453,9 @@ describe('$location', function() {
var locationUrl, locationUmlautUrl, locationIndexUrl;

beforeEach(function() {
locationUrl = new LocationHtml5Url('http://server/pre/', 'http://server/pre/', 'http://server/pre/path');
locationUmlautUrl = new LocationHtml5Url('http://särver/pre/', 'http://särver/pre/', 'http://särver/pre/path');
locationIndexUrl = new LocationHtml5Url('http://server/pre/index.html', 'http://server/pre/', 'http://server/pre/path');
locationUrl = new LocationHtml5Url('http://server/pre/', 'http://server/pre/', '#!');
locationUmlautUrl = new LocationHtml5Url('http://särver/pre/', 'http://särver/pre/', '#!');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe consider adding tests for http://server//pre/? It's not a problem in this change I think, but it might be nice to get slightly more complete test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the changes in this beforeEach were only because the setup was actually wrong! The third parameter is supposed to be the hashbang prefix.

locationIndexUrl = new LocationHtml5Url('http://server/pre/index.html', 'http://server/pre/', '#!');
});

it('should rewrite URL', function() {
Expand All @@ -2480,6 +2480,10 @@ describe('$location', function() {
expect(parseLinkAndReturn(locationUrl, 'someIgnoredAbsoluteHref', '#test')).toEqual('http://server/pre/otherPath#test');
});

it('should cope with double slashes in the path', function() {
expect(parseLinkAndReturn(locationUrl, 'http://server/pre///other/path')).toEqual('http://server/pre///other/path');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a more integration-style test that checks how the app URL is used to resolve a template would be useful? With this change, we still end up with multiple slashes in the URL, so we could still accidentally drop a path segment if we later parse it using <a href> again (not sure we do, but protecting from it would be nice).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into it...

Krzysztof also suggested that we need to deal with multiple leading backslashes too...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just multiple backslashes. Basically \s*[\\/]{2,} would be a problem (but since in parseAppUrl / is prepended, the whitespace vector goes away, so I think changing:

for (var i = 0; url.charAt(i) === '/'; i++)
to
for (var i = 0; url.charAt(i) === '/' || url.charAt(i) === '\\' ; i++)

would be enough to fix the bug.

That said, I don't know if the multiple slashes are worth preserving in the beginning of a path for Angular apps. Existing apps served on those would be broken without this patch, so there is no backwards-compatibility problem and it's hard to tell if accepting "//" would not cause further errors when resolving relative URLs for the app later on. Prohibiting that overall seems like a good, conservative choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I agree that we may as well lock this down, rather than potentially passing the issue onto some other part of the code. We can always relax this error and do something similar to this PR, later, if people complain.

});


it('should complain if no base tag present', function() {
module(function($locationProvider) {
Expand Down