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

[Sam Thompson]

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.

[anonymous]

Potentially related #19866

[Tim Graham]

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

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

[Tim Graham]

I think improving the documentation is the best way forward.

[ANUBHAV JOSHI]

Working on this in my GSoC project.

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

[ANUBHAV JOSHI]

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

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

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

[Tim Graham <timograham@…>]

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.