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

AngularJS unescapes matrix parameters #16312

Closed
1 of 3 tasks
bourey opened this issue Oct 31, 2017 · 20 comments · Fixed by #16316
Closed
1 of 3 tasks

AngularJS unescapes matrix parameters #16312

bourey opened this issue Oct 31, 2017 · 20 comments · Fixed by #16316

Comments

@bourey
Copy link
Contributor

bourey commented Oct 31, 2017

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:
AngularJS un-escapes matrix parameters, preventing matrix parameters with slashes from being used in dual-router AngularJS / Angular applications.

Expected / new behavior:
AngularJS should not un-escape matrix parameters.

Minimal reproduction of the problem with instructions:
In a hybrid setup, add a link like: <a [routerLink]="['/a/ng2', {path: '/some/path'}]">ANGULAR A
While Angular properly renders this link as "http://localhost:4200/a/ng2;path=%2Fsome%2Fpath", AngularJS rewrites it as http://localhost:4200/a/ng2;path=/some/path during navigation. Depending on the setup, this causes the application to display either a missing route mapping or a redirect loop.

AngularJS version: 1.5+

Browser: all

@petebacondarwin
Copy link
Contributor

This sounds like a valid bug. Thanks for reporting @bourey !

@petebacondarwin
Copy link
Contributor

Is it $location or $route that is doing the rewriting?

@petebacondarwin petebacondarwin added this to the 1.7.0 milestone Nov 1, 2017
@jasonaden
Copy link

@petebacondarwin I think it's $location. I've looked at this encoding/decoding before and I think it's all from $location.

@petebacondarwin
Copy link
Contributor

I think the exact problem here is that we are unescaping forward slashes, right?
We have tests that check that we unescape other special characters.
Can anyone point me to the spec for these matrix params? I can't find anything official.

@petebacondarwin
Copy link
Contributor

I have created a PR for this: #16316 - please take a look. As with all things related to the can of worms that is $location fixing this bug could well create problems with loads of other use cases.

@hanoii
Copy link

hanoii commented Nov 29, 2017

I just want to flag, without much information as I don't have it right now but finally found out that this update was causing of an issue we were having with https://github.com/angular-ui/ui-router which wasn't there before.

For some reason $state.go('catchall', {alias: URL}) we have setup stopped to work and encoded URLs appeared on the URL breaking the route handling altogether. It might be some inconsistency with uirouter or something else entirely specific to my app but as it was triggered by this update (and very likely this issue) wanted to note it.

@petebacondarwin
Copy link
Contributor

@hanoii - perhaps this regression fix will solve your problem: #16316 ?

@hanoii
Copy link

hanoii commented Nov 30, 2017

@petebacondarwin I just tried master and it didn't fix my problem. Not sure if I am testing it correctly. I copied over the built angular.js and angular-sanitize.js, but not the other bits, but I think the issue is within angular.js anyway.

@dhoko
Copy link

dhoko commented Nov 30, 2017

Hey 👋 ,
This fix https://github.com/angular/angular.js/pull/16350/files#diff-06a39a7c214933c36a5c4b080ef4d56dR425 doesn't work for me, my app is still broken. (with ui-routeur 1.0.11)

Why not using decodeURIComponent instead of decodeURI for HTML5 mode too ?

@petebacondarwin
Copy link
Contributor

@dhoko - I think I agree. See #16354

@dhoko
Copy link

dhoko commented Dec 1, 2017

This fix works, thx 👍

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 1, 2017
The original fix for angular#16312 including changing how `$location.url(value)`
decoded the special characters passed to it as a setter.
This broke a number of use cases (mostly involving the ui-router).
Further analysis appears to show that we can solve angular#16312, to prevent
urls being rewritten with decoded values, without modifying the
behaviour of `$location.url`.

This commit reverts changes to `$location.url(value)` so that encoded
chars will be decoded and passed to `$location.path(value)` is updated.
In particular it will convert encoded forward slashes, which changes how
the path is updated, since `a/b/%2Fc%2Fd while become `a/b/c/d`.
While this is arguably not "correct", appears that there are too many
use cases relying upon this behaviour.
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 1, 2017
The original fix for angular#16312 included changing how `$location.url(value)`
decoded the special characters passed to it as a setter.
This broke a number of use cases (mostly involving the ui-router).

Further analysis appears to show that we can solve angular#16312, to prevent
urls being rewritten with decoded values, without modifying the
behaviour of `$location.url`.

This commit reverts changes to `$location.url(value)` so that encoded
chars will once again be decoded and passed to `$location.path(value)`.
In particular it will convert encoded forward slashes, which changes how
the path is updated, since e.g. `a/b/%2Fc%2Fd` will become `a/b/c/d`.
While this is arguably not "correct", it appears that there are too many
use cases relying upon this behaviour.
@Narretz
Copy link
Contributor

Narretz commented Dec 8, 2017

@bourey can you check if your use case still works with AngularJS master + this patch #16354? We had to make some changes so that other use cases do not break.

@hanoii
Copy link

hanoii commented Dec 9, 2017

@Narretz at least I tried it and the issue I was having on #16312 (comment) seems to have gone with master + patch #16354

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 11, 2017
The original fix for angular#16312 included changing how `$location.url(value)`
decoded the special characters passed to it as a setter.
This broke a number of use cases (mostly involving the ui-router).

Further analysis appears to show that we can solve angular#16312, to prevent
urls being rewritten with decoded values, without modifying the
behaviour of `$location.url`.

This commit reverts changes to `$location.url(value)` so that encoded
chars will once again be decoded and passed to `$location.path(value)`.
In particular it will convert encoded forward slashes, which changes how
the path is updated, since e.g. `a/b/%2Fc%2Fd` will become `a/b/c/d`.
While this is arguably not "correct", it appears that there are too many
use cases relying upon this behaviour.
petebacondarwin added a commit that referenced this issue Dec 11, 2017
The original fix for #16312 included changing how `$location.url(value)`
decoded the special characters passed to it as a setter.
This broke a number of use cases (mostly involving the ui-router).

Further analysis appears to show that we can solve #16312, to prevent
urls being rewritten with decoded values, without modifying the
behaviour of `$location.url`.

This commit reverts changes to `$location.url(value)` so that encoded
chars will once again be decoded and passed to `$location.path(value)`.
In particular it will convert encoded forward slashes, which changes how
the path is updated, since e.g. `a/b/%2Fc%2Fd` will become `a/b/c/d`.
While this is arguably not "correct", it appears that there are too many
use cases relying upon this behaviour.
petebacondarwin added a commit that referenced this issue Dec 11, 2017
The original fix for #16312 included changing how `$location.url(value)`
decoded the special characters passed to it as a setter.
This broke a number of use cases (mostly involving the ui-router).

Further analysis appears to show that we can solve #16312, to prevent
urls being rewritten with decoded values, without modifying the
behaviour of `$location.url`.

This commit reverts changes to `$location.url(value)` so that encoded
chars will once again be decoded and passed to `$location.path(value)`.
In particular it will convert encoded forward slashes, which changes how
the path is updated, since e.g. `a/b/%2Fc%2Fd` will become `a/b/c/d`.
While this is arguably not "correct", it appears that there are too many
use cases relying upon this behaviour.
@phrough
Copy link

phrough commented Jul 10, 2018

I am definitely seeing issues related to this with AngularJS 1.7.2 and legacy ui-router 0.4.3. In html5 mode, trying to access a url directly gets rerouted to my fallback state because the the slashes are converted to %2F. Rolling back this change results in expected behavior. The deep-linked state loads as expected.

EDIT: In my particular case, the problem observed was related to the change in default hashPrefix. Setting it to its previous value of '' in config resolved the issue.
$locationProvider.hashPrefix('');

@binki
Copy link

binki commented Aug 31, 2018

I personally don’t think that / should be treated as such a special case here.

RFC3986§6.2.2.2 recommends that percent normalization only the normalize unreserved characters listed in RFC3986§2.3. These unreserved characters are -, ., _, ~, ALPHA (/[a-zA-Z]/), and DIGIT (/[0-9]/).

RFC3986§3.3 permits the entirety of the path to consist of /, percent-encoded characters, unreserved characters, sub-delims (!, $, &, ', (, ), *, +, ,, ;, and =), :, and @.

This means that the specification defines three categories of characters used within a path:

  1. Characters which must be considered equivalent regardless of whether or not they are percent encoded. This is the unreserved characters. For example, /%7e%61 and /~a should be considered equivalent URIs. This is important because older implementations may consider ~ to be a reserved character and send %7e which web servers must handle.
  2. Characters which must be percent encoded always. For example, non-latin alphabetic characters.
  3. Characters which may be present in percent encoded form or verbatim but which may not be normalized during normal URI processing. This includes the remaining characters such as (, !, /, and @. The fact that RFC3986§6.2.2.2 does not say that these may be safely normalized means that the difference between percent-encoded and unencoded variants of these characters may be considered significant by the consumer of the URI.

This last category may be further broken down for JavaScript programmers in terms of what encodeURIComponent() will escape which is everything but !, ', (, ), and *:

encodeURIComponent('/!$^\'()*+,;=:@')
"%2F!%24%5E'()*%2B%2C%3B%3D%3A%40"

And, additionally, I’m sure that the HTML spec also further defines/alters how the URI should be handled.

This patch/PR adds / as a special case targeting ui-router specifically. Instead, it should just fix AngularJS to follow the normalization guidelines which is to only normalize the characters required by RFC3986§2.3.

For example, a function like this could be used instead of decodePath():

/**
 * This takes any percent-encoded character which is listed in
 * RFC3986§2.3 (https://tools.ietf.org/html/rfc3986#section-2.3)
 * and normalizes them to their decoded form. The below is the
 * list of characters:
 *
 * unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
 *
 * "-" = %2d
 *
 * "." = %2e
 *
 * "_" = %5f
 *
 * ~ = %7e
 *
 * ALPHA = %41–%5A
 *
 * DIGIT = %30–%39
 *
 * RFC3986§6.2.2.2
 * (https://tools.ietf.org/html/rfc3986#section-6.2.2.2) states
 * that the above characters should be considered equivalent when
 * both encoded and not. Thus, the application is not allowed to
 * treat their escaped and unescaped versions differently. For
 * example, none of those characters may be used as path
 * separators because URI implementations are allowed to normalize
 * them away.
 *
 * To avoid accidentally relying on these characters as path
 * separators, normalize them away.
 */
function normalizePathPercentEncoding(s) {
  // For incoming URIs, we expect all of these characters to already
  // be decoded. This replace should be basically free when the path
  // is already percent encoding normalized.
  return s.replace(/%(?:2d|2e|5f|7e|4[1-9a-f]|5[0-9a]|3[0-9])/gi, decodeURIComponent);
}

It is not the place of AngularJS to decode the entire path. Whatever is handling the path and giving significance to segments—e.g., ui-router which considers the / character to be significant—should be the thing which handles calling decodeURIComponent() on individual components as necessary when extracting the information. A different service consuming $location might decide to use a different character which encodeURIComponent() would percent-encode as a significant character such as & or ; or ?.

@binki
Copy link

binki commented Aug 31, 2018

Additionally, I see that this is only applied for html5Mode. I actually came here because I was having this same exact problem except in hashbang mode.

@binki
Copy link

binki commented Aug 31, 2018

Here’s a demo of this same problem existing in 1.7.3 in hashPrefix() mode: http://embed.plnkr.co/Z3Lh5F2b07uAEVgBpyAd/ . Just type in anything with a slash in it and note that it is interpreted as multiple arguments instead of one even though the link is properly encoded.

@gkalpak
Copy link
Member

gkalpak commented Oct 11, 2018

Thx for the thorough write-up, @binki. You are probably correct.

Unfortunately, changing the behavior of $location is a tricky thing. $location is a very critical part of every AngularJS application and existing code (including popular libraries, such ui-router) are relying on its current (imperfect) behavior. There have been attempts to rectify similar issues in the past, but ended up being reverted, because they apparently broke significantly more usecases than they fixed.

Given that AngularJS has now entered an LTS period, such breaking changes are out of scope.

If someone desperately needs this different (more correct) behavior, it is not too difficult to overwrite $location and provide an implementation that matches your requirements. This is even easier than fixing the existing implementation, because one only has to account for their app's needs, not every possible cornercase out there.

@binki
Copy link

binki commented Oct 12, 2018

@gkalpak Thanks for the response and advice. I understand the LTS period issue and compat issues. I was surprised that some of the changes discussed in this thread actually appeared to go out in minor releases during the LTS period :-p.

I was pulled away from working on the project where I was having trouble with $location before I could solve that issue. I think when I get back to it I will try my hand at reimplementing $location. When I was looking into that earlier, it seemed unfortunate that I would need many parts of AngularJS’s core implementation but the module system prevents me from reusing those parts, forcing me to either reimplement everything “for real” or copy most of the core code into my project. I guess I’ll just have to be pragmatic about it, accept that things are the way they are, and start copy-pasting—when I get around to it ;-).

@prakharjaiswal
Copy link

We in our application have embedded callback urls in path itself and hence decodePath in non html mode (hash bang) has been causing the trouble.
Tried patching the $$parse and $$compose method using decorator pattern. But, the caveat with this approach is, that the decorators are invoked late and in scenarios where a page (with callback embedded) is refreshed, native implementations of $$parse and $$compose have already been invoked. Hence, the route mismatch.

Any reason why in native implementation; decodePath re-encodes the forward slashes to %2F only for htmlMode and not for hashbang mode?

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