Presently this module is not compatible with private files. Where the response should be "access denied" with this module active it sends the file with with the mime type text/html. This means that the browswer File > Save page as can be used to download the file, which will then have a .html extension padded to the filename. Simply stripping the html extension from the filename will make it accessible to all visitors. I have given this the critical priority because of the privacy problem.

CommentFileSizeAuthor
#6 private_files_become_accessible-2408531-6.patch489 bytesñull
#4 private_files_become_accessible-2408531-4.patch4.21 KBñull
#2 private_files_become_accessible-2408531-2.patch395 bytesñull

Comments

ñull’s picture

ñull commented

The $headers variable is always the same no matter if the file is accessible or not. The test fails and therefore access is never denied:

    if (in_array(-1, $headers)) {
      return drupal_access_denied();
    }
ñull’s picture

ñull commented
Status: Active » Needs review
StatusFileSize
new private_files_become_accessible-2408531-2.patch395 bytes

Attached the patch

ñull’s picture

ñull commented
ñull’s picture

ñull commented
StatusFileSize
new private_files_become_accessible-2408531-4.patch4.21 KB

After some debugging this patch seems to do the job.

sinasalek’s picture

sinasalek commented

Thanks, there are lots of unnecessary changes in the patch like removing and adding empty lines

ñull’s picture

ñull commented
StatusFileSize
new private_files_become_accessible-2408531-6.patch489 bytes

Sorry, I was just too lazy to find the setting in Netbeans that was stripping trailing spaces. The source code must have had some empty lines with space(s). So basically my previous patch did some useful clean-up as a bonus, but you can have it without it too, as long as this critical privacy bug is fixed.

ñull’s picture

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

After 10 months of testing in production environment this patch can be considered reviewed and tested at least by my user community. Please proceed to submit this patch at least in a dev version.

toamit’s picture

toamit commented

This patch will also fix an issue with headers that get nested due to recursive array merge used in module_invoke_all.

A similar patch is just about to be committed to core https://www.drupal.org/node/1891228
I suggest rolling this in swiftly.

Here is bit more explanation on this
Core and contributed modules use the module_invoke_all method to consolidate headers, however this function uses recursive array merge which ends up nesting arrays instead of keeping it as simple array. For example in list below the Content-Type is being nested instead of being overwritten causing the Notice and botched header
Notice: Array to string conversion in drupal_send_headers() (line 1236 of drupal-7.41/includes/bootstrap.inc)

"Content-Type" : array( 0 => "image/png", 1 => "image/png"),
"Content-Length" : "1021",
"Cache-Control" : "private,
"Content-Disposition" : "inline; filename='villi.png'"

So after calling module_invoke_all method, a flatten routine must be run to get rid of nesting. This is what most discussions and patches do or avoid doing.

    $headers = module_invoke_all('file_download', $uri);
    // Flatten any nested elements in $headers array
    foreach ($headers as $key => value) {
        if(is_array($value)) { 
            $value = reset($value);
            $headers[$key] = $value; 
        }
    }

A similar change needs to be made at line 58 in resumable_download.inc

+    // flatten any nested elements found in $header
+    if(is_array($header)){ $header = reset($header);}
      resumable_download_drupal_add_http_header($key, $header);

But if you can avoid calling module_invoke_all like the patch suggested by null, this problem does not arise and no further changes are needed

toamit’s picture

toamit commented

So this patch will fix another reported issue
https://www.drupal.org/node/2295897

sinasalek’s picture

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

NOTICE: I only applied the patches, i have not been able to test it, if someone confirm that everything works properly i can create a new release
https://www.drupal.org/project/resumable_download/releases/7.x-1.x-dev

Status: Fixed » Closed (fixed)

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