Opened 8 years ago
Closed 8 years ago
#26644 closed Bug (wontfix)
SuspiciousFileOperation when creating a File from a NamedTemporaryFile
Reported by: | Hugo Osvaldo Barrera | Owned by: | Hugo Osvaldo Barrera |
---|---|---|---|
Component: | Documentation | Version: | 1.10 |
Severity: | Normal | Keywords: | File, SuspiciousFileOperation, NamedTemporaryFile, regression 1.10 |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This code snippet worked fine on django 1.9. It should still work on django 1.10.
from tempfile import NamedTemporaryFile from django.core.files.base import File from . import pdf with NamedTemporaryFile(suffix='.pdf') as file_: pdf.generate_receipt_pdf(self.receipt_id, file_) self.pdf_file = File(file_) self.save()
However, the following exception is raised on django1.10:
Traceback (most recent call last): File "/home/hugo/workspace/Hugo/django-afip/testapp/testapp/testmain/tests.py", line 305, in test_pdf_generation pdf.save_pdf() File "/home/hugo/workspace/Hugo/django-afip/testapp/django_afip/models.py", line 863, in save_pdf self.save() File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/db/models/base.py", line 796, in save force_update=force_update, update_fields=update_fields) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/db/models/base.py", line 824, in save_base updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/db/models/base.py", line 886, in _save_table for f in non_pks] File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/db/models/base.py", line 886, in <listcomp> for f in non_pks] File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/db/models/fields/files.py", line 287, in pre_save file.save(file.name, file, save=False) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/db/models/fields/files.py", line 90, in save self.name = self.storage.save(name, content, max_length=self.field.max_length) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/core/files/storage.py", line 53, in save name = self.get_available_name(name, max_length=max_length) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/core/files/storage.py", line 77, in get_available_name while self.exists(name) or (max_length and len(name) > max_length): File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/core/files/storage.py", line 394, in exists return os.path.exists(self.path(name)) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/core/files/storage.py", line 407, in path return safe_join(self.location, name) File "/home/hugo/workspace/Hugo/django-afip/.tox/py35-django/lib/python3.5/site-packages/django/utils/_os.py", line 78, in safe_join 'component ({})'.format(final_path, base_path)) django.core.exceptions.SuspiciousFileOperation: The joined path (/tmp/tmpbwfln73d.pdf) is located outside of the base path component (/home/hugo/workspace/Hugo/django-afip/testapp/media)
I believe that there's something wrong with how File
is now determining the resulting file's name: in previous versions of django, this file *did not* end up in /tmp
, but rather inside media/
.
Change History (23)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
For some reason I'd started blindly looking at commits instead of bisecting. Dunno what came over me.
Anyway, I wrote a quick test script and here goes:
$ git bisect bad 914c72be2abb1c6dd860cb9279beaa66409ae1b2 is the first bad commit commit 914c72be2abb1c6dd860cb9279beaa66409ae1b2 Author: Cristiano <cristianocca@hotmail.com> Date: Sun Mar 20 22:51:17 2016 -0300 Fixed #26058 -- Delegated os.path bits of FileField's filename generation to the Storage. :040000 040000 58b8cffd061048624df2ec4824b13d48afb62ec5 a95fac52046826060aa64515993cc041dfc3c838 M django :040000 040000 87bed8c670d6f452c8f210c1c92b9279ac9b268b ba415dad2ee1ec6ba4bd202720241df262ef695c M docs :040000 040000 16c679a9e8de544fc17e7229460a0a5452aee0ac b17aaf89f7853bdf2a3c7a933633557cd8691793 M tests
comment:3 by , 8 years ago
A possible fix is to trim the original file's path and keep only the basename component if the path is an absolute path inside File
when saving; I believe this should fix the regression for any storage.
I can try to create a PR if that solution sounds okay. I can't really think of an alternative.
comment:4 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Sure, at least seeing the regression test in your PR will help me understand the problem a bit more.
comment:5 by , 8 years ago
I've created PR 6658.
I'm not sure if the fix is okay (it does not break anything, but you may have some reason to prefer a different behavior), but at least the test makes the problem more obvious.
comment:6 by , 8 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Summary: | [Regression?] SuspiciousFileOperation when creating a File from a NamedTemporaryFile → SuspiciousFileOperation when creating a File from a NamedTemporaryFile |
comment:7 by , 8 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | unset |
Status: | new → assigned |
comment:8 by , 8 years ago
Patch needs improvement: | set |
---|
comment:9 by , 8 years ago
Patch needs improvement: | unset |
---|
comment:10 by , 8 years ago
Patch needs improvement: | set |
---|
comment:14 by , 8 years ago
This fix might be problematic. It broke a test for djangoproject.com with the following exception:
====================================================================== ERROR: test_thumbnail (fundraising.tests.test_models.TestDjangoHero) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/tim/code/djangoproject.com/fundraising/tests/test_models.py", line 42, in test_thumbnail self.assertEqual(thumbnail.x, 170) File "/home/tim/.virtualenvs/djangoproject-djangoco/lib/python3.4/site-packages/sorl/thumbnail/images.py", line 53, in width return self.size[0] TypeError: 'NoneType' object is not subscriptable
Assigning an absolute file path to a file field as done on line 36 of the test isn't tested in Django's test suite. I tried adding a test for this but ran into an error like SuspiciousFileOperation: The joined path (/home/tim/code/django/tests/model_fields/4x8.png) is located outside of the base path component (/tmp/django_c515u6ay/tmpjxafwpc8)
so maybe this isn't a common pattern. The djangoproject.com test uses a file that's already in MEDIA_ROOT and can be fixed by replacing the right side of self.h1.logo = image_path
with
File(open(image_path, 'rb'))
.
I'm not too sure what the best behavior is here, so I guess we could wait and see if the commit is problematic for any other projects.
If we reverted this, the alternative seems to be to document that you must pass a name
to File
as done in the Django test suite in the original commit that spawned this ticket. If we keep this commit, then those name
arguments can be removed.
comment:15 by , 8 years ago
I plan to revert this in adding a test for #26772 as it causes a regression there. We'll have to reconsider how to solve this.
comment:18 by , 8 years ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Status: | closed → new |
comment:19 by , 8 years ago
It would seem that the original regression this introduces isn't trivially solved.
Not-including this fix breaks several of my applications. Adding the obvious fixes breaks djangoproject.com.
Can we consider reverting the original commit that introduced the regression, if no sane fix can be thought of? Or do you have an idea of how to cleanly solve it?
comment:20 by , 8 years ago
As I mentioned in earlier comments, you might try passing the name
argument when you wrap File
. We can document this in the release notes (and storage API docs possibly) if no other solution can be found. I don't like the idea of reverting the original commit as I believe it's a much needed cleanup of the file storage API.
comment:21 by , 8 years ago
I'm fine with documenting a workaround (especially since it works on django1.9, since I can keep code backwards-compatible).
I'll try to draft up a PR once I get a few spare minutes.
comment:22 by , 8 years ago
Component: | File uploads/storage → Documentation |
---|---|
Keywords: | 1.10 added |
Severity: | Release blocker → Normal |
comment:23 by , 8 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Closing since a patch hasn't been provided and there haven't been reports of anyone else hitting the issue. Hugo, feel free to reopen if you want to provide a patch.
Could you bisect to find the commit where the behavior changed?