See https://www.drupal.org/SA-CORE-2014-003
Private file access is based on access to the entity that references the file (where it's attached).
User's can insert a File ID in Drupal 8 and bypass the access.
In Drupal 8 we need to consider both REST and form widgets, so we need to add a constraint that applies to both those cases, rather than just a form API fix liek in Drupal 7.
Comment | File | Size | Author |
---|---|---|---|
#35 | port_private_files-2304969-35.patch | 15.65 KB | cilefen |
#35 | interdiff-23-35.txt | 2.38 KB | cilefen |
#27 | 2304969-23.patch | 15.7 KB | pwolanin |
Comments
Comment #1
klausiComment #2
Chi CreditAttribution: Chi commentedComment #3
BerdirAlso related: #2078473: Use entity access API for checking access to private files
Comment #4
cilefen CreditAttribution: cilefen commentedCorrect me if wrong, but it seems D8 is already safe against this.
Drupal\system->FileDownloadController() seems to already respect the access check.
The main implementation, file_file_download(), returns -1 appropriately.
Comment #6
Devin Carlson CreditAttribution: Devin Carlson commentedTaking a look at this. From my testing it looks like there is still an issue, though it may have changed slightly as mentioned in #4.
Comment #8
catchTagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.
Comment #9
pwolanin CreditAttribution: pwolanin commentedSo, the user should not be able to see the file even though the admin attached it?
Comment #10
pwolanin CreditAttribution: pwolanin commentedDiscussing with berdir - we don't even understand why it works (or if it does) in Drupal 7, but we need to make changes to 8 to flag each entity reference item as having access or not.
Comment #11
BerdirThis was fun.
We found yet another bug in file_get_file_references(). It was incorrectly assuming that it does not have to check if an entity/field actually has the file referenced when we do not check by reference. But then we also don't skip fields that don't have the file.
So we were returning both fields, and then check access on both, and it was granted on the second, so we were granting access. That was obviously wrong, but the test at some point was completely broken and when it got fixed, worked around this problem by switching to a different user that probably doesn't have access to the node.
This gets us to the point where we can actually fix this problem.
Comment #12
pwolanin CreditAttribution: pwolanin commentedAfter discussion with fago, berdir, and klausi about how to handle this for REST as well as form APi, we need to add the fix to ValidReferenceConstraintValidator not just to the form like we did in Drupal 7.
Looking at that class we have:
$value is a field item.
$value->getEntity() is the current entity (the one in the process of being saved)
So we need to load the original one. Find all NEW reference values and for new values, validate that the current user has access to view the newly referenced entities.
See: EntityChangedConstraintValidator for example of loading the original entity.
Comment #13
pwolanin CreditAttribution: pwolanin commentedHere's a small test fix on top to prep for real fixes.
berdir noticed that the view mode was not set up for the file field, so it was never rendered.
Comment #18
pwolanin CreditAttribution: pwolanin commentedAdds new test coverage - this demonstrates the bug that we need to fix and expect 2 fails.
Comment #20
pwolanin CreditAttribution: pwolanin commentedok, this adds the constraint hopefully the access check logic is right.
Comment #22
pwolanin CreditAttribution: pwolanin commentedOk, let's make this more focused and just apply the access constraint to the file item.
Comment #23
pwolanin CreditAttribution: pwolanin commentedoops - forgot to fix the private file test
Comment #24
Fabianx CreditAttribution: Fabianx commentedNeeds an issue summary update
Comment #25
pwolanin CreditAttribution: pwolanin commentedComment #26
pwolanin CreditAttribution: pwolanin commentedComment #27
pwolanin CreditAttribution: pwolanin commentedre-posting patch - the above is green, but not reporting back.
Comment #28
BerdirThat won't help, there are some communication issues between drupal.org and qa.drupal.org. So it won't send the new patch. But it will probably catch up again at some point.
Comment #30
Fabianx CreditAttribution: Fabianx commentedAs I am not familiar with that function, I assume debug is not an oversight here, but intentional.
nit - a/
Should this not use ->loadUncached() as in the above code - that was added.
Besides those questions and nits, this is RTBC.
Comment #31
BerdirCan be simplified to $value->target_id.
Same, $value->entity.
Does not make a different for computed properties like entity here, but it is optimized for actual properties like target_id to not create a typed data object just to get the value out again.
ImageItem should have the access check constraint too, those are files too and you could reproduce this with an image field too.
#30.1: No it is not, should be removed.
#30.3: Doesn't really matter, loadUnchanged() is a shortcut for the same code, both work. Tests are full of this pattern.
Tagging novice for re-roll and addressing those minors things.
Comment #32
cilefen CreditAttribution: cilefen commentedComment #33
cilefen CreditAttribution: cilefen commentedSorry, missed the point of the novice tag on this one.
Comment #35
cilefen CreditAttribution: cilefen commentedComment #36
BerdirOh, don't worry, I only added that because it usually helps to add that tag so that someone picks up the issue. I don't care if it is actually a novice or not :) Thanks for the updates.
Changes look good to me. A lot of the code is based on my input, so I would prefer if someone else RTBC's it, but I'm +1 on doing so.
Comment #37
klausiReviewed the patch and everything looks good. The logic is not easy to follow in a couple of places, but the variable names are reasonably verbose to understand what is going on.
Comment #38
alexpottCommitted 8f3f79a and pushed to 8.0.x. Thanks!
Small amount of clean up on commit.
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commented