nginx_accel_redirect_file_transfer() accidentally is an implementation of hook_file_transfer()

I've isolated this to a bug with another module (nginx_accel_redirect). When FE executes

module_invoke_all('file_transfer', $file->uri, $headers);

The headers can obviously be broken by other modules. I'm going to dig deeper and perhaps move this over to the nginx_accel_redirect module issue queue.

This is a bit tricky.

Inside nginx_accel_redirect.module, we look for any implementations of hook_file_download() to gather headers. That makes good sense. This code is taken from nginx_accel_redirect_file_transfer() :

    foreach (module_implements('file_download') as $module) {
      $function = $module . '_file_download';
      $result = $function($uri);
      if ($result == -1) {
        return drupal_access_denied();
      }
      if (isset($result) && is_array($result)) {
        $headers = array_merge($headers, $result);
      }
    }
    if (count($headers)) {
      foreach ($headers as $name => $value) {
        drupal_add_http_header($name, $value);
      }
      drupal_send_headers();

However, for files accessed via /file/FID/download . . . File Entity has implemented a menu callback with a custom function that *isn't* hook_file_download() :

(file_entity.module line 232)

  $items['file/%file/download'] = array(
    'title' => 'Download',
    'page callback' => 'file_entity_download_page',
    'page arguments' => array(1),
    'access callback' => 'file_entity_access',
    'access arguments' => array('download', 1),
    'file' => 'file_entity.pages.inc',
    'type' => MENU_CALLBACK,

(file_entity_pages.inc)

function file_entity_download_page($file) {
  // Ensure there is a valid token to download this file.
  if (!variable_get('file_entity_allow_insecure_download', FALSE)) {
    if (!isset($_GET['token']) || $_GET['token'] !== file_entity_get_download_token($file)) {
      return MENU_ACCESS_DENIED;
    }
  }

  // If the file does not exist it can cause problems with file_transfer().
  if (!is_file($file->uri)) {
    return MENU_NOT_FOUND;
  }

  $headers = array(
    'Content-Type' => mime_header_encode($file->filemime),
    'Content-Disposition' => 'attachment; filename="' . mime_header_encode(drupal_basename($file->uri)) . '"',
    'Content-Length' => $file->filesize,
    'Content-Transfer-Encoding' => 'binary',
    'Pragma' => 'no-cache',
    'Cache-Control' => 'must-revalidate, post-check=0, pre-check=0',
    'Expires' => '0',
  );

  // Let other modules alter the download headers.
  drupal_alter('file_download_headers', $headers, $file);

  // Let other modules know the file is being downloaded.
  module_invoke_all('file_transfer', $file->uri, $headers);

  file_transfer($file->uri, $headers);
}

As you can see, the headers are set inside this function. THEN, module_invoke_all() is executed. This is when nginx_accel_redirect_file_transfer() is finally executed. So, this function doesn't respect the headers which file_entity set earlier up the ladder.

I see two options for fixing this.

1. Move the code from file_entity_download_page() to file entity's hook_file_download() file_entity_file_download(). It seems reasonable to put the code responsible for file downloads inside hook_file_download(). However, I'm not sure of the implications of this (perhaps this hook was avoided intentionally).

2. Modify the logic which gathers headers inside nginx_accel_redirect_file_transfer() to check for both hook_file_download() and the custom file entity implementation of file_entity_download_page().

It's probably safer to modify nginx_accel_redirect rather than file_entity. But, it does seem logical to do #1.

Thoughts?

This patch applies to File Entity 7.x-2.x-dev so I have moved the issue back to File Entity. Please see comments above for an explanation of the problem.

This patch moves the declaration of headers for File Entity into its own function. This was being done inside the File Entity custom function file_entity_download_page() but now it stands alone (I did not change the code at all).

By moving it into its own function, I was able to use it inside file_entity_file_download() rather than file_get_content_headers(). This allows the hook system that Nginx Accel Redirect utilizes to work correctly, because it gathers ALL header modifications which happen inside any hook_file_download(). Setting headers with file_entity_get_content_headers() inside file_entity_file_download() solves the problem by ensuring that all required headers are set via the hook system other modules expect.

Using file_get_content_headers() leaves out critical headers for file entities (which is why it was basically over-ridden by File Entity already).

Hmm . . . not sure if the failing tests are a consequence of my code. I didn't modify anything relating to file access, so I suspect this problem is caused elsewhere.

I re-submitted a test on another issue and that one passed. So I think it's safe to assume that my patch actually does break some access stuff. I'll take a look.

5: dl_names-2278625-5.patch queued for re-testing.

@Dave Can you post a link to that commit? I agree we should address the underlying issue.