[Django] #21668: Invalid upload_to attribute results in hard-to-debug "Bad Request" 400 error.
Django
error.
-------------------------------+--------------------
Reporter: GDorn | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
It's admirable that django tries to prevent files from being uploaded
outside of MEDIA_ROOT. However, the documentation for file uploads
(https://docs.djangoproject.com/en/dev/topics/http/file-uploads/) doesn't
make this requirement clear, and getting it wrong results in several
layers of useful error messages getting eaten by a generic message
instead.
It starts with django.utils._os.safe_join raising a ValueError if the
upload_to path isn't within MEDIA_ROOT. This has a useful message payload
showing both paths and why the exception happened. This message isn't
logged.
However, django.core.files.storage.FileSystemStorage.path immediately
catches that exception, eats the message, and raises a much more generic
SuspiciousFileOperation, containing the desired path but no explanation of
what went wrong.
Finally, django.core.handlers.base.BaseHandler.get_response catches that
SuspiciousOperation, logs the message (good if you have logging turned on,
which many users do not), eats the exception and returns a 400 response
instead.
All the user sees is "400 Bad Request", no traceback and certainly not the
original and useful ValueError message.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Request" 400 error.
-------------------------------+--------------------------------------
Reporter: GDorn | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GDorn):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:1>
Django
Request" 400 error.
-------------------------------+--------------------------------------
Reporter: GDorn | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
Potentially related #19866
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:2>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.6
Component: HTTP handling | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* cc: timo (added)
* component: Uncategorized => HTTP handling
* type: Bug => Cleanup/optimization
Comment:
There may be some room for improvement here. How/why did you manage to
attempt to upload a file outside of `MEDIA_ROOT`?
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:3>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.6
Component: HTTP handling | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GDorn):
I was attempting to simulate the upload as part of a unit test. As I
didn't want the test polluting my actual uploads directory, I was
attempting to use @override_settings to change the upload path. The
resulting error was very hard to diagnose.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:4>
Django
Request" 400 error.
--------------------------------------+------------------------------------
Reporter: GDorn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* component: HTTP handling => Documentation
* stage: Unreviewed => Accepted
Comment:
I think improving the documentation is the best way forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:5>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* status: new => assigned
* owner: nobody => anubhav9042
Comment:
Working on this in my GSoC project.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:6>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Actually, it appears that this raised a more informative error message in
Django 1.3 (see #22706). We should see if that's possible to restore.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:7>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* version: 1.6 => master
Comment:
Since this appears to be a problem in not getting the correct message, I
am going to fix this alongwith #22058.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:8>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
This regressed in 1.6 with this commit:
https://github.com/django/django/commit/d228c1192ed59ab0114d9eba82ac99df611652d2
#diff-dbd7d9159676b15fc9a096b0adb919e9R173
I think we should consider returning a `technical_500_response` in the
handling of `SuspiciousOperation` as we did before if `settings.DEBUG is
True` rather than invoking `handler400`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:9>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner:
Type: | anubhav9042
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Yes, that's the idea. Please add a test. A mention in the release notes
also wouldn't hurt. I'd also consider adding a `status_code` kwarg to
`technical_500_response` so we can continue returning a `400` in this
case.
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:10>
Django
Request" 400 error.
-------------------------------------+-------------------------------------
Reporter: GDorn | Owner:
Type: | anubhav9042
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"dbbcfca476e29354c3a5c6221112b55741babc14"]:
{{{
#!CommitTicketReference repository=""
revision="dbbcfca476e29354c3a5c6221112b55741babc14"
Fixed #21668 -- Return detailed error page when SuspiciousOperation is
raised and DEBUG=True
Thanks GDorn and gox21 for report.
Thanks Tim Graham for idea and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21668#comment:11>