[Django] #32329: CSRF failure incorrectly reported when there is a problem
Django
-------------------------------------+-------------------------------------
Reporter: IO5 | Owner: nobody
Type: | Status: new
Uncategorized |
Component: File | Version: 3.1
uploads/storage |
Severity: Normal | Keywords: uploads csrf admin
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32329>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: IO5 | Owner: nobody
Component: File | Version: 3.1
uploads/storage |
Keywords: uploads csrf admin | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "csrfbug.zip" added.
minimal reproduction app
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: nobody
Type: Uncategorized | Status: new
Component: File | Version: 3.1
uploads/storage |
Keywords: uploads csrf admin | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:1>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: nobody
Component: File | Version: 3.1
uploads/storage |
Keywords: uploads csrf admin | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:2>
Django
storage
--------------------------------------+------------------------------------
Reporter: IO | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: uploads csrf admin | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:3>
Django
storage
--------------------------------------+------------------------------------
Reporter: IO | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
If we did add such a check, could we ignore handling `OSError`s 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:4>
Django
storage
------------------------------------+------------------------------------
Reporter: IO | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: uploads csrf admin | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: File uploads/storage => CSRF
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:5>
Django
storage
------------------------------------+------------------------------------
Reporter: IO | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim McCurrach):
I have created a separate ticket to add a check:
https://code.djangoproject.com/ticket/32360
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:6>
Django
storage
------------------------------------+------------------------------------
Reporter: IO | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Virtosu Bogdan):
I reproduced the error in ticket:20128 and it seems to be raised from
[https://github.com/django/django/blob/012f38f9594b35743e9ab231757b7b62db638323/django/http/request.py#L359
django/http/request.py #359]
{{{
#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:7>
Django
storage
------------------------------------+------------------------------------
Reporter: IO | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Catching `UnreadablePostError` looks good to me. Thanks for investigating!
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:8>
Django
storage
-------------------------------------+-------------------------------------
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Virtosu Bogdan
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:9>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14681 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:10>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:11>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:11>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:12>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:13>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:14>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Type: Bug | Status: assigned
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:15>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Component: CSRF | Version: 3.1
Severity: Normal | Resolution: fixed
Keywords: uploads csrf admin | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"00ea883ef56fb5e092cbe4a6f7ff2e7470886ac4" 00ea883e]:
{{{
#!CommitTicketReference repository=""
revision="00ea883ef56fb5e092cbe4a6f7ff2e7470886ac4"
Fixed #32329 -- Made CsrfViewMiddleware catch more specific
UnreadablePostError.
Thanks Chris Jerdonek for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:17>
Django
storage
-------------------------------------+-------------------------------------
Reporter: IO | Owner: Virtosu
| Bogdan
Component: CSRF | Version: 3.1
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"852fa7617e24a68a990eaf0f7a597edb434ffd76" 852fa76]:
{{{
#!CommitTicketReference repository=""
revision="852fa7617e24a68a990eaf0f7a597edb434ffd76"
Refs #32329 -- Allowed specifying request class in csrf_tests test hooks.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32329#comment:16>