Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The way that the node_access('view', $node)-check was added to upload_file_download broke a feature that previously was possible using the {upload}-table: to have the same file attached to multiple nodes. The bug only appears if these nodes are under different access control, and probabily only if the {upload}-table has been modified by another mdule than Upload module itself.
The query run by upload_file_download does not necessarily return a unique row from {upload}. The table key is (vid, fid) but the the query searches based on filepath -- so multiple rows may be returned. Only the row that happens to be returned first will determine if the user gets access to the file or not.
Example: file with {files}.fid=1 is attached to node 1, 2 and 3 (thus there are three rows for it in {upload}). User has access only to node 2. Assuming the {upload}-row for node 1 is returned first, the user is denied access altough there is a visible node where the file is attached. I want the permission checking to be generous here, so that if the file is attached to multiple nodes and at least one node is accessible to the user, then the file is also accessible.
Since Upload module itself doesn't seem to reuse a {files}-row between different nodes (only between different revisions) the problem probably doesn't occur with Upload module alone. Therefore I marked this as a feature request rather than a bug.
But I have a custom module (not committed, may do so if desired) that can copy "attachments" (rows in the {upload}-table) between nodes without uploading the same file again, thus saving disk space and reusing a {files}-row.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | upload_multiple_nodes2.diff | 1.36 KB | emok |
| upload_multiple_nodes.diff | 1.43 KB | emok | |
Comments
Comment #1
PasqualleComment #2
catchLooks like a bug to me, albeit a little hard to reproduce. Could do with a test case.
Comment #3
drewish CreditAttribution: drewish commentedYou should not be returning -1 unless the file joins to upload records and all are access denied. As per the hook_download docs:
Committing this as is would re-introducing the bug that blocks other applications from sending files via private file transfer.
This really is a side effect of not having a unique index on the files table's filepath field. There should really only be one file record per file to avoid problems like this.
Comment #4
emok CreditAttribution: emok commentedI'm not 100% sure that I interpret your comment correctly. But here goes:
I have read some more and adjusted the comments and naming a bit in the patch (still applies to Drupal 6.4). I also omitted the early
return -1, but in that case the latereturn -1will be invoked anyway. So I think this new patch will return the same in all cases. The new one costs a few extra calls sometimes, but is perhaps easier to understand.I believe that's how the patch works. If no {upload}-records are found then nothing is returned.
My issue and this patch does not concern duplicates in {files}, but duplicates in {upload} (corresponding to one common {files}-row). I tried to clarify that in the example in my original post.
The only problem I can imagine is if two modules really were set up to handle the same file. Then Upload module would find one or multiple rows in {upload} for the file and return either -1 or download-headers, but that other module would also find records of the file and return some permission. Then access will be denied if any module returns -1. But such a situation could exist even without my patch and it is not in the scope of this issue.
Comment #5
drewish CreditAttribution: drewish commentedtwo problems with the patch: not rolled from the root and no longer applies.
Comment #6
webchickUpload module has been removed from Drupal 7. Moving down to 6.x.