[Django] #26644: [Regression?] SuspiciousFileOperation when creating a File from a NamedTemporaryFile
Django
NamedTemporaryFile
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage | Keywords: File,
Severity: Release blocker | SuspiciousFileOperation,
| NamedTemporaryFile, regression
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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/`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
NamedTemporaryFile
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage:
SuspiciousFileOperation, | Unreviewed
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Could you [https://docs.djangoproject.com/en/dev/internals/contributing
/triaging-tickets/#bisecting-a-regression bisect] to find the commit where
the behavior changed?
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:1>
Django
NamedTemporaryFile
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage:
SuspiciousFileOperation, | Unreviewed
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by hobarrera):
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 <cristi...@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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:2>
Django
NamedTemporaryFile
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage:
SuspiciousFileOperation, | Unreviewed
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by hobarrera):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:3>
Django
NamedTemporaryFile
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Sure, at least seeing the regression test in your PR will help me
understand the problem a bit more.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:4>
Django
NamedTemporaryFile
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by hobarrera):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: assigned
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* needs_better_patch: 1 => 0
* owner: nobody => hobarrera
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: assigned
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: assigned
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: assigned
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: assigned
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/6771 Updated PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: closed
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1b407050dd53e56686fdd3e168f8cac4f9be8306" 1b40705]:
{{{
#!CommitTicketReference repository=""
revision="1b407050dd53e56686fdd3e168f8cac4f9be8306"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: closed
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"c37f9253a6cb3b6e7d58607c058de1f3bc9f8d2b" c37f9253]:
{{{
#!CommitTicketReference repository=""
revision="c37f9253a6cb3b6e7d58607c058de1f3bc9f8d2b"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: closed
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
This fix might be problematic. It broke
[https://github.com/django/djangoproject.com/blob/ea56005103e941814399b5881feafd12091665f7/fundraising/tests/test_models.py#L28-L46
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
[https://github.com/django/django/commit/914c72be2abb1c6dd860cb9279beaa66409ae1b2
#diff-d6396b594a8f63ee1e12a9278e1999edL57 the Django test suite] in the
original commit that spawned this ticket. If we keep this commit, then
those `name` arguments can be removed.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: closed
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
I plan to revert this in [https://github.com/django/django/pull/6797
adding a test] for #26772 as it causes a regression there. We'll have to
reconsider how to solve this.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: closed
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"cd217de6100e0101fd921dd18bc2e706bac397c9" cd217de]:
{{{
#!CommitTicketReference repository=""
revision="cd217de6100e0101fd921dd18bc2e706bac397c9"
Reverted "Fixed #26644 -- Allowed wrapping NamedTemporaryFile with File."
This reverts commit 1b407050dd53e56686fdd3e168f8cac4f9be8306 as it
introduces a regression in the test for refs #26772.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: closed
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"e2b266fdf78141a45277fb1664a6d430d7731c09" e2b266f]:
{{{
#!CommitTicketReference repository=""
revision="e2b266fdf78141a45277fb1664a6d430d7731c09"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by hobarrera):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: new
Component: File | Version: 1.10
uploads/storage |
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by hobarrera):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: hobarrera | Owner: hobarrera
Type: Bug | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* keywords: File, SuspiciousFileOperation, NamedTemporaryFile, regression
=> File, SuspiciousFileOperation, NamedTemporaryFile, regression 1.10
* component: File uploads/storage => Documentation
* severity: Release blocker => Normal
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Hugo Osvaldo | Owner: Hugo
Barrera | Osvaldo Barrera
Type: Bug | Status: closed
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: wontfix
Keywords: File, | Triage Stage: Accepted
SuspiciousFileOperation, |
NamedTemporaryFile, regression |
1.10 |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26644#comment:23>