The backup_migrate_file_download() hook returns improperly and causes webfm and other module to fail [#1567230]

There is a bug in the backup_migrate module function _backup_migrate_path_is_in_save_dir() that causes a wrong return from the backup_migrate_file_download() hook.

Line 1021 in 'back_migrate.module() is currently:
return ($path && file_exists($backup_dir)) ? file_create_path($path, $backup_dir) : FALSE;

But is should be:
return ($path && file_exists($backup_dir)) ? file_check_location($path, $backup_dir) : FALSE;

My particular problem was that webfm was failing to download files because of this bug also noted in their issue queue here - http://drupal.org/node/1496962#comment-5969506

Comment	File	Size	Author
#5	fix-hook_file_download-private-file-downloads.patch	547 bytes	caktux

Comments

Comment #1

glynnr commented 28 December 2012 at 16:31

Confirmed and thank you.

I appreciate this is an old report on an old code base, but I unfortunately have had to work on it recently and am not is position to upgrade at the moment. In case anyone else finds themselves in this position, here are my findings :

This was breaking image_browser, which calls image_fetch from the image module to stream the actual file. This function invokes hook_file_download via module_invoke_all, which collects all the headers for this image.

The bug reported above was causing the _backup_migrate_path_is_in_save_dir function to erroneously return TRUE and therefore causing execution to fall through into the access checks, instead of skipping to the end. As a result, backup_migrate_file_download was returning -1, which gets merged in to the headers array.

As the return from hook_file_download is an array, execution passes through the conditional (if ($headers == -1)) and sets the headers. Trying to set a header of -1 causes the server to return an error 500

In addition, the result of the _backup_migrate_path_is_in_save_dir check, which is a boolean value, was being assigned to the $path variable, thus invalidating call to _backup_migrate_file_info($path)

So to fix : Implement the above suggestion, and remove the assignment

Comment #2

webservant316 commented 29 December 2012 at 02:01

yes - still agreed. I am still patching the backup_migrate modules personally until this bug is fixed.

Comment #3

ghede commented 20 January 2013 at 06:49

I also had this issue with webfm, and changing the code fixed it. Researching this I noticed that this issue is flagged as happening in version 6x-2.6. I'm using the older 6x-1.3. I did a search for the code in version 6x-2.6 and 7x-2.4 and it did not show up - maybe I missed something or the version setting for this issue is incorrect?

Anyway, thanks for the help.
Jim