2 files changed, 69 insertions, 89 deletions
diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php
index aea13f7679..33d0e36a28 100644
--- a/phpBB/includes/functions.php
+++ b/phpBB/includes/functions.php
@@ -2696,89 +2696,27 @@ function redirect($url, $return = false, $disable_cd_check = false)
 		// Relative uri
 		$pathinfo = pathinfo($url);
 
-		// Also treat URLs that have a non-existing basename and fit
-		// controller style URLs
-		if (!$disable_cd_check && (!file_exists($pathinfo['dirname'] . '/') || (!file_exists($url) && preg_match('/^[\.]?+[\/]?+(?:app\.php)?+[a-zA-Z0-9\/]/', $url))))
+		// Is the uri pointing to the current directory?
+		if ($pathinfo['dirname'] == '.')
 		{
-			$url = str_replace('../', '', $url);
-			$pathinfo = pathinfo($url);
+			$url = str_replace('./', '', $url);
 
-			// Also treat URLs that have a non-existing basename
-			if (!file_exists($pathinfo['dirname'] . '/') || (!file_exists($url) && preg_match('/^[\.]?+[\/]?+(?:app\.php)?+[a-zA-Z0-9\/]/', $url)))
+			// Strip / from the beginning
+			if ($url && substr($url, 0, 1) == '/')
 			{
-				// fallback to "last known user page"
-				// at least this way we know the user does not leave the phpBB root
-				if ($phpbb_path_helper instanceof \phpbb\path_helper)
-				{
-					$url = $phpbb_path_helper->get_controller_redirect_url($url);
-				}
-				else
-				{
-					$url = generate_board_url() . '/' . $user->page['page'];
-				}
-				$failover_flag = true;
+				$url = substr($url, 1);
 			}
 		}
 
-		if (!$failover_flag)
-		{
-			// Is the uri pointing to the current directory?
-			if ($pathinfo['dirname'] == '.')
-			{
-				$url = str_replace('./', '', $url);
-
-				// Strip / from the beginning
-				if ($url && substr($url, 0, 1) == '/')
-				{
-					$url = substr($url, 1);
-				}
-
-				if ($user->page['page_dir'])
-				{
-					$url = generate_board_url() . '/' . $user->page['page_dir'] . '/' . $url;
-				}
-				else
-				{
-					$url = generate_board_url() . '/' . $url;
-				}
-			}
-			else
-			{
-				// Used ./ before, but $phpbb_root_path is working better with urls within another root path
-				$root_dirs = explode('/', str_replace('\\', '/', phpbb_realpath($phpbb_root_path)));
-				$page_dirs = explode('/', str_replace('\\', '/', phpbb_realpath($pathinfo['dirname'])));
-				$intersection = array_intersect_assoc($root_dirs, $page_dirs);
-
-				$root_dirs = array_diff_assoc($root_dirs, $intersection);
-				$page_dirs = array_diff_assoc($page_dirs, $intersection);
-
-				$dir = str_repeat('../', sizeof($root_dirs)) . implode('/', $page_dirs);
-
-				// Strip / from the end
-				if ($dir && substr($dir, -1, 1) == '/')
-				{
-					$dir = substr($dir, 0, -1);
-				}
-
-				// Strip / from the beginning
-				if ($dir && substr($dir, 0, 1) == '/')
-				{
-					$dir = substr($dir, 1);
-				}
-
-				$url = str_replace($pathinfo['dirname'] . '/', '', $url);
+		$url = generate_board_url() . '/' . $url;
+	}
 
-				// Strip / from the beginning
-				if (substr($url, 0, 1) == '/')
-				{
-					$url = substr($url, 1);
-				}
+	// Clean URL and check if we go outside the forum directory
+	$url = $phpbb_path_helper->clean_url($url);
 
-				$url = (!empty($dir) ? $dir . '/' : '') . $url;
-				$url = generate_board_url() . '/' . $url;
-			}
-			$url = $phpbb_path_helper->clean_url($url);;
-		}
+	if (!$disable_cd_check && strpos($url, generate_board_url(true)) === false)
+	{
+		trigger_error('INSECURE_REDIRECT', E_USER_ERROR);
 	}
 
 	// Make sure no linebreaks are there... to prevent http response splitting for PHP < 4.4.2
diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php
index 13db4c299b..ce2d865c44 100644
--- a/tests/security/redirect_test.php
+++ b/tests/security/redirect_test.php
@@ -13,8 +13,13 @@ require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php';
 
 class phpbb_security_redirect_test extends phpbb_security_test_base
 {
+	protected $path_helper;
+
+	protected $controller_helper;
+
 	public function provider()
 	{
+		$this->controller_helper = $this->get_controller_helper();
 		// array(Input -> redirect(), expected triggered error (else false), expected returned result url (else false))
 		return array(
 			array('data://x', false, false, 'http://localhost/phpBB'),
@@ -26,13 +31,15 @@ class phpbb_security_redirect_test extends phpbb_security_test_base
 			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/phpBB/app.php/foobar'),
-			array('./../app.php/foobar', true, false, 'http://../../../foobar'),
-			array('./../app.php/foo/bar', false, false, 'http://localhost/phpBB/app.php/foo/bar'),
-			array('./../app.php/foo/bar', true, false, 'http://../../../bar'),
-			array('./../foo/bar', false, false, 'http://localhost/phpBB/foo/bar'),
-			array('./../foo/bar', true, false, 'http://../../../bar'),
+			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($this->controller_helper->url('a'), false, false, 'http://localhost/phpBB/app.php/a'),
+			array($this->controller_helper->url(''), 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'),
@@ -46,6 +53,47 @@ class phpbb_security_redirect_test extends phpbb_security_test_base
 		);
 	}
 
+	protected function get_path_helper()
+	{
+		if (!($this->path_helper instanceof \phpbb\path_helper))
+		{
+			$this->path_helper = new \phpbb\path_helper(
+				new \phpbb\symfony_request(
+					new phpbb_mock_request()
+				),
+				new \phpbb\filesystem(),
+				$this->phpbb_root_path,
+				'php'
+			);
+		}
+		return $this->path_helper;
+	}
+
+	protected function get_controller_helper()
+	{
+		if (!($this->controller_helper instanceof \phpbb\controller\helper))
+		{
+			global $phpbb_dispatcher;
+
+			$phpbb_dispatcher = new phpbb_mock_event_dispatcher;
+			$this->user = $this->getMock('\phpbb\user');
+			$phpbb_path_helper = new \phpbb\path_helper(
+				new \phpbb\symfony_request(
+					new phpbb_mock_request()
+				),
+				new \phpbb\filesystem(),
+				$phpbb_root_path,
+				$phpEx
+			);
+			$this->template = new phpbb\template\twig\twig($phpbb_path_helper, $config, $this->user, new \phpbb\template\context());
+
+			// We don't use mod_rewrite in these tests
+			$config = new \phpbb\config\config(array('enable_mod_rewrite' => '0'));
+			$this->controller_helper = new \phpbb\controller\helper($this->template, $this->user, $config, '', 'php');
+		}
+		return $this->controller_helper;
+	}
+
 	protected function setUp()
 	{
 		parent::setUp();
@@ -54,14 +102,8 @@ class phpbb_security_redirect_test extends phpbb_security_test_base
 			'force_server_vars'	=> '0',
 		);
 
-		$this->path_helper = new \phpbb\path_helper(
-			new \phpbb\symfony_request(
-				new phpbb_mock_request()
-			),
-			new \phpbb\filesystem(),
-			$this->phpbb_root_path,
-			'php'
-		);
+		$this->path_helper = $this->get_path_helper();
+		$this->controller_helper = $this->get_controller_helper();
 	}
 
 	/**