diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index b688902e62..916c3799c2 100644 --- a/phpBB/includes/functions.php +++ b/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); - $pathinfo = pathinfo($url); + $url = str_replace('./', '', $url); - if (!file_exists($pathinfo['dirname'] . '/')) + // 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 - $url = generate_board_url() . '/' . $user->page['page']; - $failover_flag = true; + $url = substr($url, 1); } } - 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? - 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 = $user->page['page_dir'] . '/' . $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 diff --git a/phpBB/phpbb/path_helper.php b/phpBB/phpbb/path_helper.php index 8cd8808261..a8e12c4063 100644 --- a/phpBB/phpbb/path_helper.php +++ b/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); + } } diff --git a/tests/functional/extension_controller_test.php b/tests/functional/extension_controller_test.php index 37752b8fbb..4725301141 100644 --- a/tests/functional/extension_controller_test.php +++ b/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'); + } } diff --git a/tests/functional/fixtures/ext/foo/bar/config/routing.yml b/tests/functional/fixtures/ext/foo/bar/config/routing.yml index 09a30a8c67..9b1ce3cfd7 100644 --- a/tests/functional/fixtures/ext/foo/bar/config/routing.yml +++ b/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 } diff --git a/tests/functional/fixtures/ext/foo/bar/config/services.yml b/tests/functional/fixtures/ext/foo/bar/config/services.yml index 64e1163408..cec69f7807 100644 --- a/tests/functional/fixtures/ext/foo/bar/config/services.yml +++ b/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 } - diff --git a/tests/functional/fixtures/ext/foo/bar/controller/controller.php b/tests/functional/fixtures/ext/foo/bar/controller/controller.php index 259d548299..558b202948 100644 --- a/tests/functional/fixtures/ext/foo/bar/controller/controller.php +++ b/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'); + } } diff --git a/tests/functional/fixtures/ext/foo/bar/styles/prosilver/template/redirect_body.html b/tests/functional/fixtures/ext/foo/bar/styles/prosilver/template/redirect_body.html new file mode 100644 index 0000000000..2b70b0fe59 --- /dev/null +++ b/tests/functional/fixtures/ext/foo/bar/styles/prosilver/template/redirect_body.html @@ -0,0 +1,8 @@ + + +
{redirects.URL}
+ + +
{redirects_expected.URL}
+ + diff --git a/tests/path_helper/web_root_path_test.php b/tests/path_helper/web_root_path_test.php index 2e1a37e02b..2c22511402 100644 --- a/tests/path_helper/web_root_path_test.php +++ b/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)); + } } diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php index 8e36780ca4..77dc955c26 100644 --- a/tests/security/redirect_test.php +++ b/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('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('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($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; } }