#21668 closed Cleanup/optimization (fixed)

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

Reported by: GDorn Owned by: anubhav9042
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: timo 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 anubhav9042 13 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 19 months ago by GDorn

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

comment:2 Changed 19 months ago by anonymous

Potentially related #19866

comment:3 Changed 16 months ago by timo

  • Cc timo added
  • Component changed from Uncategorized to HTTP handling
  • Type changed from Bug to Cleanup/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 16 months ago 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.

comment:5 Changed 15 months ago by timo

  • Component changed from HTTP handling to Documentation
  • Triage Stage changed from Unreviewed to Accepted

I think improving the documentation is the best way forward.

comment:6 Changed 14 months ago by anubhav9042

  • Owner changed from nobody to anubhav9042
  • Status changed from new to assigned

Working on this in my GSoC project.

comment:7 Changed 13 months ago 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.

comment:8 Changed 13 months ago by anubhav9042

  • Version changed from 1.6 to master

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

comment:9 Changed 13 months ago 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.

Changed 13 months ago by anubhav9042

comment:10 Changed 13 months ago 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.

comment:11 Changed 13 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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