WebwareForPython / w4py3

Webware for Python 3
MIT License
20 stars 6 forks source link

Query string ignored by FieldStorage class on POST if content-type is binary #16

Closed zarquon5 closed 1 year ago

zarquon5 commented 1 year ago

Sorry to be the bearer of more bad news, but the following (new) FieldStorage test fails with w4py3 3.0.9:

    def testPostRequestWithBinaryDataAndQueryParams(self):
        content = b'\xfe\xff\xc0'
        with self.assertRaises(UnicodeDecodeError):
            content.decode('utf-8')
        length = len(content)
        fs = FieldStorage(fp=BytesIO(content), environ={
            'REQUEST_METHOD': 'POST',
            'CONTENT_LENGTH': length, 
            'CONTENT_TYPE': 'application/octet-stream',
            'QUERY_STRING': 'a=1&b=2&b=3&c=3'})
        self.assertEqual(fs.headers, {
            'content-type': 'application/octet-stream',
            'content-length': length})
        self.assertEqual(fs.type, 'application/octet-stream')
        self.assertEqual(fs.length, length)
        self.assertEqual(fs.bytes_read, length)
        self.assertEqual(fs.file.read(), content)
        self.assertEqual(fs.getfirst('a'), '1')    # this fails with a TypeError('not indexable')

That is, the query string is ignored if there is a non-empty binary payload in the post. The type of binary data doesn't matter - I just copied the non-utf8 case, since it was closest to my use case, to show the problem. (I ran into this when I tried to post a binary payload with some query parameters that indicate where to store that payload.)

In trying to diagnose this, I looked into the test:

    def testPostRequestWithQuery(self):
        fs = FieldStorage(fp=BytesIO(), environ={
            'REQUEST_METHOD': 'POST', 'QUERY_STRING': 'a=1&b=2&b=3&c=3'})
        self.assertEqual(fs.getfirst('a'), '1')
        self.assertEqual(fs.getfirst('b'), '2')
        self.assertEqual(fs.getfirst('c'), '3')
        self.assertEqual(fs.getlist('a'), ['1'])
        self.assertEqual(fs.getlist('b'), ['2', '3'])
        self.assertEqual(fs.getlist('c'), ['3'])

to figure out why it passes, but my new one does not. Reading the code, AFAICT, if the headers are None, the content-type defaults to 'application/x-www-form-urlencoded', which triggers read_urlencoded(), which calls parse_qsl; this is also called within read_multi().

However, when the content-type is a binary type, read_single() is called, but that method does not call parse_qsl. (I couldn't determine whether/how the qs or qs_on_post gets/should get set for this path, so I am not sure what the correct code fix should be.)

zarquon5 commented 1 year ago

@Cito Any updates on this issue?

Cito commented 1 year ago

Had no time to look into this so far, but will do soon.

Cito commented 1 year ago

@zarquon5 Thanks for reporting. Looks more like a so far unsupported use case than a bug, but I fixed this now in 72c94e8. Can you confirm that this solves your issue?

zarquon5 commented 1 year ago

@Cito Thanks for the change; it does solve my issue. (BTW, I think this used to work with w4py2, so I'm not sure what changed in between...) Any expected timeframe for the next point release?

Cito commented 1 year ago

Probably over the weekend. Will then also check if/why it worked with w4py2 to make sure that this is the right fix.

Cito commented 1 year ago

@zarquon5 New release 3.0.10 is out. You were right, it worked in w4py2. What changed in between was that w4py2 used the original FieldStorage method with a wrapper that handled POST requests, while w4py3 uses an intermal modified version of FieldStorage which handles POST support inside (but not in that one case). Note that query params with POST only work with Webware, the normal FieldStorage never supported it.