Patch http://drupal.org/node/52092 has enabled upload.module to hijack file downloads, even if they're nothing to do with upload.module.

In upload_file_download:

if (user_access('view uploaded files')) {
...
}
else return -1

The return value of -1 means access to the file is denied, regardless of what happens in the hook in other modules.

But permission 'view uploaded files' is specific to upload.module. I am entitled to disregard it for file downloads in my own module. Hence upload_file_download should not return -1 if the user doesn't have permission 'view uploaded files'. It should just return false (or nothing) instead.

If another module doesn't specifically give permission to view the said file, access to the file will be denied as wished for.

In upload_file_download there are two instances of return -1. I suggest just getting rid of the second one, as the first only returns -1 if there is an entry in the 'files' db table, which is a table specific to upload.module anyway which is a good thing. Ideally, the table 'files' would be renamed 'uploads' due to this reason, but obviously that isn't going to happen for 4.7.

My patch simply removes the last return - 1.

Note that it may be an idea to do more work on this patch (but not so critically) so that the drupal_access_denied is returned correctly rather than drupal_not_found. This would probably require adding another option for a return value.

CommentFileSizeAuthor
#17 file_download.patch.txt2.04 KBkilles@www.drop.org
#5 upload_hijack_3.patch1.02 KBjakeg
#4 upload_hijack_2.patch1.02 KBjakeg
upload_hijack.patch373 bytesjakeg

Comments

jakeg’s picture

jakeg commented

Just to put this in context, here's the function that calls the hook. Notice that if any -1 return values are found, permission is denied.

function file_download() {
  // Merge remainder of arguments from GET['q'], into relative file path.
  $args = func_get_args();
  $filepath = implode('/', $args);

  // Maintain compatibility with old ?file=paths saved in node bodies.
  if (isset($_GET['file'])) {
    $filepath =  $_GET['file'];
  }

  if (file_exists(file_create_path($filepath))) {
    $headers = module_invoke_all('file_download', $filepath);
    if (in_array(-1, $headers)) {
        return drupal_access_denied();
    }
    if (count($headers)) {
        file_transfer($filepath, $headers);
    }
  }
  return drupal_not_found();
}
jakeg’s picture

jakeg commented

... also notice the problem that this will create with drupal_not_found/access_denied()

jakeg’s picture

jakeg commented

One way around the access_denied/not_found problem would be to put the check on 'view uploaded files' permission inside the other condition in upload_file_download() rather than outside it.

However, this presumes that the 'files' db table is uses solely for upload.module (or modules which piggy back on it), which I suggest it is anyway, as it is not mentioned at all in file.mdoule.

jakeg’s picture

jakeg commented
StatusFileSize
new upload_hijack_2.patch1.02 KB

Here is a patch which does just that, thus denied message will be given if no permission 'view uploaded files', but only if the file can be found in the files table first.

jakeg’s picture

jakeg commented
StatusFileSize
new upload_hijack_3.patch1.02 KB

i put in a new line by mistake, this patch has it removed again.

dopry’s picture

dopry commented

This is a duplicate of an issue we had before. Can you do what you need to do through node access? The thinking for this was to make 'view uploaded files' an umbrella permission. It presents a clear ui. Drupal's current fileAPI isn't really usable for much beside upload.module. The two are very closely coupled, since file.inc doesn't provide any database interface to keep the files table consistant. It is similar to the view content permission which is an umbrella for node_access I believe....

someone please correct me if I'm wrong.

Dave Cohen’s picture

Dave Cohen commented

In my opinion, upload_file_download should never return -1. (Or, the system/files callback should ignore the -1s). Instead of returning -1, it should simply return null.

Regarding Dopry's question. If Dopry is correct, and upload.module is the only useful file handling module, then no other module will be implementing hook_file_download. So noone will see the files they are not supposed to see. If on the other hand Dopry is wrong, and some other module does need to implement hook_file_download, then upload_file_download is out of the way and the other hook can do its job.

In short, I don't see the harm in never returning -1.

jakeg’s picture

jakeg commented

Exactly. I want to use hook_file_download... that's why its a hook. So all modules can use it. And its upload.module that introduces this 'view uploaded files' permission, so regardless of what you say, its not an umbrella permission by default. Only if you choose it to be, which I don't.

I choose to have a file implementation without using a database table at all. File.inc enables me to do this, but upload.module hijacks my ability to do this. Associating the files with the database table is upload.module's task not file.inc. I find the file API vary useful... but not if upload.module gets in my way.

Hence my patch stands.

killes@www.drop.org’s picture

killes@www.drop.org commented

I am inclined to commit this patch. Drupal is all about flexibility. and if this patch allows people to to things they coul dnot do otherwise while not taking away things from other people I think it is a good thing.

dopry’s picture

dopry commented

I finally got time to look closely. Jakeg and Yogadex are right on this one. file_download does have a default deny bahavior. Upload.module blocks all file viewing if you do not have the view uploaded files permission. My concern is the change of meaning for 'view uploaded files'. I'm worried about the novice admin who disables the 'view uploaded files' permission and is curious why people can still watch the movie he uploaded. It is a file isn't it...

Maybe a little renaming or documentation would be good to go with the patch.

With the semantic reservation +1.

jakeg’s picture

jakeg commented

Agree. The 'files' db table should be renamed 'uploads' as its used by upload.module not file.inc... but this isn't going to happen at this late stage. Lets relook at files again as soon as 4.7 is out the door with that patch in it.

Documentatation can be changed after 4.7.0, e.g. for 4.7.1 in theory. I have no time to look at that right now, but lets get this patch in.

jakeg’s picture

jakeg commented
Status: Needs review » Reviewed & tested by the community

killes mentioned wiating for dopry for his comments before committing this patch. As he's now weighed in positively, I think we can make this patch 'ready to be committed'.

Dave Cohen’s picture

Dave Cohen commented

It's unclear to me that the files table belongs exclusively to the upload module. I've written node types that prompt for files, not using the upload module, but still save information about the files in that table. The files table comes in core drupal, so I did not see any problem doing this. There are other tables (i.e. node_access) that are intended to be written by many modules, so why not files?

If the files table (and files_revisions) really does belong to upload, then it must be renamed and perhaps be created when upload is enabled. If the table had been named 'upload', I would have assumed to corresponded to the upload module and I would not have used it from my own modules.

killes@www.drop.org’s picture

killes@www.drop.org commented

the files tables don't belong to the upload module, other modules can use them as well.

I would like to see the behaviour of this hook properly documented before I apply the patch.

Dave Cohen’s picture

Dave Cohen commented

Perhaps the files table (or files_revisions) would benefit from a 'realm' column, so that modules can easily identify the entries that apply to them. I don't mean to get off topic, but I had the thought and wanted to mention it here.

drewish’s picture

drewish commented

yogadex, that's been a long standing request, see http://drupal.org/node/33482 "add 'module' column to files table"

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new file_download.patch.txt2.04 KB

Please no feature discussions on bug reports.

I've rerolled the patch from the docroot and added some blurb to the help hook. Please proofread and maybe expand a bit.

moshe weitzman’s picture

moshe weitzman commented

personally, i think 'view uploaded files' should have been deprecated when we got node_access just like hook_access('view') for node listings. if you can see a node, you can see its attachments.

killes@www.drop.org’s picture

killes@www.drop.org commented

I agree that this permission is in principle rather useless. I could imagine it makes sense in an ecommerce setting.

moshe weitzman’s picture

moshe weitzman commented

nothing stops ecommerce from implementing the file_download hook itself. anyway, the usual ecommerce implementation sells access to nodes or grants a role ... perhaps someone can roll a patch which removes this perm.

dopry’s picture

dopry commented

I wouldn't say that in all cases you want people to be able to see the attachments just because they can read the node... I can think of say bid proposal and specifications that are attached to an item description, but are restricted to privileged individuals, as one use case.

Eitherway they're all great ideas, but we're way to late in the release cycle to be jinking/improving/futzing or messing with anything that is not specifically a bug. Which was my argument last time this issue came up.

If you want these things changed, join the fileAPI sig on groups.drupal.org, post your roadmap, or plans, comment on other peoples' ideas, and lets hammer out a roadmap for 4.8's filehandling, ***AFTER*** 4.7 is out the door.

Changing the behavior of the permission to only affect files solely handled by upload.module is one thing, deprecating a permission at rc3 is another.

Please stick to reviewing the current patch....

@killes,
so you are moving the 'view uploaded files' so that it only catches private files in the files table?

Dave Cohen’s picture

Dave Cohen commented

Moshe writes:
nothing stops ecommerce from implementing the file_download hook itself.

Actually, upload_file_download stops it, which is the point of this bug.

I really hope this simple patch makes it into 4.7. I'm planning to work on an MP3 ecommerce product where a user would be able to download only the files they have explicitly purchased. It would be a shame if one had to disable upload.module in order to sell such a product.

moshe weitzman’s picture

moshe weitzman commented

i don't think deprecating a permission is an overly aggressive remedy for this issue even at this stage in the release. it is a tiny change, no?

as mentioned, if we deprecate the permisison and remove upload's implementation of this file_download hook then any module can deny access to attachments as needed. this is really best of both worlds. we remove code and give more flexibility.

dopry’s picture

dopry commented

I still haven't heard an explanation from anyone as to why they can't....

1) enabled view uploaded file permissions,
2) control access through node_access,
3) block access as necessary by their own hook_file_download.

---

removing the default return -1, in effect changes the 'view upload files' permission to an enable node_access configuration option for downloads.

only doing it for files stored in the files table will prevent undue interaction with modules that implement their on files table, which is probably good as well.

---
The problems with changing or deprecating this permission, this late in the game is
1) creating and debugging the upgrade path for 4.6 users.
2) affects any contrib module that works with upload.module as it is that has been upgraded for 4.7 already.
3) it only seems that 1 person has actually reviewed and tested this patch so far, and that is me.
---

please stop with the freaking feature discussion, and test if dropping the -1 does undue harm to your file system permissions and restrictions. Setup a site add some files, observe if it throws errors or acts correctly/incorrectly. It would be really nice to hear back from people with working ecommerce/files installations, since those are the users who would be mostly affected by this change.

---

Steven’s picture

Steven commented

I don't see a problem with the patch killes posted. We can look at realm extensions of files later, no?

dopry’s picture

dopry commented

I don't see a problem with it either. It works for me with a stock private files setup.
I don't have any fancy permissions stuff set up...

Dave Cohen’s picture

Dave Cohen commented

+1 to the patch from comment #17. I hope its committed and we can close this.

To dopry (comment #24): Let's say I make a node type called iTune. Each iTune node has an MP3 file attached, and each is a product (costs only 99 cents). I want to allow only customers who have purchased an iTune to download the MP3. I want all visitors to be able to view the iTune node, so they can see what is for sale.

Your suggestion to me is that I configure my site to allow all users to 'view uploaded files'. Then, in my own hook_file_download for iTune, I return -1 if the user has not purchased the requested tune. Thus my hook_file_download is responsible for denying access to those who don't deserve it, rather than granting to those who do. Do I understand you correctly?

Now, I share my iTune module with the world. Someone downloads and installs it. But for some wacky reason, that person has a site in which not every user has 'view uploaded files' permission. One such user purchases an iTune from that site. They attempt to download the file they just purchased. upload_file_download notices they do not have 'view uploaded files' permission so with extreme prejudice it prevents the download. User has wasted 99 cents. (In effect, my iTunes module has a dependency that all users have 'view uploaded files' permission).

I described another user case here: http://drupal.org/node/56624. Forgive me if I sound like a broken record.

Also in http://drupal.org/node/56624 I provided a patch which is the equivalent of #17 here. I tested it and had no problems.

dries’s picture

dries commented

I'm OK with Gerhard's patch, but have no hands on experience working with the file download hooks.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Needs review » Fixed

applied

dopry’s picture

dopry commented

@yogadex, ok cool I get it :)... Good example. Nice module.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)