-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC2398: proposal to allow mxc:// in the "a" tag within messages #2398
base: old_master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: Erkki Seppälä <flux@modeemi.fi>
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||||
# MSC2398: Proposal to allow mxc:// in the `a` tag within messages | ||||||
|
||||||
Currently message events (m.room.message) can contain formatted data | ||||||
in its formatted_body field, if the format of the message is | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
org.matrix.custom.html. In this subset of HTML `img` tags are permitted | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if they refer to media repository contents with the mxc schema and | ||||||
clients are able to resolve these URIs to (ie.) HTTPs to retrieve the | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Sorry if these feel nitpicky, I'm just having to re-read a couple of times to work out what the MSC is saying so attempting to clarify. |
||||||
actual contents. | ||||||
|
||||||
However, the spec currently forbids `a` tags from receiving the same | ||||||
treatment. These links could be useful for example for systems that | ||||||
upload PDF files to the media storage and then produce messages | ||||||
linking to such files. | ||||||
|
||||||
## Proposal | ||||||
|
||||||
The spec for `a` tags says: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
-`a`: | ||||||
`name`, `target`, `href` (provided the value is not relative and has a scheme | ||||||
matching one of: `https`, `http`, `ftp`, `mailto`, `magnet`) | ||||||
|
||||||
Clients would be able to handle the mxc schema here in the exact same | ||||||
way they handle `img` tags. Thus the proposal is to modify that fragment | ||||||
to say: | ||||||
|
||||||
-`a`: | ||||||
`name`, `target`, `href` (provided the value is not relative and has a scheme | ||||||
matching one of: `https`, `http`, `ftp`, `mailto`, `magnet`, or `mxc` | ||||||
for `Matrix Content (MXC) URI`) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much as it's obvious, presumably the spec would also need to explicitly say that the client must transform the mxc link into an appropriate link the browser will understand (ie. what you talk about above with img tags). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like something which is best served by a note rather than requirement. Web clients will probably need to convert it, but other clients might be able to handle a custom scheme for network requests. I'd suggest that this not be a blocking concern for FCP, as the spec text can provide guidance on how to handle it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would really prefer the MSC say explicitly that clients should manage this scheme in one way or another - ultimately a custom handler will have to translate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait but isn't "Clients would be able to handle the mxc schema here in the exact same way they handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think @dbkr? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it just seemed odd to me that an explicit change to the spec text was given but then additional behaviour specified in the MSC. I suppose it's all there though so it doesn't really matter. |
||||||
|
||||||
## Potential issues ## | ||||||
|
||||||
As far as I see, this is quite a benign change. | ||||||
|
||||||
The only problem I see (in addition to possible problems already | ||||||
present in `img` tags) is that current clients may not be able to handle | ||||||
mxc tags in this position; thus such clients would not be able to | ||||||
retrieve resources linked this way. Some clients may already | ||||||
accidentally support this, though, due to consistent handling of URIs. | ||||||
|
||||||
## Security considerations ## | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably worth spelling out explicitly here that the security risks of linking directly to content in the repository is not just HTML, but any format which can execute code and potentially XSS needs to be mitigated by a valid CSP - e.g. JS, PDF, SWF, SVG. This isn't anything new, but it's still a security consideration. I can't think of a world where a HTTPS url derived from an MXC URL wouldn't be subject to CSP when a plain HTTPS url to the same content would, but still worth spelling it out at least in the MSC, if not the final spec. |
||||||
|
||||||
There is some potential for security issues in hosting HTML in general | ||||||
in the matrix media repository, but using MXC schema does not affect | ||||||
much the scope of such attacks as such URIs can already be constructed | ||||||
with current support - unless protections for such attacks are | ||||||
fundamentally based on the URI format. | ||||||
|
||||||
## Alternatives | ||||||
|
||||||
The alternative to using `mxc://` would be to construct direct | ||||||
relative links via `/download` (though relative links are also | ||||||
prohibited by the current spec). Using the `mxc://` scheme seems | ||||||
cleaner, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this proposal interact with MSC3916, which basically adds authentication to the media API? I assume if we can make it work for
img
tags we can make it work fora
, but I think that needs to be verified.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The media ID would be attributed to a single event somewhere, and clients would be translating
mxc://
tohttps://
for the user. This then behaves no differently than if a user were to get linked to the download endpoint directly - authentication would be required, and if that user can't see the original event then the link is broken for that user.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But getting linked directly to the download endpoint won't work, as there is no way for a user to authenticate AIUI? I thought for media web clients would potentially need to download the media in service workers or whatever to make the authentication work, in a similar manner as today to make encrypted attachments work. Is that method viable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A service worker would be required, which should be workable. The client can essentially iframe it and append the appropriate headers. This then ties into Matthew's comment about security considerations: #2398 (comment)
I'm tempted to suggest a blocking concern on this as it feels valuable to test/implement, particularly while MSC3916 and friends are being experimented with. It's akin to custom emoji/stickers where I think we want to avoid making the problem worse short-term if we can avoid it.