Merge lp:~stevenk/launchpad/handle-invalid-unicode-in-query-string-redux into lp:launchpad

[Steve Kowalik]

If we fail to decode the query string parameters, forcibly decode it to utf-8 with replacements, again. We also work around a Zope bug by grabbing the request and pulling the un-decoded file contents out for IProductReleaseFile.add_file and IBug.addAttachment.

This branch resurrects the changes from r1646[12] which were reverted due to discovering that lazr.restfulclient does not set filename on file uploads, so Zope will encode the contents, resulting in garbage being stored.

We work around this by grabbing the current request in the two exported methods (IProductRelease.add_file, and IBug.addAttachment) and reaching into the body and pulling out the un-encoded file contents and using that instead.

I have also ripped out the export of get_current_browser_request in lp.services.webapp.publisher, and fixed all imports that were expecting it there to pull from lazr.restful.utils, as well as some whitespace cleanups.

[William Grant (wgrant)]

You'll want to also apply the hack to signature_content, but otherwise fine.

review: Approve (code)

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2013-01-30 05:31:20 +0000
+++ lib/lp/app/browser/tales.py	2013-02-07 01:27:23 +0000
@@ -19,6 +19,7 @@
 import urllib
 
 from lazr.enum import enumerated_type_registry
+from lazr.restful.utils import get_current_browser_request
 from lazr.uri import URI
 import pytz
 from z3c.ptcompat import ViewPageTemplateFile
@@ -94,7 +95,6 @@
     get_facet,
     )
 from lp.services.webapp.publisher import (
-    get_current_browser_request,
     LaunchpadView,
     nearest,
     )

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2013-01-07 02:40:55 +0000
+++ lib/lp/bugs/interfaces/bug.py	2013-02-07 01:27:23 +0000
@@ -685,14 +685,14 @@
 class IBugEdit(Interface):
     """IBug attributes that require launchpad.Edit permission."""
 
-    @call_with(owner=REQUEST_USER)
+    @call_with(owner=REQUEST_USER, from_api=True)
     @operation_parameters(
         data=Bytes(constraint=attachment_size_constraint),
         comment=Text(), filename=TextLine(), is_patch=Bool(),
         content_type=TextLine(), description=Text())
     @export_factory_operation(IBugAttachment, [])
     def addAttachment(owner, data, comment, filename, is_patch=False,
-                      content_type=None, description=None):
+                      content_type=None, description=None, from_api=False):
         """Attach a file to this bug.
 
         :owner: An IPerson.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-12-20 14:55:13 +0000
+++ lib/lp/bugs/model/bug.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Launchpad bug-related database table classes."""
@@ -226,6 +226,7 @@
     )
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import ILaunchBag
+from lp.services.webapp.publisher import get_raw_form_value_from_current_request
 
 
 def snapshot_bug_params(bug_params):
@@ -1250,8 +1251,13 @@
         bug_watch.destroySelf()
 
     def addAttachment(self, owner, data, comment, filename, is_patch=False,
-                      content_type=None, description=None):
+                      content_type=None, description=None, from_api=False):
         """See `IBug`."""
+        # XXX: StevenK 2013-02-06 bug=1116954: We should not need to refetch
+        # the file content from the request, since the passed in one has been
+        # wrongly encoded.
+        if from_api:
+            data = get_raw_form_value_from_current_request('data')
         if isinstance(data, str):
             filecontent = data
         else:

=== modified file 'lib/lp/registry/interfaces/productrelease.py'
--- lib/lp/registry/interfaces/productrelease.py	2013-01-25 05:25:34 +0000
+++ lib/lp/registry/interfaces/productrelease.py	2013-02-07 01:27:23 +0000
@@ -224,7 +224,7 @@
 class IProductReleaseEditRestricted(Interface):
     """`IProductRelease` properties which require `launchpad.Edit`."""
 
-    @call_with(uploader=REQUEST_USER)
+    @call_with(uploader=REQUEST_USER, from_api=True)
     @operation_parameters(
         filename=TextLine(),
         signature_filename=TextLine(),
@@ -232,16 +232,14 @@
         file_content=Bytes(constraint=productrelease_file_size_constraint),
         signature_content=Bytes(
             constraint=productrelease_signature_size_constraint),
-        file_type=copy_field(IProductReleaseFile['filetype'], required=False)
-        )
-    @export_factory_operation(
-        IProductReleaseFile, ['description'])
+        file_type=copy_field(IProductReleaseFile['filetype'], required=False))
+    @export_factory_operation(IProductReleaseFile, ['description'])
     @export_operation_as('add_file')
     def addReleaseFile(filename, file_content, content_type,
                        uploader, signature_filename=None,
                        signature_content=None,
                        file_type=UpstreamFileType.CODETARBALL,
-                       description=None):
+                       description=None, from_api=False):
         """Add file to the library and link to this `IProductRelease`.
 
         The signature file will also be added if available.

=== modified file 'lib/lp/registry/model/productrelease.py'
--- lib/lp/registry/model/productrelease.py	2013-01-25 05:25:34 +0000
+++ lib/lp/registry/model/productrelease.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -56,6 +56,7 @@
     )
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.propertycache import cachedproperty
+from lp.services.webapp.publisher import get_raw_form_value_from_current_request
 
 
 class ProductRelease(SQLBase):
@@ -148,7 +149,7 @@
                        uploader, signature_filename=None,
                        signature_content=None,
                        file_type=UpstreamFileType.CODETARBALL,
-                       description=None):
+                       description=None, from_api=False):
         """See `IProductRelease`."""
         if not self.can_have_release_files:
             raise ProprietaryProduct(
@@ -157,31 +158,35 @@
             raise InvalidFilename
         # Create the alias for the file.
         filename = self.normalizeFilename(filename)
+        # XXX: StevenK 2013-02-06 bug=1116954: We should not need to refetch
+        # the file content from the request, since the passed in one has been
+        # wrongly encoded.
+        if from_api:
+            file_content = get_raw_form_value_from_current_request(
+                'file_content')
         file_obj, file_size = self._getFileObjectAndSize(file_content)
 
         alias = getUtility(ILibraryFileAliasSet).create(
-            name=filename,
-            size=file_size,
-            file=file_obj,
+            name=filename, size=file_size, file=file_obj,
             contentType=content_type)
         if signature_filename is not None and signature_content is not None:
+            # XXX: StevenK 2013-02-06 bug=1116954: We should not need to 
+            # refetch the file content from the request, since the passed in
+            # one has been wrongly encoded.
+            if from_api:
+                signature_content = get_raw_form_value_from_current_request(
+                    'signature_content')
             signature_obj, signature_size = self._getFileObjectAndSize(
                 signature_content)
-            signature_filename = self.normalizeFilename(
-                signature_filename)
+            signature_filename = self.normalizeFilename(signature_filename)
             signature_alias = getUtility(ILibraryFileAliasSet).create(
-                name=signature_filename,
-                size=signature_size,
-                file=signature_obj,
-                contentType='application/pgp-signature')
+                name=signature_filename, size=signature_size,
+                file=signature_obj, contentType='application/pgp-signature')
         else:
             signature_alias = None
-        return ProductReleaseFile(productrelease=self,
-                                  libraryfile=alias,
-                                  signature=signature_alias,
-                                  filetype=file_type,
-                                  description=description,
-                                  uploader=uploader)
+        return ProductReleaseFile(
+            productrelease=self, libraryfile=alias, signature=signature_alias,
+            filetype=file_type, description=description, uploader=uploader)
 
     def getFileAliasByName(self, name):
         """See `IProductRelease`."""

=== modified file 'lib/lp/registry/stories/webservice/xx-project-registry.txt'
--- lib/lp/registry/stories/webservice/xx-project-registry.txt	2012-12-10 13:43:47 +0000
+++ lib/lp/registry/stories/webservice/xx-project-registry.txt	2013-02-07 01:27:23 +0000
@@ -1020,8 +1020,8 @@
 
     >>> pr_url = '/firefox/1.0/1.0.0'
     >>> ff_100 = webservice.get(pr_url).jsonBody()
-    >>> file_content="first attachment file content"
-    >>> sig_file_content="hash hash hash"
+    >>> file_content="first attachment file content \xff"
+    >>> sig_file_content="hash hash hash \xff"
     >>> response = webservice.named_post(ff_100['self_link'], 'add_file',
     ...     filename='filename.txt',
     ...     file_content=file_content,
@@ -1043,6 +1043,20 @@
     >>> print_self_link_of_entries(ff_100_files)
     http://.../firefox/1.0/1.0.0/+file/filename.txt
 
+And it has been uploaded correctly.
+
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.product import IProductSet
+    >>> from lp.testing import login, logout
+    >>> login('[e-mail]')
+    >>> concrete_one_zero = getUtility(IProductSet)['firefox'].getRelease(
+    ...     '1.0.0')
+    >>> concrete_one_zero.files[0].libraryfile.read() == file_content
+    True
+    >>> concrete_one_zero.files[0].signature.read() == sig_file_content
+    True
+    >>> logout()
+
 The file type and description are optional.  If no signature is
 available then it must be explicitly set to None.
 
@@ -1106,12 +1120,8 @@
 If a project has a commercial-use subscription then it can be retrieved
 through the API.
 
-    >>> from zope.component import getUtility
-    >>> from lp.registry.interfaces.product import IProductSet
-    >>> from lp.testing import login, logout
     >>> login('[e-mail]')
-    >>> product_set = getUtility(IProductSet)
-    >>> mmm = product_set.getByName('mega-money-maker')
+    >>> mmm = getUtility(IProductSet)['mega-money-maker']
     >>> print mmm.commercial_subscription
     None
 

=== modified file 'lib/lp/registry/subscribers.py'
--- lib/lp/registry/subscribers.py	2012-11-26 08:40:20 +0000
+++ lib/lp/registry/subscribers.py	2013-02-07 01:27:23 +0000
@@ -11,6 +11,7 @@
 from datetime import datetime
 import textwrap
 
+from lazr.restful.utils import get_current_browser_request
 import pytz
 from zope.security.proxy import removeSecurityProxy
 
@@ -23,10 +24,7 @@
     simple_sendmail,
     )
 from lp.services.webapp.escaping import structured
-from lp.services.webapp.publisher import (
-    canonical_url,
-    get_current_browser_request,
-    )
+from lp.services.webapp.publisher import canonical_url
 
 
 def product_licenses_modified(product, event):

=== modified file 'lib/lp/registry/tests/test_subscribers.py'
--- lib/lp/registry/tests/test_subscribers.py	2012-12-10 13:43:47 +0000
+++ lib/lp/registry/tests/test_subscribers.py	2013-02-07 01:27:23 +0000
@@ -7,6 +7,7 @@
 
 from datetime import datetime
 
+from lazr.restful.utils import get_current_browser_request
 import pytz
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -23,7 +24,6 @@
     product_licenses_modified,
     )
 from lp.services.webapp.escaping import html_escape
-from lp.services.webapp.publisher import get_current_browser_request
 from lp.testing import (
     login_person,
     logout,

=== modified file 'lib/lp/services/webapp/escaping.py'
--- lib/lp/services/webapp/escaping.py	2012-12-10 22:25:44 +0000
+++ lib/lp/services/webapp/escaping.py	2013-02-07 01:27:23 +0000
@@ -8,6 +8,7 @@
     'structured',
     ]
 
+from lazr.restful.utils import get_current_browser_request
 from zope.i18n import (
     Message,
     translate,
@@ -15,7 +16,6 @@
 from zope.interface import implements
 
 from lp.services.webapp.interfaces import IStructuredString
-from lp.services.webapp.publisher import get_current_browser_request
 
 
 HTML_REPLACEMENTS = (

=== modified file 'lib/lp/services/webapp/menu.py'
--- lib/lp/services/webapp/menu.py	2012-12-12 04:59:52 +0000
+++ lib/lp/services/webapp/menu.py	2013-02-07 01:27:23 +0000
@@ -21,6 +21,7 @@
 import types
 
 from lazr.delegates import delegates
+from lazr.restful.utils import get_current_browser_request
 from lazr.uri import (
     InvalidURIError,
     URI,
@@ -45,7 +46,6 @@
     )
 from lp.services.webapp.publisher import (
     canonical_url,
-    get_current_browser_request,
     LaunchpadView,
     UserAttributeCache,
     )

=== modified file 'lib/lp/services/webapp/pgsession.py'
--- lib/lp/services/webapp/pgsession.py	2012-01-01 03:00:09 +0000
+++ lib/lp/services/webapp/pgsession.py	2013-02-07 01:27:23 +0000
@@ -9,6 +9,7 @@
 import time
 from UserDict import DictMixin
 
+from lazr.restful.utils import get_current_browser_request
 from storm.zope.interfaces import IZStorm
 from zope.app.security.interfaces import IUnauthenticatedPrincipal
 from zope.component import getUtility
@@ -21,7 +22,6 @@
     )
 
 from lp.services.helpers import ensure_unicode
-from lp.services.webapp.publisher import get_current_browser_request
 
 
 SECONDS = 1

=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py	2012-11-26 18:44:34 +0000
+++ lib/lp/services/webapp/publisher.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Publisher of objects as web pages.
@@ -8,13 +8,13 @@
 __metaclass__ = type
 __all__ = [
     'DataDownloadView',
+    'get_raw_form_value_from_current_request',
     'LaunchpadContainer',
     'LaunchpadView',
     'LaunchpadXMLRPCView',
     'canonical_name',
     'canonical_url',
     'canonical_url_iterator',
-    'get_current_browser_request',
     'nearest',
     'Navigation',
     'rootObject',
@@ -26,6 +26,7 @@
     'UserAttributeCache',
     ]
 
+from cgi import FieldStorage
 import httplib
 import re
 
@@ -800,6 +801,23 @@
         return None
 
 
+def get_raw_form_value_from_current_request(field_name):
+    # XXX: StevenK 2013-02-06 bug=1116954: We should not need to refetch
+    # the file content from the request, since the passed in one has been
+    # wrongly encoded.
+    # Circular imports.
+    from lp.services.webapp.servers import WebServiceClientRequest
+    request = get_current_browser_request()
+    assert isinstance(request, WebServiceClientRequest)
+    # Zope wrongly encodes any form element that doesn't look like a file,
+    # so re-fetch the file content if it has been encoded.
+    if request and request.form.has_key(field_name) and isinstance(
+        request.form[field_name], unicode):
+        request._environ['wsgi.input'].seek(0)
+        fs = FieldStorage(fp=request._body_instream, environ=request._environ)
+        return fs[field_name].value
+
+
 class RootObject:
     implements(ILaunchpadApplication, ILaunchpadRoot)
 

=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py	2013-02-03 01:49:37 +0000
+++ lib/lp/services/webapp/servers.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Definition of the internet servers that Launchpad uses."""
@@ -18,6 +18,7 @@
     WebServicePublicationMixin,
     WebServiceRequestTraversal,
     )
+from lazr.restful.utils import get_current_browser_request
 from lazr.uri import URI
 import transaction
 from transaction.interfaces import ISynchronizer
@@ -100,10 +101,7 @@
     )
 from lp.services.webapp.opstats import OpStats
 from lp.services.webapp.publication import LaunchpadBrowserPublication
-from lp.services.webapp.publisher import (
-    get_current_browser_request,
-    RedirectionView,
-    )
+from lp.services.webapp.publisher import RedirectionView
 from lp.services.webapp.vhosts import allvhosts
 from lp.services.webservice.interfaces import IWebServiceApplication
 from lp.testopenid.interfaces.server import ITestOpenIDApplication
@@ -657,6 +655,15 @@
         """As per zope.publisher.browser.BrowserRequest._createResponse"""
         return LaunchpadBrowserResponse()
 
+    def _decode(self, text):
+        text = super(LaunchpadBrowserRequest, self)._decode(text)
+        if isinstance(text, str):
+            # BrowserRequest._decode failed to do so with the user-specified
+            # charsets, so decode as UTF-8 with replacements, since we always
+            # want unicode.
+            text = unicode(text, 'utf-8', 'replace')
+        return text
+
     @cachedproperty
     def form_ng(self):
         """See ILaunchpadBrowserApplicationRequest."""

=== modified file 'lib/lp/services/webapp/tests/test_menu.py'
--- lib/lp/services/webapp/tests/test_menu.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/webapp/tests/test_menu.py	2013-02-07 01:27:23 +0000
@@ -3,6 +3,7 @@
 
 __metaclass__ = type
 
+from lazr.restful.utils import get_current_browser_request
 from zope.security.management import newInteraction
 
 from lp.services.webapp.menu import (
@@ -10,7 +11,6 @@
     MENU_ANNOTATION_KEY,
     MenuBase,
     )
-from lp.services.webapp.publisher import get_current_browser_request
 from lp.testing import (
     ANONYMOUS,
     login,
@@ -77,7 +77,7 @@
         login(ANONYMOUS)
         context = object()
         menu = TestMenu(context)
-        link = menu._get_link('test_link')
+        menu._get_link('test_link')
         cache = get_current_browser_request().annotations.get(
             MENU_ANNOTATION_KEY)
         self.assertEquals(len(cache.keys()), 1)

=== modified file 'lib/lp/services/webapp/tests/test_servers.py'
--- lib/lp/services/webapp/tests/test_servers.py	2013-02-03 01:49:37 +0000
+++ lib/lp/services/webapp/tests/test_servers.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -50,6 +50,7 @@
     EventRecorder,
     TestCase,
     )
+from lp.testing.layers import FunctionalLayer
 
 
 class SetInWSGIEnvironmentTestCase(TestCase):
@@ -230,8 +231,7 @@
 
         for method in denied_methods:
             env = self.wsgi_env(self.non_api_path, method)
-            self.assert_(self.factory.canHandle(env),
-                "Sanity check")
+            self.assert_(self.factory.canHandle(env), "Sanity check")
             # Returns a tuple of (request_factory, publication_factory).
             rfactory, pfactory = self.factory.checkRequest(env)
             self.assert_(rfactory is not None,
@@ -352,6 +352,8 @@
 class TestBasicLaunchpadRequest(TestCase):
     """Tests for the base request class"""
 
+    layer = FunctionalLayer
+
     def test_baserequest_response_should_vary(self):
         """Test that our base response has a proper vary header."""
         request = LaunchpadBrowserRequest(StringIO.StringIO(''), {})
@@ -387,6 +389,16 @@
         request = LaunchpadBrowserRequest(StringIO.StringIO(''), env)
         self.assertEquals(u'fnord/trunk\ufffd', request.getHeader('PATH_INFO'))
 
+    def test_request_with_invalid_query_string_recovers(self):
+        # When the query string has invalid utf-8, it is decoded with
+        # replacement.
+        env = {'QUERY_STRING': 'field.title=subproc\xe9s '}
+        request = LaunchpadBrowserRequest(StringIO.StringIO(''), env)
+        # XXX: Python 2.6 and 2.7 handle unicode replacement differently.
+        self.assertIn(
+            request.query_string_params['field.title'],
+            ([u'subproc\ufffd'], [u'subproc\ufffds ']))
+
 
 class TestFeedsBrowserRequest(TestCase):
     """Tests for `FeedsBrowserRequest`."""

=== modified file 'lib/lp/services/webapp/tests/test_user_requested_oops.py'
--- lib/lp/services/webapp/tests/test_user_requested_oops.py	2012-03-06 23:39:08 +0000
+++ lib/lp/services/webapp/tests/test_user_requested_oops.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,5 @@
-# Copyright 2009 Canonical Ltd.  All rights reserved.
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the user requested oops using ++oops++ traversal."""
 

=== modified file 'lib/lp/services/webapp/tests/test_view_model.py'
--- lib/lp/services/webapp/tests/test_view_model.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/webapp/tests/test_view_model.py	2013-02-07 01:27:23 +0000
@@ -1,4 +1,5 @@
-# Copyright 2011 Canonical Ltd.  All rights reserved.
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the user requested oops using ++oops++ traversal."""
 

=== modified file 'lib/lp/testing/tests/test_publication.py'
--- lib/lp/testing/tests/test_publication.py	2012-04-12 06:06:49 +0000
+++ lib/lp/testing/tests/test_publication.py	2013-02-07 01:27:23 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from lazr.restful import EntryResource
+from lazr.restful.utils import get_current_browser_request
 from zope.app.pagetemplate.simpleviewclass import simple
 from zope.component import (
     getSiteManager,
@@ -24,7 +25,6 @@
     ILaunchpadRoot,
     )
 from lp.services.webapp.publisher import (
-    get_current_browser_request,
     Navigation,
     stepthrough,
     )