securedrop
securedrop copied to clipboard
freedomofpress
Large upload failure should result in comprehensible error message
Steps to reproduce
(To see production-like behavior below, make sure to test in Tor Browser against a staging or production instance.)
In the Source Interface, upload a file that is greater than the permissible size limit (currently: 500MB)
Expected behavior
Source Interface should display a comprehensible error message, ideally as a standard flash error above the submission form.
Actual behavior
Browser displays built-in "Connection was reset" error
Notes
Based on the discussion below, the preferred way of handling this would be to throw an exception in the application and update the user with a flash message on /lookup, which is consistent with how other submission errors are handled. (See https://flask.palletsprojects.com/en/2.0.x/patterns/fileuploads/#improving-uploads for some docs on how this can be achieved in Flask.)
The issue with doing this as a flash message would be that the error (altho it's not really an error) is coming from Apache before the application sees it. TheLimitRequestBody directive in the apache config is what determines the max file (actually request but same diff) size.
One thing to investigate could be adding an ErrorDocument 413 (413 == Entity Too Large) directive as well - at the very least you could have a nicer error page. It would be worth doing some ticket spelunking into the history of this issue though to see if there was any reason for not doing so, of if there's some other obstacle to making it work.
Yeah I figured we might have to do an ErrorDocument here - it could at least have a "< Return to submission form" link on it. Briefly spelunking - the ErrorDocument removal for the JI in #4023 suggests that we may want to confirm that we see the expected application-level response headers.
altho looking at said issue history and the Flask docs, it seems like it might be possible to defer this check to the application instead: https://flask.palletsprojects.com/en/2.0.x/patterns/fileuploads/#improving-uploads - basically it will also throw a 413 in that scenario.
If there could be any way to keep the user in the SI w/o having to throw a 413—no matter the volume of required work—I'd advocate for that as a goal to target ahead of most existing UI changes to remediate other usability problems.
Sources are likely to be in an very anxious emotional space when submitting, and the screen-state change from showing an application UI to showing a browser-based error is so jarring. Even though the words "too large" are shown in a 413, and it's possible they might make the leap from "an entity" to connect that to the file size of the thing they tried to upload—the "irrational" emotional impacts of getting a browser error could compromise their whistle-blowing experience in opsec failures later on in their session.
If we did go the custom error page route, it would look like part of the SI.
According to my testing, Apache's LimitRequestBody will always trigger the The connection was reset screen. This wouldn't by itself be a huge issue if Flask/Werkzeug handled upload limits nicely, but that doesn't seem to be the case either.
When using setting LimitRequestBody to 0 and FlaskConfig.MAX_CONTENT_LENGTH (in prod, that is), setting a custom error page (rather than a flash message using the same form) is possible, but: that error page will only be shown after the full upload has concluded, no matter the file size. That of course makes this useless as DoS mitigation.
As far as I can tell, this is the issue that we'd need fixed: https://github.com/pallets/werkzeug/issues/1513 - and that looks like a larger undertaking
I wonder if setting LimitRequestBody to 500M + 1K and .MAX_CONTENT_LENGTH to 500M would allow you to control the error for all but the 500M +1K case...
Unfortunately not: Apache only hands off data to mod_wsgi once it's sure that the content-length is appropriate. So the error message would work for uploads that were right between the limits of Flask and Apache …