Opened 6 years ago
Closed 6 years ago
#30004 closed Cleanup/optimization (fixed)
Set default FILE_UPLOAD_PERMISSION to 0o644.
Reported by: | Evgeny Arshinov | Owned by: | Himanshu Lakhara |
---|---|---|---|
Component: | File uploads/storage | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hello,
As far as I can see, the 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 on GitHub, but did not manage to find any existing bug report in Django's bug tracker.
Change History (19)
follow-up: 2 comment:1 by , 6 years ago
Type: | Uncategorized → Cleanup/optimization |
---|
comment:2 by , 6 years ago
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). 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_PERMISSIONS
on the Settings page and the Deployment checklist page like this:
https://gist.github.com/earshinov/0340f741189a14d4fd10e3e902203ad6/revisions#diff-14151589d5408f8b64b7e0e580770f0e
Pros:
- It makes more clear that one gets different permissions for the *uploaded* files.
- 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.
comment:3 by , 6 years ago
Now that I think about, maybe Django could provide
# <Commentary about inconsistent permissions when this setting is omitted> FILE_UPLOAD_PERMISSINS=0o600
in the 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.
comment:4 by , 6 years ago
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.
follow-up: 6 comment:5 by , 6 years ago
Summary: | Document TemporaryUploadedFile potential permission issues → Set default FILE_UPLOAD_PERMISSION to 0o644. |
---|---|
Triage Stage: | Unreviewed → Accepted |
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 toNone
to restore previous behaviour' type comment. - Adjust the references in the settings docs and deployment checklist.
- Make sure any other references are adjusted.
comment:6 by , 6 years ago
Replying to 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.
comment:7 by , 6 years ago
Hello everyone,
I would like to work on this. But before that there are few important questions:
- 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?
- 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.
follow-up: 9 comment:8 by , 6 years ago
That note is referring to that non-leaf directories are created using the process umask
. (See `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.)
comment:9 by , 6 years ago
Replying to 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 previous comment.
Only question remaining is about introducing this change in 3.0 version. Shall we move it to 3.0 release?
comment:11 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:12 by , 6 years ago
Component: | Documentation → File uploads/storage |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Version: | 2.1 → master |
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. (Will proceed if others agree.)
comment:13 by , 6 years ago
Replying to Carlton Gibson:
Did you mean we should remove section FILE_UPLOAD_PERMISSIONS
from docs/howto/deployment/checklist.txt
?
follow-up: 15 comment:14 by , 6 years ago
Yes, that's what I meant. But I'd prefer to wait for another pair of eyes before just deciding that.
comment:15 by , 6 years ago
Replying to 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.
follow-up: 18 comment:16 by , 6 years ago
Has patch: | set |
---|
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.
comment:17 by , 6 years ago
Grrr. Commit hashes. I meant ef70af77ec53160d5ffa060c1bdf5ed93322d84f (where we added the note to the deployment checklist).
comment:18 by , 6 years ago
Replying to Tim Graham:
The bit about "When :setting:
FILE_UPLOAD_PERMISSIONS
is set toNone
, ..." 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 running as some other user than file's own 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.
I think you're talking about ef70af77ec53160d5ffa060c1bdf5ed93322d84f (#28540). I guess the question is whether or not that documentation should be duplicated elsewhere.