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

fix: parseKeyValue function does not allow additional equals signs #12351

Closed
wants to merge 1 commit into from
Closed

fix: parseKeyValue function does not allow additional equals signs #12351

wants to merge 1 commit into from

Conversation

jniebuhr
Copy link

In some cases people will not follow all URL standards and may have unescaped = characters in their GET parameter values. Currently $location will not parse them correctly (or rather too correctly) and in combination with the routing will make a pushState that removes them from the URL.
My proposed fix will just join everything after the key back together with the '=' character it is split by before.

In some cases people will not follow all URL standards and may have unescaped = characters in their GET parameter values. Currently $location will not parse them correctly (or rather too correctly) and in combination with the routing will make a pushState that removes them from the URL.
My proposed fix will just join everything after the key back together with the '=' character it is split by before.
@jniebuhr
Copy link
Author

Well I guess the build failing wasn't rly my fault.

@petebacondarwin
Copy link
Member

I am a bit torn about this.

On the surface don't really like this idea since we are undermining the standard.

But then again I see that accepting = chars after the first char is not going to break valid fully encoded URL query strings.

While I can see that it would make life much easier in the cases where = characters were inadvertently un-encoded, it is a special case. What happens if there is an unencoded & in the value?

@jniebuhr
Copy link
Author

If there is an unencoded & in the string, something is rly wrong and that can't be solved. The case I needed this for are base64 encoded strings.

@petebacondarwin petebacondarwin modified the milestones: Backlog, Purgatory Jul 15, 2015
@petebacondarwin petebacondarwin self-assigned this Jul 15, 2015
@@ -1129,9 +1129,9 @@ function parseKeyValue(/**string*/keyValue) {
forEach((keyValue || "").split('&'), function(keyValue) {
if ( keyValue ) {
key_value = keyValue.replace(/\+/g,'%20').split('=');
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is slightly more expensive, but you can save some of the weirdness later in the code if by changing .split('=') with .split(/=(.*)/, 2)

Copy link
Member

Choose a reason for hiding this comment

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

I think it will be nicer just to use indexOf:

  var key, value, splitPoint;
  ...
    if ( keyValue ) {
      key = keyValue = keyValue.replace(/\+/g,'%20');

      splitPoint = keyValue.indexOf('=');
      if (splitPoint != -1) {
        key = keyValue.substring(0, splitPoint);
        value = keyValue.substring(splitPoint+1);
      }

      key = tryDecodeURIComponent(key);
      if ( isDefined(key) ) {

        value = tryDecodeURIComponent(value) || true;
        if (!hasOwnProperty.call(obj, key)) {
          obj[key] = val;
        } else if(isArray(obj[key])) {
          obj[key].push(val);
        } else {
          obj[key] = [obj[key],val];
        }
      }
    }

@lgalfaso
Copy link
Contributor

Even when I agree with @petebacondarwin that this does not follow the spec, the behavior in the PR is how most servers handle unencoded =

@petebacondarwin
Copy link
Member

OK, if @lgalfaso is happy then I am happy. I think we can improve the algorithm and then land this.

@jniebuhr
Copy link
Author

Sounds great, will this be available in any 1.2.* version? That's the version my project currently runs on.

@petebacondarwin
Copy link
Member

We are only putting security fixes into 1.2 but I guess since you asked so nicely we could consider backporting this in.

@lgalfaso
Copy link
Contributor

The version I posted or the version from @petebacondarwin works fine with me. @jniebuhr can you please update the PR?

petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Jul 27, 2015
In some cases people will not follow all URL standards and may have
unescaped = characters in their GET parameter values. Currently $location
will not parse them correctly dropping everything after the unescaped =.

This change includes all characters after the first `=` up to the next `&`.

Closes angular#12351
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
In some cases people will not follow all URL standards and may have
unescaped = characters in their GET parameter values. Currently $location
will not parse them correctly dropping everything after the unescaped =.

This change includes all characters after the first `=` up to the next `&`.

Closes angular#12351
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
In some cases people will not follow all URL standards and may have
unescaped = characters in their GET parameter values. Currently $location
will not parse them correctly dropping everything after the unescaped =.

This change includes all characters after the first `=` up to the next `&`.

Closes angular#12351
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants