Paths outside of Drupal root incorrectly detected

Do you have a slash (or anything else) in your "Also considered local" field in the filter's configuration settings?

Okay, I was able to replicate this bug, and got a fix in in the most recent release. Please give it a download and a try. If it still seems broken, please reopen this issue.

The commit, for reference: http://drupalcode.org/project/pathologic.git/commitdiff/94a085869959c427...

I think this is the wrong logic. We need to be comparing the url paths to determine if the file's in the Drupal root, not the system paths. The use of realpath here means that a variety of use-cases (in my case, having a files directory that's symbolically linked to elsewhere on the filesystem) break. I'll try to whip up a patch this afternoon.

This patch changes more than I would have really liked, but I felt it was necessary. It needs plenty of review because there are a lot of edge-cases out there with URLs that may have escaped me. Patch is against 7.x-2.x-dev

Updated the title to hopefully make it clearer.

It needs plenty of review because there are a lot of edge-cases out there with URLs that may have escaped me.

Yeah… applying your patch causes a lot of Pathologic's tests to fail. It would cause more problems than it would solve, I'm afraid. (If you're unfamiliar with Drupal's testing system, the easiest way to check if your code breaks Pathologic's tests is to enable the Simpletest module, then run drush test-run PathologicTestCase and give it a minute (testing is slow). If tests fail, you can search pathologic.test for the text of the fail message to see what sort of input Pathologic is trying to process what is causing the failure.)

Thanks for the lead that the cause of this is indeed symlinks outside of the Drupal root directory. Going off that, I was able to easily recreate the problem. I'll work from there and hopefully have a fix in place before you even have time to post a new patch. :)

Okay, I tried this tweak, and it seems to work, at least insofar as I was able to replicate the problem, even though it's much simpler. Do you think you could give it a try on your end once a new dev release becomes available?

Thanks for reviewing the patch.

Frankly, I didn't even think about running it through tests! Haha. Oops. I'll look into that.

Your follow-up doesn't seem to account for all cases. Consider a case where $_SERVER['DOCUMENT_ROOT'] is symlinked *and* DRUPAL_ROOT is symlinked from somewhere else. Then, $settings['current_settings']['docroot'] won't match DRUPAL_ROOT down on line 224.

if (strpos($filepath, DRUPAL_ROOT) !== 0) {

adding realpath() like so still doesn't fix the problem because DRUPAL_ROOT could be symlinked from an entirely different part of the filesystem.

if (strpos($filepath, realpath(DRUPAL_ROOT)) !== 0) {

And you still have a problem if Drupal is being run from inside a subdirectory as, at that point you haven't run through the host-and-path-matching foreach loop. So you don't know if $parts['path'] is relative to the drupal root or the web root, thus making the comparison unreliable (especially so if this comparison is being done with drush and we don't have access to $_SERVER['DOCUMENT_ROOT'] which is a very big deal).

There are a lot of nuances to the logic here, but I think we should try to stick to parsing the web path itself rather than attempting to figure things out at the file system level. My patch tries to repair what I saw as faulty logic in the foreach loop for determining if something's really inside the Drupal root. I think I'll take a stab at what tests it's failing before giving up on it.

Garrett: I took a look at the test failures and they reflect a difference in how we detect whether a given path references a file.

Your original logic tries to determine if the file exists on the filesystem to determine if something is a file.

My patch considers something a file if menu_get_item() doesn't find anything in the Drupal menu system to handle the request.

I think an argument could be made in favor of either perspective.

I'll see if I can update my patch to match your logic.

Ok, that was easy. Try this patch on for size.

I should mention that the patch in #14 passes all Pathologic tests for me.

Fixed comment that references old logic.

azinck, I presume you're not running a multi-lingual site, because some of the stuff your patch took out was breaking that - taking out the "fake" language object when $settings['is_file'] causes paths to images to get prefixed with the language prefix. There was some other stuff I had to clean up, but for the most part, I'm going to take your patch and hope it clears things up with this problem, because I'm pretty much at a loss as to how to move forward, so maybe you're on to something.

(Yes, all the tests pass, but there's some stuff I don't know how to test for, such as multi-lingual-related stuff, and others which I'm not sure it's possible to write tests for, like the symlink issues…)

i can confirm this commit fixes pathologic for our setup with a spider web of symlinks. thanks!!

Garrett,

Thanks for continuing to look into this. I did some testing with multi-lingual when I was tweaking that part of the code. I thought I had it sorted but maybe I missed something. I'll look at it again when I get back to work (just had a new baby).