CSRF failure incorrectly reported on upload when there is a problem with storage

[IO]

Minimal reproduction app is in the attachment, although the only changes from the default template are:
csrfbug/settings.py:

MEDIA_URL = '/media/'
MEDIA_ROOT = 'media/'
FILE_UPLOAD_MAX_MEMORY_SIZE = 1024 * 1024
FILE_UPLOAD_TEMP_DIR = MEDIA_ROOT + 'tmp'

app/models.py

class File(models.Model):
    file = models.FileField()

app/admin.py

from .models import File
admin.site.register(File)

Required setup for the attached app:

python manage.py migrate
python manage.py createsuperuser

Steps to reproduce

1) runserver
2) navigate and login to /admin/
3) navigate to /admin/app/file/add/

Scenario 1. default state - file uploads works as expected
Scenario 2. remove media/tmp directory - file uploads works only for files that fit in FILE_UPLOAD_MAX_MEMORY_SIZE, error otherwise (see below)
Scenario 3. remove whole media directory - error reported for all file uploads (see below)

Exact error message:

Forbidden (403)
CSRF verification failed. Request aborted.
Reason given for failure: CSRF token missing or incorrect.

Expected behaviour:

Filesystem error or similar reporting incorrect media storage setup.

Comment:

Yes, the setup in this scenario is invalid to begin with, but the error message has nothing to do with the actual problem.
I suspect a real problem with an underlying filesystem will also get covered in the same way.

[IO]

minimal reproduction app

[Tim Graham]

I think the "problem with storage" is that TemporaryUploadedFile fails silently if FILE_UPLOAD_TEMP_DIR doesn't exist. That may merit a separate ticket to add a system check or a more helpful exception if possible.

I could reproduce this except for "Scenario 3. remove whole media directory - error reported for all file uploads". FileSystemStorage._save() creates the (media) directory if needed so when MemoryFileUploadHandler was used, there was no problem. The problem seems limited to when TemporaryFileUploadHandler is used but FILE_UPLOAD_TEMP_DIR doesn't exist.

I didn't track down exactly why the CSRF error happens or if it can be fixed, but I'll accept the ticket for investigation.

[Tim McCurrach]

I think this is the offending code (django.middleware.csrf lines 295-304):

            if request.method == "POST":
                try:
                    request_csrf_token = request.POST.get('csrfmiddlewaretoken', '')
                except OSError:
                    # Handle a broken connection before we've completed reading
                    # the POST data. process_view shouldn't raise any
                    # exceptions, so we'll ignore and serve the user a 403
                    # (assuming they're still listening, which they probably
                    # aren't because of the error).
                    pass

I think what's happening is:

• request.POST isn't actually accessed until the csrf middleware.
• When it's accessed here, request._load_post_and_files is called.
• The actual error is raised during the creation of NamedTemporaryFile (see traceback below)
• The error remains unhandled until it is caught in CSRF middleware, and a 403 is returned.

File ".../lib/python3.8/site-packages/django/core/files/uploadedfile.py", line 63, in __init__
    file = tempfile.NamedTemporaryFile(suffix='.upload' + ext, dir=settings.FILE_UPLOAD_TEMP_DIR)
  File "/usr/local/Cellar/python@3.8/3.8.6/Frameworks/Python.framework/Versions/3.8/lib/python3.8/tempfile.py", line 540, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "/usr/local/Cellar/python@3.8/3.8.6/Frameworks/Python.framework/Versions/3.8/lib/python3.8/tempfile.py", line 250, in _mkstemp_inner
    fd = _os.open(file, flags, 0o600)
FileNotFoundError: [Errno 2] No such file or directory: 'media/tmp/tmpg5wy5s9f.upload.pages'

I'm happy to write a patch, but I'm not sure what the best solution is:

• Raise a 500?
• Raise a 400?
• Ignore the file? (probably not, since this would lead to pretty confusing and hard to debug behaviour)

Either way, I think we should probably add a check that if FILE_UPLOAD_TEMP_DIR is set then it also exists (EDIT: I've just seen this was suggested above).

If we did add such a check, could we ignore handling OSErrors in TemporaryUploadedFile.__init__? My immediate feeling is that even if a check would mitigate this particular error, there are probably other errors that could occur here, and letting them bubble up to wherever they happen to get caught is confusing and difficult to debug. As such, a decision is needed as to how they should be handled.

I suppose another solution would be to create some sort of FailedUploadedFile that subclasses UploadedFile that could be added to request._files and then the error could be handled by the view/form and a decision can be made at that point how it should be handled.

[Tim Graham]

Maybe there's an opportunity to use more specific exception catching than OSError (#20128 added it as IOError before it was updated to OSError in 7785e03ba89aafbd949191f126361fb9103cb980). Too bad we don't have the original exception in #20128 so we can see exactly where that exception comes from and determine if it's actually OSError/IOError on Python 3.

[Tim McCurrach]

I have created a separate ticket to add a check: https://code.djangoproject.com/ticket/32360

[Virtosu Bogdan]

I reproduced the error in ticket:20128 and it seems to be raised from ​django/http/request.py #359

Traceback (most recent call last):
  File "/project/django/django/middleware/csrf.py", line 345, in _check_token
    request_csrf_token = request.POST.get('csrfmiddlewaretoken', '')
  File "/project/django/django/core/handlers/wsgi.py", line 102, in _get_post
    self._load_post_and_files()
  File "/project/django/django/http/request.py", line 328, in _load_post_and_files
    self._post, self._files = self.parse_file_upload(self.META, data)
  File "/project/django/django/http/request.py", line 288, in parse_file_upload
    return parser.parse()
  File "/project/django/django/http/multipartparser.py", line 240, in parse
    for chunk in field_stream:
  File "/project/django/django/http/multipartparser.py", line 402, in __next__
    output = next(self._producer)
  File "/project/django/django/http/multipartparser.py", line 534, in __next__
    for bytes in stream:
  File "/project/django/django/http/multipartparser.py", line 402, in __next__
    output = next(self._producer)
  File "/project/django/django/http/multipartparser.py", line 465, in __next__
    data = self.flo.read(self.chunk_size)
  File "/project/django/django/http/request.py", line 359, in read
    raise UnreadablePostError(*e.args) from e
django.http.request.UnreadablePostError: Apache/mod_wsgi request data read error: Partial results are valid but processing is incomplet

As it was suggested, catching UnreadablePostError instead of OSError fixes this ticket and still conforms to ticket:20128. Only the test from that ticket needs to be updated.

I was wondering if logging the ignored exception wouldn't be enough as an alternative. Figuring out the issue from logs seems to be the main reported issue.

Please let me know if any of these solutions is acceptable

[Tim Graham]

Catching UnreadablePostError looks good to me. Thanks for investigating!

[Virtosu Bogdan]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 852fa76:

Refs #32329 -- Allowed specifying request class in csrf_tests test hooks.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 00ea883e:

Fixed #32329 -- Made CsrfViewMiddleware catch more specific UnreadablePostError.

Thanks Chris Jerdonek for the review.