launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/url-validation into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #819841 in Launchpad itself: "OOPs rendering page with non-ascii URL"
  https://bugs.launchpad.net/launchpad/+bug/819841
  Bug #819859 in Launchpad itself: "launchpad urls permit spurious whitespace"
  https://bugs.launchpad.net/launchpad/+bug/819859

For more details, see:
https://code.launchpad.net/~abentley/launchpad/url-validation/+merge/71068

= Summary =
Add validation to ensure traversal names are ascii, with no whitesppace
Fixes:
Bug #819841: OOPs rendering page with non-ascii URL
Bug #819859: launchpad urls permit spurious whitespace

== Proposed fix ==
Add validation as the ultimate step of beforeTraversal.

== Pre-implementation notes ==
Discussed with lifeless and gary_poster

== Implementation details ==
Traversal is done using the request's "traversalStack", which are
url-unescaped, unicode path fragments.

It must be done as the final step to ensure the environment is all set up.  Otherwise, the normal Launchpad error handling won't work.

== Tests ==
bin/test test_publication

== Demo and Q/A ==
Visit
https://bugs.qastaging.launchpad.net/launchpad/+bug/240067%20
https://bugs.qastaging.launchpad.net/launchpad/+bug/240067%C2%A0

Both urls should give the standard Launchpad "Lost something?" 404 page.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/webapp/publication.py
  lib/canonical/launchpad/webapp/tests/test_publication.py
-- 
https://code.launchpad.net/~abentley/launchpad/url-validation/+merge/71068
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/url-validation into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2011-07-01 15:36:23 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2011-08-10 15:44:47 +0000
@@ -45,6 +45,7 @@
     )
 from zope.publisher.interfaces import (
     IPublishTraverse,
+    NotFound,
     Retry,
     )
 from zope.publisher.interfaces.browser import (
@@ -83,6 +84,7 @@
     IPersonSet,
     ITeam,
     )
+from lp.services.encoding import is_ascii_only
 from lp.services import features
 from lp.services.features.flags import NullFeatureController
 from lp.services.osutils import open_for_writing
@@ -151,7 +153,7 @@
         # exception was added as a result of bug 597324 (message #10 in
         # particular).
         return
-    referrer = request.getHeader('referer') # match HTTP spec misspelling
+    referrer = request.getHeader('referer')  # match HTTP spec misspelling
     if not referrer:
         raise NoReferrerError('No value for REFERER header')
     # XXX: jamesh 2007-04-26 bug=98437:
@@ -308,6 +310,19 @@
         self.maybeRestrictToTeam(request)
         maybe_block_offsite_form_post(request)
         self.maybeNotifyReadOnlyMode(request)
+        if not self._validTraversalStack(request):
+            raise NotFound(self.getApplication(request), '')
+
+    @staticmethod
+    def _validTraversalStack(request):
+        """Elements of the traversal stack must be ascii non-whitespace."""
+        whitespace_re = re.compile(r'\s')
+        for element in request.getTraversalStack():
+            if not is_ascii_only(element):
+                return False
+            if whitespace_re.search(element):
+                return False
+        return True
 
     def maybeNotifyReadOnlyMode(self, request):
         """Hook to notify about read-only mode."""
@@ -533,7 +548,7 @@
         if request.method in ['GET', 'HEAD']:
             self.finishReadOnlyRequest(txn)
         elif txn.isDoomed():
-            txn.abort() # Sends an abort to the database, even though
+            txn.abort()  # Sends an abort to the database, even though
             # transaction is still doomed.
         else:
             txn.commit()
@@ -734,11 +749,11 @@
             if IBrowserRequest.providedBy(request):
                 OpStats.stats['http requests'] += 1
                 status = request.response.getStatus()
-                if status == 404: # Not Found
+                if status == 404:  # Not Found
                     OpStats.stats['404s'] += 1
-                elif status == 500: # Unhandled exceptions
+                elif status == 500:  # Unhandled exceptions
                     OpStats.stats['500s'] += 1
-                elif status == 503: # Timeouts
+                elif status == 503:  # Timeouts
                     OpStats.stats['503s'] += 1
 
                 # Increment counters for status code groups.

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2011-05-20 04:46:43 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2011-08-10 15:44:47 +0000
@@ -18,6 +18,7 @@
     )
 from storm.exceptions import DisconnectionError
 from storm.zope.interfaces import IZStorm
+from testtools.testcase import ExpectedException
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import directlyProvides
@@ -42,6 +43,7 @@
     remove_read_only_file,
     touch_read_only_file,
     )
+from canonical.launchpad.webapp import canonical_url
 import canonical.launchpad.webapp.adapter as dbadapter
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector,
@@ -68,6 +70,7 @@
     FunctionalLayer,
     )
 from lp.testing import (
+    logout,
     TestCase,
     TestCaseWithFactory,
     )
@@ -97,6 +100,30 @@
         publication.callTraversalHooks(request, obj2)
         self.assertEquals(request.traversed_objects, [obj1])
 
+    def test_validTraversalStack_ascii_no_space(self):
+        # ascii path with no whitespace is valid.
+        request = LaunchpadTestRequest(PATH_INFO="foo")
+        result = LaunchpadBrowserPublication._validTraversalStack(request)
+        self.assertTrue(result)
+
+    def test_validTraversalStack_space(self):
+        # ascii path with whitespace is invalid.
+        request = LaunchpadTestRequest(PATH_INFO="foo ")
+        result = LaunchpadBrowserPublication._validTraversalStack(request)
+        self.assertFalse(result)
+
+    def test_validTraversalStack_nonascii(self):
+        # path with non-ascii is invalid.
+        request = LaunchpadTestRequest(PATH_INFO=u"foo\xC0".encode('utf-8'))
+        result = LaunchpadBrowserPublication._validTraversalStack(request)
+        self.assertFalse(result)
+
+    def test_validTraversalStack_nonascii_space(self):
+        # path with non-ascii whitespace is invalid.
+        request = LaunchpadTestRequest(PATH_INFO=u"foo\xa0".encode('utf-8'))
+        result = LaunchpadBrowserPublication._validTraversalStack(request)
+        self.assertFalse(result)
+
 
 class TestReadOnlyModeSwitches(TestCase):
     # At the beginning of every request (in publication.beforeTraversal()), we
@@ -466,3 +493,26 @@
             browser.open,
             'http://launchpad.dev/%ED%B4%B5')
         self.assertEqual(0, len(self.oopses))
+
+    def test_beforeTraversal_non_ascii_url(self):
+        # The NotFound is raised by beforeTraversal
+        publication = LaunchpadBrowserPublication(None)
+        request = LaunchpadTestRequest(PATH_INFO='\xED\xB4\xB5')
+        logout()
+        with ExpectedException(NotFound, ''):
+            publication.beforeTraversal(request)
+        # Remove database policy created by beforeTraversal
+        getUtility(IStoreSelector).pop()
+
+
+class TestWhitespacePath(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_whitespace_bugtask(self):
+        # Bug tasks should not permit whitespace in their number.
+        bugtask = self.factory.makeBugTask()
+        url = canonical_url(bugtask) + '%20'
+        browser = self.getUserBrowser()
+        with ExpectedException(NotFound, ''):
+            browser.open(url)