fix($location): don't remove multiple slashes from urls #15359
Conversation
| 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/', '#!'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
BTW the changes in this beforeEach were only because the setup was actually wrong! The third parameter is supposed to be the hashbang prefix.
| @@ -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'); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I'll look into it...
Krzysztof also suggested that we need to deal with multiple leading backslashes too...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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) { |
There was a problem hiding this comment.
Do we need the extra leading / for slashCount > 1?
There was a problem hiding this comment.
Yes, because it gets stripped off at some other point in the code
There was a problem hiding this comment.
But don't we add it back 5 lines above with locationObj.$$path = decodeURIComponent(slashes + match.pathname.substr(1))?
There was a problem hiding this comment.
It's complicated. Try it! Tests will fail :-)
|
Closing to put a PR together that errors instead of trying to stumble along: in favour of #15365 |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: