Hi,

I was working on a customer website and I spotted that small problem in file_download()

If you set a file system path not shared on the web and if you set the download method to Private, you'll never enter the condition. (if (file_exists(file_create_path($filepath))) {...)

This is the original function is:

function file_download() {
  // Merge remainder of arguments from GET['q'], into relative file path.
  $args = func_get_args();
  $filepath = implode('/', $args);

  // Maintain compatibility with old ?file=paths saved in node bodies.
  if (isset($_GET['file'])) {
    $filepath =  $_GET['file'];
  }

  if (file_exists(file_create_path($filepath))) {
    $headers = module_invoke_all('file_download', $filepath);
    if (in_array(-1, $headers)) {
      return drupal_access_denied();
    }
    if (count($headers)) {
      file_transfer($filepath, $headers);
    }
  }
  return drupal_not_found();
}

The corrected version is:

function file_download() {
  // Merge remainder of arguments from GET['q'], into relative file path.
  $args = func_get_args();
  $filepath = implode('/', $args);	

  // Maintain compatibility with old ?file=paths saved in node bodies.
  if (isset($_GET['file'])) {
    $filepath =  $_GET['file'];
  }

  if (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PRIVATE) {
    $filepath = file_directory_path()."/".array_pop(explode('system/files/',$filepath));
  }

  if (file_exists(file_create_path($filepath))) {
  	$headers = module_invoke_all('file_download', $filepath);
    if (in_array(-1, $headers)) {
      return drupal_access_denied();
    }
    if (count($headers)) {
      file_transfer($filepath, $headers);
    }
  }
  return drupal_not_found();
}

I've just added those 3 lines:

  if (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PRIVATE) {
    $filepath = file_directory_path()."/".array_pop(explode('system/files/',$filepath));
  }

I provide the patch, it's my first time, I hope I'm doing it well ;-)

CommentFileSizeAuthor
#6 filedownload.patch824 bytespol
#4 filedownload.patch820 bytespol
#1 file_download.patch745 bytespol
file_download.patch709 bytespol

Comments

pol’s picture

pol
the/that
Location Brussels
commented
StatusFileSize
new file_download.patch745 bytes

Here is the updated patch, changed it a bit to respect coding standards.

pol’s picture

pol
the/that
Location Brussels
commented
Status: Active » Needs review
deviantintegral’s picture

deviantintegral commented
Version: 6.10 » 7.x-dev
Status: Needs review » Needs work

Can you update your code style so that there are spaces in between the concat operators? A comment about why this is needed in the code would also be useful.

I haven't tested this patch, but I wonder if the actual bug is in file_create_path() or file_exists(). If that's the case then the fix should be in one of those functions.

Can you check to see if this is still an issue in HEAD? If it isn't, then perhaps whatever changes fixes the issue should be backported instead. If it still happens in D7, then create a patch for that first which can be backported.

pol’s picture

pol
the/that
Location Brussels
commented
StatusFileSize
new filedownload.patch820 bytes

Hi DeviantIntegral,

The fonction file_exists() comes from PHP and don't have any problem.

The problems comes from file_download().

I made a patch against HEAD, this is the attached file. It must be backported to D6.

I discovered this while playing with file_force module. When you set the download method to Private, it returns an error 404. Why ?
Easy, file_download() do not translate the path when the file is private ! That's what the patch is correcting !

deviantintegral’s picture

deviantintegral commented
Status: Needs work » Needs review

Thanks for the updated patch, though it still needs to have spaces between the concat operators - such as:

$filepath = file_directory_path() . "/" . array_pop(explode('system/files/',$filepath));

I'm setting to CNR though so the test bot can give it a run.

pol’s picture

pol
the/that
Location Brussels
commented
Status: Needs review » Needs work
StatusFileSize
new filedownload.patch824 bytes

Sorry this is my first patch ;-)

Here it is corrected

deviantintegral’s picture

deviantintegral commented
Status: Needs work » Needs review

Great, setting to CNR for testbot'ing then.

pol’s picture

pol
the/that
Location Brussels
commented

It seems that it passed the test successfully... now what's next ? :)

c960657’s picture

c960657 commented

Hmm, this doesn't sound right. I use private downloads a lot without problems, so I think you are doing something wrong. What is the URL, you are calling, and what is the value of $args?

cburschka’s picture

cburschka
they
commented 
array_pop(explode('system/files/', $filepath))

This array stuff looks, um, "clever" (read hack). Would a plain strstr or substr not work here?

cburschka’s picture

cburschka
they
commented
Status: Needs review » Needs work

Addendum: This is definitely the wrong way to fix whatever bug you might have. file_download() is not supposed to receive a path prefixed with system/files. system/files is the menu callback:

  $items['system/files'] = array(
    'title' => 'File download',
    'page callback' => 'file_download',
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
  );

Menu callbacks are supposed to receive only the path arguments following the callback path - system/files shouldn't be in there.

This might be a rare menu bug. I don't know how many places in core still use menu callbacks with non-explicit wildcards, so it'd be easy to miss.

In any case, $filepath in file_download() is intended to contain only the path below the files directory, without system/files or sites/default/files. file_create_path will then prefix sites/default/files, and everything works as it should.

Next, we need to figure out why you're seeing system/files passed to the callback. Please first make sure you're using an up-to-date and clean checkout of core.

codecowboy’s picture

codecowboy commented
Status: Needs work » Closed (fixed)

This issue is six months dead, and the file handling has changed significantly.

marcoka’s picture

marcoka commented

has this been backported to D6? i took a look in file.inc and i can not see the fix.

pol’s picture

pol
the/that
Location Brussels
commented

This has never been accepted... :( sadly.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Closed (fixed) » Closed (works as designed)

Proper status.

pol’s picture

pol
the/that
Location Brussels
commented

??? What happened ???

damien tournoud’s picture

damien tournoud commented

There is nothing to fix here. Private file download (and the file_download() function) on Drupal 6 is working as it should.

arski’s picture

arski commented

Hmm, PoI, I tend to agree with the guys here who say that there is nothing that should be fixed in file_download. What's happening here is that file_force prepends "system/files" as the filepath when private downloads are enabled, whereas in this case "system/files" is not a 'real' path, but a menu callback. Let's discuss it further over at #377998: Doesn't work with private downloads.