Merge lp:~sinzui/launchpad/path-info-bad-request into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Curtis Hovey | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 16384 | ||||
Proposed branch: | lp:~sinzui/launchpad/path-info-bad-request | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
39 lines (+18/-0) 2 files modified
lib/lp/services/webapp/servers.py (+9/-0) lib/lp/services/webapp/tests/test_servers.py (+9/-0) |
||||
To merge this branch: | bzr merge lp:~sinzui/launchpad/path-info-bad-request | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+140698@code.launchpad.net |
Commit message
Recode PATH_INFO to UTF-8 th ensure a UnicodeDecodeError is not raised before oops reporting is available.
Description of the change
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-
* 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:/
* 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/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
LoC
I have more than 10,000 lines of credit this week.
TEST
./bin/test -vc -t errorlog -t publication -t servers lp.services.
./bin/test -vc -t TestErrorViews lp.app.
IMPLEMENTATION
I updated BasicLaunchpadR
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-
can make a proper error for it when oops reports is available.
lib/
lib/
I added BadRequestError. LaunchpadBrowse
already raises errors when it is known that Lp cannot continue. I add a
rule to check for the X-Launchpad-
BadRequestError from it.
lib/
lib/
lib/
Added BadRequestError to the list of error to ignore when Lp is not the
referrer.
lib/
lib/
I redefined InvalidBatchSiz
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/
lib/
lib/
lib/
Did you consider a less invasive fix along the lines of http:// paste.ubuntu. com/1451678/? 'replace' will replace the invalid UTF-8 sequences with U+FFFD, which is not ideal, but it's an uncommon case and probably less risky than your proposal.