launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-759340 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-759340/+merge/57422

BugActivityItem.change_details is rendered unescaped on BugTask:+index, and it lets some strings from BugActivity through unescaped. Only assignee was actually exploitable, but tags and milestones were saved only by DB constraints on their values. I've escaped those that were unescaped, and added tests to confirm that the two potential vulnerabilities (title and assignee) are escaped.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-759340/+merge/57422
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-759340 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-03-30 11:36:37 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-04-13 02:31:03 +0000
@@ -3877,17 +3877,22 @@
         elif attribute == 'tags':
             # We special-case tags because we can work out what's been
             # added and what's been removed.
-            return self._formatted_tags_change.replace('\n', '<br />')
+            return cgi.escape(self._formatted_tags_change).replace(
+                '\n', '<br />')
 
         elif attribute == 'assignee':
             for key in return_dict:
                 if return_dict[key] is None:
                     return_dict[key] = 'nobody'
+                else:
+                    return_dict[key] = cgi.escape(return_dict[key])
 
         elif attribute == 'milestone':
             for key in return_dict:
                 if return_dict[key] is None:
                     return_dict[key] = 'none'
+                else:
+                    return_dict[key] = cgi.escape(return_dict[key])
 
         else:
             # Our default state is to just return oldvalue and newvalue.

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-04-05 06:13:39 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-04-13 02:31:03 +0000
@@ -5,10 +5,14 @@
 
 from datetime import datetime
 
+from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
 from pytz import UTC
 from storm.store import Store
 from testtools.matchers import LessThan
 from zope.component import getUtility
+from zope.event import notify
+from zope.interface import providedBy
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.ftests import (
@@ -25,6 +29,7 @@
     LaunchpadFunctionalLayer,
     )
 from lp.bugs.browser.bugtask import (
+    BugActivityItem,
     BugTaskEditView,
     BugTasksAndNominationsView,
     )
@@ -764,3 +769,33 @@
         contents = view.render()
         help_link = find_tag_by_id(contents, 'getting-started-help')
         self.assertIs(None, help_link)
+
+
+class TestBugActivityItem(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setAttribute(self, obj, attribute, value):
+        obj_before_modification = Snapshot(obj, providing=providedBy(obj))
+        setattr(removeSecurityProxy(obj), attribute, value)
+        notify(ObjectModifiedEvent(
+            obj, obj_before_modification, [attribute],
+            self.factory.makePerson()))
+
+    def test_escapes_assignee(self):
+        with celebrity_logged_in('admin'):
+            task = self.factory.makeBugTask()
+            self.setAttribute(
+                task, 'assignee',
+                self.factory.makePerson(displayname="Foo &<>", name='foo'))
+        self.assertEquals(
+            "nobody &#8594; Foo &amp;&lt;&gt; (foo)",
+            BugActivityItem(task.bug.activity[-1]).change_details)
+
+    def test_escapes_title(self):
+        with celebrity_logged_in('admin'):
+            bug = self.factory.makeBug(title="foo")
+            self.setAttribute(bug, 'title', "bar &<>")
+        self.assertEquals(
+            "- foo<br />+ bar &amp;&lt;&gt;",
+            BugActivityItem(bug.activity[-1]).change_details)