[TASK] Optimize GeneralUtility path methods 05/26505/14
authorMichiel Roos <michiel@maxserv.nl>
Thu, 19 Dec 2013 23:40:49 +0000 (00:40 +0100)
committerMarkus Klein <klein.t3@mfc-linz.at>
Fri, 14 Feb 2014 14:40:15 +0000 (15:40 +0100)
The following methods are amongst the most frequently called ones:
* getFileAbsFileName()
* validPathStr()
* isAllowedAbsPath()
* verifyFilenameAgainstDenyPattern()

They can be optimized to return early, save some variable assignments and
save some preg_match calls.

Change-Id: Id30b2f9b5a053d4267d9c24339f414821ba661ea
Resolves: #54525
Releases: 6.2
Reviewed-on: https://review.typo3.org/26505
Reviewed-by: Helmut Hummel
Tested-by: Helmut Hummel
Reviewed-by: Stefan Neufeind
Reviewed-by: Markus Klein
Tested-by: Markus Klein
typo3/sysext/core/Classes/Utility/GeneralUtility.php
typo3/sysext/core/Tests/Unit/Utility/GeneralUtilityTest.php

index deaa741..4150beb 100644 (file)
@@ -2953,7 +2953,7 @@ Connection: close
                }
                $fileArr = array_merge($fileArr, self::getFilesInDir($path, $extList, 1, 1, $excludePattern));
                $dirs = self::get_dirs($path);
-               if (is_array($dirs) && $recursivityLevels > 0) {
+               if ($recursivityLevels > 0 && is_array($dirs)) {
                        foreach ($dirs as $subdirs) {
                                if ((string) $subdirs != '' && (!strlen($excludePattern) || !preg_match(('/^' . $excludePattern . '$/'), $subdirs))) {
                                        $fileArr = self::getAllFilesAndFoldersInPath($fileArr, $path . $subdirs . '/', $extList, $regDirs, $recursivityLevels - 1, $excludePattern);
@@ -2989,7 +2989,7 @@ Connection: close
         * @return string
         */
        static public function fixWindowsFilePath($theFile) {
-               return str_replace('//', '/', str_replace('\\', '/', $theFile));
+               return str_replace(array('\\', '//'), '/', $theFile);
        }
 
        /**
@@ -3006,17 +3006,17 @@ Connection: close
                $parts = explode('/', $pathStr);
                $output = array();
                $c = 0;
-               foreach ($parts as $pV) {
-                       if ($p== '..') {
+               foreach ($parts as $part) {
+                       if ($part === '..') {
                                if ($c) {
                                        array_pop($output);
-                                       $c--;
+                                       --$c;
                                } else {
-                                       $output[] = $pV;
+                                       $output[] = $part;
                                }
                        } else {
-                               $c++;
-                               $output[] = $pV;
+                               ++$c;
+                               $output[] = $part;
                        }
                }
                return implode('/', $output);
@@ -3609,27 +3609,27 @@ Connection: close
         *
         *************************/
        /**
-        * Returns the absolute filename of a relative reference, resolves the "EXT:" prefix (way of referring to files inside extensions) and checks that the file is inside the PATH_site of the TYPO3 installation and implies a check with \TYPO3\CMS\Core\Utility\GeneralUtility::validPathStr(). Returns FALSE if checks failed. Does not check if the file exists.
+        * Returns the absolute filename of a relative reference, resolves the "EXT:" prefix
+        * (way of referring to files inside extensions) and checks that the file is inside
+        * the PATH_site of the TYPO3 installation and implies a check with
+        * \TYPO3\CMS\Core\Utility\GeneralUtility::validPathStr().
         *
         * @param string $filename The input filename/filepath to evaluate
         * @param boolean $onlyRelative If $onlyRelative is set (which it is by default), then only return values relative to the current PATH_site is accepted.
         * @param boolean $relToTYPO3_mainDir If $relToTYPO3_mainDir is set, then relative paths are relative to PATH_typo3 constant - otherwise (default) they are relative to PATH_site
-        * @return string Returns the absolute filename of $filename IF valid, otherwise blank string.
+        * @return string Returns the absolute filename of $filename if valid, otherwise blank string.
         */
        static public function getFileAbsFileName($filename, $onlyRelative = TRUE, $relToTYPO3_mainDir = FALSE) {
                if ((string)$filename === '') {
                        return '';
                }
+               $relPathPrefix = PATH_site;
                if ($relToTYPO3_mainDir) {
-                       if (!defined('PATH_typo3')) {
-                               return '';
-                       }
                        $relPathPrefix = PATH_typo3;
-               } else {
-                       $relPathPrefix = PATH_site;
                }
+
                // Extension
-               if (substr($filename, 0, 4) == 'EXT:') {
+               if (strpos($filename, 'EXT:') === 0) {
                        list($extKey, $local) = explode('/', substr($filename, 4), 2);
                        $filename = '';
                        if ((string)$extKey !== '' && \TYPO3\CMS\Core\Utility\ExtensionManagementUtility::isLoaded($extKey) && (string)$local !== '') {
@@ -3646,6 +3646,7 @@ Connection: close
                        // checks backpath.
                        return $filename;
                }
+               return '';
        }
 
        /**
@@ -3661,10 +3662,8 @@ Connection: close
         * @todo Possible improvement: Should it rawurldecode the string first to check if any of these characters is encoded?
         */
        static public function validPathStr($theFile) {
-               if (strpos($theFile, '//') === FALSE && strpos($theFile, '\\') === FALSE && !preg_match('#(?:^\\.\\.|/\\.\\./|[[:cntrl:]])#u', $theFile)) {
-                       return TRUE;
-               }
-               return FALSE;
+               return strpos($theFile, '//') === FALSE && strpos($theFile, '\\') === FALSE
+                       && !preg_match('#(?:^\\.\\.|/\\.\\./|[[:cntrl:]])#u', $theFile);
        }
 
        /**
@@ -3674,12 +3673,7 @@ Connection: close
         * @return boolean
         */
        static public function isAbsPath($path) {
-               // On Windows also a path starting with a drive letter is absolute: X:/
-               if (TYPO3_OS === 'WIN' && (substr($path, 1, 2) === ':/' || substr($path, 1, 2) === ':\\')) {
-                       return TRUE;
-               }
-               // Path starting with a / is always absolute, on every system
-               return $path[0] === '/';
+               return $path[0] === '/' || TYPO3_OS === 'WIN' && (strpos($path, ':/') === 1 || strpos($path, ':\\') === 1);
        }
 
        /**
@@ -3689,32 +3683,29 @@ Connection: close
         * @return boolean
         */
        static public function isAllowedAbsPath($path) {
-               if (self::isAbsPath($path) && self::validPathStr($path) && (self::isFirstPartOfStr($path, PATH_site) || $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] && self::isFirstPartOfStr($path, $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']))) {
-                       return TRUE;
-               }
+               $lockRootPath = $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'];
+               return self::isAbsPath($path) && self::validPathStr($path)
+                       && (self::isFirstPartOfStr($path, PATH_site)
+                               || $lockRootPath && self::isFirstPartOfStr($path, $lockRootPath));
        }
 
        /**
         * Verifies the input filename against the 'fileDenyPattern'. Returns TRUE if OK.
         *
+        * Filenames are not allowed to contain control characters. Therefore we
+        * allways filter on [[:cntrl:]].
+        *
         * @param string $filename File path to evaluate
         * @return boolean
         */
        static public function verifyFilenameAgainstDenyPattern($filename) {
-               // Filenames are not allowed to contain control characters
-               if (preg_match('/[[:cntrl:]]/', $filename)) {
-                       return FALSE;
-               }
+               $pattern = '/[[:cntrl:]]/';
                if ((string)$filename !== '' && (string)$GLOBALS['TYPO3_CONF_VARS']['BE']['fileDenyPattern'] !== '') {
-                       $result = preg_match('/' . $GLOBALS['TYPO3_CONF_VARS']['BE']['fileDenyPattern'] . '/i', $filename);
-                       if ($result) {
-                               return FALSE;
-                       }
+                       $pattern = '/(?:[[:cntrl:]]|' . $GLOBALS['TYPO3_CONF_VARS']['BE']['fileDenyPattern'] . ')/i';
                }
-               return TRUE;
+               return !preg_match($pattern, $filename);
        }
 
-
        /**
         * Low level utility function to copy directories and content recursive
         *
index 212da7d..58be263 100644 (file)
@@ -4062,12 +4062,31 @@ class GeneralUtilityTest extends \TYPO3\CMS\Core\Tests\UnitTestCase {
        }
 
        /**
-        * Tests whether verifyFilenameAgainstDenyPattern detects the NULL character.
+        * @return array
+        */
+       public function deniedFilesDataProvider() {
+               return array(
+                       'Nul character in file' => array('image' . chr(0) . '.gif'),
+                       'Nul character in file with .php' => array('image.php' . chr(0) . '.gif'),
+                       'Regular .php file' => array('file.php'),
+                       'Regular .php5 file' => array('file.php5'),
+                       'Regular .php3 file' => array('file.php3'),
+                       'Regular .phpsh file' => array('file.phpsh'),
+                       'Regular .phtml file' => array('file.phtml'),
+                       'PHP file in the middle' => array('file.php.txt'),
+                       '.htaccess file' => array('.htaccess'),
+               );
+       }
+
+       /**
+        * Tests whether verifyFilenameAgainstDenyPattern detects denied files.
         *
+        * @param string $deniedFile
         * @test
+        * @dataProvider deniedFilesDataProvider
         */
-       public function verifyFilenameAgainstDenyPatternDetectsNullCharacter() {
-               $this->assertFalse(Utility\GeneralUtility::verifyFilenameAgainstDenyPattern('image\0.gif'));
+       public function verifyFilenameAgainstDenyPatternDetectsNotAllowedFiles($deniedFile) {
+               $this->assertFalse(Utility\GeneralUtility::verifyFilenameAgainstDenyPattern($deniedFile));
        }