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

@tus/server: support Tus-Max-Size #517

Merged
merged 4 commits into from Dec 21, 2023
Merged

Conversation

fenos
Copy link
Collaborator

@fenos fenos commented Nov 13, 2023

Based on the Tusd implementation, this PR implements a configurable maxSize option which will limit and validate incoming file size.

The only addition I've made is that in tusd it doesn't seem are limiting the request body to the maxSize specified.
They simply validate the upload-length header against the maxSize.

However, in this PR if the body reaches the maxSize bytes read, it will throw a 413 error and stop uploading.
Not sure about the consequences as in what happens to the part of the file that has been already uploaded?

In S3 we can set a policy to delete uncompleted multi-part but what about for fileStore?

@fenos fenos force-pushed the feat/max-file-size branch 5 times, most recently from eef69c5 to a987021 Compare November 20, 2023 10:33
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

There is still some docs missing on this new option.

packages/server/src/handlers/BaseHandler.ts Outdated Show resolved Hide resolved
packages/server/src/handlers/BaseHandler.ts Outdated Show resolved Hide resolved
packages/server/src/handlers/BaseHandler.ts Show resolved Hide resolved
packages/server/src/handlers/BaseHandler.ts Show resolved Hide resolved
packages/server/src/models/DataStore.ts Outdated Show resolved Hide resolved
packages/server/src/models/StreamLimiter.ts Show resolved Hide resolved
@Murderlon Murderlon changed the title feat: Max file size @tus/server: support Tus-Max-Size Nov 21, 2023
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thanks to @fenos, we discovered a flaw in tusd's logic around handling size limits, so I would be a bit hesitant to copying its logic for now until we fixed that bug: tus/tusd#1032

Has this code been tested extensively with requests that use Transfer-Encoding: chunked? In these cases, no Content-Length is set and the server must keep track of the read bytes and stop if necessary on its own.

@fenos
Copy link
Collaborator Author

fenos commented Nov 22, 2023

@Acconut I've implemented the bug fix on this PR, however will test more in depth the Transfer-Encoding: chunked in theory it should work with the current implementation

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Some small comments + docs are missing, then we're good to go.

packages/server/src/models/StreamLimiter.ts Show resolved Hide resolved
packages/server/test/PostHandler.test.ts Outdated Show resolved Hide resolved
@fenos
Copy link
Collaborator Author

fenos commented Dec 13, 2023

@Acconut @Murderlon I've made the following changes:

  • I've rebased this PR
  • added a test when using Transfer-Encoding: chuncked
  • Validate early when Content-Lenght + offest > maxFileSize
  • Added documentation
  • clean up code
  • no default maxFileSize set

Ready for a final review 🎉

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Nice work, @fenos!

packages/server/src/handlers/BaseHandler.ts Show resolved Hide resolved
packages/server/src/handlers/BaseHandler.ts Outdated Show resolved Hide resolved
packages/server/src/handlers/BaseHandler.ts Show resolved Hide resolved
packages/server/README.md Outdated Show resolved Hide resolved
packages/server/src/handlers/OptionsHandler.ts Outdated Show resolved Hide resolved
@fenos
Copy link
Collaborator Author

fenos commented Dec 18, 2023

@Murderlon S3 tests are now failing with:

InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records.

did you revoke the access key id?

@fenos fenos force-pushed the feat/max-file-size branch 4 times, most recently from 507948e to ed17203 Compare December 20, 2023 08:38
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Looking great ✨ Thanks for the elaborate tests for this feature.

Only one small Q.

packages/server/src/constants.ts Show resolved Hide resolved
packages/server/src/handlers/BaseHandler.ts Show resolved Hide resolved
@Murderlon Murderlon merged commit aafb40d into tus:main Dec 21, 2023
2 checks passed
@fenos fenos mentioned this pull request Dec 29, 2023
@fenos fenos deleted the feat/max-file-size branch December 29, 2023 12:36
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.

None yet

3 participants