2 files changed, 39 insertions, 34 deletions
diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php
index da90dfea10..0a10a9604c 100644
--- a/phpBB/includes/functions.php
+++ b/phpBB/includes/functions.php
@@ -2645,7 +2645,7 @@ function generate_board_url($without_script_path = false)
 */
 function redirect($url, $return = false, $disable_cd_check = false)
 {
-	global $db, $cache, $config, $user, $phpbb_root_path;
+	global $db, $cache, $config, $user, $phpbb_root_path, $phpbb_filesystem;
 
 	$failover_flag = false;
 
@@ -2713,16 +2713,7 @@ function redirect($url, $return = false, $disable_cd_check = false)
 				$root_dirs = array_diff_assoc($root_dirs, $intersection);
 				$page_dirs = array_diff_assoc($page_dirs, $intersection);
 
-				// Due to the dirname returned by pathinfo, the
-				// realpath for the $page_dirs array will be 2
-				// superordinate folders up from the phpBB root
-				// path even if the supplied path is in the
-				// phpBB root path. We need to subtract these 2
-				// folders in order to be able to correctly
-				// prepend '../' to the supplied path.
-				$superordinate_dirs_count = sizeof($root_dirs) - 2;
-
-				$dir = (($superordinate_dirs_count > 0) ? str_repeat('../', $superordinate_dirs_count) : '') . implode('/', $page_dirs);
+				$dir = str_repeat('../', sizeof($root_dirs)) . implode('/', $page_dirs);
 
 				// Strip / from the end
 				if ($dir && substr($dir, -1, 1) == '/')
@@ -2765,6 +2756,8 @@ function redirect($url, $return = false, $disable_cd_check = false)
 		trigger_error('INSECURE_REDIRECT', E_USER_ERROR);
 	}
 
+	$url = $phpbb_filesystem->clean_path($url);
+
 	if ($return)
 	{
 		return $url;
diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php
index e934a4ab1b..6ea94d33be 100644
--- a/tests/security/redirect_test.php
+++ b/tests/security/redirect_test.php
@@ -17,26 +17,32 @@ class phpbb_security_redirect_test extends phpbb_security_test_base
 	{
 		// array(Input -> redirect(), expected triggered error (else false), expected returned result url (else false))
 		return array(
-			array('data://x', false, 'http://localhost/phpBB'),
-			array('bad://localhost/phpBB/index.php', 'INSECURE_REDIRECT', false),
-			array('http://www.otherdomain.com/somescript.php', false, 'http://localhost/phpBB'),
-			array("http://localhost/phpBB/memberlist.php\n\rConnection: close", 'INSECURE_REDIRECT', false),
-			array('javascript:test', false, 'http://localhost/phpBB/javascript:test'),
-			array('http://localhost/phpBB/index.php;url=', 'INSECURE_REDIRECT', false),
-			array('http://localhost/phpBB/app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'),
-			array('./app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'),
-			array('app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'),
-			array('./../app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'),
-			array('./../app.php/foo/bar', false, 'http://localhost/phpBB/app.php/foo/bar'),
-			array('./../foo/bar', false, 'http://localhost/phpBB/foo/bar'),
-			array('app.php/', false, 'http://localhost/phpBB/app.php/'),
-			array('./app.php/', false, 'http://localhost/phpBB/app.php/'),
-			array('foobar', false, 'http://localhost/phpBB/foobar'),
-			array('./foobar', false, 'http://localhost/phpBB/foobar'),
-			array('foo/bar', false, 'http://localhost/phpBB/foo/bar'),
-			array('./foo/bar', false, 'http://localhost/phpBB/foo/bar'),
-			array('./../index.php', false, 'http://localhost/phpBB/index.php'),
-			array('../index.php', false, 'http://localhost/phpBB/index.php'),
+			array('data://x', false, false, 'http://localhost/phpBB'),
+			array('bad://localhost/phpBB/index.php', false, 'INSECURE_REDIRECT', false),
+			array('http://www.otherdomain.com/somescript.php', false, false, 'http://localhost/phpBB'),
+			array("http://localhost/phpBB/memberlist.php\n\rConnection: close", false, 'INSECURE_REDIRECT', false),
+			array('javascript:test', false, false, 'http://localhost/phpBB/javascript:test'),
+			array('http://localhost/phpBB/index.php;url=', false, 'INSECURE_REDIRECT', false),
+			array('http://localhost/phpBB/app.php/foobar', false, false, 'http://localhost/phpBB/app.php/foobar'),
+			array('./app.php/foobar', false, false, 'http://localhost/phpBB/app.php/foobar'),
+			array('app.php/foobar', false, false, 'http://localhost/phpBB/app.php/foobar'),
+			array('./../app.php/foobar', false, false, 'http://localhost/app.php/foobar'),
+			array('./../app.php/foobar', true, false, 'http://localhost/app.php/foobar'),
+			array('./../app.php/foo/bar', false, false, 'http://localhost/app.php/foo/bar'),
+			array('./../app.php/foo/bar', true, false, 'http://localhost/app.php/foo/bar'),
+			array('./../foo/bar', false, false, 'http://localhost/foo/bar'),
+			array('./../foo/bar', true, false, 'http://localhost/foo/bar'),
+			array('app.php/', false, false, 'http://localhost/phpBB/app.php/'),
+			array('./app.php/', false, false, 'http://localhost/phpBB/app.php/'),
+			array('foobar', false, false, 'http://localhost/phpBB/foobar'),
+			array('./foobar', false, false, 'http://localhost/phpBB/foobar'),
+			array('foo/bar', false, false, 'http://localhost/phpBB/foo/bar'),
+			array('./foo/bar', false, false, 'http://localhost/phpBB/foo/bar'),
+			array('./../index.php', false, false, 'http://localhost/index.php'),
+			array('./../index.php', true, false, 'http://localhost/index.php'),
+			array('../index.php', false, false, 'http://localhost/index.php'),
+			array('../index.php', true, false, 'http://localhost/index.php'),
+			array('./index.php', false, false, 'http://localhost/phpBB/index.php'),
 		);
 	}
 
@@ -52,21 +58,27 @@ class phpbb_security_redirect_test extends phpbb_security_test_base
 	/**
 	* @dataProvider provider
 	*/
-	public function test_redirect($test, $expected_error, $expected_result)
+	public function test_redirect($test, $disable_cd_check, $expected_error, $expected_result)
 	{
-		global $user;
+		global $user, $phpbb_root_path;
+
+		$temp_phpbb_root_path = $phpbb_root_path;
+		// We need to hack phpbb_root_path here, so it matches the actual fileinfo of the testing script.
+		// Otherwise the paths are returned incorrectly.
+		$phpbb_root_path = '';
 
 		if ($expected_error !== false)
 		{
 			$this->setExpectedTriggerError(E_USER_ERROR, $expected_error);
 		}
 
-		$result = redirect($test, true);
+		$result = redirect($test, true, $disable_cd_check);
 
 		// only verify result if we did not expect an error
 		if ($expected_error === false)
 		{
 			$this->assertEquals($expected_result, $result);
 		}
+		$phpbb_root_path = $temp_phpbb_root_path;
 	}
 }