Merge lp:~benji/launchpad/bug-597324 into lp:launchpad

[Benji York]

Fix a unicode sin: use the decoded query parameters instead of creating
new, undecoded copies.

This /should/ fix the OOPS introduced in on edge by this branch (see bug
609029).

I also added a test to ensure this particular failure doesn't reoccur.

[Gary Poster (gary)]

merge-conditional

Wow, great find!

Also, your cover letter neglected to mention that this fixes the problems discovered in QA for the original bug, but your commit messages clarify it. :-)

- Please file a bug against canonical-openid-provider about removing the additional "csrfmiddlewaretoken" field and add a reference to the bug at the top of the pertinent comment below (e.g., something like ``# XXX benji 2010-07-23 bug=12345``)

- I don't love the assert error for multi-valued fields. It feels like it ought to be a real exception that has a chance of being handled by one of our error views. However, the right error doesn't jump out at me, so I'll let it go unless you see where I'm coming from and have a good idea.

Thank you!

Gary

review: Approve

[Benji York (benji)]

On Fri, Jul 23, 2010 at 5:52 PM, Gary Poster <email address hidden> wrote:
> - Please file a bug against canonical-openid-provider about removing
> the additional "csrfmiddlewaretoken" field and add a reference to the
> bug at the top of the pertinent comment below (e.g., something like
> ``# XXX benji 2010-07-23 bug=12345``)

https://bugs.edge.launchpad.net/canonical-identity-provider/+bug/608920

> - I don't love the assert error for multi-valued fields.  It feels
> like it ought to be a real exception that has a chance of being
> handled by one of our error views.  However, the right error doesn't
> jump out at me, so I'll let it go unless you see where I'm coming from
> and have a good idea.

Switched to a ValueError in r11111.
--
Benji York

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-07-21 13:32:53 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-07-24 13:17:56 +0000
@@ -248,9 +248,21 @@
 
     def _gather_params(self, request):
         params = dict(request.form)
-        query_string = request.get('QUERY_STRING')
-        if query_string is not None:
-            params.update(cgi.parse_qsl(query_string))
+        for key, value in request.query_string_params.iteritems():
+            if len(value) > 1:
+                raise ValueError(
+                    'Did not expect multi-valued fields.')
+            params[key] = value[0]
+
+        # XXX benji 2010-07-23 bug=608920
+        # The production OpenID provider has some Django middleware that
+        # generates a token used to prevent XSRF attacks and stuffs it into
+        # every form.  Unfortunately that includes forms that have off-site
+        # targets and since our OpenID client verifies that no form values have
+        # been injected as a security precaution, this breaks logging-in in
+        # certain circumstances (see bug 597324).  The best we can do at the
+        # moment is to remove the token before invoking the OpenID library.
+        params.pop('csrfmiddlewaretoken', None)
         return params
 
     def _get_requested_url(self, request):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-07-21 13:32:53 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-07-24 13:17:56 +0000
@@ -187,6 +187,48 @@
         }
         self.assertEquals(params, expected_params)
 
+    def test_gather_params_with_unicode_data(self):
+        # If the currently requested URL includes a query string, the
+        # parameters in the query string will be included when constructing
+        # the params mapping (which is then used to complete the open ID
+        # response) and if there are non-ASCII characters in the query string,
+        # they are properly decoded.
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://example.com',
+            QUERY_STRING='foo=%E1%9B%9D',
+            environ={'PATH_INFO': '/'})
+        view = OpenIDCallbackView(context=None, request=None)
+        params = view._gather_params(request)
+        self.assertEquals(params['foo'], u'\u16dd')
+
+    def test_unexpected_multivalue_fields(self):
+        # The parameter gatering doesn't expect to find multi-valued form
+        # field and it reports an error if it finds any.
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://example.com',
+            QUERY_STRING='foo=1&foo=2',
+            environ={'PATH_INFO': '/'})
+        view = OpenIDCallbackView(context=None, request=None)
+        self.assertRaises(ValueError, view._gather_params, request)
+
+    def test_csrfmiddlewaretoken_is_ignored(self):
+        # Show that the _gather_params filters out the errant
+        # csrfmiddlewaretoken form field.  See comment in _gather_params for
+        # more info.
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://example.com',
+            QUERY_STRING='foo=bar',
+            form={'starting_url': 'http://launchpad.dev/after-login',
+                'csrfmiddlewaretoken': '12345'},
+            environ={'PATH_INFO': '/'})
+        view = OpenIDCallbackView(context=None, request=None)
+        params = view._gather_params(request)
+        expected_params = {
+            'starting_url': 'http://launchpad.dev/after-login',
+            'foo': 'bar',
+        }
+        self.assertEquals(params, expected_params)
+
     def test_get_requested_url(self):
         # The OpenIDCallbackView needs to pass the currently-being-requested
         # URL to the OpenID library.  OpenIDCallbackView._get_requested_url