launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/oops-polish into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #885974 in Launchpad itself: "attach_http_request injects file upload objects into OOPSes"
  https://bugs.launchpad.net/launchpad/+bug/885974
  Bug #888866 in Launchpad itself: "req_vars is documented as a list of tuples"
  https://bugs.launchpad.net/launchpad/+bug/888866
  Bug #890975 in Launchpad itself: "OOPS report urls are sent as unicode"
  https://bugs.launchpad.net/launchpad/+bug/890975
  Bug #890976 in Launchpad itself: "hard to determine causes of late evaluation"
  https://bugs.launchpad.net/launchpad/+bug/890976
  Bug #896959 in Launchpad itself: "UnicodeDecodeError: 'utf8' codec can't decode byte 0xad in position 7: unexpected code byte on some bson reports"
  https://bugs.launchpad.net/launchpad/+bug/896959
  Bug #897039 in Launchpad itself: "+login failures to handle non-utf8 query param keys"
  https://bugs.launchpad.net/launchpad/+bug/897039

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/oops-polish/+merge/83544

Two unrelated fixes; the one causing the oopses that were not importing on lp-oopses, and the serialisation of said oopses.

I stumbled on the first and fixed it in passing.
-- 
https://code.launchpad.net/~lifeless/launchpad/oops-polish/+merge/83544
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/oops-polish into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2011-11-17 01:28:19 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-11-28 03:10:57 +0000
@@ -234,6 +234,11 @@
             value = '<hidden>'
         if not isinstance(value, basestring):
             value = oops.createhooks.safe_unicode(value)
+        # keys need to be unicode objects. The form items (a subset of
+        # request.items) are generally just the url query_string url decoded,
+        # which means the keys may be invalid in bson docs (bson requires that
+        # they be unicode).
+        key = oops.createhooks.safe_unicode(key)
         report['req_vars'][key] = value
     if IXMLRPCRequest.providedBy(request):
         args = request.getPositionalArguments()

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2011-09-18 18:37:58 +0000
+++ lib/canonical/launchpad/webapp/login.py	2011-11-28 03:10:57 +0000
@@ -10,7 +10,6 @@
     )
 import urllib
 
-from BeautifulSoup import UnicodeDammit
 from openid.consumer.consumer import (
     CANCEL,
     Consumer,
@@ -252,6 +251,10 @@
 
         Exclude things such as 'loggingout' and starting with 'openid.', which
         we don't want.
+
+        Coerces all keys and values to be ascii decode safe - either by making
+        them unicode or by url quoting them. keys are known to be urldecoded 
+        bytestrings, so are simply re urlencoded.
         """
         for name, value in self.request.form.items():
             if name == 'loggingout' or name.startswith('openid.'):
@@ -261,9 +264,12 @@
             else:
                 value_list = [value]
             for value_list_item in value_list:
-                # Thanks to apport (https://launchpad.net/bugs/61171), we need
-                # to do this here.
-                value_list_item = UnicodeDammit(value_list_item).markup
+                # The form items are byte strings simply url decoded. They may
+                # fail to coerce to unicode as they can include arbitrary
+                # bytesequences after url decoding. We can restore their
+                # original url value by url quoting them again.
+                value_list_item = urllib.quote(value_list_item)
+                name = urllib.quote(name)
                 yield "%s=%s" % (name, value_list_item)
 
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-11-17 01:03:35 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-11-28 03:10:57 +0000
@@ -262,6 +262,22 @@
             report = utility.raising(sys.exc_info(), request)
         self.assertEqual("(1, 2)", report['req_vars']['xmlrpc args'])
 
+    def test_raising_non_utf8_request_param_key_bug_896959(self):
+        # When a form has a nonutf8 request param, the key in req_vars must
+        # still be unicode (or utf8).
+        request = TestRequest(form={'foo\x85': 'bar'})
+        utility = ErrorReportingUtility()
+        del utility._oops_config.publishers[:]
+        try:
+            raise ArbitraryException('foo')
+        except ArbitraryException:
+            report = utility.raising(sys.exc_info(), request)
+        for key in report['req_vars'].keys():
+            if isinstance(key, str):
+                key.decode('utf8')
+            else:
+                self.assertIsInstance(key, unicode)
+
     def test_raising_with_webservice_request(self):
         # Test ErrorReportingUtility.raising() with a WebServiceRequest
         # request. Only some exceptions result in OOPSes.

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2011-10-27 11:36:13 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2011-11-28 03:10:57 +0000
@@ -19,7 +19,9 @@
     )
 import httplib
 import unittest
+import urllib
 import urllib2
+import urlparse
 
 import mechanize
 from openid.consumer.consumer import (
@@ -28,6 +30,7 @@
     )
 from openid.extensions import sreg
 from openid.yadis.discover import DiscoveryFailure
+from testtools.matchers import Contains
 from zope.component import getUtility
 from zope.security.management import newInteraction
 from zope.security.proxy import removeSecurityProxy
@@ -661,22 +664,46 @@
         return FakeOpenIDConsumer()
 
 
+class ForwardsCorrectly:
+    """Match query_strings which get forwarded correctly.
+
+    Correctly is defined as the form parameters ending up simply urllib quoted
+    wrapped in the return_to url.
+    """
+
+    def match(self, query_string):
+        args = dict(urlparse.parse_qsl(query_string))
+        request = LaunchpadTestRequest(form=args)
+        # This is a hack to make the request.getURL(1) call issued by the view
+        # not raise an IndexError.
+        request._app_names = ['foo']
+        view = StubbedOpenIDLogin(object(), request)
+        view()
+        escaped_args = tuple(map(urllib.quote, args.items()[0]))
+        expected_fragment = urllib.quote('%s=%s' % escaped_args)
+        return Contains(
+            expected_fragment).match(view.openid_request.return_to)
+
+    def __str__(self):
+        return 'ForwardsCorrectly()'
+
+
 class TestOpenIDLogin(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
-    def test_return_to_with_non_ascii_chars(self):
-        # Sometimes the +login link will have non-ascii characters in the
-        # query string, and we need to include those in the return_to URL that
-        # we pass to the OpenID provider, so we must utf-encode them.
-        request = LaunchpadTestRequest(
-            form={'non_ascii_field': 'subproc\xc3\xa9s'})
-        # This is a hack to make the request.getURL(1) call issued by the view
-        # not raise an IndexError.
-        request._app_names = ['foo']
-        view = StubbedOpenIDLogin(object(), request)
-        view()
-        self.assertIn(
-            'non_ascii_field%3Dsubproc%C3%A9s', view.openid_request.return_to)
+    def test_return_to_with_non_ascii_value_bug_61171(self):
+        # Sometimes the +login link will have non-ascii characters in the
+        # query param values, and we need to include those in the return_to URL
+        # that we pass to the OpenID provider. The params may not be legimate
+        # utf8 even.
+        self.assertThat('key=value\x85', ForwardsCorrectly())
+
+    def test_return_to_with_non_ascii_key_bug_897039(self):
+        # Sometimes the +login link will have non-ascii characters in the
+        # query param keys, and we need to include those in the return_to URL
+        # that we pass to the OpenID provider. The params may not be legimate
+        # utf8 even.
+        self.assertThat('key\x85=value', ForwardsCorrectly())
 
     def test_sreg_fields(self):
         # We request the user's email address and Full Name (through the SREG