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

Fix EncryptedFile lacking mimetype #2581

Closed

Conversation

Sorunome
Copy link
Contributor

The example already had the mimetype parameter, but the definition lacked it.

Signed-off-by: Sorunome mail@sorunome.de

@turt2live turt2live requested a review from a team May 26, 2020 14:05
@turt2live
Copy link
Member

From the room: it's somewhat unknown if this is an implementation bug bleeding into the spec ( https://github.com/matrix-org/matrix-doc/issues/2374 ) or an omission.

@Sorunome
Copy link
Contributor Author

MSC to remove it: #2582

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also a changelog please :)

@@ -299,6 +299,7 @@ hashes {string: string} **Required.** A map from an algorithm name to a hash
``sha256``.
v string **Required.** Version of the encrypted attachments
protocol. Must be ``v2``.
mimetype string The mimetype of the file, e.g. ``image/jpeg``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepbluev7
Copy link
Contributor

I don't like adding this as a required field, when the intention is to remove it again later. That will just break interaction between clients, that implemented this according to the current spec, which is missing the mimetype. If this should be added as a required field, this should go through a MSC imo, since it will break client interactions. I don't see, why the original intention being different, which isn't really documented anywhere publicly from what I can tell, should remove the need for an MSC. Last time that was done (#1939), it caused multiple bug reports and complaints against my client, because my client followed the spec, but the spec was changed, when I wasn't looking (and the example wasn't updated).

If this should be added to the spec, this would also need an explanation regarding what should be sent as the mimetype. Should it be the mimetype of the file after decryption or should it be "application/octet-stream", which matches the encrypted blob? It would also need to be documented, how conflicts with the info objects should be handled. If my client sends 2 different mimetypes in info and EncryptedFile, is the event considered invalid, the one from info taken or from EncryptedFile?

Also how should events be handled, that don't have this required field? Should clients just not decrypt such media anymore just because the spec was changed?

Since I got burned by such changed a few times already, I would prefer, if this doesn't get merged without an MSC, even though it would fix the spec issue I actually raised initially (#2374). After a bit of discussion in #matrix-spec I chose to not send mimetype and and opened that issue, but I don't remember, if I was encouraged to do that or I misunderstood/decided that myself.

@turt2live
Copy link
Member

Let's just leave this as an omission then until the MSC gets resolved in some way.

@turt2live turt2live closed this May 26, 2020
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 this pull request may close these issues.

3 participants