angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.82k stars 27.51k forks source link

+ (Plus) in URL parameter converted to %2B #3042

Closed nlwillia closed 10 years ago

nlwillia commented 11 years ago

The + character is valid in url query strings as a substitute for space or its hex-coded equivalent, %20.

When Angular bootstraps, $LocationProvider.$get initializes $location. $location.$$parse uses parseKeyValue (from inside matchAppURL) to decompose the query string into parameters. It calls the built-in decodeURIComponent which replaces hex escape sequences, but it does not replace "+" with space.

As a consequence, the plus character remains for $location.$$compile to encode as %2B which changes the unescaped, actual value of the parameter (as returned from $location.search().paramName) from "a b" to "a+b". This is incorrect.

The parseKeyValue function should be changed to scan the decoded value for '+' and replace it with space. This function is also used for manipulation of $location.search(), and in that scenario, '+' replacement is not correct, so the change would need to be parameterized so that '+' is replaced when parsing a parameter from the browser and left alone when parsing a parameter from the application.

Verified in angular 1.0.7 and 1.1.5.

czerwingithub commented 11 years ago

We ran into this problem as well. For now, I put a hack in place to get around this by overriding $browser.url() to change all '+' signs in the query string to '%20'. Otherwise, as the original bug poster noted, we incorrectly get plus signs returned in the values for our query string parameters.

At the very least, there should be a fix to respect the plus signs in the original URLs, before they are rewritten by Angular. We cannot prevent users of our API from using plus signs to encode spaces when they are constructing URLs to send to us.

BrainBacon commented 11 years ago

@czerwingithub we are having a similar issue. How did you override $browser.url()?

czerwingithub commented 11 years ago

I just used a decorator on the $browser service and overrode the url method. It does depend a little on the implementation details of $browser, but I know this works for 1.1.5, and for the limited use case we have. Luckily, the watch functions for the $location service invoke $browser.url(), so search param lookups through $location work. Just a note though, the mockBrowser service is implemented slightly differently, so this fix won't work for it.. which means you can't end up testing this fix in unit tests.. just real production usage.

Here's the sample code. I don't know if this exact code compiles/works since I had to cut and paste it from several different files to give you one coherent view of it.. but it should be enough to at least give you the idea.

Oh, to use it, you just have to be sure to load the '$browserFixer' module in your app.

angular.module('$browserFixer', []) .config(['$provide', function($provide) { // We use a decorator to agument the behavior of the default $browser // service. This is guaranteed to be called before anyone uses // $browser which is great. $provide.decorator('$browser', [$delegate', function ($browser) { // Copy the old url method to superUrl. $browser.superUrl = $browser.url;

// Our new url method which does the conversion.
$browser.url = function fixedUrl() {
  // We do not do anything when used as a setter.  In this
  // case, the returned value is the $browser object itself.
  if (arguments.length > 0)
    return this.superUrl.apply(this, arguments);
  // Just convert on the way out.  We do not convert the value
  // before we invoke url() as a setter since that could cause
  // other problems.
  return convertQueryPlusSigns(this.superUrl());
};

return $browser;

function convertQueryPlusSigns(originalUrl) {
// Grab the query portion of the URL and do the rewrite.
var questionMark = originalUrl.indexOf('?');

if (questionMark < 0)
  return originalUrl;

// Split the string up into three components.  Base
// which is everything up to the first questionMark,
// query which is from questionMark to the pound sign,
// and then tail which is pound sign to the end.
var base = originalUrl.substring(0, questionMark);
var query = originalUrl.substring(questionMark);

if (query.indexOf('+') < 0)
  return originalUrl;

var tail = '';
var pound = originalUrl.indexOf('#');
if (pound > questionMark) {
  query = originalUrl.substring(questionMark, pound);
  tail = originalUrl.substring(pound);
}

query = query.replace(/\+/g,"%20");
return base + query + tail;
}

}]); }]);

BrainBacon commented 11 years ago

Thanks @czerwingithub I modified the fix a bit and made it vastly simpler. Seems to work for my purposes since I don't anticipate a + anywhere else besides the search section.

    $provide.decorator('$browser', function($delegate) {
        var superUrl = $delegate.url;
        $delegate.url = function(url, replace) {
            if(url !== undefined) {
                return superUrl(url.replace(/\%20/g,"+"), replace);
            } else {
                return superUrl().replace(/\+/g,"%20");
            }
        }
        return $delegate;
    });
czerwingithub commented 11 years ago

Glad it helped. Yup, that works as well. This is part of our generic URL handling libraries, so we try to follow spec as much as possible.

On Thu, Aug 8, 2013 at 3:34 PM, Brian Jesse notifications@github.comwrote:

Thanks @czerwingithub https://github.com/czerwingithub I modified the fix a bit and made it vastly simpler. Seems to work for my purposes since I don't anticipate a + anywhere else besides the search section.

$provide.decorator('$browser', function($delegate) {
    var superUrl = $delegate.url;
    $delegate.url = function(url, replace) {
        if(url !== undefined) {
            return superUrl(url.replace(/\%20/g,"+"), replace);
        } else {
            return superUrl().replace(/\+/g,"%20");
        }
    }
    return $delegate;
});

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/3042#issuecomment-22363310 .

jrabbe commented 10 years ago

Just FYI, you may need to use the array notation to prevent unknown provider exception after minification. That is:

    $provide.decorator('$browser', ['$delegate', function($delegate) {
        var superUrl = $delegate.url;
        $delegate.url = function(url, replace) {
            if(url !== undefined) {
                return superUrl(url.replace(/\%20/g,"+"), replace);
            } else {
                return superUrl().replace(/\+/g,"%20");
            }
        }
        return $delegate;
    }]);
mjj1409 commented 10 years ago

Has anyone noticed that $routeChangeStart/$routeChangeSuccess events are fired twice in these situations, one time for the original query string containing the '+' and then again when it is converted to '%2B'? The net effect is that the associated controllers are also instantiated twice. This is happening for me in 1.2.10.

Furizaa commented 10 years ago

In addition to @BrainBacon fix I also use the following snippet to fix links that have an "+" encoded whitespace in the href:

  $provide.decorator('$location', function($delegate) {
    var superRewrite = $delegate.$$rewrite;
    $delegate.$$rewrite = function(url) {
      return superRewrite(url.replace(/\+/g, '%20'));
    };
    return $delegate;
  });

Don't forget to use the array syntax if you need minification.

bpossolo commented 10 years ago

any idea why this issue hasn't been addressed in 11 months? seems like it affects everyone and is an easy fix

gkop commented 10 years ago

+1. For what it's worth I am getting around this by using commas instead of pluses.

arkarkark commented 10 years ago

also +1

honzajde commented 10 years ago

+1

altmind commented 10 years ago

+1

kirka121 commented 10 years ago

are you typing +1's just to increase your comment count?

altmind commented 10 years ago

its ok as long as github dont have any other means to vote vor an issue. google code had stars, jira had votes, github has nothing for that.

arkarkark commented 10 years ago

@jrabbe I really liked your workaround, thank you.

My karma tests failed with it (as forewarned by @czerwingithub) but this small change made it work both in prod and my tests.

superUrl = angular.bind($delegate, $delegate.url)

I'd still really like to see valid url parameters supported by angular though.

IgorMinar commented 10 years ago

@caguillen214 can you take a look at this one please?

Write a test for both initializing $location and setting $location.search with a search query containing + characters.

The fix will most likely be to just replace all + with %20 in the parseKeyValue fn

caguillen214 commented 10 years ago

@IgorMinar will do

caitp commented 10 years ago

@caguillen214 I could take this one if you don't have time for it

LorDOniX commented 10 years ago

Hi, I need help with these two cases:

If I use url parameter, for example "search", which will contain a text from a user. User will be searching "one + one" and then in the url there will be "?search=one%20%20%20one", which will be decoded as "one one".

I think this change breaks many peoples codes, because in this case plus sign should be encoded as %2B. Also, what about SEO ? What If I use plus sign to distinguish one unique URL from another ?

Thanks for reply.

firlaj commented 10 years ago

Hello, This fix causes me a problem. In my app I have a value in search param, which can contain a plus sign, for example "flat-size=2+1". This is a common way how to describe flat size in my country and it is important for SEO that the value is exactly like this. The fix changes the parameter to "flat-size=2 1", which is nonsense (and broke my app :-( ). Also, as @LorDOniX mentioned above, it can change values from search text inputs, which does not seem right.

In the opening post to this issue, @nlwillia writes:

"The parseKeyValue function should be changed to scan the decoded value for '+' and replace it with space. This function is also used for manipulation of $location.search(), and in that scenario, '+' replacement is not correct, so the change would need to be parameterized so that '+' is replaced when parsing a parameter from the browser and left alone when parsing a parameter from the application."

The fix does not do anything like that. Maybe it should?

nlwillia commented 10 years ago

Take a look at this demo. So far as I can tell, this is now being handled correctly. If your application assigns a value containing "+" via the $location.search API, then angular encodes it with %2B in the URL and preserves the literal plus character for the application. But "+" in a URL whether manually in the browser's address bar or from an internal link gets interpreted the same as %20 (space) at the API level, and that's how it should be. I verified this in both hashbang and HTML5 modes.

@LorDOniX If a search for "one + one" is being encoded in the URL as "one%20%20%20one", then there is something wrong with your encoding process. An entered "+" character should encode as %2B because "+" is a reserved character in URL parameters.

bpossolo commented 10 years ago

The behaviour should be configurable either on the locationProvider or as an argument to $location.search({}). Some people want $location.search( { "fl": "foo bar' } ) to become /path?fl=foo+bar others want /path?fl=foo%20bar

LorDOniX commented 10 years ago

Hi, yes $location.search working well, but problem is with $location.url. Url:

/en/search/for-sale/apartments/praha-9?disposition=1+kt

is encoded to:

/en/search/for-sale/apartments/praha-9?disposition=1%20kt

Please take a look at "disposition" parametr, which is encoded as a space, not a plus sign.

nlwillia commented 10 years ago

@LorDOniX I get where you're coming from, but the use of %20 vs. + in an encoded URL is not really a bug. From a browser or robot perspective, the two encodings are interchangeable. Or put differently this may be a change, but it isn't really a regression...at least not the sort of thing that would require that this issue be reopened. This is just my opinion as a fellow angular user, but I'd suggest that you'll get more traction by logging a new issue asking for an enhancement to $locationProvider allowing you to configure which encoding style is used.

micmmakarov commented 9 years ago

Solution that was the best for me:

  $provide.decorator "$browser", [
    "$delegate"
    ($delegate) ->
      superUrl = $delegate.url
      $delegate.url = (url, replace) ->
        superUrl().replace /\+/g, "%2B" if url is 'undefined'
      $delegate
  ]

Just replaces +es to %2B which fix everything and makes them as "+" in the url

dinofx commented 8 years ago

This bug is invalid. A '+' in a URL always means '+'. It's a valid character in the path and query. Space is always encoded as %20.

The reporter has made the common mistake of confusing the encoding used for "application/x-www-form-urlencoded" content in the BODY of an HTTP request, with the encoding used for URLs.

jrabbe commented 8 years ago

From W3:

Within the query string, the plus sign is reserved as shorthand notation for a space. Therefore, real plus signs must be encoded. This method was used to make query URIs easier to pass in systems which did not allow spaces.

So while you are correct that + is a valid character in path and query, the fact remains that many of us have to interact with legacy systems and support the world as it looks, not like we want it to look.

dinofx commented 8 years ago

That link is far from being a meaningful specification and just shows another confused developer who probably doesn't know the difference between a URL and URI. The fact that it says some systems allow spaces in URIs should say something about its accuracy.

Web browsers use URLs. Try pasting this into your web browser: github.com/angular/angular.js/issues/3042?encode=this query

Every browser in the world will automatically convert the SPACE to %20.

The URL specification has been around for over 20 years (RFC1738). They all agree that the only way to encode characters is using %.

nlwillia commented 8 years ago

Probably better to have this discussion under #13815.

jrabbe commented 8 years ago

@dinofx it may not be a meaningful specification, but I'm sure you're aware of what W3 is, and the about this document page specifically says:

This document defines the syntax used by the World-Wide Web initiative to encode the names and addresses of objects on the Internet.

That said, the problem seems to be not so much the decoding, but that $location.$$compile encodes a plus sign as %2B which is perfectly valid, but shouldn't happen since + is a valid character in a URI and doesn't need to be percent encoded.

bpossolo commented 8 years ago

@jrabbe is correct here. the plus symbol is perfectly valid in the query portion of the string as an alternative to %20

vijaybabu4589 commented 3 years ago

use new HttpUrlEncodingCodec().encodeValue("+91")