Merge lp:~salgado/launchpad/bug-61171 into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

Non-ASCII characters in an ASCII-encoded (and escaped) query string
cause our +login page to OOPS: https://launchpad.net/bugs/61171

== Proposed fix ==

Fix the bug by replacing the non-ASCII characters when decoding them
into unicode. That seems to be the only thing we can do.

I was hoping to test it at the view level, exercising just the method we
care about but that didn't work and I don't want to spend much time on
this, so I wrote a pagetest using testbrowser.

== Tests ==

./bin/test -vvt test_login

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/webapp/login.py
  lib/canonical/launchpad/webapp/tests/test_login.py

[Leonard Richardson (leonardr)]

This is a good start, but stripping non-ASCII characters is an overzealous way to handle clients that generating URLs containing non-utf8 values. (In this case the encoding is windows-1252 or iso-8859-1.) This is bad behavior on the client's part, but we can do better than forcing everything to ASCII, especially since windows-1252 and iso-8859-1 are likely to be the vast majority of the problematic encodings.

It would be better to sniff the encoding (I recommend UnicodeDammit, which comes with Beautiful Soup, but you can just try to .decode with a bunch of different encodings) and only force the data to ASCII if the sniffing fails.

I recommend putting this code in a helper method that can be used at any Launchpad entry point used by a non-browser client.

This is why you're seeing this oops a lot from specific clients (apport and TenCent Traveler) -- those clients are programmed to use some encoding other than utf-8 for everything.

[Guilherme Salgado (salgado)]

Thanks for the help with this branch, Leonard.

On Thu, 2009-07-30 at 20:03 +0000, Leonard Richardson wrote:
> This is a good start, but stripping non-ASCII characters is an
> overzealous way to handle clients that generating URLs containing
> non-utf8 values. (In this case the encoding is windows-1252 or
> iso-8859-1.) This is bad behavior on the client's part, but we can do
> better than forcing everything to ASCII, especially since windows-1252
> and iso-8859-1 are likely to be the vast majority of the problematic
> encodings.

Agreed.
>
> It would be better to sniff the encoding (I recommend UnicodeDammit,
> which comes with Beautiful Soup, but you can just try to .decode with
> a bunch of different encodings) and only force the data to ASCII if
> the sniffing fails.

I've changed my implementation to use UnicodeDammit
>
> I recommend putting this code in a helper method that can be used at
> any Launchpad entry point used by a non-browser client.

I'll leave that for later, when the need arises.
>
> This is why you're seeing this oops a lot from specific clients
> (apport and TenCent Traveler) -- those clients are programmed to use

I think apport uses what is specified on the user's locale but anyway.

I'm including the incremental diff with this change. Please let me know
how it looks like now.

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py 2009-07-30 18:00:42 +0000
+++ lib/canonical/launchpad/webapp/login.py 2009-07-30 19:56:28 +0000
@@ -9,6 +9,8 @@
 import urllib
 from datetime import datetime, timedelta

+from BeautifulSoup import UnicodeDammit
+
 from zope.component import getUtility
 from zope.session.interfaces import ISession, IClientIdManager
 from zope.event import notify
@@ -380,7 +382,8 @@
         for name, value in self.iter_form_items():
             # Thanks to apport (https://launchpad.net/bugs/61171), we need to
             # do this here.
- value = value.decode('ascii', 'replace')
+ dammit = UnicodeDammit(value)
+ value = value.decode(dammit.originalEncoding, 'replace')
             L.append(html % (name, cgi.escape(value, quote=True)))
         return '\n'.join(L)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py 2009-07-30 18:00:42 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py 2009-07-30 20:13:46 +0000
@@ -19,14 +19,15 @@
         # Apport can construct ASCII-encoded URLs containing non-ASCII
         # characters (in the query string), where they send users to so that
         # they can finish reporting bugs. Apport shouldn't do that but we
- # can't OOPS when it does, so we decode the query string back and
- # replace those non-ASCII characters. For more details, see
- # https://launchpad.net/bugs/61171.
+ # can't OOPS when it does, so we use UnicodeDammit to figure out its
+ # original encoding and decode it back. If UnicodeDammit can't figure
+ # out the correct encoding, we replace those non-ASCII characters. For
+ # more details, see https://launchpad.net/bugs/61171.
       ...

[Leonard Richardson (leonardr)]

This is much better, but there's no need to call value.decode. UnicodeDammit makes the Unicode string available as .markup. So approved* if you use that instead of decoding again.

review: Approve

[Paul Hummer (rockstar)]

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/login.py	2009-07-30 18:00:42 +0000
@@ -376,11 +376,12 @@
     def preserve_query(self):
         """Return zero or more hidden inputs that preserve the URL's query."""
         L = []
+        html = '<input type="hidden" name="%s" value="%s" />'
         for name, value in self.iter_form_items():
-            L.append('<input type="hidden" name="%s" value="%s" />' % (
-                name, cgi.escape(value, quote=True)
-                ))
-
+            # Thanks to apport (https://launchpad.net/bugs/61171), we need to
+            # do this here.
+            value = value.decode('ascii', 'replace')
+            L.append(html % (name, cgi.escape(value, quote=True)))
         return '\n'.join(L)
 
 

=== added file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2009-07-30 18:00:42 +0000
@@ -0,0 +1,33 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+
+from canonical.launchpad.ftests import logout
+from canonical.launchpad.testing.pages import setupBrowser
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.testing import TestCase
+
+
+class TestLoginOrRegister_preserve_query(TestCase):
+    layer = DatabaseFunctionalLayer
+
+    def test_non_ascii_characters_in_query_string_ascii_encoded(self):
+        # Apport can construct ASCII-encoded URLs containing non-ASCII
+        # characters (in the query string), where they send users to so that
+        # they can finish reporting bugs. Apport shouldn't do that but we
+        # can't OOPS when it does, so we decode the query string back and
+        # replace those non-ASCII characters. For more details, see
+        # https://launchpad.net/bugs/61171.
+        logout()
+        browser = setupBrowser()
+        browser.open('http://launchpad.dev/+login?foo=subproc%E9s')
+        self.assertIn(
+            'subproc', browser.getControl(name='foo', index=0).value)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)