launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/safer-ajax-strings into lp:launchpad.

Commit message:
Make the JS webservice client stringify string parameters rather than relying on lazr.restful's dodgy special casing.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/safer-ajax-strings/+merge/274166

lazr.restful has a special case in marshall_from_request that allows clients to get away without stringifying string parameters, but this is somewhat dodgy: for example, it crashes if a parameter consists only of whitespace, which is possible if you try to submit a vote on a merge proposal including a single space as the comment (https://oops.canonical.com/?oopsid=OOPS-bb42b8f55c6448f820a2539605e06ea3).  Rather than relying on this, I think it's better to encode string parameters as JSON strings.

I left ws.op unencoded, as that seemed likely to result in test fallout, and that parameter will always be an identifier anyway.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/safer-ajax-strings into lp:launchpad.

=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js	2014-01-17 04:10:34 +0000
+++ lib/lp/app/javascript/client.js	2015-10-12 16:11:49 +0000
@@ -183,7 +183,8 @@
             for (index = 0; index < value.length; index++) {
                 elems.push(enc(key) + "=" + enc(value[index]));
             }
-        } else if (Y.Lang.isObject(value)) {
+        } 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));

=== modified file 'lib/lp/app/javascript/tests/test_lp_client.js'
--- lib/lp/app/javascript/tests/test_lp_client.js	2014-01-20 22:09:20 +0000
+++ lib/lp/app/javascript/tests/test_lp_client.js	2015-10-12 16:11:49 +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=Perell%C3%83%C2%B3", qs,
+                "P%C3%83%C2%B6ll%C3%83%C2%A4=%22Perell%C3%83%C2%B3%22", qs,
                 'This tests is known to fail in Chrome.');
         },