launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-969043 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #969043 in MAAS: "Unescaped ssh-key comment in UI's HTML"
  https://bugs.launchpad.net/maas/+bug/969043

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-969043/+merge/100118

The code that renders ssh keys in a shortened form produces HTML.  But the key's comment (well and to be honest, for all I know the other parts as well) may contain characters that have special meaning in HTML.

-- 
https://code.launchpad.net/~jtv/maas/bug-969043/+merge/100118
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-969043 into lp:maas.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-03-30 10:39:56 +0000
+++ src/maasserver/models.py	2012-03-30 12:36:29 +0000
@@ -24,6 +24,7 @@
     "UserProfile",
     ]
 
+from cgi import escape
 from collections import (
     defaultdict,
     OrderedDict,
@@ -836,7 +837,8 @@
 
     :param key: The key for which we want an HTML representation.
     :type name: basestring
-    :param size: The maximum size of the representation.
+    :param size: The maximum size of the representation.  This may not be
+        met exactly.
     :type size: int
     :return: The HTML representation of this key.
     :rtype: basestring
@@ -852,12 +854,16 @@
             size - (len(key_type) + len(comment) + len(HELLIPSIS) + 2))
         if room_for_key > 0:
             return '%s %.*s%s %s' % (
-                key_type, room_for_key, key_string, HELLIPSIS, comment)
+                escape(key_type),
+                room_for_key,
+                escape(key_string),
+                HELLIPSIS,
+                escape(comment))
 
     if len(key) > size:
-        return '%.*s%s' % (size - len(HELLIPSIS), key, HELLIPSIS)
+        return '%.*s%s' % (size - len(HELLIPSIS), escape(key), HELLIPSIS)
     else:
-        return key
+        return escape(key)
 
 
 MAX_KEY_DISPLAY = 50

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-03-30 10:39:56 +0000
+++ src/maasserver/tests/test_models.py	2012-03-30 12:36:29 +0000
@@ -63,6 +63,7 @@
     )
 from provisioningserver.enum import POWER_TYPE
 from testtools.matchers import (
+    EndsWith,
     GreaterThan,
     LessThan,
     )
@@ -709,6 +710,25 @@
 class GetHTMLDisplayForKeyTest(TestCase):
     """Testing for the method `get_html_display_for_key`."""
 
+    def make_key(self, type_len=7, key_len=360, comment_len=None):
+        """Produce a fake ssh public key containing arbitrary data.
+
+        :param type_len: The length of the "key type" field.  (Default is
+            sized for the real-life "ssh-rsa").
+        :param key_len: Length of the key text.  (With a roughly realistic
+            default).
+        :param comment_len: Length of the comment field.  The comment may
+            contain spaces.  Leave it None to omit the comment.
+        :return: A string representing the combined key-file contents.
+        """
+        fields = [
+            factory.getRandomString(type_len),
+            factory.getRandomString(key_len),
+            ]
+        if comment_len is not None:
+            fields.append(factory.getRandomString(comment_len, spaces=True))
+        return " ".join(fields)
+
     def test_display_returns_unchanged_if_unknown_and_small(self):
         # If the key does not look like a normal key (with three parts
         # separated by spaces, it's returned unchanged if its size is <=
@@ -730,13 +750,57 @@
         self.assertEqual(
             '%.*s%s' % (size - len(HELLIPSIS), key, HELLIPSIS), display)
 
+    def test_display_escapes_commentless_key_for_html(self):
+        # The key's comment may contain characters that are not safe for
+        # including in HTML, and so get_html_display_for_key escapes the
+        # text.
+        # There are several code paths in get_html_display_for_key; this
+        # test is for the case where the key has no comment, and is
+        # brief enough to fit into the allotted space.
+        self.assertEqual(
+            "&lt;type&gt; &lt;text&gt;",
+            get_html_display_for_key("<type> <text>", 100))
+
+    def test_display_escapes_short_key_for_html(self):
+        # The key's comment may contain characters that are not safe for
+        # including in HTML, and so get_html_display_for_key escapes the
+        # text.
+        # There are several code paths in get_html_display_for_key; this
+        # test is for the case where the whole key is short enough to
+        # fit completely into the output.
+        key = "<type> <text> <comment>"
+        display = get_html_display_for_key(key, 100)
+        # This also verifies that the entire key fits into the string.
+        # Otherwise we might accidentally get one of the other cases.
+        self.assertThat(display, EndsWith("&lt;comment&gt;"))
+        # And of course the check also implies that the text is
+        # HTML-escaped:
+        self.assertNotIn("<", display)
+        self.assertNotIn(">", display)
+
+    def test_display_escapes_long_key_for_html(self):
+        # The key's comment may contain characters that are not safe for
+        # including in HTML, and so get_html_display_for_key escapes the
+        # text.
+        # There are several code paths in get_html_display_for_key; this
+        # test is for the case where the comment is short enough to fit
+        # completely into the output.
+        key = "<type> %s <comment>" % ("<&>" * 50)
+        display = get_html_display_for_key(key, 50)
+        # This verifies that we are indeed getting an abbreviated
+        # display.  Otherwise we might accidentally get one of the other
+        # cases.
+        self.assertIn("&hellip;", display)
+        self.assertIn("comment", display)
+        # And now, on to checking that the text is HTML-safe.
+        self.assertNotIn("<", display)
+        self.assertNotIn(">", display)
+        self.assertThat(display, EndsWith("&lt;comment&gt;"))
+
     def test_display_limits_size_with_large_comment(self):
         # If the key has a large 'comment' part, the key is simply
         # cropped and HELLIPSIS appended to it.
-        key_type = factory.getRandomString(10)
-        key_string = factory.getRandomString(10)
-        comment = factory.getRandomString(100, spaces=True)
-        key = '%s %s %s' % (key_type, key_string, comment)
+        key = self.make_key(10, 10, 100)
         display = get_html_display_for_key(key, 50)
         self.assertEqual(50, len(display))
         self.assertEqual(
@@ -745,10 +809,7 @@
     def test_display_limits_size_with_large_key_type(self):
         # If the key has a large 'key_type' part, the key is simply
         # cropped and HELLIPSIS appended to it.
-        key_type = factory.getRandomString(100)
-        key_string = factory.getRandomString(10)
-        comment = factory.getRandomString(10, spaces=True)
-        key = '%s %s %s' % (key_type, key_string, comment)
+        key = self.make_key(100, 10, 10)
         display = get_html_display_for_key(key, 50)
         self.assertEqual(50, len(display))
         self.assertEqual(
@@ -798,7 +859,7 @@
         self.assertEqual(
             'ssh-rsa AAAAB3NzaC1yc2E&hellip; ubuntu@server-7476', display)
 
-    def test_sshkey_display_is_safe(self):
+    def test_sshkey_display_is_marked_as_HTML_safe(self):
         key_string = get_data('data/test_rsa.pub')
         user = factory.make_user()
         key = SSHKey(key=key_string, user=user)