File uploads without a content type are treated as ordinary fields

[sjaensch]

Long story short

Multipart file uploads do not work correctly since aiohttp 3.6.0 unless a content type is provided. This used to work up to aiohttp 3.5.4 and broke due to #3905, this line specifically.

Expected behaviour

File upload data is treated as a binary data stream if no content type is provided.

Actual behaviour

File upload data is treated as an ordinary text field, possibly causing an UnicodeDecodeError.

Steps to reproduce

1. Send a request to an aiohttp server with a multipart/form-data payload, where one item is a file without a content type. The requests library will not provide a content type unless you specifically set one.
2. Call await request.post() in your aiohttp server.

Your environment

This issue is causing integration tests for bravado-asyncio to fail - the build before this one passed, the only change is the upgrade from aiohttp 3.5.4 to aiohttp 3.6.0.

[sjaensch]

@kornicameister why did you add the additional check for field_ct to the if condition?

[kornicameister]

@sjaensch the reason was that FileField expected content type that is set. It actually made a lot of sense to know what was sent to be able to handle that.

It is actually kind of surprising to hear that you have sent files without a content type. Is that actually RFC, or sth, compliant? If we look here there is a note that part may have a content type that, if not set, defaults to text/plain. Effectively that means that you have sent a .txt file if a filename is present IMHO. Or maybe it means that a string has been sent. All in all, I am inclined to say that first thing is correct which means that my change was not that ok.

I am thinking now that my change could be better if I read attached RFC first.

[sjaensch]

@kornicameister I'm not arguing that a content type can be beneficial - the more metadata the better (usually). My point is that it should be valid to send a file without a content type, and we should still treat it as a file. In particular that means it should be treated as a stream of bytes that will not be modified; with your change this is not the case anymore, and in fact it's highly unlikely that file uploads without a content type work at all, since it treats them as utf8 text that it will decode.

[kornicameister]

Well...I changed my response at least 3 times already (phone typing) and I think that I agree with you. The way I would fix that to match RFC is to check if filename is set and there do sth like part_ct or 'text/plain' for FileField case.

[sjaensch]

This is the code in the requests library that deals with multipart file uploads. The content type for a file is None unless set explicitly.

Also I don't think we should set a content type we didn't receive. For the user it would be impossible to distinguish between a file we received with content type text/plain and one we received without content type. I don't see any advantage to that.

[webknjaz]

Each part MAY have an (optional) "Content-Type" header field, which defaults to "text/plain".

This explicitly says that if there's no header, the value is text/plain.

However, this point is also valid:

since it treats them as utf8 text that it will decode.

I think, that maybe we should assume text/plain but not attempt to decode it, keeping bytes as-is. But honestly, it smells like a hack...

Let's also ask @asvetlov.

[webknjaz]

Meanwhile, it'd be useful if somebody summarised how other web-server implementations handle this case.

[sjaensch]

I think the part about the Content-Type header was written with the non-file cases in mind. It does have a sentence about handing files. Unfortunately it is not written as clearly as it could, making it sound like the client has to send it:

If the contents of a file are to be sent, the file data SHOULD be labeled with an appropriate media type, if known, or "application/octet-stream".

It clearly intends files to be treated as a stream of bytes that is not to be modified, and if the media type is unknown we should assume application/octet-stream.

[kornicameister]

@webknjaz I'd like to point out that it might be that there was a mistake in FileField in the first place. My change was dictated by single simple fact: FileField content-type resolved to a str that cannot be Optional.

Back then, when @asvetlov and me were working on pushing the typing improvement, neither noticed that it might be invalid to assume that lack of content type doesn't mean not dealing with files. As linked RFC pointed out, content-type is totally optional and, like @sjaensch , mentioned for files most likely it defaults to application/octet-stream.