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.

Sign up for GitHub

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

Jump to bottom

MSC2246 Asynchronous uploads updates from Travis's review #2

Merged
merged 7 commits into from
Mar 31, 2023
Merged

MSC2246 Asynchronous uploads updates from Travis's review #2

merged 7 commits into from
Mar 31, 2023

Conversation

sumnerevans
Copy link

@sumnerevans sumnerevans commented Mar 29, 2023 •
edited
Loading

Addresses some of the feedback from Travis's review.

matrix-org#2246 (comment)

Things that we still need to do:

  • Security considerations

  • Discuss additional spam reduction options

    • The main vector of attack that is sending a ton of un-uploaded MXC URIs to a room and then creating an amplification attack against the media repo due to all clients opening a connection to download the never-to-be-uploaded media. Travis's suggestion is to limit the number of upload-pending URIs per-user.
    • Travis also wants to allow for uploads to take an infinite amount of time (which sounds like a horrible idea for chat). For us, I think we want to avoid specifying what clients should do as much as possible, so maybe we actually don't care if uploads are allowed to take infinite time?

@sumnerevans
Change /create endpoint to use v1
f438754 
Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Reorganize /upload spec and integrate feedback
725675c 
* Explicitly specify that M_NOT_FOUND should be used for expired media
* Explicitly specify that M_FORBIDDEN should be used when a user other
  than the one who created the media ID tries to upload to it
* Remove content-length failure note

Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Rename max_stall_ms -> timeout_ms
d55f1f9 
Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Mention that maximum value for timeout_ms should be imposed by the se…
955177b 
…rver

Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Mention that the timeout_ms can be ignored if the media exists already
823fcca 
Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Change M_NOT_YET_UPLOADED to use 504 status code
3b00026 
Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans
Remove retry_after_ms optional field
9627af2 
Signed-off-by: Sumner Evans <sumner@beeper.com>
@sumnerevans sumnerevans changed the title Asynchronous uploads updates from Travis's review MSC2246 Asynchronous uploads updates from Travis's review Mar 29, 2023
@sumnerevans sumnerevans mentioned this pull request Mar 30, 2023
Merged
@bradtgmurray
Copy link

lgtm

@tulir tulir merged commit 9627af2 into tulir:asynchronous_uploads Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradtgmurray bradtgmurray bradtgmurray left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sumnerevans @bradtgmurray @tulir