#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)
Change History (18)
comment:1 by , 15 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 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 , 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 , 15 years ago
Yeah. Perhaps the right solution for my use case is a new kind of upload handler.
comment:5 by , 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 , 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:8 by , 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 , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:12 by , 12 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
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 , 11 years ago
Cc: | added |
---|---|
Easy pickings: | set |
Needs documentation: | set |
Resolution: | wontfix |
Status: | closed → new |
Version: | 1.2-alpha → master |
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 , 11 years ago
Attachment: | 12670-file-uploads.diff added |
---|
Proposed documentation change (ideally the preceding paragraph could be reworded too)
comment:14 by , 11 years ago
Keywords: | nlsprint14 added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:15 by , 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?
comment:16 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.