Opened 3 years ago

Closed 2 years ago

#21668 closed Cleanup/optimization (fixed)

Invalid upload_to FileField attribute results in hard-to-debug "Bad Request" 400 error.

Reported by: Sam Thompson Owned by: ANUBHAV JOSHI
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Tim Graham Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

21668.diff (506 bytes) - added by ANUBHAV JOSHI 2 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by Sam Thompson

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: Invalid upload_to attribute results in hard-to-debug "Bad Request" 400 error.Invalid upload_to FileField attribute results in hard-to-debug "Bad Request" 400 error.

comment:2 Changed 3 years ago by anonymous

Potentially related #19866

comment:3 Changed 3 years ago by Tim Graham

Cc: Tim Graham added
Component: UncategorizedHTTP handling
Type: BugCleanup/optimization

There may be some room for improvement here. How/why did you manage to attempt to upload a file outside of MEDIA_ROOT?

comment:4 Changed 3 years ago by Sam Thompson

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.

comment:5 Changed 2 years ago by Tim Graham

Component: HTTP handlingDocumentation
Triage Stage: UnreviewedAccepted

I think improving the documentation is the best way forward.

comment:6 Changed 2 years ago by ANUBHAV JOSHI

Owner: changed from nobody to ANUBHAV JOSHI
Status: newassigned

Working on this in my GSoC project.

comment:7 Changed 2 years ago by Tim Graham

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.

comment:8 Changed 2 years ago by ANUBHAV JOSHI

Version: 1.6master

Since this appears to be a problem in not getting the correct message, I am going to fix this alongwith #22058.

comment:9 Changed 2 years ago by Tim Graham

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.

Changed 2 years ago by ANUBHAV JOSHI

Attachment: 21668.diff added

comment:10 Changed 2 years ago by Tim Graham

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.

comment:11 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 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.

Note: See TracTickets for help on using tickets.
Back to Top