Private file download returns access denied when there is a node with at least 2 revisions on the site (that is when the nid and vid on new nodes do not match). To reproduce:
- create a node with a few revisions.
- setup the private file system path and add a private file field to your favorite content type
- create a node and upload a file
- click on the file link and enjoy the access denied.

The problem comes from file_file_download() which runs entity_load($entity_type, array($id)). $id comes from file_get_file_references() and is not an entity id, but a revision id. The private file download seems to work at first because initially (before any revision is created) all nids and vids are equal, until you start creating revisions on nodes.

CommentFileSizeAuthor
#19 992674-9-private-access-denied.patch2.87 KBalexanderpas
#14 992674_14_private_access_denied-test-only.patch1.98 KBscor
#9 992674-9-private-access-denied.patch2.87 KBcarlos8f
#6 992674-6-private-access-denied.patch2.52 KBcarlos8f
#5 992674-5-private-access-denied-fail.patch1.77 KBcarlos8f
#2 992674_2_private_access_denied-pass.patch1.55 KBscor
#2 992674_2_private_access_denied-fail.patch949 bytesscor
#1 992674_private_access_denied.patch636 bytesscor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scor’s picture

scor CreditAttribution: scor commented
Status: Active » Needs work
FileSize
992674_private_access_denied.patch636 bytes

This is just a proof of concept patch which solves the issue for nodes only. It needs to be made more generic so it works for all entities. Alternatively, file_get_file_references() could be fixed to return an array keyed by entity ids as opposed to revision ids. Actually, the doc for file_get_file_references() is wrong too, it does not return An integer value. but a reference object.

scor’s picture

scor CreditAttribution: scor commented
Status: Needs work » Needs review
FileSize
992674_2_private_access_denied-fail.patch949 bytes
992674_2_private_access_denied-pass.patch1.55 KB

I've modified file.test to reproduce this bug programatically. It sets up the test in a context which is more "real" by creating a node with a couple of revisions. I'm curious to see if more tests will break as a result. One patch contains the fix and should pass, the other should fail.

Status: Needs review » Needs work

The last submitted patch, 992674_2_private_access_denied-fail.patch, failed testing.

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented

nice catch, subscribing

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented
Status: Needs work » Needs review
FileSize
992674-5-private-access-denied-fail.patch1.77 KB

Neither patches in #2 are correct to test or fix the issue, this one is at least correct in testing for it. We should get 1 fail.

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented
FileSize
992674-6-private-access-denied.patch2.52 KB

It looks like a mistake that we check access for all revisions, so here's a solution which only checks access on the latest revision. And as a result, $nid is passed correctly to entity_load(). In HEAD, a revision ID is passed to entity_load(), which results in the wrong node/entity being used in the access check (confusing $vid with $nid). That is bad :P

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented

Marked #964400: file_file_download attempts to load entities with their revision_id as a duplicate of this.

Status: Needs review » Needs work

The last submitted patch, 992674-6-private-access-denied.patch, failed testing.

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented
Status: Needs work » Needs review
FileSize
992674-9-private-access-denied.patch2.87 KB

One test requires a $reset of node_load().

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Upsie :)

I think I wrote the part that loads the entity in hook_file_download() and wasn't sure if it's the correct way but nobody said something different :)

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented
Issue tags: +Security

Tagging with security, this bug could result in files being unintentionally downloadable because field_access() and hook_file_download_access() could be passed the wrong entity entirely.

scor’s picture

scor CreditAttribution: scor commented

@carlos8f: agreed, e.g. user has access to nid 1 but no access to nid 2; if the value of the latest vid of nid 1 is 2 then the user can download files from the node nid 2.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Patch looks good to me - could we get just the tests as a patch to show the failures? Don't have time to run locally at the moment but RTBC otherwise.

scor’s picture

scor CreditAttribution: scor commented
FileSize
992674_14_private_access_denied-test-only.patch1.98 KB

tests-only patch.

int’s picture

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

The test faill without the fix, and pass with the fix. I think that's RTBC...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 992674_14_private_access_denied-test-only.patch, failed testing.

int’s picture

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

Sorry. Change again..

scor’s picture

scor CreditAttribution: scor commented

yes, #9 is RTBC.

alexanderpas’s picture

alexanderpas CreditAttribution: alexanderpas commented
FileSize
992674-9-private-access-denied.patch2.87 KB

reupload of the patch from #9 to keep the testbot happy. (no changes)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

renat’s picture

renat CreditAttribution: renat commented
Status: Closed (fixed) » Needs work

Looks like committed patch did not solved problem completely, because it changed file_get_file_references function parameter to FIELD_LOAD_CURRENT from FIELD_LOAD_REVISION. But FIELD_LOAD_REVISION parameter was there for a good reason, with FIELD_LOAD_CURRENT we are not able to open files attached to all entity revisions except current. It's fatal in case you're trying to build, say, intranet with library for documents.

Steps to reproduce:
1.Set up clean Drupal 7.12 with private document folder (say, sites/default/files/private) and a content type with just a title and file field called field_file. It should store files in your private folder.
2.Login as UID1, create a node and add a file "testfile1.txt" to it's file field.
3.Save the node
4.Click edit
5.Remove the old file, "testfile1.txt"
6.Add "testfile2.txt"
7.Check "Create new revision" option
8.Click save
9. Click revisions tab, open first revision of your node
10. Node is fine, link to file is there, but when you'll click on this link to the file "testfile1.txt", you'll get "Page not Found"

It is possible to open old version of your file - if you'll revert old revision. But it is not an option in case you use revisions as revisions, not as backups for inaccurate users, because it will produce awful mess of revisions.

nick_t’s picture

nick_t CreditAttribution: nick_t commented

Subscribing. I'm also unable to access some private files, even when signed in as the admin user.

jlongbottom’s picture

jlongbottom CreditAttribution: jlongbottom commented
Status: Needs work » Needs review
Issue tags: -Security

#19: 992674-9-private-access-denied.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Security

The last submitted patch, 992674-9-private-access-denied.patch, failed testing.

jlongbottom’s picture

jlongbottom CreditAttribution: jlongbottom commented

This patch did not work for me. I submitted it for re-testing and it failed.

jlongbottom’s picture

jlongbottom CreditAttribution: jlongbottom commented

Me too. I can't access any private files and the patch didn't help at all. I submitted it for re-testing and it failed. hmmm

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

The patch has already been commited a *looong* time ago, you can't retest that, that makes no sense.

It might make more sense to open a new issue about that rather than re-open a one that has been closed more than a year ago.

jlongbottom’s picture

jlongbottom CreditAttribution: jlongbottom commented

ok. done: http://drupal.org/node/1452100

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Needs work » Closed (fixed)

Closing this issue as another issue has been created.

biblos’s picture

biblos CreditAttribution: biblos as a volunteer commented
Issue summary: View changes

Confirm this issue with D7.