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

Resolve 413 errors by setting Flask's MAX_FORM_MEMORY_SIZE limit to 20 MB #823

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

juhoinkinen
Copy link
Member

@juhoinkinen juhoinkinen commented Dec 16, 2024

When evaluating the new, to-be-published YSO projects, the requests whose body exceeded 500 KB resulted to 413 Client Error: Request Entity Too Large for url error, which did not happen on the previous projects update round on May.

The cause was the limit introduced in Werkzeug 3.1.0 (pallets/werkzeug#2965):

  • Request.max_form_memory_size defaults to 500kB instead of unlimited. Non-file form fields over this size will cause a RequestEntityTooLarge error. #2964

In Flask 3.1 there is a new config setting MAX_FORM_MEMORY_SIZE to set a value for this limit.

This PR uses the setting to disable the limit altogether increase the value from the default (500 KB) to 20 MB.

In any case, on production Annif should be run behind e.g. NGINX, which can be used to set request size limits more tightly.

@juhoinkinen juhoinkinen requested a review from osma December 16, 2024 14:58
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.63%. Comparing base (3297ce1) to head (42fde04).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
+ Coverage   99.60%   99.63%   +0.02%     
==========================================
  Files          93       93              
  Lines        7134     7142       +8     
==========================================
+ Hits         7106     7116      +10     
+ Misses         28       26       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@osma
Copy link
Member

osma commented Dec 17, 2024

Would it be better to practice defense in depth and set some reasonably high limit on body size - maybe 10 or 20 megabytes - instead of disabling the limit entirely? Yes, nginx or similar should be there to place actual limits. But what if it isn't in some particular case, or its limits have been disabled?

Copy link

sonarcloud bot commented Dec 17, 2024

@juhoinkinen
Copy link
Member Author

Now the limit is set to 20 MB.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM

@juhoinkinen juhoinkinen added this to the 1.2 milestone Dec 18, 2024
@juhoinkinen juhoinkinen changed the title Resolve 413 errors by disabling Flask's MAX_FORM_MEMORY_SIZE limit Resolve 413 errors by setting Flask's MAX_FORM_MEMORY_SIZE limit to 20 MB Dec 18, 2024
@juhoinkinen juhoinkinen merged commit 2ffd82b into main Dec 18, 2024
17 of 18 checks passed
@juhoinkinen juhoinkinen deleted the fix-413-errors-for-over-500kb-requests branch December 18, 2024 11:55
@juhoinkinen
Copy link
Member Author

For the record, I also tried to look how the value for MAX_FORM_MEMORY_SIZE could be set by a user via environment variable, but did not find the right way to do it with Annif.

The env FLASK_MAX_FORM_MEMORY_SIZE did not work as with plain Flask it should, see docs, and neither did setting it by gunicorn option, -e MAX_FORM_MEMORY_SIZE=<limit>.

@juhoinkinen juhoinkinen restored the fix-413-errors-for-over-500kb-requests branch December 18, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants