SuspiciousFileOperation when creating a File from a NamedTemporaryFile

[Hugo Osvaldo Barrera]

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

[Tim Graham]

Could you ​bisect to find the commit where the behavior changed?

[Hugo Osvaldo Barrera]

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

[Hugo Osvaldo Barrera]

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.

[Tim Graham]

Sure, at least seeing the regression test in your PR will help me understand the problem a bit more.

[Hugo Osvaldo Barrera]

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.

[Tim Graham]

​Updated PR

[Tim Graham <timograham@…>]

In 1b40705:

Fixed #26644 -- Allowed wrapping NamedTemporaryFile with File.

914c72be2abb1c6dd860cb9279beaa66409ae1b2 introduced a regression that
causes saving a NamedTemporaryFile in a FileField to raise a
SuspiciousFileOperation. To remedy this, if a File has an absolute
path as a filename, use only the basename as the filename.

[Tim Graham <timograham@…>]

In c37f9253:

[1.10.x] Fixed #26644 -- Allowed wrapping NamedTemporaryFile with File.

914c72be2abb1c6dd860cb9279beaa66409ae1b2 introduced a regression that
causes saving a NamedTemporaryFile in a FileField to raise a
SuspiciousFileOperation. To remedy this, if a File has an absolute
path as a filename, use only the basename as the filename.

Backport of 1b407050dd53e56686fdd3e168f8cac4f9be8306 from master

[Tim Graham]

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.

[Tim Graham]

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.

[Tim Graham <timograham@…>]

In cd217de:

Reverted "Fixed #26644 -- Allowed wrapping NamedTemporaryFile with File."

This reverts commit 1b407050dd53e56686fdd3e168f8cac4f9be8306 as it
introduces a regression in the test for refs #26772.

[Tim Graham <timograham@…>]

In e2b266f:

[1.10.x] Reverted "Fixed #26644 -- Allowed wrapping NamedTemporaryFile with File."

This reverts commit 1b407050dd53e56686fdd3e168f8cac4f9be8306 as it
introduces a regression in the test for refs #26772.

Backport of cd217de6100e0101fd921dd18bc2e706bac397c9 from master

[Hugo Osvaldo Barrera]

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?

[Tim Graham]

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.

[Hugo Osvaldo Barrera]

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.

[Tim Graham]

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.