launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/fix-unicode-path-oops into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #200572 in Launchpad itself: "Non-ascii in the URL causes an OOPS"
  https://bugs.launchpad.net/launchpad/+bug/200572

For more details, see:
https://code.launchpad.net/~thumper/launchpad/fix-unicode-path-oops/+merge/56687

This branch catches a couple of unicode issues.

One is where we were trying to look for something in storm with a potentially bad byte ordering, and the second was when we got a NotFoundError, the rendering of the page checked for a webservice request, which tryied to UTF-8 decode the string, which again failed.
-- 
https://code.launchpad.net/~thumper/launchpad/fix-unicode-path-oops/+merge/56687
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/fix-unicode-path-oops into lp:launchpad.

=== modified file 'lib/canonical/launchpad/helpers.py'
--- lib/canonical/launchpad/helpers.py	2011-03-30 23:37:11 +0000
+++ lib/canonical/launchpad/helpers.py	2011-04-07 05:15:25 +0000
@@ -482,26 +482,6 @@
     return open(fullpath).read()
 
 
-def is_ascii_only(string):
-    """Ensure that the string contains only ASCII characters.
-
-        >>> is_ascii_only(u'ascii only')
-        True
-        >>> is_ascii_only('ascii only')
-        True
-        >>> is_ascii_only('\xf4')
-        False
-        >>> is_ascii_only(u'\xf4')
-        False
-    """
-    try:
-        string.encode('ascii')
-    except UnicodeError:
-        return False
-    else:
-        return True
-
-
 def truncate_text(text, max_length):
     """Return a version of string no longer than max_length characters.
 

=== modified file 'lib/canonical/launchpad/rest/configuration.py'
--- lib/canonical/launchpad/rest/configuration.py	2011-03-22 11:54:10 +0000
+++ lib/canonical/launchpad/rest/configuration.py	2011-04-07 05:15:25 +0000
@@ -60,6 +60,10 @@
 
     def createRequest(self, body_instream, environ):
         """See `IWebServiceConfiguration`."""
+        # The request is going to try to decode the 'PATH_INFO' using utf-8,
+        # so if it is currently unicode, encode it.
+        if isinstance(environ.get('PATH_INFO'), unicode):
+            environ['PATH_INFO'] = environ['PATH_INFO'].encode('utf-8')
         request = WebServiceClientRequest(body_instream, environ)
         request.setPublication(WebServicePublication(None))
         return request

=== modified file 'lib/canonical/launchpad/webapp/error.py'
--- lib/canonical/launchpad/webapp/error.py	2010-12-17 19:16:54 +0000
+++ lib/canonical/launchpad/webapp/error.py	2011-04-07 05:15:25 +0000
@@ -209,7 +209,15 @@
         """
         referrer = self.request.get('HTTP_REFERER')
         if referrer:
-            return referrer
+            # Since this is going to be included in the page template it will
+            # be coerced into unicode.  The byte string representation
+            # 'should' be ascii, but often it isn't.  The only use for this is
+            # to show a link back to the referring site, so we can't use
+            # replace or ignore.  Best to just pretent it doesn't exist.
+            try:
+                return unicode(referrer)
+            except UnicodeDecodeError:
+                return None
         else:
             return None
 

=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2011-03-21 21:33:25 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2011-04-07 05:15:25 +0000
@@ -70,6 +70,7 @@
 from canonical.launchpad.webapp.vhosts import allvhosts
 from canonical.lazr.utils import get_current_browser_request
 from lp.app.errors import NotFoundError
+from lp.services.encoding import is_ascii_only
 
 # HTTP Status code constants - define as appropriate.
 HTTP_MOVED_PERMANENTLY = 301
@@ -657,6 +658,10 @@
         This needs moving into the publication component, once it has been
         refactored.
         """
+        # Launchpad only produces ascii URLs.  If the name is not ascii, we
+        # can say nothing is found here.
+        if not is_ascii_only(name):
+            raise NotFound(self.context, name)
         nextobj = self._publishTraverse(request, name)
         getUtility(IOpenLaunchBag).add(nextobj)
         return nextobj
@@ -839,7 +844,7 @@
 
     def publishTraverse(self, request, name):
         """See zope.publisher.interfaces.browser.IBrowserPublisher."""
-        raise NotFound(name, self.context)
+        raise NotFound(self.context, name)
 
     def browserDefault(self, request):
         """See zope.publisher.interfaces.browser.IBrowserPublisher."""

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2011-02-04 14:41:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2011-04-07 05:15:25 +0000
@@ -21,12 +21,11 @@
 from storm.zope.interfaces import IZStorm
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
-from zope.interface import (
-    directlyProvides,
-    noLongerProvides,
+from zope.interface import directlyProvides
+from zope.publisher.interfaces import (
+    NotFound,
+    Retry,
     )
-from zope.publisher.interfaces import Retry
-from zope.publisher.interfaces.browser import IBrowserRequest
 
 from canonical.config import dbconfig
 from canonical.launchpad.database.emailaddress import EmailAddress
@@ -440,6 +439,32 @@
         maybe_block_offsite_form_post(request)
 
 
-def test_suite():
-    suite = unittest.TestLoader().loadTestsFromName(__name__)
-    return suite
+class TestEncodedReferer(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_not_found(self):
+        # The only oops should be a NotFound.
+        browser = self.getUserBrowser()
+        browser.addHeader('Referer', '/whut\xe7foo')
+        self.assertRaises(
+            NotFound,
+            browser.open,
+            'http://launchpad.dev/missing')
+        self.assertEqual(1, len(self.oopses))
+        self.assertEqual('NotFound', self.oopses[0].type)
+
+
+class TestUnicodePath(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_non_ascii_url(self):
+        # The only oops should be a NotFound.
+        browser = self.getUserBrowser()
+        self.assertRaises(
+            NotFound,
+            browser.open,
+            'http://launchpad.dev/%ED%B4%B5')
+        self.assertEqual(1, len(self.oopses))
+        self.assertEqual('NotFound', self.oopses[0].type)

=== modified file 'lib/lp/services/encoding.py'
--- lib/lp/services/encoding.py	2011-02-17 17:02:54 +0000
+++ lib/lp/services/encoding.py	2011-04-07 05:15:25 +0000
@@ -8,6 +8,7 @@
     'ascii_smash',
     'escape_nonascii_uniquely',
     'guess',
+    'is_ascii_only',
     ]
 
 import re
@@ -387,3 +388,23 @@
     def quote(match):
         return '\\x%x' % ord(match.group(0))
     return nonascii_regex.sub(quote, bogus_string)
+
+
+def is_ascii_only(string):
+    """Ensure that the string contains only ASCII characters.
+
+        >>> is_ascii_only(u'ascii only')
+        True
+        >>> is_ascii_only('ascii only')
+        True
+        >>> is_ascii_only('\xf4')
+        False
+        >>> is_ascii_only(u'\xf4')
+        False
+    """
+    try:
+        string.encode('ascii')
+    except UnicodeError:
+        return False
+    else:
+        return True

=== modified file 'lib/lp/services/mail/sendmail.py'
--- lib/lp/services/mail/sendmail.py	2010-12-14 18:54:38 +0000
+++ lib/lp/services/mail/sendmail.py	2011-04-07 05:15:25 +0000
@@ -50,9 +50,9 @@
 from zope.sendmail.interfaces import IMailDelivery
 
 from canonical.config import config
+from canonical.lp import isZopeless
 from lp.app import versioninfo
-from canonical.launchpad.helpers import is_ascii_only
-from canonical.lp import isZopeless
+from lp.services.encoding import is_ascii_only
 from lp.services.mail.stub import TestMailer
 from lp.services.timeline.requesttimeline import get_request_timeline
 

=== modified file 'lib/lp/services/mail/tests/test_sendmail.py'
--- lib/lp/services/mail/tests/test_sendmail.py	2010-09-04 05:37:08 +0000
+++ lib/lp/services/mail/tests/test_sendmail.py	2011-04-07 05:15:25 +0000
@@ -7,7 +7,7 @@
 from email.Message import Message
 import unittest
 
-from canonical.launchpad.helpers import is_ascii_only
+from lp.services.encoding import is_ascii_only
 from lp.services.mail import sendmail
 from lp.services.mail.sendmail import MailController
 from lp.testing import TestCase