[Django] #30004: Document TemporaryUploadedFile potential permission issues
Django
-------------------------------------------+------------------------
Reporter: Evgeny Arshinov | Owner: nobody
Type: Uncategorized | Status: new
Component: Documentation | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
Hello,
As far as I can see, the
[https://docs.djangoproject.com/en/2.1/topics/http/file-uploads/ File
Uploads] documentation page does not mention any permission issues.
What I would like to see is a warning that in absence of explicitly
configured `FILE_UPLOAD_PERMISSIONS`, the permissions for a file uploaded
to `FileSystemStorage` might not be consistent depending on whether a
`MemoryUploadedFile` or a `TemporaryUploadedFile` was used for temporary
storage of the uploaded data (which, with the default
FILE_UPLOAD_HANDLERS, in turn depends on the uploaded data size).
The `tempfile.NamedTemporaryFile` + `os.rename` sequence causes the
resulting file permissions to be 0o0600 on some systems (I experience it
here on CentOS 7.4.1708 and Python 3.6.5). In all probability, the
implementation of Python's built-in `tempfile` module explicitly sets such
permissions for temporary files due to security considerations.
I found mentions of this issue [https://github.com/divio/django-
filer/issues/1031 on GitHub], but did not manage to find any existing bug
report in Django's bug tracker.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Evgeny Arshinov | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
Comment:
I think you're talking about ef70af77ec53160d5ffa060c1bdf5ed93322d84f
(#28540). I guess the question is whether or not that documentation should
be duplicated elsewhere.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:1>
Django
Reporter: Evgeny Arshinov | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Evgeny Arshinov):
Thank you Tim, this is precisely what I was looking for! I can only see
one issue with the current docs (if you excuse me for bothering you with
such minor details). [https://docs.djangoproject.com/en/2.1/ref/settings
/#file-upload-permissions The documentation] for the
FILE_UPLOAD_PERMISSIONS setting reads:
> If this isn’t given or is None, you’ll get operating-system dependent
behavior. On most platforms, temporary files will have a mode of 0o600,
and files saved from memory will be saved using the system’s standard
umask.
As I would understand this text, only temporary files get a mode of 0o600.
I would then ask myself: "Why should I care about temporary files, they
should be gone anyway after the file is uploaded?" and skip setting
FILE_UPLOAD_PERMISSIONS. What is important but is not properly conveyed to
the user is that not only temporary files themselves, but also the actual
files which end up in the media folder get permissions of 0o600.
Currently a developer can only discover this either by careful reading of
the Deployment checklist page (`manage.py check --deploy` does not seem to
check `FILE_UPLOAD_PERMISSIONS`) or by hitting the inconsistent
permissions accidentally (like I did).
I propose to unify the docs for `FILE_UPLOAD_PERMISSINS` on the Settings
page and the Deployment checklist page like this:
https://gist.github.com/earshinov/0340f741189a14d4fd10e3e902203ad6/revisions
#diff-14151589d5408f8b64b7e0e580770f0e
Pros:
1. It makes more clear that one gets different permissions for the
*uploaded* files.
2. It makes the docs more unified and thus easier to synchronously change
in the future if/when required.
I recognize that my edits might seem too minor and insignificant to be
worth the hassle of editing the docs, committing, re-publishing them etc.,
but still I hope you will find them useful enough to be integrated into
the official docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:2>
Django
Reporter: Evgeny Arshinov | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Evgeny Arshinov):
Now that I think about, maybe Django could provide
{{{
# <Commentary about inconsistent permissions when this setting is omitted>
FILE_UPLOAD_PERMISSINS=0o600
}}}
in the
[https://github.com/django/django/blob/master/django/conf/project_template/project_name/settings
.py-tpl default project settings] so that developers don't miss it? `600`
seems a reasonable default, particularly because people would get `600`
anyway (at least on some operating systems) when the
`TemporaryFileUploadHandler` is engaged.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:3>
Django
Reporter: Evgeny Arshinov | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Since this has come up again, I've suggested on django-developers
(https://groups.google.com/d/topic/django-
developers/h9XbQAPv5-I/discussion) that we adjust the
`FILE_UPLOAD_PERMISSION` default to `0o644` (This was the conclusion I
eventually came to from the discussion on #28540.) Lets see what people
say there.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: Evgeny Arshinov | Owner: nobody
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
Thus far, no great objections on the mailing list to adjusting the
FILE_UPLOAD_PERMISSION default. Thus I'm going to rename this and
''Accept'' on that basis.
A PR would need to:
* Adjust the default.
* Add a Breaking Change note to `releases/2.2.txt` (on the assumption we
can get it in for then.) — This should include a ''set to `None` to
restore previous behaviour' type comment.
* Adjust the references in the settings docs and deployment checklist.
* Make sure any other references are adjusted.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:5>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Evgeny Arshinov):
Replying to [comment:5 Carlton Gibson]:
> Thus far, no great objections on the mailing list to adjusting the
FILE_UPLOAD_PERMISSION default. Thus I'm going to rename this and
''Accept'' on that basis.
Thank you! Hopefully, this change will prevent confusion and unpleasant
surprises for Django users in the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:6>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Himanshu Lakhara):
Hello everyone,
I would like to work on this. But before that there are few important
questions:
1. There is a related setting called `FILE_UPLOAD_DIRECTORY_PERMISSIONS`.
Its document says that
This value mirrors the functionality and caveats of the
`FILE_UPLOAD_PERMISSIONS` setting.
Shall we also change its default from `None` to `0o644`(Please suggest if
something different should be provided for directories) and update its
document as well?
2. Since 2.2 pre-release branch is now in feature freeze state, Shall we
move the change to 3.0 version?
On a side note, some tests must be refactored for new values for both of
these settings. I think that's alright.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:7>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
That note is referring to that non-leaf directories are created using the
process `umask`. (See
[https://docs.python.org/3.7/library/os.html#os.makedirs `makedirs()`
docs].) This is similar to `FILE_UPLOAD_PERMISSIONS`, when not using the
temporary file upload handler.
The underlying issue here is the **inconsistency** in file permissions,
depending on the file size, when using **the default settings** that
Django provides. There is no such inconsistency with directory
permissions. As such changes should not be needed to
`FILE_UPLOAD_DIRECTORY_PERMISSIONS`. (Any issues there would need to be
addressed under a separate ticket.)
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:8>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Himanshu Lakhara):
Replying to [comment:8 Carlton Gibson]:
I see and understand the issue better now. Thanks for the clarification.
I'll make the changes as you have suggested in your [comment:5 previous
comment].
Only question remaining is about introducing this change in 3.0 version.
Shall we move it to 3.0 release?
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:9>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> Shall we move it to 3.0 release?
Yes please.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:10>
Django
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Himanshu Lakhara):
* owner: nobody => Himanshu Lakhara
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* version: 2.1 => master
* component: Documentation => File uploads/storage
* stage: Accepted => Ready for checkin
Comment:
OK, this is ready to go.
Note to committer:
I have one query as to whether this fix means that the paragraph added to
the Deployment Checklist in 89d4d412404d31ef34ae3170c0c056eff55b2a17 is
now essentially misleading?
If so, should we drop it entirely rather than just adjusting it...?
{{{
Fixed #30004, Refs #28540 -- Set default FILE_UPLOAD_PERMISSION to 0o644.
This reverts commit 89d4d412404d31ef34ae3170c0c056eff55b2a17 as no longer
relevant.
}}}
I think I'd be +1 on that.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Himanshu Lakhara):
Did you mean we should remove section `FILE_UPLOAD_PERMISSIONS` from
`docs/howto/deployment/checklist.txt`?
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Yes, that's what I meant. But I'd prefer to wait for another pair of eyes
before just deciding that.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Himanshu Lakhara):
Replying to [comment:14 Carlton Gibson]:
I agree with you on removing the section from the checklist. As you have
suggested, It would be best to wait for others' opinion before making any
change.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
The bit about "When :setting:`FILE_UPLOAD_PERMISSIONS` is set to ``None``,
..." seems out of place in the checklist, however, there may still be some
value in describing cases in which the new default doesn't make sense (if
any).
Regarding Carlton's comment 12 -- that change you cited is in the 1.11
release notes. I'm not sure if you meant to cite something different.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Grrr. Commit hashes. I meant ef70af77ec53160d5ffa060c1bdf5ed93322d84f
(where we added the note to the deployment checklist).
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Cleanup/optimization | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Himanshu Lakhara):
Replying to [comment:16 Tim Graham]:
> The bit about "When :setting:`FILE_UPLOAD_PERMISSIONS` is set to
``None``, ..." seems out of place in the checklist, however, there may
still be some value in describing cases in which the new default doesn't
make sense (if any).
Yes, It is out of place. I could think of one case when a developer might
want to change the new default.
Consider the following scenario. We have a Django application which allows
a user to upload pictures. Let's say this app runs in a process-A running
as some system user-1. Now we have another process-B which modifies this
image in place(maybe removing colors that the human eye cannot recognize
or shrinking the image etc.). This image manipulation process is run as
some other system user-2.
Now in order for process B to modify these images, we would require to set
FILE_UPLOAD_PERMISSIONS to '0o646'(assuming process-B is other than
group).
I understand this is not a great way to do such manipulation. We probably
want to do this in a different way by making a copy original image before
process-B modifies it. This is just an example.
So there might be situations when the new default doesn't make sense. Even
in these cases, I'm not sure whether putting in the deployment checklist
is necessary. The reason is setting page now explains what is the new
behavior and default deployment option is better now. I'm not sure what
additional information we could add to the checklist.
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Himanshu
Type: | Lakhara
Component: File | Version: master
uploads/storage |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"22aab8662f0368b63f91f2526bdd0532524bc0fe" 22aab866]:
{{{
#!CommitTicketReference repository=""
revision="22aab8662f0368b63f91f2526bdd0532524bc0fe"
Fixed #30004 -- Changed default FILE_UPLOAD_PERMISSION to 0o644.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30004#comment:19>