[wptserve] Rework header encoding/decoding in Request #13246

Hexcles · 2018-09-27T21:17:08Z

This PR relands the reverted #13248 from @Ms2ger , but with some tweaks on how to encode/decode the headers. See individual commits for details.

Fixes #13204 .

tools/wptserve/tests/functional/test_request.py

tools/wptserve/wptserve/ranges.py

tools/wptserve/wptserve/request.py

gsnedders · 2018-09-27T23:40:43Z

tools/wptserve/wptserve/request.py

@@ -308,7 +308,7 @@ def POST(self):
            self.raw_input.seek(0)
            fs = cgi.FieldStorage(fp=self.raw_input,
                                  environ={"REQUEST_METHOD": self.method},
-                                  headers=self.headers,
+                                  headers=self.raw_headers,


Why the behaviour change here?

Because cgi.FieldStorage needs to parse the headers and expect the values to be string not bytes. Since self.readers is read-only, I figure it would be safe to pass in the original raw_headers here.

self.raw_headers is the headers as bytes, no? What type does cgi.FieldStorage expect? str on both Py2/3, no?

self.raw_headers is BaseHTTPRequestHandler.headers; and both its keys and values are str on both Py2/3, which is why we need to _maybe_encode them in RequestHeaders.

tools/wptserve/wptserve/request.py

Hexcles · 2018-09-28T19:07:31Z

A few clarifications from the discussions on IRC regarding whether we should encode the header values back to bytes (i.e. headers = [maybeencode(items[header])]): https://w3.logbot.info/testing/20180928#c444317

First of all, regardless of the approach (whether to encode and which encoding to use), this WILL NOT affect existing tests (namely custom handlers) at all. Currently, WPT only really works with Python 2, and in Python 2 headers (both names and values) have str type, which is a "binary" type, so maybeencode is a no-op.

People reasonably want wptserve itself to work in Python 3, and this is where the change has effects. I would hope Python 3 still gives us headers in binary type (i.e. bytes), as Python 2, but Python 3 decides to decode the headers in iso-8859-1 perhaps for better compatibility. I think the sane choice here is to return bytes, which requires re-encoding, as HTTP RFC doesn't really say what encoding headers must use and encoding non-ASCII characters in iso-8859-1 is broken. The downside is that potentially more places that manipulate headers need to be changed (e.g. literal operands of string operations need to be bytes).

Now, since many custom handlers don't work in Python 3 in the first place, I'm inclined to to maybeencode the header values (i.e. return bytes in Python 3).

Hexcles · 2018-09-28T20:00:11Z

@gsnedders This PR is ready for review again. I addressed all your comments but one (the cgi.FieldStorage one; please see my reply there).

annevk · 2018-09-29T13:34:55Z

and encoding non-ASCII characters in iso-8859-1 is broken

It's not? Using bytes is perhaps purer in a way, but if you make them strings that is the way to go. Anything else would lead to information loss.

Hexcles · 2018-09-29T17:20:00Z

To be precise, broken as in if you try to do string operations on them (counting lengths, printing, etc.), the semantics are wrong, so they are not really text strings and the type is hence misleading (you really have to treat these str as bytes). There's no information loss.

…

On Sat, Sep 29, 2018, 09:35 Anne van Kesteren ***@***.***> wrote: and encoding non-ASCII characters in iso-8859-1 is broken It's not? Using bytes is perhaps purer in a way, but if you make them strings that is the way to go. Anything else would lead to information loss. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13246 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABXsjHelUVGUsQCB-0g_64BQse884ke7ks5uf3cSgaJpZM4W9eVD> .

Ms2ger · 2018-10-01T10:18:11Z

I suspect that supporting only binary_type in this API would be the most straightforward approach.

Hexcles · 2018-10-01T14:44:48Z

@Ms2ger you mean keys should also be binary type? Yeah I can do that. It might be break a few more things (in Python 3 only) but it's probably more consistent.

gsnedders · 2018-10-01T15:05:32Z

@Ms2ger @Hexcles I have no strong preference; in principle header names are strictly ASCII only, though I don't think impls enforce that.

Hexcles · 2018-10-02T15:27:42Z

@Ms2ger @gsnedders there's one complication here. Technically, a byte string doesn't have the lower method. Yet we usually wish header names to be case-insensitive and we implement it by always converting the keys to lowercase both during storage and lookup.

gsnedders · 2018-10-02T16:36:32Z

@Hexcles Yes it does? bytes.lower does ASCII lowercasing on both Py2/3.

Hexcles · 2018-10-02T17:05:57Z

@gsnedders duh my bad. Yeah let me update the PR.

Hexcles · 2018-10-02T17:25:39Z

@gsnedders @Ms2ger PTAL again

tools/wptserve/tests/functional/base.py

tools/wptserve/tests/functional/test_pipes.py

tools/wptserve/tests/functional/test_request.py

tools/wptserve/wptserve/pipes.py

gsnedders · 2018-10-02T21:33:35Z

tools/wptserve/wptserve/request.py

@@ -308,7 +308,7 @@ def POST(self):
            self.raw_input.seek(0)
            fs = cgi.FieldStorage(fp=self.raw_input,
                                  environ={"REQUEST_METHOD": self.method},
-                                  headers=self.headers,
+                                  headers=self.raw_headers,


self.raw_headers is the headers as bytes, no? What type does cgi.FieldStorage expect? str on both Py2/3, no?

tools/wptserve/wptserve/request.py

tools/wptserve/tests/functional/test_pipes.py

tools/wptserve/wptserve/pipes.py

andypaicu · 2018-10-09T09:29:22Z

Just as an fyi guys I'm also waiting on this re-land as I suspect the revert broke the "wpt / content-security-policy / generic / only-valid-whitespaces-are-allowed.html" test.

There hasn't been any movement on this for a week and I just want to make sure that this will eventually get merged.

https://wpt.fyi/results/content-security-policy/generic/only-valid-whitespaces-are-allowed.html?label=experimental

tools/wptserve/tests/functional/test_request.py

gsnedders · 2018-10-09T11:39:08Z

@Hexcles @Ms2ger do one of you want to rewrite history to something saner?

Ms2ger · 2018-10-09T12:03:58Z

If @Hexcles hasn't done it by tomorrow, I'll squash them all together.

The test sends a request to wptserve with non-ASCII characters in a header and sets up a simple handler to return the value of that header. The server shouldn't crash in either Python 2 or 3, and the response should not be garbled. The server crashes in Python 2 (#13204).

This change changes the encoding/decoding of headers in Request and username/password in Authentication: now all keys and values have binary type (because of an implementation detail in the Python 3 standard library, we actually need to re-encode the headers back to bytes in Python 3). Documentation and comments are also improved to clarify the encoding situation. Also add test cases for non-ASCII characters in auth headers.

Hexcles · 2018-10-09T19:51:27Z

Thanks for the review, @gsnedders . (The comment threads do look quite confusing...)

I just cleaned up the history and will merge the rebase-merge PR once Travis passes.

The most important two changes to roll: * [wptserve] Rework header encoding/decoding in Request (web-platform-tests/wpt#13246): needed to fix tests with non-ASCII cookies * Fix handling of about:blank in lint (web-platform-tests/wpt#13489): needed to unblock the importer Bug: 895239 Change-Id: I10b308c19bdb20d2a1ec5af8dece42d7ada7e25b Reviewed-on: https://chromium-review.googlesource.com/c/1279195 Commit-Queue: Robert Ma <[email protected]> Reviewed-by: Philip Jägenstedt <[email protected]> Cr-Commit-Position: refs/heads/master@{#599617}

Hexcles requested review from Ms2ger and jgraham September 27, 2018 21:17

wpt-pr-bot added infra wptserve labels Sep 27, 2018

wpt-pr-bot requested a review from gsnedders September 27, 2018 21:17

gsnedders reviewed Sep 27, 2018

View reviewed changes

foolip mentioned this pull request Sep 28, 2018

Many tests in /cookies/http-state consistently time out on Taskcluster but not on BuildBot #13204

Closed

Hexcles force-pushed the do-not-decode-header-values branch from 4fdd08b to 67d7b78 Compare September 28, 2018 19:56

Hexcles requested a review from annevk September 28, 2018 20:00

gsnedders mentioned this pull request Oct 1, 2018

Stop trying to decode cookies. #13237

Closed

Hexcles force-pushed the do-not-decode-header-values branch from 67d7b78 to a69a669 Compare October 2, 2018 17:24

gsnedders reviewed Oct 2, 2018

View reviewed changes

Ms2ger force-pushed the do-not-decode-header-values branch from f10c76e to ce24036 Compare October 4, 2018 08:33

Ms2ger force-pushed the do-not-decode-header-values branch from ce24036 to 26d33f0 Compare October 9, 2018 10:06

gsnedders approved these changes Oct 9, 2018

View reviewed changes

tools/wptserve/tests/functional/test_request.py Show resolved Hide resolved

Reland "Various test fixes for python3 support. (#11769)"

4f93832

Hexcles added 2 commits October 9, 2018 15:49

Hexcles force-pushed the do-not-decode-header-values branch from 26d33f0 to 887214f Compare October 9, 2018 19:50

Hexcles merged commit 8b09ece into master Oct 9, 2018

Hexcles deleted the do-not-decode-header-values branch October 9, 2018 21:25

Hexcles mentioned this pull request Apr 4, 2020

RFC 49: Python 3 migration for wptserve web-platform-tests/rfcs#49

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wptserve] Rework header encoding/decoding in Request #13246

[wptserve] Rework header encoding/decoding in Request #13246

Hexcles commented Sep 27, 2018 •

edited

Loading

gsnedders Sep 27, 2018

Hexcles Sep 28, 2018

gsnedders Oct 2, 2018

Hexcles Oct 2, 2018

Hexcles commented Sep 28, 2018

Hexcles commented Sep 28, 2018

annevk commented Sep 29, 2018

Hexcles commented Sep 29, 2018 via email

Ms2ger commented Oct 1, 2018

Hexcles commented Oct 1, 2018

gsnedders commented Oct 1, 2018

Hexcles commented Oct 2, 2018

gsnedders commented Oct 2, 2018

Hexcles commented Oct 2, 2018

Hexcles commented Oct 2, 2018

gsnedders Oct 2, 2018

andypaicu commented Oct 9, 2018

gsnedders commented Oct 9, 2018

Ms2ger commented Oct 9, 2018

Hexcles commented Oct 9, 2018

[wptserve] Rework header encoding/decoding in Request #13246

[wptserve] Rework header encoding/decoding in Request #13246

Conversation

Hexcles commented Sep 27, 2018 • edited Loading

gsnedders Sep 27, 2018

Choose a reason for hiding this comment

Hexcles Sep 28, 2018

Choose a reason for hiding this comment

gsnedders Oct 2, 2018

Choose a reason for hiding this comment

Hexcles Oct 2, 2018

Choose a reason for hiding this comment

Hexcles commented Sep 28, 2018

Hexcles commented Sep 28, 2018

annevk commented Sep 29, 2018

Hexcles commented Sep 29, 2018 via email

Ms2ger commented Oct 1, 2018

Hexcles commented Oct 1, 2018

gsnedders commented Oct 1, 2018

Hexcles commented Oct 2, 2018

gsnedders commented Oct 2, 2018

Hexcles commented Oct 2, 2018

Hexcles commented Oct 2, 2018

gsnedders Oct 2, 2018

Choose a reason for hiding this comment

andypaicu commented Oct 9, 2018

gsnedders commented Oct 9, 2018

Ms2ger commented Oct 9, 2018

Hexcles commented Oct 9, 2018

Hexcles commented Sep 27, 2018 •

edited

Loading