Opened 20 months ago
Closed 14 months ago
#9678 defect closed fixed (fixed)
aa601a92de breaks chunked uploads
Reported by: | msdemlei | Owned by: | Glyph |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | web | Keywords: | |
Cc: | Branch: |
9678-http-multipart-py37
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: |
Description
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/ {}
Change History (6)
comment:1 follow-up: 2 Changed 19 months ago by
comment:2 Changed 19 months ago by
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.
comment:3 Changed 15 months ago by
Owner: | set to Tom Most |
---|---|
Priority: | normal → high |
Status: | new → assigned |
https://twistedmatrix.com/trac/ticket/9739 is a similar issue.
comment:4 Changed 15 months ago by
Branch: | → 9678-http-multipart-py37 |
---|---|
Keywords: | review added; web chunked upload removed |
Owner: | Tom Most deleted |
Status: | assigned → new |
PR ready for review: https://github.com/twisted/twisted/pull/1207
comment:5 Changed 14 months ago by
Keywords: | review removed |
---|---|
Owner: | set to Glyph |
I will land this when CI is happy again.
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!