aa601a92de breaks chunked uploads

[msdemlei]

Commit aa601a92de6a1e95f6525433f2e23a74040a93aa changes line ~900 in web/http.py from

        if self.method == b"POST" and ctype:

to

        if self.method == b"POST" and ctype and clength:

If an upload is chunked, no clength is going to be available, and so the whole argument parsing will be suppressed; request.args will be an empty dictionary.

I don't quite understand why the clength condition was added in this commit (which explains it's an "update for python 3.7"), so I'm not sure how to fix this. Locally, removing the "and clength" fixes my problem (albeit on python 2.7). If it remains as it is, *somewhere* some new code would have to handle chunked uploads.

To reproduce, run the following server:

from twisted.web import server, resource
from twisted.internet import reactor

class Res(resource.Resource):
   isLeaf = True

  def render_POST(self, request):
    request.setHeader("content-type", "text/plain")
    return repr(request.args)+"\r\n"

site = server.Site(Res())
reactor.listenTCP(8888, site)
reactor.run()

Correct behaviour (e.g., when "and clength" is absent):

$ curl -F foo=bar http://localhost:8888/
{'foo': ['bar']}
$ curl  --header "Transfer-Encoding: chunked" -F foo=bar http://localhost:8888/
{'foo': ['bar']}

Bad behaviour (as with current HEAD -- or so I think, at least it's the behaviour of python-twisted-core in Debian buster, which has the HEAD's code in http.py):

$ curl -F foo=bar http://localhost:8888/
{'foo': ['bar']}
$ curl  --header "Transfer-Encoding: chunked" -F foo=bar http://localhost:8888/
{}

[Tom Most]

Hi, I saw your question in #twisted. It looks like this change was made because because ​Python 3.7's cgi.parse_multipart() requires this CONTENT-LENGTH member. This seems to be the offending commit: ​https://github.com/python/cpython/commit/cc3fa204d357be5fafc10eb8c2a80fe0bca998f1

It seems like we are missing a test case for the chunked case. The first step in fixing this regression would be writing one. This would be a real help, even if you don't want to contribute a full fix!

[msdemlei]

Replying to Tom Most:

Hi, I saw your question in #twisted. It looks like this change was made because because ​Python 3.7's cgi.parse_multipart() requires this CONTENT-LENGTH member. This seems to be the offending commit: ​https://github.com/python/cpython/commit/cc3fa204d357be5fafc10eb8c2a80fe0bca998f1

Hm. Which poses the question why *they* suddenly start requiring it. Plus, I expect that will hit quite a few non-twisted users, too (or does nobody do plain cgis any more these days?).

It seems like we are missing a test case for the chunked case. The first step in fixing this regression would be writing one. This would be a real help, even if you don't want to contribute a full fix!

A test case is in ​github PR 1180; twisted.web.test.test_http.ChunkingTests.test_multipartFormData passes when clength is gone and fails with twisted as of 18.9.0. Since this replicates curl's behaviour, I'm pretty sure the test actually tests what it should.

[Tom Most]

https://twistedmatrix.com/trac/ticket/9739 is a similar issue.

[Tom Most]

PR ready for review: ​https://github.com/twisted/twisted/pull/1207

[Glyph]

I will land this when CI is happy again.

[Glyph <glyph@…>]

In 8d101b4b:

Error: Processor CommitTicketReference failed

 does not appear to be a Git repository. See the log for more information.