Simplify isSubDirectory check

Shaves off another 9ms per request as can be seen at https://blackfire.io/profiles/compare/dd54cef3-e58d-4a22-b8f4-c7c4b70697be/graph Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
nextcloud · Oct 7, 2016 · 0245dd7 · 0245dd7
1 parent bccc4e6
commit 0245dd7
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 43 deletions.
diff --git a/lib/private/L10N/Factory.php b/lib/private/L10N/Factory.php
@@ -2,6 +2,7 @@
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  * @copyright 2016 Roeland Jago Douma <roeland@famdouma.nl>
+ * @copyright 2016 Lukas Reschke <lukas@statuscode.ch>
  *
  * @author Bart Visscher <bartv@thisnet.nl>
  * @author Joas Schilling <coding@schilljs.com>
@@ -287,6 +288,27 @@ public function setLanguageFromRequest($app = null) {
 		return $requestLanguage;
 	}
 
+	/**
+	 * Checks if $sub is a subdirectory of $parent
+	 *
+	 * @param string $sub
+	 * @param string $parent
+	 * @return bool
+	 */
+	private function isSubDirectory($sub, $parent) {
+		// Check whether $sub contains no ".."
+		if(strpos($sub, '..') !== false) {
+			return false;
+		}
+
+		// Check whether $sub is a subdirectory of $parent
+		if (strpos($sub, $parent) === 0) {
+			return true;
+		}
+
+		return false;
+	}
+
 	/**
 	 * Get a list of language files that should be loaded
 	 *
@@ -302,10 +324,10 @@ public function getL10nFilesForApp($app, $lang) {
 		$i18nDir = $this->findL10nDir($app);
 		$transFile = strip_tags($i18nDir) . strip_tags($lang) . '.json';
 
-		if ((\OC_Helper::isSubDirectory($transFile, $this->serverRoot . '/core/l10n/')
-				|| \OC_Helper::isSubDirectory($transFile, $this->serverRoot . '/lib/l10n/')
-				|| \OC_Helper::isSubDirectory($transFile, $this->serverRoot . '/settings/l10n/')
-				|| \OC_Helper::isSubDirectory($transFile, \OC_App::getAppPath($app) . '/l10n/')
+		if (($this->isSubDirectory($transFile, $this->serverRoot . '/core/l10n/')
+				|| $this->isSubDirectory($transFile, $this->serverRoot . '/lib/l10n/')
+				|| $this->isSubDirectory($transFile, $this->serverRoot . '/settings/l10n/')
+				|| $this->isSubDirectory($transFile, \OC_App::getAppPath($app) . '/l10n/')
 			)
 			&& file_exists($transFile)) {
 			// load the translations file

diff --git a/lib/private/legacy/helper.php b/lib/private/legacy/helper.php
@@ -377,32 +377,6 @@ public static function buildNotExistingFileNameForView($path, $filename, \OC\Fil
 		return $newpath;
 	}
 
-	/**
-	 * Checks if $sub is a subdirectory of $parent
-	 *
-	 * @param string $sub
-	 * @param string $parent
-	 * @return bool
-	 */
-	public static function isSubDirectory($sub, $parent) {
-		$realpathSub = realpath($sub);
-		$realpathParent = realpath($parent);
-
-		// realpath() may return false in case the directory does not exist
-		// since we can not be sure how different PHP versions may behave here
-		// we do an additional check whether realpath returned false
-		if($realpathSub === false ||  $realpathParent === false) {
-			return false;
-		}
-
-		// Check whether $sub is a subdirectory of $parent
-		if (strpos($realpathSub, $realpathParent) === 0) {
-			return true;
-		}
-
-		return false;
-	}
-
 	/**
 	 * Returns an array with all keys from input lowercased or uppercased. Numbered indices are left as is.
 	 *

diff --git a/tests/lib/LegacyHelperTest.php b/tests/lib/LegacyHelperTest.php
@@ -76,19 +76,6 @@ function providesComputerFileSize(){
 		];
 	}
 
-	function testIsSubDirectory() {
-		$result = OC_Helper::isSubDirectory("./data/", "/anotherDirectory/");
-		$this->assertFalse($result);
-
-		$result = OC_Helper::isSubDirectory("./data/", "./data/");
-		$this->assertTrue($result);
-
-		mkdir("data/TestSubdirectory", 0777);
-		$result = OC_Helper::isSubDirectory("data/TestSubdirectory/", "data");
-		rmdir("data/TestSubdirectory");
-		$this->assertTrue($result);
-	}
-
 	function testMb_array_change_key_case() {
 		$arrayStart = array(
 			"Foo" => "bar",