launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/xss-deleting-ssh-key-740160 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #740160 in Launchpad itself: "this is harmless: a user can xss themselves on deleting an ssh key"
  https://bugs.launchpad.net/launchpad/+bug/740160

For more details, see:
https://code.launchpad.net/~deryck/launchpad/xss-deleting-ssh-key-740160/+merge/59535

This fixes the use of structured in the ssh keys view, so we don't end up with an XSS vector when removing ssh keys.  See the linked bug for more info.  I also added a test to ensure this message text is escaped.
-- 
https://code.launchpad.net/~deryck/launchpad/xss-deleting-ssh-key-740160/+merge/59535
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/xss-deleting-ssh-key-740160 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2011-04-27 18:41:57 +0000
+++ lib/lp/registry/browser/person.py	2011-04-29 17:25:51 +0000
@@ -3890,7 +3890,7 @@
 
         comment = sshkey.comment
         sshkey.destroySelf()
-        self.info_message = structured('Key "%s" removed' % comment)
+        self.info_message = structured('Key "%s" removed', comment)
 
 
 class PersonGPGView(LaunchpadView):

=== modified file 'lib/lp/registry/browser/tests/test_sshkey.py'
--- lib/lp/registry/browser/tests/test_sshkey.py	2010-03-03 22:33:16 +0000
+++ lib/lp/registry/browser/tests/test_sshkey.py	2011-04-29 17:25:51 +0000
@@ -5,10 +5,15 @@
 
 __metaclass__ = type
 
-import unittest
+from zope.component import getUtility
 
+from canonical.launchpad.testing.pages import (
+    extract_text,
+    find_tags_by_class,
+    )
 from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.ssh import ISSHKeySet
 from lp.testing import TestCaseWithFactory
 
 
@@ -26,6 +31,22 @@
             canonical_url(sshkey))
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
-
+class TestSSHKeyView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_escaped_message_when_removing_key(self):
+        """Confirm that messages are escaped when removing keys."""
+        person = self.factory.makePerson()
+        public_key = "ssh-rsa %s x<script>alert()</script>example.com" % (
+            self.getUniqueString())
+        # Add the key for the user here,
+        # since we only care about testing removal.
+        getUtility(ISSHKeySet).new(person, public_key)
+        browser = self.getUserBrowser(
+            canonical_url(person) + '/+editsshkeys', user=person)
+        browser.getControl('Remove').click()
+        msg = 'Key "x&lt;script&gt;alert()&lt;/script&gt;example.com" removed'
+        self.assertEqual(
+            extract_text(find_tags_by_class(browser.contents, 'message')[0]),
+            msg)