Set default FILE_UPLOAD_PERMISSION to 0o644.

[Evgeny Arshinov]

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.

[Tim Graham]

I think you're talking about ef70af77ec53160d5ffa060c1bdf5ed93322d84f (#28540). I guess the question is whether or not that documentation should be duplicated elsewhere.

[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). ​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:

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.

[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 ​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.

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

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

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.

[Evgeny Arshinov]

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.

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

[Carlton Gibson]

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

[Himanshu Lakhara]

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?

[Carlton Gibson]

Shall we move it to 3.0 release?

Yes please.

[Carlton Gibson]

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

[Himanshu Lakhara]

Did you mean we should remove section FILE_UPLOAD_PERMISSIONS from docs/howto/deployment/checklist.txt?

[Carlton Gibson]

Yes, that's what I meant. But I'd prefer to wait for another pair of eyes before just deciding that.

[Himanshu Lakhara]

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.

[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).

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.

[Carlton Gibson]

Grrr. Commit hashes. I meant ef70af77ec53160d5ffa060c1bdf5ed93322d84f (where we added the note to the deployment checklist).

[Himanshu Lakhara]

Replying to 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 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.

[Tim Graham <timograham@…>]

In 22aab866:

Fixed #30004 -- Changed default FILE_UPLOAD_PERMISSION to 0o644.