Location path 2 #15365
Location path 2 #15365
Conversation
| if (isDefined(appUrl = stripBaseUrl(appBase, url))) { | ||
| prevAppUrl = appUrl; | ||
| if (isDefined(appUrl = stripBaseUrl(basePrefix, appUrl))) { | ||
| if (basePrefix && isDefined(appUrl = stripBaseUrl(basePrefix, appUrl))) { |
There was a problem hiding this comment.
interestingly I think this was actually the cause of the bug in the first place...
There was a problem hiding this comment.
There are no tests for this change, are there?
There was a problem hiding this comment.
No new tests, specifically, but it doesn't break any of the current tests
And there are the new tests with the double slashes.
I couldn't think of a specific test for this change. Any ideas?
| }).toThrowMinErr('$location', 'badpath'); | ||
|
|
||
| expect(function() { | ||
| parseLinkAndReturn(locationUrl, 'http://server/pre/\\\\other/path'); |
There was a problem hiding this comment.
Could you add a test case with mixed slashes (pre//\\//other/path) ?
|
|
||
| function parseAppUrl(relativeUrl, locationObj) { | ||
| var prefixed = (relativeUrl.charAt(0) !== '/'); | ||
| if (url.search(DOUBLE_SLASH_REGEX) === 0) { |
There was a problem hiding this comment.
You could also add ^ at the beginning of DOUBLE_SLASH_REGEX and use DOUBLE_SLASH_REGEX.test(url)?
petebacondarwin
force-pushed
the
location-path-2
branch
from
ea818eb to
13ceb20
Compare
Previously `$location` was rewriting such paths to remove not only the double slashes but also the first segment of the path, leading to an invalid path. In this change, we deem leading double (back)slashes an invalid path and now throw a `$location:badpath` error if that occurs. Closes angular#15365
|
Looks good to me, security-wise. On Mon, Nov 7, 2016, 12:10 Pete Bacon Darwin [email protected]
|
|
LGTM |
Previously `$location` was rewriting such paths to remove not only the double slashes but also the first segment of the path, leading to an invalid path. In this change, we deem leading double (back)slashes an invalid path and now throw a `$location:badpath` error if that occurs. Closes angular#15365
petebacondarwin
force-pushed
the
location-path-2
branch
from
13ceb20 to
4aa9534
Compare
Previously `$location` was rewriting such paths to remove not only the double slashes but also the first segment of the path, leading to an invalid path. In this change, we deem leading double (back)slashes an invalid path and now throw a `$location:badpath` error if that occurs. Closes #15365
Previously `$location` was rewriting such paths to remove not only the double slashes but also the first segment of the path, leading to an invalid path. In this change, we deem leading double (back)slashes an invalid path and now throw a `$location:badpath` error if that occurs. Closes angular#15365
Previously `$location` was rewriting such paths to remove not only the double slashes but also the first segment of the path, leading to an invalid path. In this change, we deem leading double (back)slashes an invalid path and now throw a `$location:badpath` error if that occurs. Closes angular#15365
|
This fix has problems with internal routing of an app. Sometimes double slash in fragments might be important to indicate an specific state. I believe this fix should not take into account the fragment part of the URL since it's not sent to the server and breaks some applications. |
|
@ivanrey, afaict this is supposed to only check for the beginning of the client-side URL and I think it didn't work correctly before either. Do you have a specific usecase/demo you can share, that used to work before and broke as a result of this fix? |
Previously `$location` was rewriting such paths to remove not only the double slashes but also the first segment of the path, leading to an invalid path. In this change, we deem leading double (back)slashes an invalid path and now throw a `$location:badpath` error if that occurs. Closes angular#15365
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix
What is the current behavior? (You can also link to an open issue here)
see the commit message
What is the new behavior (if this is a feature change)?
see the commit message
Does this PR introduce a breaking change?
The previous behaviour would have led to a broken app, so it is highly
unlikely that any app relied on this behaviour. So this doesn't constitute
a breaking change.
Please check if the PR fulfills these requirements
Other information:
Alternative solution #15359