launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19610
[Merge] lp:~cjwatson/launchpad/testfix-safer-ajax-strings into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/testfix-safer-ajax-strings into lp:launchpad.
Commit message:
Be more careful about JSON-encoding array elements, and fix up tests.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/testfix-safer-ajax-strings/+merge/274237
Be more careful about JSON-encoding array elements: strings that are array elements have the same ambiguity problem as top-level strings, so we need to take care to encode them in the same way. Fix up tests for this and for other failures introduced by safer-ajax-strings.
The pillar sharing view needs to change to take account of this: it previously turned each key/value pair into an unquoted key,value string which lazr.restful handled, but when the keys and values are quoted separately this will no longer work. Just passing the permissions parameter as a JSON-encoded dictionary instead is simpler and safer.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/testfix-safer-ajax-strings into lp:launchpad.
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2015-10-12 16:06:06 +0000
+++ lib/lp/app/javascript/client.js 2015-10-13 11:18:19 +0000
@@ -174,20 +174,27 @@
return config[key];
};
- module.append_qs = function(qs, key, value) {
+ module._encode_param = function(key, value, ws_param) {
+ var v = value;
+ if (Y.Lang.isObject(v) || (ws_param && Y.Lang.isString(v))) {
+ v = Y.JSON.stringify(v);
+ }
+ return encodeURIComponent(key) + "=" + encodeURIComponent(v);
+ };
+
+ module.append_qs = function(qs, key, value, ws_param) {
/* Append a key-value pair to a query string. */
+ if (ws_param === undefined) {
+ ws_param = false;
+ }
var elems = (qs && qs.length > 0) ? [qs] : [];
- var enc = encodeURIComponent;
if (Y.Lang.isArray(value)) {
var index;
for (index = 0; index < value.length; index++) {
- elems.push(enc(key) + "=" + enc(value[index]));
+ elems.push(module._encode_param(key, value[index], ws_param));
}
- } else if (Y.Lang.isObject(value) ||
- (key !== "ws.op" && Y.Lang.isString(value))) {
- elems.push(enc(key) + "=" + enc(Y.JSON.stringify(value)));
} else {
- elems.push(enc(key) + "=" + enc(value));
+ elems.push(module._encode_param(key, value, ws_param));
}
return elems.join("&");
};
@@ -719,7 +726,8 @@
var name;
for (name in parameters) {
if (parameters.hasOwnProperty(name)) {
- data = module.append_qs(data, name, parameters[name]);
+ data = module.append_qs(
+ data, name, parameters[name], true);
}
}
config.data = data;
@@ -736,7 +744,8 @@
data = module.append_qs(data, "ws.op", operation_name);
for (name in parameters) {
if (parameters.hasOwnProperty(name)) {
- data = module.append_qs(data, name, parameters[name]);
+ data = module.append_qs(
+ data, name, parameters[name], true);
}
}
=== modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
--- lib/lp/app/javascript/tests/test_lp_client.js 2015-10-12 16:06:06 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.js 2015-10-13 11:18:19 +0000
@@ -73,7 +73,7 @@
var qs = "";
qs = Y.lp.client.append_qs(qs, "Pöllä", "Perelló");
Assert.areEqual(
- "P%C3%83%C2%B6ll%C3%83%C2%A4=%22Perell%C3%83%C2%B3%22", qs,
+ "P%C3%83%C2%B6ll%C3%83%C2%A4=Perell%C3%83%C2%B3", qs,
'This tests is known to fail in Chrome.');
},
@@ -97,6 +97,26 @@
"foo=%7B%22text%22%3A%22A%5Cn%26%C3%83%C2%A7%22%7D", qs);
},
+ test_append_qs_ws_string: function() {
+ var qs = "";
+ qs = Y.lp.client.append_qs(qs, "Pöllä", "Perelló", true);
+ Assert.areEqual(
+ "P%C3%83%C2%B6ll%C3%83%C2%A4=%22Perell%C3%83%C2%B3%22", qs,
+ 'This tests is known to fail in Chrome.');
+ },
+
+ test_append_qs_ws_with_array: function() {
+ // append_qs(ws_param=true) appends multiple arguments to the
+ // query string when a parameter value is an array. Strings are
+ // quoted.
+ var qs = "";
+ qs = Y.lp.client.append_qs(qs, "foo", ["bar", 1], true);
+ Assert.areEqual("foo=%22bar%22&foo=1", qs);
+ // All values in the array are encoded correctly too.
+ qs = Y.lp.client.append_qs(qs, "a&b", ["a+b"], true);
+ Assert.areEqual("foo=%22bar%22&foo=1&a%26b=%22a%2Bb%22", qs);
+ },
+
test_field_uri: function() {
var get_field_uri = Y.lp.client.get_field_uri;
Assert.areEqual(
=== modified file 'lib/lp/bugs/javascript/bug_picker.js'
--- lib/lp/bugs/javascript/bug_picker.js 2012-09-10 20:52:27 +0000
+++ lib/lp/bugs/javascript/bug_picker.js 2015-10-13 11:18:19 +0000
@@ -134,10 +134,10 @@
var qs_data
= Y.lp.client.append_qs("", "ws.accept", "application.json");
qs_data = Y.lp.client.append_qs(qs_data, "ws.op", "getBugData");
- qs_data = Y.lp.client.append_qs(qs_data, "bug_id", bug_id);
+ qs_data = Y.lp.client.append_qs(qs_data, "bug_id", bug_id, true);
if (Y.Lang.isValue(LP.cache.bug)) {
qs_data = Y.lp.client.append_qs(
- qs_data, "related_bug", LP.cache.bug.self_link);
+ qs_data, "related_bug", LP.cache.bug.self_link, true);
}
var that = this;
var config = {
=== modified file 'lib/lp/bugs/javascript/tests/test_bug_picker.js'
--- lib/lp/bugs/javascript/tests/test_bug_picker.js 2013-03-20 03:41:40 +0000
+++ lib/lp/bugs/javascript/tests/test_bug_picker.js 2015-10-13 11:18:19 +0000
@@ -20,13 +20,13 @@
this.mockio.last_request.url);
var expected_data =
'ws.accept=application.json&ws.op=getBugData&' +
- 'bug_id=' + bug_id;
+ 'bug_id=%22' + bug_id + '%22';
if (Y.Lang.isValue(LP.cache.bug)) {
var bug_uri = encodeURIComponent('api/devel/bugs/1');
- expected_data += '&related_bug=' + bug_uri;
+ expected_data += '&related_bug=%22' + bug_uri + '%22';
}
Y.Assert.areEqual(
- this.mockio.last_request.config.data, expected_data);
+ expected_data, this.mockio.last_request.config.data);
} else {
Y.Assert.areEqual(
'/api/devel/bugs/1', this.mockio.last_request.url);
=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 2012-10-26 10:00:20 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 2015-10-13 11:18:19 +0000
@@ -115,8 +115,9 @@
reviewer_uri = encodeURIComponent(
Y.lp.client.get_absolute_uri(reviewer_uri));
Y.Assert.areEqual(
- 'ws.op=getBranchVisibilityInfo&person=' +
- reviewer_uri + '&branch_names=b2&branch_names=b1',
+ 'ws.op=getBranchVisibilityInfo&person=%22' +
+ reviewer_uri + '%22&' +
+ 'branch_names=%22b2%22&branch_names=%22b1%22',
this.mockio.last_request.config.data);
Y.Assert.isTrue(confirm_reviewer_called);
},
=== modified file 'lib/lp/code/javascript/tests/test_bugspeclinks.js'
--- lib/lp/code/javascript/tests/test_bugspeclinks.js 2013-03-20 03:41:40 +0000
+++ lib/lp/code/javascript/tests/test_bugspeclinks.js 2015-10-13 11:18:19 +0000
@@ -140,7 +140,7 @@
var bug_uri = encodeURIComponent(
'file:///api/devel/bugs/' + bug_id);
var expected_data =
- 'ws.op=linkBug&bug=' + bug_uri;
+ 'ws.op=linkBug&bug=%22' + bug_uri + '%22';
Y.Assert.areEqual(
expected_data, this.mockio.last_request.config.data);
},
=== modified file 'lib/lp/registry/javascript/sharing/pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/pillarsharingview.js 2012-12-18 16:41:18 +0000
+++ lib/lp/registry/javascript/sharing/pillarsharingview.js 2015-10-13 11:18:19 +0000
@@ -545,11 +545,10 @@
this.get('information_types_by_value');
var sharing_permissions_by_value =
this.get('sharing_permissions_by_value');
- var permission_params = [];
+ var permission_params = {};
Y.each(permissions, function(permission, info_type) {
- permission_params.push(
- [information_types_by_value[info_type],
- sharing_permissions_by_value[permission]]);
+ permission_params[information_types_by_value[info_type]] =
+ sharing_permissions_by_value[permission];
});
var self = this;
var y_config = {
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js'
--- lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2013-03-20 03:41:40 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.js 2015-10-13 11:18:19 +0000
@@ -264,8 +264,8 @@
'/api/devel/+services/sharing',
this.mockio.last_request.url);
Y.Assert.areEqual(
- 'ws.op=deletePillarGrantee&pillar=~pillar' +
- '&grantee=~fred',
+ 'ws.op=deletePillarGrantee&pillar=%22~pillar%22' +
+ '&grantee=%22~fred%22',
this.mockio.last_request.config.data);
this.mockio.last_request.successJSON(['Invisible']);
Y.Assert.isTrue(remove_grantee_success_called);
@@ -359,21 +359,24 @@
this.mockio.last_request.url);
var person_uri = Y.lp.client.normalize_uri('~/joe');
person_uri = Y.lp.client.get_absolute_uri(person_uri);
- var expected_url;
- expected_url = Y.lp.client.append_qs(
- expected_url, 'ws.op', 'sharePillarInformation');
- expected_url = Y.lp.client.append_qs(
- expected_url, 'pillar', '~pillar');
- expected_url = Y.lp.client.append_qs(
- expected_url, 'grantee', person_uri);
- expected_url = Y.lp.client.append_qs(
- expected_url, 'permissions', 'Policy 1,Nothing');
- expected_url = Y.lp.client.append_qs(
- expected_url, 'permissions', 'Policy 2,All');
- expected_url = Y.lp.client.append_qs(
- expected_url, 'permissions', 'Policy 3,Nothing');
- Y.Assert.areEqual(
- expected_url, this.mockio.last_request.config.data);
+ var expected_qs = {
+ 'ws.op': 'sharePillarInformation',
+ 'pillar': '"~pillar"',
+ 'grantee': '"' + person_uri + '"'
+ };
+ var expected_permissions = {
+ 'Policy 1': 'Nothing',
+ 'Policy 2': 'All',
+ 'Policy 3': 'Nothing'
+ };
+ var observed_qs = Y.QueryString.parse(
+ this.mockio.last_request.config.data);
+ var observed_permissions = Y.JSON.parse(
+ observed_qs['permissions']);
+ delete observed_qs['permissions'];
+ Y.ObjectAssert.areEqual(expected_qs, observed_qs);
+ Y.ObjectAssert.areEqual(
+ expected_permissions, observed_permissions);
this.mockio.last_request.successJSON({
grantee_entry: {
'name': 'joe',
@@ -407,17 +410,22 @@
this.mockio.last_request.url);
var person_uri = Y.lp.client.normalize_uri('~fred');
person_uri = Y.lp.client.get_absolute_uri(person_uri);
- var expected_url;
- expected_url = Y.lp.client.append_qs(
- expected_url, 'ws.op', 'sharePillarInformation');
- expected_url = Y.lp.client.append_qs(
- expected_url, 'pillar', '~pillar');
- expected_url = Y.lp.client.append_qs(
- expected_url, 'grantee', person_uri);
- expected_url = Y.lp.client.append_qs(
- expected_url, 'permissions', 'Policy 1,Some');
- Y.Assert.areEqual(
- expected_url, this.mockio.last_request.config.data);
+ var expected_qs = {
+ 'ws.op': 'sharePillarInformation',
+ 'pillar': '"~pillar"',
+ 'grantee': '"' + person_uri + '"'
+ };
+ var expected_permissions = {
+ 'Policy 1': 'Some'
+ };
+ var observed_qs = Y.QueryString.parse(
+ this.mockio.last_request.config.data);
+ var observed_permissions = Y.JSON.parse(
+ observed_qs['permissions']);
+ delete observed_qs['permissions'];
+ Y.ObjectAssert.areEqual(expected_qs, observed_qs);
+ Y.ObjectAssert.areEqual(
+ expected_permissions, observed_permissions);
this.mockio.last_request.successJSON({
grantee_entry: {
'name': 'fred',
@@ -578,8 +586,9 @@
'/api/devel/+services/sharing',
this.mockio.last_request.url);
Y.Assert.areEqual(
- 'ws.op=updatePillarSharingPolicies&pillar=~pillar&' +
- artifact_type + '_sharing_policy=' + title + '%20Policy%201',
+ 'ws.op=updatePillarSharingPolicies&pillar=%22~pillar%22&' +
+ artifact_type + '_sharing_policy=%22' + title +
+ '%20Policy%201%22',
this.mockio.last_request.config.data);
},
@@ -694,6 +703,7 @@
})));
}, '0.1', {'requires': ['test', 'test-console', 'event', 'node-event-simulate',
+ 'querystring-parse', 'json-parse',
'lp.testing.mockio', 'lp.registry.sharing.granteepicker',
'lp.registry.sharing.granteetable', 'lp.app.errors',
'lp.registry.sharing.pillarsharingview']});
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js 2015-04-29 10:33:33 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetailsview.js 2015-10-13 11:18:19 +0000
@@ -255,11 +255,11 @@
expected_qs = Y.lp.client.append_qs(
expected_qs, 'ws.op', 'revokeAccessGrants');
expected_qs = Y.lp.client.append_qs(
- expected_qs, 'pillar', '/pillar');
- expected_qs = Y.lp.client.append_qs(
- expected_qs, 'grantee', '~fred');
- expected_qs = Y.lp.client.append_qs(
- expected_qs, 'bugs', 'api/devel/bugs/2');
+ expected_qs, 'pillar', '/pillar', true);
+ expected_qs = Y.lp.client.append_qs(
+ expected_qs, 'grantee', '~fred', true);
+ expected_qs = Y.lp.client.append_qs(
+ expected_qs, 'bugs', 'api/devel/bugs/2', true);
Y.Assert.areEqual(expected_qs, mockio.last_request.config.data);
mockio.last_request.successJSON({});
Y.Assert.isTrue(remove_grant_success_called);
@@ -294,11 +294,12 @@
expected_qs = Y.lp.client.append_qs(
expected_qs, 'ws.op', 'revokeAccessGrants');
expected_qs = Y.lp.client.append_qs(
- expected_qs, 'pillar', '/pillar');
- expected_qs = Y.lp.client.append_qs(
- expected_qs, 'grantee', '~fred');
- expected_qs = Y.lp.client.append_qs(
- expected_qs, 'branches', 'api/devel/~someone/+junk/somebranch');
+ expected_qs, 'pillar', '/pillar', true);
+ expected_qs = Y.lp.client.append_qs(
+ expected_qs, 'grantee', '~fred', true);
+ expected_qs = Y.lp.client.append_qs(
+ expected_qs, 'branches', 'api/devel/~someone/+junk/somebranch',
+ true);
Y.Assert.areEqual(expected_qs, mockio.last_request.config.data);
mockio.last_request.successJSON({});
Y.Assert.isTrue(remove_grant_success_called);
@@ -334,12 +335,12 @@
expected_qs = Y.lp.client.append_qs(
expected_qs, 'ws.op', 'revokeAccessGrants');
expected_qs = Y.lp.client.append_qs(
- expected_qs, 'pillar', '/pillar');
+ expected_qs, 'pillar', '/pillar', true);
expected_qs = Y.lp.client.append_qs(
- expected_qs, 'grantee', '~fred');
+ expected_qs, 'grantee', '~fred', true);
expected_qs = Y.lp.client.append_qs(
expected_qs, 'gitrepositories',
- 'api/devel/~someone/+git/somerepo');
+ 'api/devel/~someone/+git/somerepo', true);
Y.Assert.areEqual(expected_qs, mockio.last_request.config.data);
mockio.last_request.successJSON({});
Y.Assert.isTrue(remove_grant_success_called);
Follow ups
