Opened 7 weeks ago

Last modified 5 weeks ago

#30004 new Cleanup/optimization

Set default FILE_UPLOAD_PERMISSION to 0o644.

Reported by: Evgeny Arshinov Owned by: nobody
Component: Documentation Version: 2.1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no 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 (6)

comment:1 Changed 7 weeks ago by Tim Graham

Type: UncategorizedCleanup/optimization

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

comment:2 in reply to:  1 Changed 6 weeks ago 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). 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.

Last edited 6 weeks ago by Evgeny Arshinov (previous) (diff)

comment:3 Changed 6 weeks ago 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 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 Changed 6 weeks ago 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.

comment:5 Changed 5 weeks ago by Carlton Gibson

Summary: Document TemporaryUploadedFile potential permission issuesSet default FILE_UPLOAD_PERMISSION to 0o644.
Triage Stage: UnreviewedAccepted

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.

comment:6 in reply to:  5 Changed 5 weeks ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top