I'm tracking down a permission-issue with Media and multiform, and it led me to this part of the code in this module (file_entity.pages.inc line 237). The function checks update access on the file and if the multiform module exists before sending the user off the /admin/content/file/edit-multiple/%; but, the media module provides that path. There is no check to see if that module exists before sending the user there.

I didn't dig too much deeper - so perhaps I'm wrong about this. Thought I should report it though.

The other issue, which I think is media's fault, is that that path is only accessible to users with the 'edit all files' permission. This module is not checking that before sending them - and it's causing me to land on a 403 after multiple uploading.

CommentFileSizeAuthor
#10 file_entity-missing_checks_on_multiupload-1870532-10.patch1.12 KBParisLiakos
#4 file_entity-missing_checks_on_multiupload-1870532-1.patch1.26 KBmstef

Comments

mstef’s picture

mstef commented

Well actually, even if it's not redirecting to the multiform, it sends the user to admin/content/file, which requires "administer files" permission (never checked before redirecting there).

mstef’s picture

mstef commented
Title: Missing check if media module exists before redirecting to admin/content/file/edit-multiple/% ? » Missing check module and permission checks on multiple upload submission
mstef’s picture

mstef commented
Title: Missing check module and permission checks on multiple upload submission » Missing module and permission checks on multiple upload submission
mstef’s picture

mstef commented
Status: Active » Needs review
StatusFileSize
new file_entity-missing_checks_on_multiupload-1870532-1.patch1.26 KB

This corrects all the issues in the submit handler; but needs to be reviewed to make sure that's how you want to handle it.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Needs work

thanks

+++ b/file_entity.pages.incundefined
@@ -385,8 +385,11 @@ function file_entity_add_upload_multiple_submit($form, &$form_state) {
+  if (file_entity_access('update', $file) && module_exists('multiform') && module_exists('media')) {

seems good, besides that module_exists on media is not needed

mstef’s picture

mstef commented

We don't have to check that media exists before sending a user to a page provided by the module? I don't see a check anywhere else. Maybe I missed something.

ParisLiakos’s picture

ParisLiakos commented

admin/content/file is provided by file_entity module if i am not wrong:)

mstef’s picture

mstef commented

Yes, but we are potentially redirecting to admin/content/file/edit-multiple, which is declared in media_menu() (not sure why though..).

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Reviewed & tested by the community

ah...yeah, i suppose this path should be registered in file_entity...
hrmpff..i see..thats a different issue..i think this is rtbc
thanks!

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new file_entity-missing_checks_on_multiupload-1870532-10.patch1.12 KB

a bit simpler patch, checking with bot

ParisLiakos’s picture

ParisLiakos commented
Status: Reviewed & tested by the community » Fixed

commited here http://drupalcode.org/project/file_entity.git/commit/f7b9744

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.