launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/xss_911632 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rharding/launchpad/xss_911632/+merge/87507

= Summary =
A user discovered that there was a XSS script avenue in the code branch listing page for a user.

== Proposed Fix ==
The template was using a content:structure that prevented escaping of the html entities.

== Implementation Details ==
In searching, no one implementing the  no_branch_message had any html in the constructed message. The structure was just removed to allow for escaping to take place at the template level.

== Tests ==
./bin/test -cvvt test_branchlisting

== Demo and Q/A ==
A user's full name needs to be changed to "'/><script>alert(true);</script>. Then you need to load up the user's code.launchpad.net page. Make sure there are no branches in the table view by changing the status filter until you trigger the "no branch message". The output should be escaped.
-- 
https://code.launchpad.net/~rharding/launchpad/xss_911632/+merge/87507
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/xss_911632 into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2012-01-04 19:05:46 +0000
@@ -674,6 +674,15 @@
             branch.product, name="+branches", rootsite='code')
         self.assertIn('a moment ago', view())
 
+    def test_no_branch_message_escaped(self):
+        # make sure we escape any information put into the no branch message
+        badname = '<script>Test</script>'
+        escapedname = '\\u003cscript\\u003eTest\\u003c/script\\u003e'
+        baduser = self.factory.makePerson(displayname=badname)
+        browser = self.getUserBrowser(canonical_url(baduser, rootsite='code'))
+
+        self.assertTrue(badname not in browser.contents)
+        self.assertTrue(escapedname in browser.contents)
 
 class TestProjectGroupBranches(TestCaseWithFactory,
                                AjaxBatchNavigationMixin):

=== modified file 'lib/lp/code/templates/branch-listing.pt'
--- lib/lp/code/templates/branch-listing.pt	2011-10-18 01:17:22 +0000
+++ lib/lp/code/templates/branch-listing.pt	2012-01-04 19:05:46 +0000
@@ -186,7 +186,7 @@
       <tr tal:condition="not:context/batch/total">
         <td tal:attributes="colspan context/column_count">
           <div id="no-branch-message"
-               tal:content="structure context/view/no_branch_message" />
+               tal:content="context/view/no_branch_message" />
         </td>
       </tr>
     </tbody>