| msg272856 - (view) |
Author: (rr-) |
Date: 2016-08-16 13:55 |
| Sending requests with Content-Length but without Content-Disposition headers causes following error: Traceback (most recent call last): File "./test", line 19, in form = cgi.FieldStorage(fp=env['wsgi.input'], environ=env) File "/usr/lib/python3.5/cgi.py", line 561, in __init__ self.read_single() File "/usr/lib/python3.5/cgi.py", line 740, in read_single self.read_binary() File "/usr/lib/python3.5/cgi.py", line 762, in read_binary self.file.write(data) TypeError: write() argument must be str, not bytes I've attached a test file that reproduces the issue. The issue is because read_single decides whether to read the content as binary or text depending on content-length - if it's > 0, it uses read_binary which assumes binary input, and rewrites this input to self.file, assuming self.file is opened in binary mode. At the same, self.file is opened in text mode, because self._binary_file is set to False, which in turn is because there's no Content-Disposition header. At very least, the decision whether to use binary or text should be consistent in both places (self.length >= 0 vs self._binary_file). Related: https://bugs.python.org/issue27308 Note that unlike https://bugs.python.org/issue24764 this issue does NOT concern multipart requests. |
|
|
| msg272857 - (view) |
Author: Decorater (Decorater) * |
Date: 2016-08-16 14:12 |
| hmm into looking it should check if it is in actuality a binary file the length of the file data does not really determine anything on encoding really. if self._binary_file: would suffice on determining binary mode or not. |
|
|
| msg272858 - (view) |
Author: Decorater (Decorater) * |
Date: 2016-08-16 14:16 |
| Here is a patch to review (note I only had disc space to clone 3.6 so I had to manually download this version of the file). |
|
|
| msg277482 - (view) |
Author: Bert JW Regeer (X-Istence) * |
Date: 2016-09-27 03:40 |
| On line #890 in self.make_file() the check for _binary_file should be changed to also check for self.length >= 0. https://github.com/python/cpython/blob/3.4/Lib/cgi.py#L890 becomes: if self._binary_file or self.length >= 0: _binary_file is only ever set if there is a content disposition, which there is not in the test case provided. In the case of no content disposition we can use the content-length as a hint that we have a file that has been uploaded. All files uploaded should be treated as binary if they are not a text type. This is a duplicate of #27308, however the patch in that report is incorrect IMHO. |
|
|
| msg277483 - (view) |
Author: Bert JW Regeer (X-Istence) * |
Date: 2016-09-27 03:44 |
| Updated versions this applies to. |
|
|
| msg277536 - (view) |
Author: Berker Peksag (berker.peksag) *  |
Date: 2016-09-27 18:27 |
| Thanks for triaging this, Bert. Would you like to propose a patch with a test case? Note that we can't fix this in 3.3 and 3.4 because they are in security-fix-only mode. See https://docs.python.org/devguide/index.html#status-of-python-branches for details. |
|
|
| msg281075 - (view) |
Author: Bert JW Regeer (X-Istence) * |
Date: 2016-11-18 05:19 |
| @berker.peksag: Attached is a patch with a test case that exercises this issue. Code path is that read_single() checks if the length is greater than 0, and then it reads binary, otherwise it reads it as a single line. This fixes make_file so that if self.length is greater than or equal to 0, it opens the file in binary mode, this matches the checks that write stuff as binary. This also undoes the change that was made in https://bugs.python.org/issue24764. Fixing this issue fixed that one as well, and arguably throwing data away doesn't seem like a good idea. |
|
|
| msg281793 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2016-11-26 19:30 |
| Berker asks in IRC whether this change should go into 3.6.0 (at rc1). While it is affecting a relatively self-contained part of the standard library (cgi), the issue doesn't seem to be "release critical". Further, it is changing behavior that was changed barely a year ago for Issue24764. My preference would be to try to have this change reviewed and/or tested by at least some of the people involved with the earlier issue and, if there is a consensus for it, target the change for 3.6.1. |
|
|
| msg281794 - (view) |
Author: Bert JW Regeer (X-Istence) * |
Date: 2016-11-26 19:44 |
| Unfortunately I need to spin another patch, the one I created didn't solve the issue for one of WebOb's users: https://github.com/Pylons/webob/pull/300 (Thanks Julien Meyer!) I have his permission to grab his test/patch and update this patch, I will get this done later today. That being said, this is a real issue, and WebOb will be shipping a fix for Python less than 3.6 anyway, so whether this gets released in 3.6 or not doesn't matter to me. I'd prefer this to be fixed in the standard library for all users, rather than just for WebOb users. Even if this were released for 3.6.1, WebOb will have to carry the fix for the foreseeable future. |
|
|
| msg319179 - (view) |
Author: Chris Eykamp (watusimoto) * |
Date: 2018-06-09 20:25 |
| I've been experiencing the same issue, which is triggered in the exception handling of web.py. Bert's proposed fix, adding the zero byte check (if self._binary_file or self.length >= 0:) addresses the issue I'm seeing (tested on 3.5, it's what's available where I can reproduce the error). This issue seems to be languishing. Is there any way we could push this forward, even if it doesn't address every problem with the lib? |
|
|
| msg319438 - (view) |
Author: Chris Eykamp (watusimoto) * |
Date: 2018-06-13 09:06 |
| This also manifests itself when using web.py: if the underlying code throws an exception, this is emitted: File "/usr/local/lib/python3.5/dist-packages/web/webapi.py", line 364, in input out = rawinput(_method) File "/usr/local/lib/python3.5/dist-packages/web/webapi.py", line 341, in rawinput a = cgi.FieldStorage(fp=fp, environ=e, keep_blank_values=1) File "/usr/lib/python3.5/cgi.py", line 561, in __init__ self.read_single() File "/usr/lib/python3.5/cgi.py", line 740, in read_single self.read_binary() File "/usr/lib/python3.5/cgi.py", line 762, in read_binary self.file.write(data) TypeError: write() argument must be str, not bytes |
|
|
| msg319443 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2018-06-13 09:57 |
| Thank you for the ping, Chris. I will try to combine Bert's and Julien's patches and prepare a PR this weekend. |
|
|
| msg319451 - (view) |
Author: Chris Eykamp (watusimoto) * |
Date: 2018-06-13 10:45 |
| I've already got a PR based on the patch listed under the Files section (it's prepared, not yet submitted), but if you want to do something more, I'll step back and let you do it. |
|
|
| msg319480 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2018-06-13 17:54 |
| That's even better! :) Please submit your work as a pull request. Did you take a look at https://github.com/Pylons/webob/pull/300 as well? Can we use the test in the PR? Is it possible to adapt it solve both this and WebOb issues? |
|
|
| msg319534 - (view) |
Author: Chris Eykamp (watusimoto) * |
Date: 2018-06-14 17:42 |
| I'll get a PR submitted this weekend, and post back here. It will not explicitly address that other case, as I don't have the capacity or wherewithal for that. Alas. |
|
|
| msg319993 - (view) |
Author: Chris Eykamp (watusimoto) * |
Date: 2018-06-19 19:07 |
| Packaged patch offered below into PR 7804 https://github.com/python/cpython/pull/7804 |
|
|
| msg320503 - (view) |
Author: Bert JW Regeer (Bert JW Regeer) |
Date: 2018-06-26 16:50 |
| I'll take a look and see if I can get the other fixes from WebOb and add them to a patch, and create a follow-up PR. If I can stop carrying a monkey patch for the standard library I am all for it! Thanks for running with this! |
|
|
| msg320506 - (view) |
Author: Chris Eykamp (watusimoto) * |
Date: 2018-06-26 17:48 |
| I don't know if you've read the dialog on the PR (there was also some offline between Ned and myself), but the patch breaks a test when running under a fresh build of Python. I can't reproduce it here without setting up a build system, which I haven't had time to do, and won't be able to do at leat through the end of next week. If you can run the tests on a fresh build of Python, and confirm that they break, that would be helpful. |
|
|
| msg330626 - (view) |
Author: Pierre Quentel (quentel) * |
Date: 2018-11-28 18:05 |
| I have submitted another Pull Request (10771) that seems to fix the bug while passing all the tests in test_cgi.py |
|
|
| msg334882 - (view) |
Author: Aron Podrigal (Aron Podrigal) * |
Date: 2019-02-05 18:55 |
| I am experiencing the same issue. https://github.com/python/cpython/pull/10771 looks good. While were at it, and if PR 10771 is accepted, maybe we can change https://github.com/python/cpython/blob/6613b56173d26f32da9945691ff9f824304224a2/Lib/cgi.py#L717 to `read` instead of `readline` since we anyway read till EOF. |
|
|
| msg334890 - (view) |
Author: Aron Podrigal (Aron Podrigal) * |
Date: 2019-02-05 23:01 |
| A different approach. Always honor content length, and do not try to read more than. |
|
|
| msg342504 - (view) |
Author: Edward Gow (elgow) |
Date: 2019-05-14 18:55 |
| This bug is triggered by xml-rpc calls from the xmlrpc.client in the Python 3.5 standard library to a mod_wsgi/Python 3.5 endpoint. |
|
|
| msg349264 - (view) |
Author: Christoph Zwerschke (cito) * |
Date: 2019-08-08 22:25 |
| This also happens when sending POST requests with JSON payload from a browser with XMLHttpRequest to a Python 3.7 backend using FieldStorage. It seems XMLHttpRequest adds the content length automatically. |
|
|
| msg366308 - (view) |
Author: Fran Boon (Fran Boon) |
Date: 2020-04-13 12:07 |
| What is happening with this bug? I am amazed that nearly 4 years on it doesn't seem to have been resolved. The issue took me a fairly long time to debug the cause of, but once known the issue seems relatively simple to resolve & there are a couple of Pull Requests which fix the issue. This is my first time looking into the core of Python's own development (which I guess is a testament to how well this normally works), so I may be being naive, but what is the blocker here? Is there anything I can do to help? Test/Review existing PRs? (They both look good to me) Create a new PR? (Seems unnecessary) I really am genuinely keen to help resolve this for at least Python 3.7+ (Am aware that 3.6 is security fixes only) |
|
|
| msg368837 - (view) |
Author: Tim Nyborg (Tim Nyborg2) |
Date: 2020-05-14 13:05 |
| Echoing Fran Boon, I'm wondering what needs to happen to get the fixes merged and this issue resolved. It affects web servers run on several frameworks, which is more of a problem now, since so many of us migrated to py3 in advance of py2 EOL. |
|
|
| msg377493 - (view) |
Author: Jakub Kulik (kulikjak) * |
Date: 2020-09-25 14:10 |
| We internally tested the most recent PR and found some issues with it: https://github.com/python/cpython/pull/21457#issuecomment-698845895 We ended up using a much simpler patch, which seems to work as expected. --- Python-3.7.8/Lib/cgi.py +++ Python-3.7.8/Lib/cgi.py @@ -703,7 +703,10 @@ if not data: self.done = -1 break - self.file.write(data) + if self._binary_file: + self.file.write(data) + else: + self.file.write(data.decode()) todo = todo - len(data) def read_lines(self): |
|
|
| msg386175 - (view) |
Author: Bradley Miller (bnmnetp) |
Date: 2021-02-03 00:31 |
| Thanks Jakub, Your patch fixed an increasingly frequent problem with my site. How can I help to get this merged so I don't have to have a custom version of cgi.py?? |
|
|
| msg387809 - (view) |
Author: Christoph Zwerschke (cito) * |
Date: 2021-02-28 12:47 |
| Just created a test case for this problem after a pentest provoked this error on one of my web apps. Then I found this bug report which already has a similar test case attached. The problem is that read_binary() as the name says reads binary data, but then writes it to a file which may or may not be binary, depending on whether self._binary_file is set, which depends on whether a filename was set via the content-disposition header. Jakub's patch looks good and works for me. Please merge this! |
|
|
| msg413071 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2022-02-11 13:48 |
| Both active PRs have comments pointing out issues, that’s why this is still open. A clean fix with unit tests and no regression is needed. |
|
|