launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14841
[Merge] lp:~sinzui/launchpad/path-info-bad-request into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/path-info-bad-request into lp:launchpad.
Commit message:
Report a 400 Bad Request when the PATH_INFO is not UTF-8 encoded.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #852205 in Launchpad itself: "UnicodeDecodeError in sane_enviromnent closes the connection"
https://bugs.launchpad.net/launchpad/+bug/852205
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/path-info-bad-request/+merge/140698
Launchpad/Zope fails without raising an error if PATH_INFO is not UTF-8
encoded. The error is deep in Zope and happens before any error handling
is setup
RULES
Pre-implementation: flacoste, wgrant
* We want to make the creation of the request object robust to carry
on for a little bit longer. Something need to indicate that setup
is incomplete and that an error needs to be reported
* After error reporting is setup, and before traversal starts, something
needs to raise a real error. The Error is a generic 400, but it would
be nice to explain what went wrong since Lp does know for this
case that the PATH_INFO must be UTF-8 encoded.
* The New error is exempt from oopses when Lp is not the referrer. Like
404s and 410s, offsite requests are not an issue with Lp's code.
Addendum:
* The very-plain-oops template cannot be used because it claims that
something when wrong with Lp. In this case though, it was the other
site, browser, or user.
* Since the request was not properly setup, the templates in lp.app
fail when the require a proper request.
* We need a template that looks like an Lp page, but does not ever
use the request.
QA
* Visit https://bugs.qastaging.launchpad.net/mahara/+bug/630891%E4
* Verify the page:
* Uses Lp's styles
* Explains Lp did not understand the request
* Shows you the traceback when you are logged in as an Lp developer
LINT
lib/lp/app/browser/configure.zcml
lib/lp/app/browser/tests/test_launchpad.py
lib/lp/app/templates/launchpad-badrequest.pt
lib/lp/services/webapp/error.py
lib/lp/services/webapp/errorlog.py
lib/lp/services/webapp/interfaces.py
lib/lp/services/webapp/publication.py
lib/lp/services/webapp/servers.py
lib/lp/services/webapp/tests/test_errorlog.py
lib/lp/services/webapp/tests/test_publication.py
lib/lp/services/webapp/tests/test_servers.py
LoC
I have more than 10,000 lines of credit this week.
TEST
./bin/test -vc -t errorlog -t publication -t servers lp.services.webapp.tests
./bin/test -vc -t TestErrorViews lp.app.browser.tests
IMPLEMENTATION
I updated BasicLaunchpadRequest to handle the UnicodeDecodeError
exception. The request is made with a sanitised PATH_INFO. The response
status is immediately set to 400 in case something looks at the
response. The X-Launchpad-Bad-Request header is added so that something
can make a proper error for it when oops reports is available.
lib/lp/services/webapp/servers.py
lib/lp/services/webapp/tests/test_servers.py
I added BadRequestError. LaunchpadBrowserPublication.beforeTraversal
already raises errors when it is known that Lp cannot continue. I add a
rule to check for the X-Launchpad-Bad-Request header and make a
BadRequestError from it.
lib/lp/services/webapp/interfaces.py
lib/lp/services/webapp/publication.py
lib/lp/services/webapp/tests/test_publication.py
Added BadRequestError to the list of error to ignore when Lp is not the
referrer.
lib/lp/services/webapp/errorlog.py
lib/lp/services/webapp/tests/test_errorlog.py
I redefined InvalidBatchSizeView to extend the new InvalidBatchSizeView
because both views are 400s and we want to explain what the user did. I
configured the Error view to use the Lp specific template. Since the
error is about the request, we cannot use standard templates. The
very-plain-oops.pt template from webapp was not appropriate because it
claimed there was something wrong with Lp, which was not true. I
provided a page that looks like Lp's error pages, but avoids anything
that requires a sane request object.
lib/lp/services/webapp/error.py
lib/lp/app/browser/configure.zcml
lib/lp/app/browser/tests/test_launchpad.py
lib/lp/app/templates/launchpad-badrequest.pt
--
https://code.launchpad.net/~sinzui/launchpad/path-info-bad-request/+merge/140698
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/path-info-bad-request into lp:launchpad.
=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml 2012-11-12 15:58:15 +0000
+++ lib/lp/app/browser/configure.zcml 2012-12-19 15:12:39 +0000
@@ -397,6 +397,15 @@
class="lp.services.webapp.error.UnexpectedFormDataView"
/>
+ <!-- BadRequestError -->
+ <browser:page
+ for="lp.services.webapp.interfaces.BadRequestError"
+ name="index.html"
+ permission="zope.Public"
+ template="../templates/launchpad-badrequest.pt"
+ class="lp.services.webapp.error.BadRequestView"
+ />
+
<!-- InvalidBatchSizeError -->
<browser:page
for="lazr.batchnavigator.interfaces.InvalidBatchSizeError"
=== modified file 'lib/lp/app/browser/tests/test_launchpad.py'
--- lib/lp/app/browser/tests/test_launchpad.py 2012-12-10 13:43:47 +0000
+++ lib/lp/app/browser/tests/test_launchpad.py 2012-12-19 15:12:39 +0000
@@ -31,6 +31,7 @@
from lp.services.webapp import canonical_url
from lp.services.webapp.escaping import html_escape
from lp.services.webapp.interfaces import (
+ BadRequestError,
BrowserNotificationLevel,
ILaunchpadRoot,
)
@@ -442,6 +443,12 @@
self.assertEqual('Error: Page gone', view.page_title)
self.assertEqual(410, view.request.response.getStatus())
+ def test_BadRequestError(self):
+ error = BadRequestError('PATH_INFO is not UTF-8 encoded.')
+ view = create_view(error, 'index.html')
+ self.assertEqual('Error: Bad request', view.page_title)
+ self.assertEqual(400, view.request.response.getStatus())
+
class ExceptionHierarchyTestCase(TestCaseWithFactory):
=== added file 'lib/lp/app/templates/launchpad-badrequest.pt'
--- lib/lp/app/templates/launchpad-badrequest.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/app/templates/launchpad-badrequest.pt 2012-12-19 15:12:39 +0000
@@ -0,0 +1,22 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ i18n:domain="launchpad"
+>
+ <head>
+ <title>Error: Bad request</title>
+ <link rel="stylesheet" href="/+icing/import.css" />
+ </head>
+ <body>
+ <div class="top-portlet">
+ <h1 class="exception">Bad request</h1>
+ <p>
+ Your request was not understood.<br />
+ <span tal:replace="view/error_message" />
+ </p>
+ <tal:replace tal:replace="structure view/maybeShowTraceback" />
+ </div>
+ </body>
+</html>
=== modified file 'lib/lp/services/webapp/error.py'
--- lib/lp/services/webapp/error.py 2012-02-17 23:00:56 +0000
+++ lib/lp/services/webapp/error.py 2012-12-19 15:12:39 +0000
@@ -233,10 +233,9 @@
request.response.setHeader('Retry-After', 900)
-class InvalidBatchSizeView(SystemErrorView):
- """View rendered when an InvalidBatchSizeError is raised."""
+class BadRequestView(SystemErrorView):
- page_title = "Error: Invalid Batch Size"
+ page_title = "Error: Bad request"
response_code = httplib.BAD_REQUEST
@@ -252,6 +251,12 @@
return ""
+class InvalidBatchSizeView(BadRequestView):
+ """View rendered when an InvalidBatchSizeError is raised."""
+
+ page_title = "Error: Invalid Batch Size"
+
+
class TranslationUnavailableView(SystemErrorView):
page_title = 'Error: Translation page is not available'
=== modified file 'lib/lp/services/webapp/errorlog.py'
--- lib/lp/services/webapp/errorlog.py 2012-08-09 03:36:26 +0000
+++ lib/lp/services/webapp/errorlog.py 2012-12-19 15:12:39 +0000
@@ -296,7 +296,7 @@
_ignored_exceptions = set(['TranslationUnavailable', 'NoReferrerError'])
_ignored_exceptions_for_offsite_referer = set([
- 'GoneError', 'InvalidBatchSizeError', 'NotFound'])
+ 'BadRequestError', 'GoneError', 'InvalidBatchSizeError', 'NotFound'])
_default_config_section = 'error_reports'
def __init__(self):
=== modified file 'lib/lp/services/webapp/interfaces.py'
--- lib/lp/services/webapp/interfaces.py 2012-09-28 06:47:29 +0000
+++ lib/lp/services/webapp/interfaces.py 2012-12-19 15:12:39 +0000
@@ -68,6 +68,10 @@
"""Marker interface for a Launchpad protocol error exception."""
+class BadRequestError(Exception):
+ """The request was not understood; the client must fix the issue"""
+
+
class OffsiteFormPostError(Exception):
"""An attempt was made to post a form from a remote site."""
=== modified file 'lib/lp/services/webapp/publication.py'
--- lib/lp/services/webapp/publication.py 2012-09-28 06:25:44 +0000
+++ lib/lp/services/webapp/publication.py 2012-12-19 15:12:39 +0000
@@ -76,6 +76,7 @@
from lp.services.osutils import open_for_writing
import lp.services.webapp.adapter as da
from lp.services.webapp.interfaces import (
+ BadRequestError,
FinishReadOnlyRequestEvent,
ILaunchpadRoot,
IOpenLaunchBag,
@@ -249,6 +250,9 @@
threadid = thread.get_ident()
threadrequestfile = open_for_writing(
'logs/thread-%s.request' % threadid, 'w')
+ bad_request = request.response.getHeader('X-Launchpad-Bad-Request', '')
+ if bad_request:
+ raise BadRequestError(bad_request)
try:
request_txt = unicode(request).encode('UTF-8')
except Exception:
=== modified file 'lib/lp/services/webapp/servers.py'
--- lib/lp/services/webapp/servers.py 2012-07-06 16:39:38 +0000
+++ lib/lp/services/webapp/servers.py 2012-12-19 15:12:39 +0000
@@ -390,7 +390,6 @@
the request does comply, (None, None).
"""
method = environment.get('REQUEST_METHOD')
-
if method in self.getAcceptableMethods(environment):
factories = (None, None)
else:
@@ -574,8 +573,22 @@
def __init__(self, body_instream, environ, response=None):
self.traversed_objects = []
self._wsgi_keys = set()
- super(BasicLaunchpadRequest, self).__init__(
- body_instream, environ, response)
+ try:
+ super(BasicLaunchpadRequest, self).__init__(
+ body_instream, environ, response)
+ except UnicodeDecodeError:
+ # The environ must be santised for the request so that a
+ # ProtocolErrorPublicationFactory can handle this issue
+ # before traversal starts.
+ safe_path_info = environ['PATH_INFO'].decode('utf-8', 'ignore')
+ safe_env = dict(environ)
+ safe_env['PATH_INFO'] = safe_path_info
+ super(BasicLaunchpadRequest, self).__init__(
+ body_instream, safe_env, response)
+ self.response.setStatus(
+ 400, reason='PATH_INFO is not UTF-8 encoded')
+ self.response.setHeader(
+ 'X-Launchpad-Bad-Request', 'PATH_INFO is not UTF-8 encoded')
# Our response always vary based on authentication.
self.response.setHeader('Vary', 'Cookie, Authorization')
=== modified file 'lib/lp/services/webapp/tests/test_errorlog.py'
--- lib/lp/services/webapp/tests/test_errorlog.py 2012-06-29 08:40:05 +0000
+++ lib/lp/services/webapp/tests/test_errorlog.py 2012-12-19 15:12:39 +0000
@@ -46,6 +46,7 @@
ScriptRequest,
)
from lp.services.webapp.interfaces import (
+ BadRequestError,
IUnloggedException,
NoReferrerError,
)
@@ -419,7 +420,7 @@
del utility._oops_config.publishers[:]
errors = set([
GoneError.__name__, InvalidBatchSizeError.__name__,
- NotFound.__name__])
+ NotFound.__name__, BadRequestError.__name__])
self.assertEqual(
errors, utility._ignored_exceptions_for_offsite_referer)
=== modified file 'lib/lp/services/webapp/tests/test_publication.py'
--- lib/lp/services/webapp/tests/test_publication.py 2012-08-09 03:41:10 +0000
+++ lib/lp/services/webapp/tests/test_publication.py 2012-12-19 15:12:39 +0000
@@ -31,6 +31,7 @@
)
import lp.services.webapp.adapter as dbadapter
from lp.services.webapp.interfaces import (
+ BadRequestError,
NoReferrerError,
OAuthPermission,
OffsiteFormPostError,
@@ -79,6 +80,15 @@
publication.callTraversalHooks(request, obj2)
self.assertEquals(request.traversed_objects, [obj1])
+ def test_beforeTraversal_x_launchapd_bad_request(self):
+ # A BadRequestError is raised when the request's response has the
+ # X-Launchpad-Bad-Request header.
+ request = LaunchpadTestRequest()
+ request.response.setHeader('X-Launchpad-Bad-Request', 'Bad encoding')
+ publication = LaunchpadBrowserPublication(None)
+ self.assertRaises(
+ BadRequestError, publication.beforeTraversal, request)
+
class TestWebServicePublication(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/services/webapp/tests/test_servers.py'
--- lib/lp/services/webapp/tests/test_servers.py 2012-02-29 10:35:12 +0000
+++ lib/lp/services/webapp/tests/test_servers.py 2012-12-19 15:12:39 +0000
@@ -380,6 +380,20 @@
response.getHeader(
'Strict-Transport-Security'), 'max-age=2592000')
+ def test_baserequest_recovers_from_bad_path_info_encoding(self):
+ # The request object handles the cases when sane_environment raises a
+ # UnicodeDecodeError decoding the path_info.
+ bad_path = 'fnord/trunk\xE4'
+ env = {'PATH_INFO': bad_path}
+ request = LaunchpadBrowserRequest(StringIO.StringIO(''), env)
+ self.assertEquals('fnord/trunk', request.getHeader('PATH_INFO'))
+ self.assertEquals(
+ 'PATH_INFO is not UTF-8 encoded',
+ request.response.getHeader('X-Launchpad-Bad-Request'))
+ self.assertEquals(
+ '400 PATH_INFO is not UTF-8 encoded',
+ request.response.getStatusString())
+
class TestFeedsBrowserRequest(TestCase):
"""Tests for `FeedsBrowserRequest`."""
