fix(ngResource): fix query url params encoding #12201

alediaferia · 2015-06-24T09:19:30Z

Encoding URL Query Parameters should be "more aggressive" than the URL Segment encoding.
You can see what I mean in the added test case.

I'm currently using AngularJS 1.3.16 but I found this bug is still there in master.

googlebot · 2015-06-24T09:19:32Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

alediaferia · 2015-06-24T09:21:11Z

I signed it!

googlebot · 2015-06-24T09:21:14Z

CLAs look good, thanks!

nacmacfeegle · 2015-06-25T10:05:31Z

+1 to this fix, thanks @alediaferia

gkalpak · 2015-06-27T09:17:03Z

src/ngResource/resource.js

@@ -431,16 +431,20 @@ angular.module('ngResource', ['ng']).
            }
            if (!(new RegExp("^\\d+$").test(param)) && param &&
              (new RegExp("(^|[^\\\\]):" + param + "(\\W|$)").test(url))) {
-              urlParams[param] = true;
+              urlParams[param] = { isQueryParam: (new RegExp("\\?(.*)\\:" + param)).test(url) };


You need the '(\\W|$)' part or else you might get false positives. (Better yet '(?:\\W|$)'.)
E.g. param: 'foo' and url: '/path/foo?key=:foobar' will lead to a false positive.

I agree my regex is too broad but I'm not sure I completely get the one proposed by you.
Would something like the following be more accurate?
new RegExp("\\?((\\w+)\\=\\w+&)*\\w+=:" + param + "(\\W|$)").test(url)

No need to make it too strict either :)

IMO, new RegExp('\\?(.*):' + param + '(\\W|$)') (or new RegExp('\\?(?:.*):' + param + '(?:\\W|$)')) is fine.

So what does ?: match exactly? This is the only bit I don't understand. :)

Ooh, it's the non-capturing group! :)

alediaferia · 2015-06-29T14:23:31Z

I've updated the code accordingly and split my commit messages to better comply with contribution guidelines.

gkalpak · 2015-06-29T14:51:24Z

src/ngResource/resource.js

@@ -431,16 +431,20 @@ angular.module('ngResource', ['ng']).
            }
            if (!(new RegExp("^\\d+$").test(param)) && param &&
              (new RegExp("(^|[^\\\\]):" + param + "(\\W|$)").test(url))) {
-              urlParams[param] = true;
+              urlParams[param] = { isQueryParam: (new RegExp("\\?(?:.*)\\:" + param + "(?:\\W|$)")).test(url) };


Nitpick: You don't need \\ before : (it's not a special character in this context).

gkalpak · 2015-06-29T16:17:19Z

LGTM

davidesantangelo · 2015-06-30T08:19:26Z

LGTM

alediaferia · 2015-07-02T10:16:24Z

Is there a plan for when this will go in the main line?

alediaferia · 2015-07-06T21:34:30Z

Bump!

gkalpak · 2015-09-14T09:40:11Z

test/ngResource/resourceSpec.js

@@ -338,6 +338,17 @@ describe("resource", function() {
  });


+  it('should handle url encoding in mapped params', function() {


I'd prefer a more descriptive testcase name (e.g. should encode & in query params unless in query param value or something).

gkalpak · 2015-09-14T09:47:18Z

@alediaferia, thx for the update. I left a couple of comments (mostly minor).
Feel free to ping me once you've pushed changes, so I can take another look.

alediaferia · 2015-09-14T14:14:11Z

@gkalpak BTW do you have any hint on why now my grunt package fails with stderr maxBuffer exceeded ?

gkalpak · 2015-09-14T14:47:54Z

@alediaferia, no idea :) Does it happen with the code currently on the PR ?
(If not, you can push the code somewhere so I can take a look.)

petebacondarwin · 2015-09-14T16:30:15Z

Sounds like something to do with the closure compiler?

alediaferia · 2015-09-15T10:46:10Z

@gkalpak @petebacondarwin sorry for not getting back earlier. It's happening with the current PR but it's happening with updated master as well. It may be something wrong with my npm/node configuration, but not sure. I'm not much into JavaScript :)

gkalpak · 2015-09-16T09:00:47Z

@alediaferia, (un)fortunately, I'm not getting any issues with grunt package 😞

alediaferia · 2015-09-16T11:21:09Z

test/ngResource/resourceSpec.js

@@ -338,6 +338,17 @@ describe("resource", function() {
  });


+  it('should encode & in query params unless in query param value', function() {


I'm wondering if part of this test is a duplicate of the one preceding this test.

'should encode & in url params'

I would say close but not quite.
I would be happy to have them merged into one it block (keeping all 3 cases).

Feel free to merge them.

alediaferia · 2015-09-16T11:21:59Z

@gkalpak ping

gkalpak · 2015-09-16T12:07:52Z

scripts/npm/install-dependencies.sh

@@ -10,7 +10,7 @@ if diff -q $SHRINKWRAP_FILE $SHRINKWRAP_CACHED_FILE; then
 else
  echo 'Blowing away node_modules and reinstalling npm dependencies...'
  rm -rf node_modules
-  npm install
+  npm install --registry http://registry.npmjs.org/


I guess this has to do with your grunt package issues, but it definitely does not belong in this PR :)

Woops! sorry :)

Similar e2e tests assessing URL params encoding have been merged Dropped unwanted changes on install-dependencies.sh script

gkalpak · 2015-09-17T07:35:51Z

LGTM, thx @alediaferia !

/cc @petebacondarwin

alediaferia · 2015-09-17T08:03:08Z

If this is ok, shall I clean up the git history squashing the latest commits and following your commit message conventions?

petebacondarwin · 2015-09-17T09:17:21Z

OK, LGTM. @alediaferia thanks for your work on this. If you want to squash and tidy up the commit message that would be great. Otherwise, perhaps @gkalpak could do it when merging?

gkalpak · 2015-09-17T09:27:21Z

Sure, I'll take care of squashing and merging !

alediaferia · 2015-09-17T10:00:22Z

Thank you @gkalpak @petebacondarwin

gkalpak · 2015-09-18T13:56:03Z

@petebacondarwin, should this be backported to 1.4.x ?

petebacondarwin · 2015-09-18T14:05:40Z

If you are confident it is not a breaking change.

gkalpak · 2015-09-18T14:27:18Z

It's one of those times that I find it hard to distinguish between a fix and a BC.

The only change introduced is when a param (:xyz) is used as the value part of a URL query key-value pair. In any other case, there is no difference. But it could still break an app (theoretically).

E.g., assuming $resource('/path?key=:val').get({val: 'foo&bar'}), the request is sent to this URL:

1.4.x: /path?key=foo&bar
1.5.x: /path?key=foo%26bar

alediaferia · 2015-09-18T14:36:15Z

Although I feel the "new" behaviour is the right one I unfortunately think there may be a lot of apps out there relying on a workaround for this "issue".

If you look for e.g. "angularjs ngresource query parameters" on Google there are some issues open on SO with a few workarounds that usually involve using $http directly.

Just my 2 cents.

Happy to have contributed to such a big project anyway :)

petebacondarwin · 2015-09-18T14:57:36Z

OK, so no backport.

gkalpak · 2015-09-18T14:58:04Z

@alediaferia, I couldn't find any SO questions/answers that would be broken by this "fix". Relying on a workaround isn't a problem (it won't get broken).
Do you have any specific SO question/answer in mind ?

The problem is if someone has as a URL template such as /path?key=:val and expects to set other query param via :val (e.g. /path?key=1&key2=2&key3=3). It is a weird usecase imo (but theoretically possible nontheless).

alediaferia · 2015-09-18T15:14:41Z

@gkalpak I cannot manage to find the question I had in mind, strange. Anyway, as you point out, relying on a workaround won't make things break. That's true. I don't have further objections.

googlebot added the cla: no label Jun 24, 2015

googlebot added cla: yes and removed cla: no labels Jun 24, 2015

gkalpak reviewed Jun 27, 2015
View reviewed changes

alediaferia force-pushed the hotfix/query-url-encoding branch from fbd119c to dc1a67c Compare June 29, 2015 14:19

gkalpak reviewed Jun 29, 2015
View reviewed changes

alediaferia force-pushed the hotfix/query-url-encoding branch from dc1a67c to cd4a591 Compare June 29, 2015 15:00

alediaferia changed the title ~~Do not encode URL query parameters the same way as URL path components~~ fix(ngResource): fix query url params encoding Jun 30, 2015

alediaferia changed the title ~~fix(ngResource): fix query url params encoding~~ fix(ngResource): fix query url params encoding Jun 30, 2015

gkalpak added this to the 1.4.3 milestone Jul 7, 2015

gkalpak added type: bug component: ngResource needs: merging severity: broken expected use labels Jul 7, 2015

petebacondarwin modified the milestones: 1.4.3, 1.4.4 Jul 16, 2015

Narretz modified the milestones: 1.4.4, 1.4.5 Aug 14, 2015

gkalpak reviewed Sep 14, 2015
View reviewed changes

Minor updates according to PR review suggestions

87b8cc7

alediaferia reviewed Sep 16, 2015
View reviewed changes

gkalpak reviewed Sep 16, 2015
View reviewed changes

Unify similar e2e tests

1ac0e7e

Similar e2e tests assessing URL params encoding have been merged Dropped unwanted changes on install-dependencies.sh script

petebacondarwin modified the milestones: 1.4.6, 1.4.7 Sep 17, 2015

gkalpak closed this in 1c97a60 Sep 18, 2015

		@@ -338,6 +338,17 @@ describe("resource", function() {
		});


		it('should handle url encoding in mapped params', function() {

		@@ -338,6 +338,17 @@ describe("resource", function() {
		});


		it('should encode & in query params unless in query param value', function() {

fix(ngResource): fix query url params encoding #12201

fix(ngResource): fix query url params encoding #12201

Conversation

alediaferia commented Jun 24, 2015

googlebot commented Jun 24, 2015

alediaferia commented Jun 24, 2015

googlebot commented Jun 24, 2015

nacmacfeegle commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alediaferia commented Jun 29, 2015

Choose a reason for hiding this comment

gkalpak commented Jun 29, 2015

davidesantangelo commented Jun 30, 2015

alediaferia commented Jul 2, 2015

alediaferia commented Jul 6, 2015

Choose a reason for hiding this comment

gkalpak commented Sep 14, 2015

alediaferia commented Sep 14, 2015

gkalpak commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

alediaferia commented Sep 15, 2015

gkalpak commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alediaferia commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Sep 17, 2015

alediaferia commented Sep 17, 2015

petebacondarwin commented Sep 17, 2015

gkalpak commented Sep 17, 2015

alediaferia commented Sep 17, 2015

gkalpak commented Sep 18, 2015

petebacondarwin commented Sep 18, 2015

gkalpak commented Sep 18, 2015

alediaferia commented Sep 18, 2015

petebacondarwin commented Sep 18, 2015

gkalpak commented Sep 18, 2015

alediaferia commented Sep 18, 2015