launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/inline-multicheckbox-widget-xss into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/inline-multicheckbox-widget-xss/+merge/54645

When the inline multicheckbox edit widget renders the checkbox labels, it needs to escape the text.

== Implementation ==

Fix the Javascript

== Test ==

Modify the exiting test_multicheckboxwidget.js to ensure the test data contained "<", ">" so that the escaping could be checked.

-- 
https://code.launchpad.net/~wallyworld/launchpad/inline-multicheckbox-widget-xss/+merge/54645
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/inline-multicheckbox-widget-xss into lp:launchpad.

=== modified file 'lib/lp/app/javascript/multicheckbox.js'
--- lib/lp/app/javascript/multicheckbox.js	2011-03-17 05:29:08 +0000
+++ lib/lp/app/javascript/multicheckbox.js	2011-03-24 03:19:28 +0000
@@ -153,7 +153,7 @@
         var checkbox_html = Y.Lang.substitute(
             CHECKBOX_TEMPLATE,
             {field_name: "field."+attribute_name, field_index:i,
-            field_value: data.token, field_text: data.name,
+            field_value: data.token, field_text: Y.Escape.html(data.name),
             item_style: data.style, item_checked: checked_html});
 
         var choice_item = Y.Node.create("<li/>");
@@ -228,5 +228,5 @@
 }
 
 }, "0.1", {"requires": [
-    "dom", "lazr.overlay", "lazr.activator", "lp.client"
+    "dom", "escape", "lazr.overlay", "lazr.activator", "lp.client"
     ]});

=== modified file 'lib/lp/app/javascript/tests/test_multicheckboxwidget.js'
--- lib/lp/app/javascript/tests/test_multicheckboxwidget.js	2011-03-17 05:28:47 +0000
+++ lib/lp/app/javascript/tests/test_multicheckboxwidget.js	2011-03-24 03:19:28 +0000
@@ -60,9 +60,9 @@
       var mock_client = new MockClient();
       this.widget = Y.lp.app.multicheckbox.addMultiCheckboxPatcher(
         [{"token": "0", "style": "font-weight: normal;", "checked": true,
-            "name": "Item 0", "value": "item1"},
+            "name": "Item 0<foo/>", "value": "item1"},
         {"token": "1", "style": "font-weight: normal;", "checked": false,
-            "name": "Item 1", "value": "item2"}],
+            "name": "Item 1<foo/>", "value": "item2"}],
         'A test',
         '/~fred/+recipe/a-recipe',
         'multicheckboxtest',
@@ -125,7 +125,7 @@
             var txt = item.get('textContent');
             //remove any &nbsp in the text
             txt = txt.replace(/^[\s\xA0]+/g,'').replace(/[\s(&nbsp;)]+$/g,'');
-            Y.Assert.areEqual(txt, 'Item '+ x);
+            Y.Assert.areEqual(txt, 'Item '+ x + '<foo/>');
         }
     },