Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestMediaWithoutFileName test does not allow changes in content type between media upload and download #49

Closed
anoadragon453 opened this issue Dec 7, 2020 · 8 comments · Fixed by #51

Comments

@anoadragon453
Copy link
Member

anoadragon453 commented Dec 7, 2020

The tests:

  • Can download without a file name locally
  • Can download without a file name over federation

that are a part of TestMediaWithoutFileName both have a check for the Content-Type header value that was provided during initial upload of the test media. They ensure this value does not change between uploading and downloading media and its associated metadata.

Synapse will however augment the content type of text files in order to specify UTF-8 as their encoding. The reasoning behind this is at matrix-org/synapse#7043.

While the spec doesn't explicitly state that the Content-Type response header when downloading a file needs to exactly match what was provided during upload, it does state that it should be the content type of the file, and so homeservers should at least try to get it right.

The original sytest this was taken from, Can download without a file name locally, did not check Content-Header. I personally think it is valuable to do so, but we may want to make an exception for this modification, as it does seem to be beneficial and not against the spec, however vague it might be.

@richvdh
Copy link
Member

richvdh commented Dec 8, 2020

both have a check for the request's Content-Type header value.

which request? The one Complement sends to the server to fetch the media? That seems to make no sense?

@anoadragon453
Copy link
Member Author

both have a check for the request's Content-Type header value.

which request? The one Complement sends to the server to fetch the media? That seems to make no sense?

No sorry, the Content-Type header of the initial upload media request.

@kegsay
Copy link
Member

kegsay commented Dec 8, 2020

Maybe just check the mime-type on the Content-Type header and not a literal string match?

@anoadragon453
Copy link
Member Author

Right, looks like we can use http.DetectContentType for this.

@anoadragon453
Copy link
Member Author

http.DetectContentType returns text/plain; charset=utf-8 lol.

I'll just do a split on the first string before ;.

@turt2live
Copy link
Member

fwiw this is a conversation that should happen in the spec at some point - I have fairly strong opinions that the server should not be doing certain things to the header (like interpreting the body to pick a different header) or changing charsets on people, however the spec does indeed do a poor job of explaining what the requirements are. In general, the server is expected to be largely a passthrough for the content-type header, but whether that makes sense or not is a spec concern.

Ideally complement doesn't stray away from the spec like sytest did/does, so it would be good to solve this for everyone and not patch over it.

@anoadragon453
Copy link
Member Author

@turt2live Good point. Is there a spec issue for this already?

@turt2live
Copy link
Member

@anoadragon453 matrix-org/matrix-spec-proposals#2701

anoadragon453 added a commit that referenced this issue Dec 11, 2020
…ad (#51)

Fixes #49

By checking just the mimetype instead of an exact string match, homeservers are allowed to add charsets and other directives if they choose. As long as the mimetype stays the same, we're happy.

A comment on the original issue states that the spec is a bit vague about this, and that ideally it would be more explicit about what should happen here. At the moment though this is all we're going to test as it matches the expected behaviour from homeservers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants