Opened 15 years ago

Closed 11 years ago

Last modified 11 years ago

#12670 closed Cleanup/optimization (fixed)

TemporaryUploadedFile objects do not respect FILE_UPLOAD_PERMISSIONS

Reported by: Andrew Badr Owned by: Rik
Component: File uploads/storage Version: dev
Severity: Normal Keywords: nlsprint14
Cc: andrewbadr.etc@…, simon@… Triage Stage: Design decision needed
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Attachments (1)

12670-file-uploads.diff (605 bytes ) - added by Simon Litchfield 11 years ago.
Proposed documentation change (ideally the preceding paragraph could be reworded too)

Download all attachments as: .zip

Change History (18)

comment:1 by Karen Tracey, 15 years ago

Triage Stage: UnreviewedDesign decision needed

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.

comment:2 by Andrew Badr, 15 years ago

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?

comment:3 by Karen Tracey, 15 years ago

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.

comment:4 by Andrew Badr, 15 years ago

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

comment:5 by Karen Tracey, 15 years ago

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?

comment:6 by Andrew Badr, 15 years ago

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.

comment:7 by Karen Tracey, 14 years ago

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

comment:8 by Simon Litchfield, 14 years ago

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.

comment:9 by Julien Phalip, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:10 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:11 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:12 by Florian Apolloner, 12 years ago

Resolution: wontfix
Status: newclosed

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.

comment:13 by Simon Litchfield, 11 years ago

Cc: simon@… added
Easy pickings: set
Needs documentation: set
Resolution: wontfix
Status: closednew
Version: 1.2-alphamaster

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.

by Simon Litchfield, 11 years ago

Attachment: 12670-file-uploads.diff added

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

comment:14 by Rik, 11 years ago

Keywords: nlsprint14 added
Owner: changed from nobody to Rik
Status: newassigned

comment:15 by Rik, 11 years ago

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?

Last edited 11 years ago by Rik (previous) (diff)

comment:16 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 355572ac56389a8d02cb93ea6a859e0d546bc6fb:

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

Thanks simon29 for the suggestion.

comment:17 by Tim Graham <timograham@…>, 11 years ago

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

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