I suggest to add an option in module configure form to allow administrator using some code like:

<IfModule mod_rewrite.c>
  RewriteEngine on
  RewriteBase /system/files/private
  RewriteRule ^(.*)$ $1 [L,R=301]
</IfModule>

in the .htaccess file in private upload directory, instead of the current code:

Deny from all

In this way, when someone try to access the private file using a public download path,
the request will be redirected to perform access check before allowing download,
instead of immediately giving an access deny to client.

The idea is taken from the private_download module.
Although this method will lose the protection if mod_rewrite is not enabled,
we can add some check in configure page to only allow enabling this function if mod_rewrite is working.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rjacobs’s picture

Hummm, this seems like a fantastic idea.

Getting routed to the incorrect internal file download path is a common problem with Private Upload is seems. It's quite a salient issue when it comes to views for example. We are working on a patch that will make views "aware" of the correct path at #306933: Accessing file from views, but there are still lots of cases where various view plugins/handlers will get it wrong... and will link to the direct "real" file path (leading to that access denied error).

It seems that this is related to the fact that there is no "file_create_url" hook... so it's up to each module to build the file download link (not Drupal). So I think getting every module to play nice with Private Upload would mean patching every module that builds file paths for node file attachments. I could be wrong about this, but this seems accurate based on some experience.

A rewrite would totally short-circuit this problem for sites that support it.

I'm wondering if it would make sense to place a rewrite in the root .htaccess file instead of the private files folder? Placing it in the private folder seems the most graceful, but I'm wondering if the module code will check and overwrite this on upgrades or other changes?

I agree that some form of check is required to ensure mod_rewrite is working. Are there any other security factors at play here?

rjacobs’s picture

Just noticed that the "status" report at admin/settings/private_upload remains happy if implementing the rewrite suggested (even if removing the Deny from all line).

Without either the rewrite or the Deny from all line, it reports an error, but so long as one of the two are in place, the internal security checks pass.

I'm not sure how this check is implemented, but it seems to be looking for more than just a specific .htaccess structure, and presumably would also report an error is nod_rewite was not active. If implementing the rewrite suggested, perhaps this mechanism could help notify special case users where the rewrite is not possible, or even enforce a different .htaccess structure in these cases.

rjacobs’s picture

Title: Option to use "mod_rewrite" in .htaccess rather than "deny from all" » Option to use "mod_rewrite" in .htaccess rather than "deny from all" (avoid "Access Denied" errors from incorrect file paths)
Status: Active » Needs review
FileSize
921 bytes

I'm searching for a solution for this in regards to a number of projects (specifically one that uses Open Atrium which exposes download links via all sorts of views and views handlers), so I had a look at the code.

It looks like private_upload checks the security of your private directory by placing a "privacy_test.txt" file there and then seeing if it's directly accessible. So no matter what method is used to protect the directory (Deny access, rewrite, etc.) it's security checks should still apply.

Furthermore, it seems that using the IfModule !mod_rewrite.c check could allow us to see if a user does not have mod_rewrite enabled, and enforce protection the original way if this is the case. So the .htaccess file could look like:

SetHandler This_is_a_Drupal_security_line_do_not_remove
<IfModule !mod_rewrite.c>
  Deny from all
</IfModule>
<IfModule mod_rewrite.c>
  RewriteEngine on
  RewriteBase /system/files/private
  RewriteRule ^(.*)$ $1 [L,R=301]
</IfModule>

I'm attaching a patch that implements this structure. It defines the RewriteBase path via base_path() . "system/files/" . variable_get('private_upload_path', 'private'), so I think it's smart enough to work with a Drupal install that's in a subdirectory or uses a custom-named private folder (though I have not fully tested this).

Patch is relative to the module folder and should work via patch -p0 < 636516-3-private_upload_add_htaccess_rewrite.patch

If you try this patch you'll want to visit "admin/private_upload/add_htaccess" to re-create your .htaccess file

IMHO this could be hugely useful, and I'm curious if others see any security issues or other problems with this approach

jrsinclair’s picture

This patch looks perfect for a project I will be working on very soon. I will download and give it a go.

rjacobs’s picture

Hi jrsinclair - let us know how things go, and if you have any reason to believe there are security/access implications with this. I think this solution could have some impact on a number of link-related issues with Private Upload, so I'd love to see if the ideas in this thread lead anywhere.

jrsinclair’s picture

Well, I installed the module and applied the patch, and it all seemed to work great. Also couldn't see any security issues with it.

Unfortunately though, I'm going to have to abandon private_upload for my project as I need finer grained control over permissions (see #496688: Add permission to separate files marked private from the generic 'view uploaded files')

rjacobs’s picture

I've been having good success with this patch across a number of sites, with varying structures (including a couple open atrium site where I have setup private upload... something which I feel should be pre-bundled with Open Atrium, but that's another story). Of course I am always using a pretty standard LAMP environment, and have not considered what could happen elsewhere. I've also only been testing sites where Drupal is installed in the domain root (as opposed to a sub-directory).

Is anyone else willing to review under other circumstances... or have conceptual feedback on the idea?

Best,
Ryan

rjacobs’s picture

This has been idle for over a year (with "needs review" status). I'm just checking back to see if anyone sees value here?