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 a file upload size limit #471

Closed
Tracked by #468
ml-evs opened this issue Oct 23, 2023 · 2 comments · Fixed by #475
Closed
Tracked by #468

Add a file upload size limit #471

ml-evs opened this issue Oct 23, 2023 · 2 comments · Fixed by #475

Comments

@ml-evs
Copy link
Member

ml-evs commented Oct 23, 2023

Related to #468, but this is something we need to do anyway as Flask does not enforce any limit itself and will happily fill its own memory and then temporary space (trying to force my own laptop to crash right now to see what happens).

We should limit this in a few places:

  • UI file upload selection (fairly large but arbitrary restriction ~ 100 GB)
  • API check disk space during upload
  • API restrict uploads to some large number of GB that fit in memory, then figure out how to do chunked uploads in the future
@ml-evs
Copy link
Member Author

ml-evs commented Oct 24, 2023

As it stands, Flask will happily accept a giant file upload, which will first fill the server RAM and then will begin writing to a temporary directory. I've managed to get my own laptop stuck in a place where that temporary directory has disappeared but the space hasn't been reclaimed on disk (/tmp is infrequently reclaimed/on-demand), so probably we need to sort something manual out here...

Unfortunately we don't know the size of a file request until it has already reached us, so the only protection we can add is the Flask MAX_CONTENT_SIZE option which will reject any giant requests. Potential strategy --- set this content size to be something very large, e.g., 100 GB, but when saving files, always make sure there is 100 GB free so that we will never have a crash, but instead just lose 100 GB of capacity.

@ml-evs
Copy link
Member Author

ml-evs commented Oct 24, 2023

always make sure there is 100 GB free so that we will never have a crash, but instead just lose 100 GB of capacity.

Scratch that part, looks like a massive faff to overwrite Flask's default behavior of writing to /tmp, adds too much configurability and seems fairly safe that if /tmp isnt big enough, the file upload will just silently fail. Also the files are streamed eventually from /tmp to the real files dir directly to the other without needing to maintain two copies.

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 a pull request may close this issue.

1 participant