Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2246: Asynchronous media uploads #2246
MSC2246: Asynchronous media uploads #2246
Changes from 12 commits
b439277
a83c79c
9a395ed
29e3463
7cf22be
658aac8
bbd7d08
0bffcb7
4d009a9
1cbc04e
c65f2bf
63cef50
8ccf85f
12e907b
d582bb3
173edf3
f438754
725675c
d55f1f9
955177b
823fcca
3b00026
9627af2
045c21e
011031b
6cb7e31
fedc697
7652f59
098dd90
9559ab0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This endpoint, but not the newly-proposed
/upload
, doesn't qualify for a v3 version just yet under the new versioning scheme:(
PUT /upload
does qualify for thev3
version because the versioning is done on the path rather than the method, therefore since we're adding a backwards-compatible change to the client-server API version we do not need to bump it to v4. We're also not intending to support the v1 or (implied) v2 versions of/upload
which literally exist, so we stay with v3 - the current version).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.
I'm a bit unsure about this. The resources taken up by an unused media ID is minimal (just a row in the DB), whereas failing to upload media in time means that the associated event forever links to broken data.
The scenario I'm thinking of is dodgy/slow internet connection, where it wouldn't be unreasonable to struggle to upload the media in time. We could increase the recommended default expiry, but I'm not entirely sure what the expiry really buys in practice.
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 expiry is to prevent clients from retrying/waiting forever for files that are never uploaded. Increasing the suggested time may be reasonable if the upload fails for any reason as it would prevent retrying if you're uploading a large file making that media ID stale.
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.
I've been thinking about this a bit, and I agree it would be good to support both requests for the media a) initially blocking until the media starts getting uploaded, and b) after a while returning an error code immediately if the media hasn't been uploaded yet.
I guess the questions are:
Allowing late uploads means that every time a client comes to display the media it will retry, whereas if we return a 404 it could potentially cache that the media will never be uploaded. My hunch though is that clients won't ever bother caching error responses, so there's not much need to disallow late uploads?
I don't think this is a huge issue either way ftr.
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.
Web clients will cache the error response implicitly, particularly with aggressive cache-control headers, fwiw.
A possible attack angle here is that a malicious user/client could reserve a ton of event IDs (hundreds per minute with current rate limits and how cheap this request is to call) and send those into a room. The clients in that room then valiantly try to download that media, exhausting the available connections on the media server while it sits there blocking for media. This is somewhat of a risk if the user were to upload a ton of media to a room today, however servers generally cache or keep the recently uploaded media close so can respond to an influx of download requests quickly, reducing the attack surface by enough to not be as much of a concern.
To resolve this, maybe we don't use rate limits as our primary defence. We should still rate limit to avoid clients sending a billion requests per second, but we could additionally ask that servers employ an internal (undocumented/not-revealed) quota for the number of incomplete uploads a user can have. When the user exceeds that quota, the oldest incomplete upload(s) are expired such that they return immediate 404s to
/download
and similar.If the server uses a small enough quota (say, 10?), then at worst the user can cause fewer requests which contribute to the server's maximum open file limit, similar to if the client uploaded large files which force the connection to be held open for longer.
This sort of quota system also allows clients to take an eternity to download and re-upload their files, which I think should be a feature of an async upload. A use case would be a non-messenger client which knows that an MXC URI is needed but the file upload is reliant on user input: it could take a few minutes for the user to find the file on their harddrive or they might realize that it's on a whole other machine and finish the upload the next day.
For clarity: we wouldn't have an expiration timestamp with the above quota system.
Also: I don't think quota is the word I'm after, but it's the word I'm using.
(I am concerned about this enough to block FCP, ftr)
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.
An invisible quota system might be more a problem than a solution, as if a client expects to be able to upload 50 concurrent files, while the server internally only allows 10, then it'll result in unstable/unusable UX. (E.g. those first 40 files erroring out on others' screens, and/or erroring on the uploader's side, depending how this is implemented)
If there's a quota, the server should expose it, so clients can schedule uploads accordingly.
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.
it's quite a bit different, yes. With this MSC the attacker can exhaust a server's resources trivially, per my comment above.
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.
If we go with the quota idea, it seems like we would want to have an expiration timeout still, in case a client crashes and never finishes the upload.
We don't want a dead upload to permanently cause the user's quota to be lowered.
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.
What if we make the download/thumbnail endpoints return immediately when if the media isn't uploaded yet, but give some metadata?
For example,
upload_status
ofUPLOAD_NOT_STARTED
,UPLOAD_STARTED
in those cases. If the media is uploaded, then it can just return immediately as well.This will allow clients to show something useful in these cases as well.
I think this solution would prevent malicious actors from causing servers to keep open tons of connections as they can respond immediately, and close the connection.
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.
There's also a new potential usecase on the horizon where we'd want extremely long-lived media uploads (could take literal hours to finish uploading the media), so would be good to consider what a timeout-less system looks like I think.
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.
I believe that these concerns have been addressed in the latest version of the MSC. The main mechanisms are:
timeout_ms
if desired (for example, if the server is running out of available connections)I think that exploring a timeout-less system should be done in a separate MSC.
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.
Is the intention that this endpoint support the
content-type
header andfilename
query parameter, as https://spec.matrix.org/v1.6/client-server-api/#post_matrixmediav3upload does? It looks like the sample implementation at https://github.com/turt2live/matrix-media-repo/pull/364/files#diff-3ddbc505e50723b8440369eef4dc1fa055026668151b1dbecbd1725fc6765727 does, but this isn't mentioned in the MSC.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.
Yes. In the future those details could be moved to the /create call request body, but that wasn't defined here