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

Check file size less than UPLOAD_MAX_SIZE #332

Merged
merged 2 commits into from
Apr 21, 2021
Merged

Conversation

GraemeWatt
Copy link
Member

I've increased the DEFAULT_TIMEOUT for the hepdata-cli package to allow for the increased request timeout in the Kubernetes settings from 1 minute to 5 minutes. I've also added a check to the hepdata-cli code that the uploaded file size does not exceed UPLOAD_MAX_SIZE. However, this limit could easily be modified in client-side code, so the check should also be made in the server-side code. Currently, we only make the UPLOAD_MAX_SIZE check for web uploads and not for CLI uploads. This PR modifies process_payload to make the check for both. Alternatively, the Flask MAX_CONTENT_LENGTH could be set, but this did not return an informative error message when I tried it. I've written a new test test_upload_max_size in tests/records_test.py.

* This check previously only made for web upload not for CLI upload.
@GraemeWatt GraemeWatt added type: bug Indicates an unexpected problem or unintended behaviour priority: high complexity: low labels Apr 20, 2021
@GraemeWatt GraemeWatt self-assigned this Apr 20, 2021
@coveralls
Copy link

coveralls commented Apr 20, 2021

Coverage Status

Coverage increased (+0.02%) to 86.831% when pulling e9cfbf1 on check-uploadmaxsize into d03c1c0 on master.

Copy link
Contributor

@alisonrclarke alisonrclarke left a comment

Choose a reason for hiding this comment

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

Looks good to me.

* Don’t read more than this many bytes from the incoming request data.
* Check large uploads in /convert endpoints before file saved to disk.
@GraemeWatt
Copy link
Member Author

The check for UPLOAD_MAX_SIZE only in process_payload will first save the file to disk, which is not ideal for a very large upload. I've added the MAX_CONTENT_LENGTH (= UPLOAD_MAX_SIZE) Flask configuration key to limit the size of the incoming request data. It gives an error message ConnectionError: ('Connection aborted.', BrokenPipeError(32, 'Broken pipe')) when testing locally with the hepdata-cli, but the Flask docs indicate that it should return the correct 413 Request Entity Too Large status response when running the app with a production WSGI server.

@GraemeWatt GraemeWatt merged commit 52daa3b into master Apr 21, 2021
@GraemeWatt GraemeWatt deleted the check-uploadmaxsize branch April 21, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity: low priority: high type: bug Indicates an unexpected problem or unintended behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants