Require Content-Type for /upload #1761
Conversation
Because this is what error message from the server says. Signed-off-by: Alexey Rusakov <[email protected]>
|
This kinda feels like a synapse bug and not a spec bug. Although good to provide, I can't really think of a reason why the media repo absolutely needs to know what you're uploading. If it wants to verify that you're only uploading images (for example), it shouldn't be relying on the user-supplied value anyways. |
|
Well, given that the sending client by definition has a bit more information on the MIME type and that the parameter is there anyway, I believe the question of verification does not really stand. With that said, being able to rely on the server's guesswork would be handy for clients. Qt knows how to detect MIME types so I can deduce them from the content; so I as a QMatrixClient developer I don't have a strong opinion on this. But other clients might not necessarily have a MIME database at hand. |
|
The mimetype is optional everywhere else in the spec. Although helpful to supply, the sending client shouldn't be required to send the type to the media repo. My argument about server side validation is more about preventing users from uploading certain types of media. The server shouldn't trust that the client is sending the right mimetype and may have to figure out what the real mimetype is. The client -supplied value is really only a guideline for future downloaders, and the media repo can easily serve a generic non-descript file. |
|
I've since been convinced that it should be required, although I didn't document it here (sorry). Asking for review from the rest of the team to spark conversation. |
There was a problem hiding this comment.
Given that synapse currently requires you to give a content-type, we should certainly either fix synapse to match the spec, or the spec to match synapse.
I've got to say, though, my first reaction is to agree with Travis's first impression that this is a synapse bug rather than a spec bug. My expectation would be that the server should treat the content-type as application/octet-stream by default.
We should also consider specifying what the server is actually supposed to do with the content-type. Does it just parrot it back for future download requests (in which case, is it supposed to validate it somehow? Are there security risks associated with allowing arbitrary content-types?) or does it also use it as a basis for decisions such as whether to thumbnail?
|
Should this be turned to a Synapse bug then? |
|
@KitsuneRal I suspect this needs a bit more input from the spec core team. Possibly just writing a MSC to answer vdh's questions will be enough to make the spec and Synapse match once again. |
|
Can I just turn this PR into an MSC or is it better to spawn a separate thing? |
|
I wouldn't be bothered by it, though the changes to the spec can't be in the same PR as the MSC |
|
Oh, right; a separate PR then. |
|
#2701 clarifies this |
|
this seems to have been superceded by #2701. |
Because this is what the error message from the server says if I omit Content-Type. There might be other endpoints with the same bug - feel free to indicate and I'll add them to the PR.