launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/devel-744204-escaping-soyuz-base into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-744204-escaping-soyuz-base/+merge/56978

= Summary =

Bug 744204

The javascript code in Y.lp.soyuz.base was not safe against xss
attacks becuse it set innerHTML to values of unknown origin. One of
the call sites actually passes in response.responseText which is one
the attack vectors wgrant described.

== Proposed fix ==

Use string templates and DOM manipulation to create the markup. Most
importantly use set('text', text) to insert the text passed in. This
way it is escaped properly.

== Pre-implementation notes ==

I'd call wgrants email about xss attacks a pre-imp discussion. ;-)

== Implementation details ==

In distroseriesdifferences_details.js Y.Escape had been applied to all
parameters passed into the vulnurable functions to relieve the danger.
Those could now be removed.

Between JSLINT and me we found some dodgy js code which I fixed along
the way.

== Tests ==

As this is a pure refactoring, no tests needed to be changed or added.
The WindmillLayer tests are currently not working on natty but the
relevant js unit test can be run like this:

firefox lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.html

== Demo and Q/A ==

I tried it out locally on 
https://launchpad.dev/~mark/+archive/ppa?field.series_filter=breezy-autotest

There is a spinner while the archive size is retrieved and an error
message when it fails. I triggered that by halting the execution in
the debugger and stopping the dev server underneath it. Not sure how
to reproduce that for Q/A. Take down my network?

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/javascript/base.js
  lib/lp/soyuz/templates/archive-macros.pt
  lib/lp/soyuz/templates/archive-packages.pt
  lib/lp/registry/javascript/distroseriesdifferences_details.js

./lib/lp/registry/javascript/distroseriesdifferences_details.js
      37: Expected '===' and instead saw '=='.
      39: Expected '===' and instead saw '=='.
      57: Expected '===' and instead saw '=='.
     247: 'get_extra_diff_info' has not been fully defined yet.
     266: Expected an identifier and instead saw 'arguments' (a reserved word).
     285: Expected '===' and instead saw '=='.
     334: Move 'var' declarations to the top of the function.
     334: Stopping.  (59% scanned).
       0: JSLINT had a fatal error.
-- 
https://code.launchpad.net/~henninge/launchpad/devel-744204-escaping-soyuz-base/+merge/56978
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-744204-escaping-soyuz-base into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-04-06 15:26:34 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-04-08 17:21:51 +0000
@@ -247,8 +247,7 @@
                     args.uri, args.container, args.source_name);
             };
             var failure_message = Y.lp.soyuz.base.makeFailureNode(
-                Y.Escape.html('Failed to fetch difference details.'),
-                retry_handler);
+                'Failed to fetch difference details.', retry_handler);
             container.insert(failure_message, 'replace');
 
             var anim = Y.lazr.anim.red_flash({
@@ -261,7 +260,7 @@
             headers: {'Accept': 'application/json;'},
             on: {
                 'success': success_cb,
-                'failure': failure_cb,
+                'failure': failure_cb
             },
             arguments: {
                 'container': container,
@@ -280,11 +279,12 @@
         toggle.toggleClass('treeCollapsed').toggleClass('treeExpanded');
 
         // Only insert if there isn't already a container row there.
-        next_row = row.next();
+        var next_row = row.next();
+        var details_row;
         if (next_row == null || !next_row.hasClass('diff-extra')) {
             var source_name = row.one('a.toggle-extra').get('text');
             var nb_columns = row.all('td').size();
-            var details_row = Y.Node.create([
+            details_row = Y.Node.create([
                 '<table><tr class="diff-extra unseen ' + source_name + '">',
                 '  <td colspan="'+nb_columns+'">',
                 '    <div class="diff-extra-container"></div>',
@@ -294,7 +294,7 @@
             var uri = toggle.get('href');
             get_extra_diff_info(uri, details_row.one('td'), source_name);
         } else {
-            details_row = next_row
+            details_row = next_row;
         }
         details_row.toggleClass('unseen');
 
@@ -304,7 +304,7 @@
     Y.all('table.listing a.toggle-extra').each(function(toggle){
         toggle.addClass('treeCollapsed').addClass('sprite');
         toggle.on("click", expander_handler);
-    })
+    });
 
 };
 
@@ -358,8 +358,7 @@
                     namespace.add_link_to_package_diff(container, url_uri);
                 };
                 var failure_message = Y.lp.soyuz.base.makeFailureNode(
-                    Y.Escape.html('Failed to fetch package diff url.'),
-                    retry_handler);
+                    'Failed to fetch package diff url.', retry_handler);
                 container.insert(failure_message);
             }
         }
@@ -538,8 +537,7 @@
         if (response.status !== 400) {
             msg = 'Failed to compute package diff.';
         }
-        var failure_msg = Y.lp.soyuz.base.makeFailureNode(
-            Y.Escape.html(msg), recompute);
+        var failure_msg = Y.lp.soyuz.base.makeFailureNode(msg, recompute);
         namespace.add_msg_node(container, failure_msg);
     };
     var config = {
@@ -555,7 +553,7 @@
     lp_client.named_post(dsd_link, 'requestPackageDiffs', config);
 };
 
-}, "0.1", {"requires": ["escape", "event-simulate", "io-base",
+}, "0.1", {"requires": ["event-simulate", "io-base",
                         "lp.soyuz.base", "lp.client",
                         "lazr.anim", "lazr.effects",
                         "lp.soyuz.dynamic_dom_updater"]});

=== modified file 'lib/lp/soyuz/javascript/base.js'
--- lib/lp/soyuz/javascript/base.js	2011-03-28 16:33:48 +0000
+++ lib/lp/soyuz/javascript/base.js	2011-04-08 17:21:51 +0000
@@ -19,19 +19,15 @@
  * in XHR-based page updates.
  */
 namespace.makeFailureNode = function (text, handler) {
-    var failure_message = Y.Node.create('<p>');
+    var failure_message = Y.Node.create('<p><span></span><a></a></p>');
     failure_message.addClass('update-failure-message');
-
-    var message = Y.Node.create('<span>');
-    message.set('innerHTML', text);
-    failure_message.appendChild(message);
-
-    var retry_link = Y.Node.create('<a>');
+    failure_message.one('span').set('text', text);
+
+    var retry_link = failure_message.one('a');
     retry_link.addClass('update-retry');
     retry_link.set('href', '');
-    retry_link.set('innerHTML', 'Retry');
+    retry_link.set('text', 'Retry');
     retry_link.on('click', handler);
-    failure_message.appendChild(retry_link);
 
     return failure_message;
 };
@@ -42,12 +38,9 @@
  * in XHR-based page updates.
  */
 namespace.makeInProgressNode = function (text) {
-    var in_progress_message = Y.Node.create('<p>');
-    var message = Y.Node.create('<span>');
-
-    message.set('innerHTML', text);
+    var in_progress_message = Y.Node.create('<p><span></span></p>');
     in_progress_message.addClass('update-in-progress-message');
-    in_progress_message.appendChild(message);
+    in_progress_message.one('span').set('text', text);
 
     return in_progress_message;
 };

=== modified file 'lib/lp/soyuz/templates/archive-macros.pt'
--- lib/lp/soyuz/templates/archive-macros.pt	2011-02-23 22:30:41 +0000
+++ lib/lp/soyuz/templates/archive-macros.pt	2011-04-08 17:21:51 +0000
@@ -48,7 +48,7 @@
  * Dispatch a XHR for updating the given container.
  */
 function startUpdate(container) {
-    in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
+    var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
         'Fetching package details ...')
 
     container.set('innerHTML', '');

=== modified file 'lib/lp/soyuz/templates/archive-packages.pt'
--- lib/lp/soyuz/templates/archive-packages.pt	2010-10-10 21:54:16 +0000
+++ lib/lp/soyuz/templates/archive-packages.pt	2011-04-08 17:21:51 +0000
@@ -49,7 +49,7 @@
  * Communicate an update is in progress and fire the XHR.
  */
 function dispatchUpdate () {
-    in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
+    var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
         'Fetching repository size ...')
 
     var container = Y.one('#package-counters');