Hi,
in contrary to this already fixed issue, I've encountered a similar problem - only the other way round:

When a logged in user (even UID 1) clicks on a download link having a download token generated for anonymous users, he will get an access denied error. Imho this is wrong, because anonymous download links should always work for privileged users too. A good example, where you'd need this, is, when you send out a newsletter with a download block inside.

Proposed solution

The menu callback function (file_entity_download_page) should do an additional check, comparing the given token from the request with an anonymous download token.

For achieving this, you may want to add an additional parameter (e.g. "$force_anonymous_token") to the file_entity_get_download_token() function, that allows the generation of anonymous tokens, even if the current user is logged in.

CommentFileSizeAuthor
#8 2267483-remove-uid-from-download-token-calculation.patch3.12 KBDave Reid
#4 2267483-remove-uid-from-download-token-calculation.patch1.93 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

When a logged in user (even UID 1) clicks on a download link having a download token generated for anonymous users, he will get an access denied error.

Could you detail more about where this link is being generated and why it's being cached and displayed for different user roles?

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

Hi Dave,
for a customer, we added the possibility to have downloads block (list of file downloads) within a Simplenews newsletter. When the newsletter is sent, it is behaving like being viewed by an anonymous guest, and so the download tokens are generated for guests. This is correct because most of the subscribers do not have an account, and the downloads should be available for everyone.

But it may happen, that also an account owner receives this newsletter. When he clicks on a download link, while his session is active in his browser, he will not get access to the download, even if you are UID 1.

What I did, is implementing hook_menu_alter to override file_entity_download_page(). It only differs in this line:

<?php
if (!isset($_GET['token']) || ($_GET['token'] !== file_entity_get_download_token($file) && $_GET['token'] !== download_center_get_download_token($file, TRUE))) {
?>

Additionally, I've also cloned file_entity_get_download_token() and extended the functionality a little bit:

<?php
function download_center_get_download_token($file, $force_anonymous_token = FALSE) {
  $identifier = !$force_anonymous_token && !empty($GLOBALS['user']->uid) ? session_id() : '';
  return drupal_hmac_base64("file/$file->fid/download" . ':' . filemtime($file->uri) . ':' . $file->filesize, $identifier . drupal_get_private_key() . drupal_get_hash_salt());
}
?>
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Assigned: Unassigned » Dave Reid

Yeah, I'm starting to re-think having a unique identifier to the user in the token at all. The main purpose was to prevent crawlers from just being able to hit file/1/download, file/2/download, file/3/download, etc. While it would be ideal to have the token unique per user, it seems to be causing more problems than it is worth. If we had a token just based on data about the file itself, and removed the user ID part, then tokens would work across all users, as long as the token was correct. It's not like we're losing access control either, we still check file_entity_access() if the token passes, the user just may get an access denied if they don't actually have access to download the file...

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Title: Access denied for privileged users clicking on download links having anonymous download token » Remove user ID in download token generation and allow file download URLs to work across all users
Status: Active » Needs review
FileSize
2267483-remove-uid-from-download-token-calculation.patch1.93 KB

Let's try this out.

mike503’s picture

mike503 CreditAttribution: mike503 commented

Won't this still result in the same issue? If the filemtime changes, the token will change.

The file ID won't change. The filename might change. The file modified time might change. etc. etc. The filesize might even change if the file gets updated (the token should be updated too in that case)

The Drupal salt should not change, so drupal_get_private_key() . drupal_get_hash_salt() should be okay.

For the token to be useful and work, shouldn't it be something that is portable and doesn't require the same exact state (only the same file ID and site's salt) - it seems like the purpose of this token is to thwart people from incremental file downloads (and pseudo-DDoSing a site)

I would only use the ID and key/salt. Nothing else, and should be sufficiently secret and random enough, right?

sheldonkreger’s picture

sheldonkreger CreditAttribution: sheldonkreger commented

The use case I've run into is that users want a simple way to insert links to files inside content bodies which trigger a direct download.

CKeditor_link_file module does exactly this, but as discussed on #1995154: Add token protection to file/%file/download callback?, the current implementation of file download tokens ensures that only users who uploaded the file will be able to access them via file/fid/download?token=xxxx

What users ended up doing as a work-around was pointing directly to file URLS on the server href = sites/default/files/filename.ext . Thankfully, Drupal's permission still checks those links, but they exist outside file entity. Therefore, those usages go untracked, and if the files are updated in any way, or replaced, the links break. I've worked on sites where we have to implement mass changes to file names, move them to new directories, etc. In those cases, utilizing the File Entity abstraction system is very valuable, because even if paths to files change, File Entity is still able to map the new URI via the file_managed table.

Unfortunately, using filemtime() to generate the tokens could also lead to faulty access denied scenarios. For example, if a link is generated and used in content (node bodies, emails, etc), then the file is updated in any way, I believe the token will become invalid.

I'm willing to help implement and test a solution which ensures that tokens, once generated, will continue to work in congruence with Drupal's permission architecture, regardless of future changes to the file itself.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Ok how about we basically copy what image_style_path_token() does and just use "file/[fid]/download" as the identifier and only return a 8-character version of it?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
2267483-remove-uid-from-download-token-calculation.patch3.12 KB

This changes the token to more like a clone of image_style_path_token().

mike503’s picture

mike503 CreditAttribution: mike503 commented

yes, this is more reliable and portable.

in theory you could get away with *just* the fid, probably, unless the path is always going to be file/fid/download. the fid is a static token, everything else *could* be variable, really...

gmclelland’s picture

gmclelland CreditAttribution: gmclelland commented
Related issues: +#2158107: files: link inserted does not point to download but to entity page ("/file/FID" instead of "/system/files/FILEPATH")

So what is the best practice for making some text link to a file inside body fields?

Currently I've been linking to the hard coded path. ex sites/default/files/here-is-a.pdf

There is also a related issue in the linkit module issue queues about how best to link to files when the media and file entity modules are enabled #2158107: files: link inserted does not point to download but to entity page ("/file/FID" instead of "/system/files/FILEPATH").

An alternative is making links in the body link to the file entity (ex. file/%id) then use something like the https://drupal.org/project/rabbit_hole to redirect file entities to the actual file at sites/default/files/here-is-a.pdf

I really prefer links in my body field to not force a saveas download, but simply link directly to the file.

Another alternative is using https://drupal.org/project/portable_path so that paths to files like sites/default/files/here-is-a.pdf would be changed to file/%id

https://drupal.org/node/1900474#comment-8432261 explains why linking to sites/default/files/here-is-a.pdf is a bad idea.

Not sure what do...Lots of guessing. I would love to hear some input if anyone can share some tips.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

Thank you Dave!

This solution makes really sense. I've already tried it out and it works perfectly :-)

@gmclelland: This is definitely the wrong issue for that question, so just a short tip: try using Media browser (from Media module) and add or customize a view mode to output a download link - never tried, but that should be easy, quick and clean

sheldonkreger’s picture

sheldonkreger CreditAttribution: sheldonkreger commented
Status: Needs review » Reviewed & tested by the community

I tested #8 today. I can insert the download link with the token and Drupal's permissions system works as expected. I also tested modifying the file and testing the old download links. They continue to work.

Great work everybody. Thanks Dave for being patient and open to suggestions.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Reviewed & tested by the community » Fixed

Thanks all. Committed to 7.x-2.x.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

Hi Dave,
are you sure, that it is committed? It is not showing up here: http://cgit.drupalcode.org/file_entity

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

My bad, forgot to push.

  • Commit 6a15501 on 7.x-2.x by Dave Reid: 
    Issue #2267483 by Dave Reid | agoradesign: Fixed file download URL...

Status: Fixed » Closed (fixed)

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

chewtoy’s picture

chewtoy CreditAttribution: chewtoy commented

Hi,

Have a newb question about updating this module.

I'm experiencing the issue where the person who uploaded the file can see the link, but other users cannot.

My understanding is that I need to upgrade to 7.x-2.x-dev, but the existing module on my site updates to 7.x-2.0-beta2 when I use the update process through admin > modules.

Can you direct me to the upgrade process for me to get 7.x-2.x-dev up on my site?

Thanks!

sangeetharaog’s picture

sangeetharaog CreditAttribution: sangeetharaog commented

Hi All,

I have encountered a strange issue with file download token, the scenario I tried is that the file linking had happened on Prod environment but I imported production database into staging/dev environment and clicked on file download link and I got access denied error.

Any reason for different tokens are being generated for the same file object on different environments?

Thanks
Sangeeth

sangeetharaog’s picture

sangeetharaog CreditAttribution: sangeetharaog commented

Hi,

I have figured out the root cause for the issue I posted. It is due to $drupal_hash_salt value. If drupal_hash_salt value empty, a hash of the serialized database credentials is used as a fallback salt. This salt value will be the return value of drupal_get_hash_salt() function call.

return empty($drupal_hash_salt) ? hash('sha256', serialize($databases)) : $drupal_hash_salt;

In the scenario I posted this error, drupal_get_hash_salt return value is not same as production environment in staging or any other environment because the database credentials of each environment is unique. Hence, file_entity_get_download_token function does not generate the same token in all the environments even though the file object is same across all the environments.

Thanks
Sangeeth