TemporaryUploadedFile objects do not respect FILE_UPLOAD_PERMISSIONS

[Andrew Badr]

From what I can tell, TemporaryUploadedFile objects do not respect the FILE_UPLOAD_PERMISSIONS setting. This setting is only used by FileSystemStorage, which is the default DEFAULT_FILE_STORAGE, which is used by *model* FileFields, but not by forms. Having temporary uploaded files respect FILE_UPLOAD_PERMISSIONS seems to have been the original intention if you read #8454, and to me the docs at ​http://docs.djangoproject.com/en/dev/topics/http/file-uploads/ implied that this was the case as well.

[Karen Tracey]

It isn't clear to me that the intent of #8454 was ever to affect the mode for the temporarily-uploaded version of the file. Rather I read the intent as ensuring a consistent mode for the ultimately saved version of the uploaded file, regardless of whether it was just uploaded to memory and then saved from there or first saved in an intermediate temporary file. The committed fix does that, for the case where Django code is responsible for saving the permanent version of the uploaded file.

If you are using just a form FileField and saving the file manually, then the question of permissions for the ultimately-saved file is entirely under your control. It isn't clear to me that having the temporary files created with the FILE_UPLOAD_PERMISSIONS would necessarily be helpful. That will set the mode for files that are saved in intermediate temporary files -- but what about the case where the file is just uploaded to memory? In that case what code is responsible for setting the mode of the ultimately-saved file? If it is application code that may not respect this FILE_UPLOAD_PERMISSIONS setting then I don't see how having the TemporaryUploadedFile code respect it is helpful, since ultimately the application will have to ensure the mode is correct and consistent regardless of whether the uploaded data was first saved in a temporary file.

Maybe I'm missing something here -- some example code that shows how the lack of doing this currently can result in permission inconsistency depending on upload file size, and how just having the TemporaryUploadedFile object use this setting for temporarily-uploaded files would fix the inconsistency, would be helpful. (Not to mention code that implemented the change in TemporaryUploadedFile.)

I do agree that the current doc can easily be read to imply that the setting applies to the temporarily-uploaded version of the file. If we don't change the TemporaryUploadedFile behavior we should at least clarify the doc on what this setting applies to, exactly.

[Andrew Badr]

This is my use case: using a FileField in my form, setting FILE_UPLOAD_MAX_MEMORY_SIZE to 0 so as to always write to disk, and then saving the TemporaryUploadedFile's filename in a task queue for processing. Models aren't involved at upload time. Does that make sense?

[Karen Tracey]

OK, I can see how for that setup just changing TemporaryUploadedFile behavior would be a "fix". But it seems to me like an incomplete fix, since that use case has simply prohibited the in-memory case. For the general case where app code is handling a form-field-only uploaded file and has not disallowed in-memory storage of uploaded files, then the app needs to ensure permissions are set appropriately in both cases. For this general case I don't see how changing the TemporaryUploadedFile behavior is helpful, since it ensures a particular mode for only one of two cases that must be dealt with.

Given that use case, I'm tending to think the right fix here is a doc change to clarify that the setting only applies to files uploaded into model FileFields. If an app is using just a form FileField, then the app is responsible for properly setting permissions on the file that it ultimately uses to save the uploaded data.

[Andrew Badr]

Yeah. Perhaps the right solution for my use case is a new kind of upload handler.

[Karen Tracey]

That seems extreme. Why can't you just chmod the temporary file to the permissions you need in the view that processes the upload and puts it on the task queue?

[Andrew Badr]

Sure, that's what I'm doing. My point above was that Django doesn't have a built-in way to handle my use case, and maybe it should (not in 1.2 of course). For example, if someone else's site needs both this kind of upload and the typical model case, they aren't going to want to set FILE_UPLOAD_MAX_MEMORY_SIZE to 0 and make FILE_UPLOAD_TEMP_DIR not be a real temp dir.

[Karen Tracey]

#13857 reported this again and has patches for both alternatives.

[Simon Litchfield]

This thread confuses me. Simply put, as it stands it works; according to my tests. It's just not clear.

My patches on #13857 offer two alternatives, either fix the doc; or fix the code and doc. I'm +1 on the latter.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Florian Apolloner]

There are good reasons security-wise to leave tmp files as 600, especially in shared environments. If you need other permissions for the file, move it out of /tmp/ and chmod it, otherwise other users can access it which can be dangerous. A new setting isn't worth it, as such I am closing this.

[Simon Litchfield]

Sorry, re-opening, this issue isn't resolved. It just bit me again.

If we must have an implicit 0600 on temporary file uploads, and an explicit setting FILE_UPLOAD_PERMISSIONS that doesn't work, then at the least we need to clearly document the inconsistent behaviour.

I understand the security concern, but having a setting that tells me I can choose the mode leads me to think, well, er, I can change the mode.

[Simon Litchfield]

Proposed documentation change (ideally the preceding paragraph could be reworded too)

[Rik]

I fixed the documentation and created a pull request for it:
​https://github.com/django/django/pull/2341

apollo13, maybe you want to review this?

[Tim Graham <timograham@…>]

In 355572ac56389a8d02cb93ea6a859e0d546bc6fb:

Fixed #12670 -- Added a note about permissions of files stored in FILE_UPLOAD_TEMP_DIR.

Thanks simon29 for the suggestion.

[Tim Graham <timograham@…>]

In dde67de0f656014821942ee8abe50f5187924288:

[1.6.x] Fixed #12670 -- Added a note about permissions of files stored in FILE_UPLOAD_TEMP_DIR.

Thanks simon29 for the suggestion.

Backport of 355572ac56 from master