Merge remote-tracking branch 'Marc/ticket/11997' into develop

* Marc/ticket/11997: (23 commits)
  [ticket/11997] Use functional test cases that should always work
  [ticket/11997] Fix redirect tests for mod rewrite
  [ticket/11997] Add user's page dir to redirect path and fix unit tests for it
  [ticket/11997] Remove obsolete function get_controller_redirect_url()
  [ticket/11997] Use path_helper in in foo/bar extension for redirect URLs
  [ticket/11997] Add remove_web_root_path() in order to prevent incorrect URLs
  [ticket/11997] Do not check if file or dir we redirect to exist
  [ticket/11997] Modifiy tests after adding path_helper clean_url method
  [ticket/11997] Add clean_url() method to path_helper
  [ticket/11997] Allow redirects to parent folders like previously
  [ticket/11997] Move expected redirect returns to controller and output to HTML
  [ticket/11997] Fix tests for path_helper's get_controller_redirect_url()
  [ticket/11997] Use get_controller_redirect_url() in redirect() function
  [ticket/11997] Add method for controller redirect URLs to path helper
  [ticket/11997] Undo changes to phpbb_own_realpath()
  [ticket/11997] Remove obsolete failover_flag in function redirect()
  [ticket/11997] Add functional test for redirects in controller
  [ticket/11997] Fix missing global
  [ticket/11997] Fix redirects from inside controllers
  [ticket/11997] Use $phpbb_filesystem->clean_path() for proper redirect paths
  ...

phpBB/includes/functions.php

@@ -2434,7 +2434,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, $phpbb_path_helper, $phpEx;
 
 	$failover_flag = false;
 
@@ -2477,78 +2477,34 @@ function redirect($url, $return = false, $disable_cd_check = false)
 		// Relative uri
 		$pathinfo = pathinfo($url);
 
-		if (!$disable_cd_check && !file_exists($pathinfo['dirname'] . '/'))
+		// Is the uri pointing to the current directory?
+		if ($pathinfo['dirname'] == '.')
 		{
-			$url = str_replace('../', '', $url);
+			$url = str_replace('./', '', $url);
-			$pathinfo = pathinfo($url);
 
-			if (!file_exists($pathinfo['dirname'] . '/'))
+			// Strip / from the beginning
+			if ($url && substr($url, 0, 1) == '/')
 			{
-				// fallback to "last known user page"
+				$url = substr($url, 1);
-				// at least this way we know the user does not leave the phpBB root
-				$url = generate_board_url() . '/' . $user->page['page'];
-				$failover_flag = true;
 			}
 		}
 
-		if (!$failover_flag)
+		$url = $phpbb_path_helper->remove_web_root_path($url);
+
+		if ($user->page['page_dir'])
 		{
-			// Is the uri pointing to the current directory?
+			$url = $user->page['page_dir'] . '/' . $url;
-			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);
-
-				// Strip / from the beginning
-				if (substr($url, 0, 1) == '/')
-				{
-					$url = substr($url, 1);
-				}
-
-				$url = (!empty($dir) ? $dir . '/' : '') . $url;
-				$url = generate_board_url() . '/' . $url;
-			}
 		}
+
+		$url = generate_board_url() . '/' . $url;
+	}
+
+	// Clean URL and check if we go outside the forum directory
+	$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

phpBB/phpbb/path_helper.php

@@ -101,6 +101,27 @@ class path_helper
 		return $path;
 	}
 
+	/**
+	* Strips away the web root path and prepends the normal root path
+	*
+	* This replaces get_web_root_path() . some_url with
+	*	$phpbb_root_path . some_url
+	*
+	* @param string $path The path to be updated
+	* @return string
+	*/
+	public function remove_web_root_path($path)
+	{
+		if (strpos($path, $this->get_web_root_path()) === 0)
+		{
+			$path = substr($path, strlen($this->get_web_root_path()));
+
+			return $this->phpbb_root_path . $path;
+		}
+
+		return $path;
+	}
+
 	/**
 	* Get a relative root path from the current URL
 	*
@@ -162,4 +183,27 @@ class path_helper
 		*/
 		return $this->web_root_path = $this->phpbb_root_path . str_repeat('../', $corrections - 1);
 	}
+
+	/**
+	* Eliminates useless . and .. components from specified URL
+	*
+	* @param string $url URL to clean
+	*
+	* @return string Cleaned URL
+	*/
+	public function clean_url($url)
+	{
+		$delimiter_position = strpos($url, '://');
+		// URL should contain :// but it shouldn't start with it.
+		// Do not clean URLs that do not fit these constraints.
+		if (empty($delimiter_position))
+		{
+			return $url;
+		}
+		$scheme = substr($url, 0, $delimiter_position) . '://';
+		// Add length of URL delimiter to position
+		$path = substr($url, $delimiter_position + 3);
+
+		return $scheme . $this->filesystem->clean_path($path);
+	}
 }

tests/functional/extension_controller_test.php

@@ -111,4 +111,32 @@ class phpbb_functional_extension_controller_test extends phpbb_functional_test_c
 		$this->assert_response_html(404);
 		$this->assertContains('No route found for "GET /does/not/exist"', $crawler->filter('body')->text());
 	}
+
+	/**
+	* Check the output of a controller using the template system
+	*/
+	public function test_redirect()
+	{
+		$filesystem = new \phpbb\filesystem();
+		$this->phpbb_extension_manager->enable('foo/bar');
+		$crawler = self::request('GET', 'app.php/foo/redirect');
+
+		$nodes = $crawler->filter('div')->extract(array('id'));
+
+		foreach ($nodes as $redirect)
+		{
+			if (strpos($redirect, 'redirect_expected') !== 0)
+			{
+				continue;
+			}
+
+			$row_num = str_replace('redirect_expected_', '', $redirect);
+
+			$redirect = $crawler->filter('#redirect_' . $row_num)->text();
+			$redirect = substr($redirect, 0, strpos($redirect, 'sid') - 1);
+			$this->assertEquals($crawler->filter('#redirect_expected_' .  $row_num)->text(), $redirect);
+		}
+
+		$this->phpbb_extension_manager->purge('foo/bar');
+	}
 }

tests/functional/fixtures/ext/foo/bar/config/routing.yml

@@ -13,3 +13,7 @@ foo_template_controller:
 foo_exception_controller:
     pattern: /foo/exception
     defaults: { _controller: foo_bar.controller:exception }
+
+foo_redirect_controller:
+    pattern: /foo/redirect
+    defaults: { _controller: foo_bar.controller:redirect }

tests/functional/fixtures/ext/foo/bar/config/services.yml

@@ -3,7 +3,12 @@ services:
         class: foo\bar\controller\controller
         arguments:
             - @controller.helper
+            - @path_helper
             - @template
+            - @config
+            - %core.root_path%
+            - %core.php_ext%
+
     foo_bar.listener.permission:
         class: foo\bar\event\permission
         tags:
@@ -12,4 +17,3 @@ services:
         class: foo\bar\event\user_setup
         tags:
             - { name: event.listener }
-

tests/functional/fixtures/ext/foo/bar/controller/controller.php

@@ -7,11 +7,18 @@ use Symfony\Component\HttpFoundation\Response;
 class controller
 {
 	protected $template;
+	protected $helper;
+	protected $path_helper;
+	protected $config;
 
-	public function __construct(\phpbb\controller\helper $helper, \phpbb\template\template $template)
+	public function __construct(\phpbb\controller\helper $helper, \phpbb\path_helper $path_helper, \phpbb\template\template $template, \phpbb\config\config $config, $root_path, $php_ext)
 	{
 		$this->template = $template;
 		$this->helper = $helper;
+		$this->path_helper = $path_helper;
+		$this->config = $config;
+		$this->root_path = $root_path;
+		$this->php_ext = $php_ext;
 	}
 
 	public function handle()
@@ -35,4 +42,75 @@ class controller
 	{
 		throw new \phpbb\controller\exception('Exception thrown from foo/exception route');
 	}
+
+	public function redirect()
+	{
+		$url_root = generate_board_url();
+
+		$rewrite_prefix = (!empty($this->config['enable_mod_rewrite'])) ? '' : 'app.php/';
+
+		$redirects = array(
+			array(
+				append_sid($this->root_path . 'index.' . $this->php_ext),
+				'index.php',
+			),
+			array(
+				append_sid($this->root_path . 'foo/bar/index.' . $this->php_ext),
+				'foo/bar/index.php',
+			),
+			array(
+				append_sid($this->root_path . 'tests/index.' . $this->php_ext),
+				'tests/index.php',
+			),
+			array(
+				$this->helper->url('index'),
+				$rewrite_prefix . 'index',
+			),
+			array(
+				$this->helper->url('tests/index'),
+				$rewrite_prefix . 'tests/index',
+			),
+			array(
+				$this->helper->url('tests/../index'),
+				$rewrite_prefix . 'index',
+			),
+			/*
+			// helper URLs starting with  ../ are prone to failure.
+			// Do not test them right now.
+			array(
+				$this->helper->url('../index'),
+				'../index',
+			),
+			array(
+				$this->helper->url('../../index'),
+				'../index',
+			),
+			array(
+				$this->helper->url('../tests/index'),
+				$rewrite_prefix . '../tests/index',
+			),
+			array(
+				$this->helper->url('../tests/../index'),
+				'../index',
+			),
+			array(
+				$this->helper->url('../../tests/index'),
+				'../tests/index',
+			),
+			*/
+		);
+
+		foreach ($redirects as $redirect)
+		{
+			$this->template->assign_block_vars('redirects', array(
+				'URL'		=> redirect($redirect[0], true),
+			));
+
+			$this->template->assign_block_vars('redirects_expected', array(
+				'URL'		=> $this->path_helper->clean_url($url_root . '/' . $redirect[1]),
+			));
+		}
+
+		return $this->helper->render('redirect_body.html');
+	}
 }

tests/functional/fixtures/ext/foo/bar/styles/prosilver/template/redirect_body.html

@@ -0,0 +1,8 @@
+<!-- INCLUDE overall_header.html -->
+<!-- BEGIN redirects -->
+<div id="redirect_{redirects.S_ROW_COUNT}">{redirects.URL}</div>
+<!-- END redirects -->
+<!-- BEGIN redirects_expected -->
+<div id="redirect_expected_{redirects_expected.S_ROW_COUNT}">{redirects_expected.URL}</div>
+<!-- END redirects_expected -->
+<!-- INCLUDE overall_footer.html -->

tests/path_helper/web_root_path_test.php

@@ -146,4 +146,27 @@ class phpbb_path_helper_web_root_path_test extends phpbb_test_case
 
 		$this->assertEquals($expected, $path_helper->update_web_root_path($input, $symfony_request));
 	}
+
+	public function clean_url_data()
+	{
+		return array(
+			array('', ''),
+			array('://', '://'),
+			array('http://', 'http://'),
+			array('http://one/two/three', 'http://one/two/three'),
+			array('http://../one/two', 'http://../one/two'),
+			array('http://one/../two/three', 'http://two/three'),
+			array('http://one/two/../three', 'http://one/three'),
+			array('http://one/two/../../three', 'http://three'),
+			array('http://one/two/../../../three', 'http://../three'),
+		);
+	}
+
+	/**
+	* @dataProvider clean_url_data
+	*/
+	public function test_clean_url($input, $expected)
+	{
+		$this->assertEquals($expected, $this->path_helper->clean_url($input));
+	}
 }

tests/security/redirect_test.php

@@ -13,19 +13,87 @@ 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, 'http://localhost/phpBB'),
+			array('data://x', false, false, 'http://localhost/phpBB'),
-			array('bad://localhost/phpBB/index.php', 'INSECURE_REDIRECT', false),
+			array('bad://localhost/phpBB/index.php', false, 'INSECURE_REDIRECT', false),
-			array('http://www.otherdomain.com/somescript.php', false, 'http://localhost/phpBB'),
+			array('http://www.otherdomain.com/somescript.php', false, false, 'http://localhost/phpBB'),
-			array("http://localhost/phpBB/memberlist.php\n\rConnection: close", 'INSECURE_REDIRECT', false),
+			array("http://localhost/phpBB/memberlist.php\n\rConnection: close", false, 'INSECURE_REDIRECT', false),
-			array('javascript:test', false, 'http://localhost/phpBB/../javascript:test'),
+			array('javascript:test', false, false, 'http://localhost/phpBB/javascript:test'),
-			array('http://localhost/phpBB/index.php;url=', 'INSECURE_REDIRECT', false),
+			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($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'),
+			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'),
 		);
 	}
 
+	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();
@@ -33,26 +101,41 @@ class phpbb_security_redirect_test extends phpbb_security_test_base
 		$GLOBALS['config'] = array(
 			'force_server_vars'	=> '0',
 		);
+
+		$this->path_helper = $this->get_path_helper();
+		$this->controller_helper = $this->get_controller_helper();
 	}
 
 	/**
 	* @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, $phpbb_path_helper;
+
+		$phpbb_path_helper = $this->path_helper;
+
+		$temp_phpbb_root_path = $phpbb_root_path;
+		$temp_page_dir = $user->page['page_dir'];
+		// We need to hack phpbb_root_path and the user's page_dir here
+		// so it matches the actual fileinfo of the testing script.
+		// Otherwise the paths are returned incorrectly.
+		$phpbb_root_path = '';
+		$user->page['page_dir'] = '';
 
 		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;
+		$user->page['page_dir'] = $temp_page_dir;
 	}
 }