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 → Foo &<> (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 &<>",
+ BugActivityItem(bug.activity[-1]).change_details)
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
