Code

Opened 4 years ago

Closed 5 months ago

Last modified 5 months ago

#12670 closed Cleanup/optimization (fixed)

TemporaryUploadedFile objects do not respect FILE_UPLOAD_PERMISSIONS

Reported by: andrewbadr Owned by: rednaw
Component: File uploads/storage Version: master
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 simon29 8 months ago.
Proposed documentation change (ideally the preceding paragraph could be reworded too)

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 Changed 4 years ago by andrewbadr

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 Changed 4 years ago by kmtracey

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 Changed 4 years ago by andrewbadr

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

comment:5 Changed 4 years ago by kmtracey

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 Changed 4 years ago by andrewbadr

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 Changed 4 years ago by kmtracey

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

comment:8 Changed 4 years ago by simon29

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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:10 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:11 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:12 Changed 16 months ago by apollo13

  • Resolution set to wontfix
  • Status changed from new to 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 Changed 8 months ago by simon29

  • Cc simon@… added
  • Easy pickings set
  • Needs documentation set
  • Resolution wontfix deleted
  • Status changed from closed to new
  • Version changed from 1.2-alpha to 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.

Changed 8 months ago by simon29

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

comment:14 Changed 5 months ago by rednaw

  • Keywords nlsprint14 added
  • Owner changed from nobody to rednaw
  • Status changed from new to assigned

comment:15 Changed 5 months ago by rednaw

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 5 months ago by rednaw (previous) (diff)

comment:16 Changed 5 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 355572ac56389a8d02cb93ea6a859e0d546bc6fb:

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

Thanks simon29 for the suggestion.

comment:17 Changed 5 months ago by 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.