launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/bug-683406-xss-ppa into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-683406-xss-ppa/+merge/62158

= Summary =

An old problem: unescaped text in notifications.

== Proposed fix ==

Apply structured to whereever a PPA title is used in a notification
message.

== Pre-implementation notes ==

I refer you to wgrant's treatment of XSS vulnabilities in LP.

== Implementation details ==

The challenge here was that the code was structured in a way that it
collected notification messages to eventually join them for output.
The joining does not work with structured, so I had to add a helper
function that gets the escapedtext attribute.

== Tests ==

bin/test -vvcm lp.soyuz -t xx-edit-dependencies
bin/test -vvcm lp.soyuz.browser.tests -t archive-views.txt

== Demo and Q/A ==

Got to https://launchpad.dev/~mark and add a second PPA called 'ppa2'
and give it a title that contains markup.
Now go to https://launchpad.dev/~mark/+archive/ppa/+edit-dependencies
and add a PPA dependcy 'mark/ppa2'.
Click 'Save' and the notfication box should display the markup from the
PPA's title but not render it.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/browser/archive.py
-- 
https://code.launchpad.net/~henninge/launchpad/bug-683406-xss-ppa/+merge/62158
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-683406-xss-ppa into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-05-19 15:10:28 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-05-24 16:07:59 +0000
@@ -74,7 +74,10 @@
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.badge import HasBadgeBase
 from canonical.launchpad.webapp.batching import BatchNavigator
-from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
+from canonical.launchpad.webapp.interfaces import (
+    ICanonicalUrlData,
+    IStructuredString,
+    )
 from canonical.launchpad.webapp.menu import (
     NavigationMenu,
     structured,
@@ -1596,6 +1599,14 @@
             self.setNextURL()
 
 
+def get_escapedtext(message):
+    """Return escapedtext if message is an `IStructuredString`."""
+    if IStructuredString.providedBy(message):
+        return message.escapedtext
+    else:
+        return message
+
+
 class ArchiveEditDependenciesView(ArchiveViewBase, LaunchpadFormView):
     """Archive dependencies view class."""
 
@@ -1792,7 +1803,7 @@
 
     @property
     def messages(self):
-        return '\n'.join(self._messages)
+        return '\n'.join(map(get_escapedtext, self._messages))
 
     def _remove_dependencies(self, data):
         """Perform the removal of the selected dependencies."""
@@ -1808,7 +1819,8 @@
         # Present a page notification describing the action.
         self._messages.append('<p>Dependencies removed:')
         for dependency in selected_dependencies:
-            self._messages.append('<br/>%s' % dependency.displayname)
+            self._messages.append(
+                structured('<br/>%s', dependency.displayname))
         self._messages.append('</p>')
 
     def _add_ppa_dependencies(self, data):
@@ -1821,8 +1833,8 @@
             dependency_candidate, PackagePublishingPocket.RELEASE,
             getUtility(IComponentSet)['main'])
 
-        self._messages.append(
-            '<p>Dependency added: %s</p>' % dependency_candidate.displayname)
+        self._messages.append(structured(
+            '<p>Dependency added: %s</p>', dependency_candidate.displayname))
 
     def _add_primary_dependencies(self, data):
         """Record the selected dependency."""
@@ -1867,8 +1879,8 @@
         primary_dependency = self.context.addArchiveDependency(
             self.context.distribution.main_archive, dependency_pocket,
             dependency_component)
-        self._messages.append(
-            '<p>Primary dependency added: %s</p>' % primary_dependency.title)
+        self._messages.append(structured(
+            '<p>Primary dependency added: %s</p>', primary_dependency.title))
 
     def validate(self, data):
         """Validate dependency configuration changes.