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

Add the Tus-Max-Size extension #310

Closed
wants to merge 1 commit into from
Closed

Add the Tus-Max-Size extension #310

wants to merge 1 commit into from

Conversation

mitjap
Copy link
Collaborator

@mitjap mitjap commented Oct 4, 2022

Closes #330

This PR is first step to support Tus-Max-Size extension. It limits amount of data uploaded to not exceed Upload-Length.

Current implementation does not wait for request stream to end. When limit is reached response successful response is sent back to the client which closes a socket. After receiving a response with status 204 client will also receive an error that connection was closed (indicating failed upload). This is similar to tusd behavior.
Alternative approach would be to wait for request stream to end and then send a response back to the client. This can lead to large delay between file being complete and response and potentially lots of data being uploaded that is just ignored. Upside is that no client error is reported.

Some test will need to be rewritten. When testing PATCH request it is now required that stream is passed to PatchHandler instead of just plain object with header property. I'll wait with this PR for TS migration (#308) to be finished. Meanwhile we have time for discussion.

We should discuss how the server should handle such scenario. Another option is to send a 413 (enitity too large) instead of 204.

So we need to decide:

  • How to respond? (error or succes)
  • When to respond? (immediately or delayed)

@mitjap mitjap mentioned this pull request Oct 4, 2022
13 tasks
@Murderlon Murderlon marked this pull request as draft October 4, 2022 20:40
@Murderlon Murderlon changed the base branch from master to 1.x October 4, 2022 20:40
@Murderlon Murderlon added the 1.0 This is for the 1.0 major version label Oct 4, 2022
@Murderlon
Copy link
Member

This PR is now ready to be worked on again. As for the approach, let's immediately respond with 413 if a PATCH request arrives with a Content-Length that exceeds the upload size.

@Murderlon Murderlon changed the title Prevent data upload beyond 'uplaod-length' bytes Add the Tus-Max-Size extension Oct 25, 2022
@Murderlon Murderlon mentioned this pull request Nov 7, 2022
@Murderlon Murderlon linked an issue Nov 7, 2022 that may be closed by this pull request
@Murderlon
Copy link
Member

@mitjap would you be willing to get this over the finish line soon?

Base automatically changed from 1.x to master December 8, 2022 17:58
@BlakeB415
Copy link

BlakeB415 commented Jan 4, 2023

Any updates on this?

May I also suggest a way to set this limit on a per-upload basis using hooks?

@mitjap
Copy link
Collaborator Author

mitjap commented Jan 25, 2023

@Murderlon @Acconut Sorry for the delay. I'm still not sure when I'll be able to finish this. But don't lose hope :)

I think we should first decide on how and when to respond. Any feedback on this? I think for this we can not use tusd implementation as reference for this as it returns error 500 when sending more data than announced with Upload-Length.

@BlakeB415
Copy link

BlakeB415 commented Jan 26, 2023

@Murderlon @Acconut Sorry for the delay. I'm still not sure when I'll be able to finish this. But don't lose hope :)

I think we should first decide on how and when to respond. Any feedback on this? I think for this we can not use tusd implementation as reference for this as it returns error 500 when sending more data than announced with Upload-Length.

For exceeding the max file size, 413 Request Entity Too Large is what the spec defines.

If the length of the upload exceeds the maximum, which MAY be specified using the Tus-Max-Size header, the Server MUST respond with the 413 Request Entity Too Large status.

https://tus.io/protocols/resumable-upload.html

@Acconut
Copy link
Member

Acconut commented Feb 10, 2023

I think for this we can not use tusd implementation as reference for this as it returns error 500 when sending more data than announced with Upload-Length

This should not be the case, unless it's a bug. tusd is programmed to return a 413, just as @BlakeB415 mentions:

https://github.com/tus/tusd/blob/0e1169bdf182253d73c58bd118d6679cfb64b496/pkg/handler/unrouted_handler.go#L58
https://github.com/tus/tusd/blob/0e1169bdf182253d73c58bd118d6679cfb64b496/pkg/handler/unrouted_handler.go#L65

If you know more about when it returns a 500, let me know and I will try to fix it.

So +1 from me for 413.

@BlakeB415
Copy link

Can we pick this up again? This is a needed feature.

@Murderlon
Copy link
Member

I can take this PR over. However I'm about to release 1.0.0 today or tomorrow and I don't have a date for you when I'll work on this yet. In the meantime the codebase has significantly changed as well, it's all TS while this is still in JS.

@fenos
Copy link
Collaborator

fenos commented Feb 11, 2024

@Murderlon closing this, since has been implemented in #517

@fenos fenos closed this Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 This is for the 1.0 major version enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Max upload size limit
5 participants