launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15098
lp:~stevenk/launchpad/handle-invalid-unicode-in-query-string-redux into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/handle-invalid-unicode-in-query-string-redux into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #897053 in Launchpad itself: "apport generated urls can trigger UnicodeDecodeError"
https://bugs.launchpad.net/launchpad/+bug/897053
Bug #1113932 in Launchpad itself: "Files uploaded through lazr.restful get decoded as Unicode"
https://bugs.launchpad.net/launchpad/+bug/1113932
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/handle-invalid-unicode-in-query-string-redux/+merge/146773
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.
--
https://code.launchpad.net/~stevenk/launchpad/handle-invalid-unicode-in-query-string-redux/+merge/146773
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/handle-invalid-unicode-in-query-string-redux into lp:launchpad.
=== 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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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,10 @@
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`."""
+ 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-06 04:43:28 +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-06 04:43:28 +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,26 @@
raise InvalidFilename
# Create the alias for the file.
filename = self.normalizeFilename(filename)
+ if from_api:
+ file_content = StringIO(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:
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/subscribers.py'
--- lib/lp/registry/subscribers.py 2012-11-26 08:40:20 +0000
+++ lib/lp/registry/subscribers.py 2013-02-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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,21 @@
return None
+def get_raw_form_value_from_current_request(field_name):
+ # 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)
+ # cgi.FieldStorage handily gives us StringIO, so use that.
+ return fs[field_name].file
+
+
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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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-06 04:43:28 +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,
)
