Skip to content

bpo-27777: fix cgi.FieldStorage parsing for body with Content-Length and no Content-Disposition#21457

Closed
ar45 wants to merge 4 commits into
python:mainfrom
ar45:issue27777
Closed

bpo-27777: fix cgi.FieldStorage parsing for body with Content-Length and no Content-Disposition#21457
ar45 wants to merge 4 commits into
python:mainfrom
ar45:issue27777

Conversation

@ar45
Copy link
Copy Markdown

@ar45 ar45 commented Jul 13, 2020 •
edited
Loading

cgi.FieldStorage can't parse simple body with Content-Length and no Content-Disposition

  • If content length is present, read at most len bytes.
  • When reading read_lines_to_eof use read instead of readline.
  • Use self.__write even when content length is present and > 1000.

https://bugs.python.org/issue27777

ar45 added 4 commits July 13, 2020 00:55
@ar45
Fixes issue 27777: cgi.FieldStorage can't parse simple body with Cont…
8e491e7 
…ent-Length and no Content-Disposition

- If content length is present, read at most len bytes.
- When reading `read_lines_to_eof` use `read` instead of `readline`.
- Use `self.__write` even when content length is present and > 1000.
@ar45
Updated test_cgi.py
74afddd 
Renamed `test_fieldstorage_readline` to `test_fieldstorage_read`
and test `read` instead of `readline` since we use `read`.
@ar45
Update 2019-02-05-16-57-27.bpo-27777.AKUxDR.rst
1dd58c9 
Added mention of FieldStorage
@ar45
Added test test_content_length_no_content_disposition from python#10771
8d1a64b
@ar45 ar45 requested a review from ethanfurman as a code owner July 13, 2020 06:03
@ar45 ar45 changed the title Issue27777 bpo-27777 Jul 13, 2020
@ar45 ar45 mentioned this pull request Jul 13, 2020
Closed
@merwok
Copy link
Copy Markdown
Member

merwok commented Jul 14, 2020

Another PR is open: #10771

Long discussion in the bug report.

@merwok merwok changed the title bpo-27777 bpo-27777: fix cgi.FieldStorage parsing for body with Content-Length and no Content-Disposition Jul 14, 2020
@ar45
Copy link
Copy Markdown
Author

ar45 commented Jul 14, 2020

Another PR is open: #10771

Long discussion in the bug report.

See comment on that PR for why I opted to do it differently
#10771 (comment)

@jugmac00 jugmac00 mentioned this pull request Aug 3, 2020
Closed
cjwatson added a commit to cjwatson/zope.publisher that referenced this pull request Aug 4, 2020
@cjwatson
Fix parsing for body with Content-Length and no Content-Disposition
f425c11 
By Aron Podrigal; backported from
python/cpython#21457.
@cjwatson cjwatson mentioned this pull request Aug 4, 2020
Closed
@ar45
Copy link
Copy Markdown
Author

ar45 commented Aug 12, 2020

@ethanfurman Do you mind reviewing this?

@kulikjak
Copy link
Copy Markdown
Contributor

kulikjak commented Sep 25, 2020 •
edited
Loading

We did some internal testing with this patch and found a bug with the following trace:

Traceback (most recent call last):
  File "/var/ai/image-server/cgi-bin/telemetry.py", line 834, in <module>
    sys.exit(main())
  File "/var/ai/image-server/cgi-bin/telemetry.py", line 815, in main
    form = cgi.FieldStorage()
  File "/usr/lib/python3.7/cgi.py", line 494, in __init__
    self.read_single()
  File "/usr/lib/python3.7/cgi.py", line 687, in read_single
    self.read_content()
  File "/usr/lib/python3.7/cgi.py", line 710, in read_content
    self.__write(data)
  File "/usr/lib/python3.7/cgi.py", line 729, in __write
    data = self.__file.getvalue()
AttributeError: _io.TextIOWrapper object has no attribute getvalue

This happens when self.length > 1000:

    if self.length > 1000:
        self.file = self.make_file()

make_file() internally calls tempfile.TemporaryFile(), which returns TextIOWrapper.

Then, in the __write(), self._write(data) is called, which fails with the aforementioned error:

def __write(self, line):
    """line is always bytes, not string"""
    if self.__file is not None:
        if self.__file.tell() + len(line) > 1000:
            self.file = self.make_file()
            data = self.__file.getvalue()    <- here

You can reproduce this simply by sending much bigger body in added test_content_length_no_content_disposition()

@rr rr mannequin mentioned this pull request Apr 11, 2022
Closed
@AlexWaygood
Copy link
Copy Markdown
Member

AlexWaygood commented Apr 11, 2022

@ar45, thanks for the PR. Unfortunately, the cgi module is now deprecated following the acceptance of PEP 594, so bugfixes and improvements for this module will no longer be accepted. I am therefore closing this PR.

I hope that this does not dissuade you from contributing to CPython in the future 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ethanfurman ethanfurman Awaiting requested review from ethanfurman

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ar45 @merwok @kulikjak @AlexWaygood @the-knights-who-say-ni @bedevere-bot