replace cgi.FieldStorage by multipart

[d-maurer]

This PR replaces cgi.FieldStorage by multipart, avoiding the cgi module deprecated by Python 3.11.

In addition:

• fixes the encoding handling and lets the :bytes modifier work as expected
• marks binary converters with a true binary attribute

The PR uses the surrogateescape encoding error handler for its encoding handling (see PEP 383). If form data in an unexpected encoding is processed, surrogates may reach application code and cause delayed problems because the default strict encoding error handler cannot process them. Potentially, a check for surrogates should be implemented to report such problems early.

[icemac]

Looks good from quickly scanning though the code.

[mauritsvanrees]

@d-maurer Fist: thanks for having worked on this!

But when I use a checkout of Zope in the Plone 6 core development buildout, I get test failures due to this change. See plone/buildout.coredev#844 and mostly this comment by @jensens. With your merged PR we get this error in the tests of three packages:

FileUpload.__init__() missing 1 required positional argument: 'charset'

@ale-rt suggests to update Zope to change the signature of FileUpload and make charset an optional keyword argument. That fixes most of the test failures. I think this makes sense, and is an easy fix.

Then the question is what we should use as default value for charset. I see two options:

1. charset="latin-1". This is what Alessandro proposes, and it fixes all Plone tests.
2. charset=default_encoding. This is defined earlier on in HTTPRequest.py as utf-8 (with a remark that it might be overridden in config). With this, some tests in plone.formwidget.namedfile still fail. That starts here with a test that explicitly uses a non-ascii filename. This test was added 12 years ago and it would be sad if this now fails. ;-)

This test makes me wonder if the following part of your fix is correct:

class FileUpload:
...
    def __init__(self, aFieldStorage, charset):
...
        self.filename = aFieldStorage.filename.encode("latin-1").decode(charset)

Why is latin-1 hardcoded here? The plone.formwidget.namedfile test fails because of this when I would change the FileUpload class to use utf-8 as charset. The test plus your code can be simplified like this in pure Python:

>>> filename = b'rand\xc3\xb8m.txt'.decode('utf8')
>>> print(filename)
randøm.txt
>>> def handle_filename(filename, charset):
...     return filename.encode('latin-1').decode(charset)
... 
# Good:
>>> handle_filename(filename, 'latin-1')
'randøm.txt'
# Bad:
>>> handle_filename(filename, 'utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in handle_filename
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xf8 in position 4: invalid start byte

In other words, charset='latin-1' seems the only safe option. But then I wonder if the encoding and decoding is still needed.

I have prepared a PR.

Belatedly, I thought I would try to upload a non-ascii file in an actual browser and a running Plone Site, instead of in a 12 year old test that may or may not make sense. I created a file like this:

echo "randøm.txt" > randøm.txt

This is on Mac with Firefox.
Uploading this file went fine, both with charset latin-1 and utf-8. So that is good.
And actually when I go in with a pdb, I get this:

(Pdb++) aFieldStorage.filename
'randøm.txt'
(Pdb++) aFieldStorage.filename.encode("latin-1")
b'rand\xc3\xb8m.txt'
(Pdb++) aFieldStorage.filename.encode("latin-1").decode("utf-8")
'randøm.txt'
(Pdb++) aFieldStorage.filename.encode("latin-1").decode("latin-1")
'randøm.txt'

So here we do actually get a filename as input which is latin-1. So your encode("latin-1") works fine here.
And no matter whether we then decode to utf-8 or latin-1, the end result when I look in Plone is fine: the id of the content item is normalised to random.txt (done by Plone I think), the title is randøm.txt, and when you download it, you again get a file randøm.txt.

In other words, I am not sure what to make of this, and which part of the code fixes the filename after FileUpload.__init__ has finished. Maybe another part of your PR does that.
Anyway, I still think adding a default value for the charset parameter is helpful. I am happy to get advice on what its value should be.

[d-maurer]

Maurits van Rees wrote at 2023-3-7 15:52 -0800:

... This test makes me wonder if the following part of your fix is correct: ``` class FileUpload: ... def __init__(self, aFieldStorage, charset): ... self.filename = aFieldStorage.filename.encode("latin-1").decode(charset) ``` Why is `latin-1` hardcoded here?

The data usually used as filename of a `FileUpload` comes from an HTTP header and HTTP/1.1 uses `latin-1` as `charset` for its headers (even though modern versions deprecate the use of non-ASCCI characters). Note that "latin-1" here does not stand for a "true" charset: it is a technical device to decode a byte sequence into an `str` and postpone the real decoding until the "true" charset is known. I made file upload tests with `firefox`. Apparently, it encodes the filename with the charset associated with the form to get a sequences of bytes. It puts this byte sequence into the HTTP request's `Content-Disposition` header (for the upload). Thus, logically, the header value is the byte sequence corresponding to the encoding of the filename with the form's charset. `FileUpload` does not know about the form's charset; it must be told about it -- which does the new `charset` parameter. I would not mind to make `charset` optional. If it is used from `HTTPRequest`, `charset` will be provided. Thus, from `HTTPRequest`'s point if view, the default value could be anything. Note, however, that `FileUpload` assumes that `aFieldStorage` has values which come from an HTTP request, especially that its `filename` has a (transparently encoded) byte sequence as value which gives the correct filename once recoded with the provided `charset`. Maybe, this will force you to rework the tests anyway.

The `plone.formwidget.namedfile` test fails because of this when I would change the FileUpload class to use utf-8 as charset. The test plus your code can be simplified like this in pure Python: ``` >>> filename = b'rand\xc3\xb8m.txt'.decode('utf8') >>> print(filename) randøm.txt >>> def handle_filename(filename, charset): ... return filename.encode('latin-1').decode(charset) ... # Good: >>> handle_filename(filename, 'latin-1') 'randøm.txt' # Bad: >>> handle_filename(filename, 'utf-8') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 2, in handle_filename UnicodeDecodeError: 'utf-8' codec can't decode byte 0xf8 in position 4: invalid start byte ```

The test does not reflect the typical browser situation: The browser has a unicode filename; it must encode it with the form's charset and put the result into an HTTP `Content-Disposition` header which by the HTTP/1.1 spec uses `latin-1` as charset. Thus, your test should start with a unicode filename and encode this with the target charset -- leading to a sequence of bytes. You can decode any sequence of bytes with `latin-1` to get its standard representation as `str`. `handle_filename` assumes that the process above has been applied and recomputes the original unicode filename based on this assumption.