ericmckean / minify

Automatically exported from code.google.com/p/minify
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

URI rewriter should try resolving against docroot AND realpath(docroot) #199

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Minify version: 2.1 (that's what the README says, is there a better way to 
tell? Downloaded just 2 months ago)
PHP version: 5.3.3

What steps will reproduce the problem?
1. Pass a currentDir option that maps 1-1 to the URL when the document root is 
lopped off
2. If it happens to involve symbolic links pointing outside of the document 
root, Minify_CSS_UriRewriter will aggressively resolve them with realpath()
3. Now it can't figure out how to create the right absolute URL because the 
real path is not in the document root at all

I patched this by removing the realpath() call on $currentDir. Then my URLs 
resolved just fine.

I understand that there is a symbolic links option for expressly specifying 
some symbolic links. What I don't get is why this is a desirable approach when 
one could just pass paths that take advantage of symlinks and not pre-resolve 
them before rewriting. This seems to work great and is a lot less complicated. 
It would help to have an example of a situation where it is not desirable. I 
think it should at least be an option to do it the simpler way.

Original issue reported on code.google.com by tommybgo...@gmail.com on 22 Oct 2010 at 4:38

GoogleCodeExporter commented 9 years ago
Which realpath call did you remove?

Original comment by mrclay....@gmail.com on 22 Oct 2010 at 6:14

GoogleCodeExporter commented 9 years ago
Here you are. I don't know how this would affect people who are using the 
option to pass an explicit list of symbolic links. But what makes me scratch my 
head is why you would want to use that option when it would be better to just 
leave symbolic links unresolved so they continue to look just like the web path.

Index: lib/minify/Minify/CSS/UriRewriter.php
===================================================================
--- lib/minify/Minify/CSS/UriRewriter.php       (revision 2480)
+++ lib/minify/Minify/CSS/UriRewriter.php       (revision 2481)
@@ -52,9 +52,13 @@         self::$_docRoot = self::_realpath(
             $docRoot ? $docRoot : $_SERVER['DOCUMENT_ROOT']
         );
-        self::$_currentDir = self::_realpath($currentDir);
+        // tom@punkave.com: the webserver does not resolve symlinks before 
resolving
+        // relative paths, so we shouldn't either
+        // self::$_currentDir = self::_realpath($currentDir);
+        self::$_currentDir = $currentDir;

Original comment by tommybgo...@gmail.com on 22 Oct 2010 at 10:03

GoogleCodeExporter commented 9 years ago
I think you're right. Just because we use realpath to determine file locations 
for security purposes doesn't mean we have to send resolved paths into the URI 
rewriter. We should send in paths as they're given (in 
$_SERVER['DOCUMENT_ROOT']) and try resolving first against that and then 
secondly against realpath($_SERVER['DOCUMENT_ROOT']). In most cases one of 
those should work.

The $min_symlinks feature is occasionally useful in some weird configurations 
so it should be left in.

FYI, the realpath() calls were done to solve problems on IIS, where 
$_SERVER['DOCUMENT_ROOT'] was often not a real file path. e.g.:
f:\mywebsite  (realpath is c:\server01\sites\mywebsite )

Original comment by mrclay....@gmail.com on 22 Oct 2010 at 11:12

GoogleCodeExporter commented 9 years ago

Original comment by mrclay....@gmail.com on 3 Feb 2011 at 10:56

GoogleCodeExporter commented 9 years ago
I know this issue is pretty old, but we use a lot of symlinks in our project 
and minify has a hard time dealing with them.

I ended up modifying the source code and replacing the calls to realpath with a 
function I wrote myself that basically does all of the ../.. directory 
resolution without following symlinks.

It really simplified things for us and made it so that min_symlinks() wasn't 
even required for us. Not sure if this would impact other usecases though.

Here is the function if you wish.

function realpath2($path) {
   $parts = explode('/', str_replace('\\', '/', $path));
   $result = array();

   foreach ($parts as $part) {
      if (!$part || $part == '.')
         continue;
      if ($part == '..')
         array_pop($result);
      else
         $result[] = $part;
   }
   $result = '/'.implode('/', $result);

   // Do a sanity check.
   if (realpath($result) != realpath($path))
      $result = realpath($path);

   return $result;
}

Original comment by cont...@toddburry.com on 9 Jun 2011 at 11:19

GoogleCodeExporter commented 9 years ago
#5, Yes, this is kind of the plan in comment 3: Reserve realpath for verifying 
the file's physical location, but only remove traversals before handing the 
path to the URI rewriter. Thanks.

Original comment by mrclay....@gmail.com on 10 Jun 2011 at 12:48