From f4317bc864f9c19a15de83ea30cb46a04c95a295 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Tue, 17 Sep 2013 11:41:46 -0500 Subject: [PATCH 1/9] [ticket/11850] Fix $user->page on pages through the controller PHPBB3-11850 --- phpBB/phpbb/session.php | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index dc33786666..52f621dbf6 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -40,13 +40,13 @@ class phpbb_session */ static function extract_current_page($root_path) { - global $request; + global $request, $symfony_request, $phpbb_filesystem; $page_array = array(); // First of all, get the request uri... - $script_name = htmlspecialchars_decode($request->server('PHP_SELF')); - $args = explode('&', htmlspecialchars_decode($request->server('QUERY_STRING'))); + $script_name = $symfony_request->getScriptName(); + $args = explode('&', $symfony_request->getQueryString()); // If we are unable to get the script name we use REQUEST_URI as a failover and note it within the page array for easier support... if (!$script_name) @@ -103,10 +103,19 @@ class phpbb_session } // Current page from phpBB root (for example: adm/index.php?i=10&b=2) - $page = (($page_dir) ? $page_dir . '/' : '') . $page_name . (($query_string) ? "?$query_string" : ''); + $symfony_request_path = $phpbb_filesystem->clean_path($symfony_request->getPathInfo()); + $page = (($page_dir) ? $page_dir . '/' : '') . $page_name; + if ($symfony_request_path !== '/') + { + $page .= $symfony_request_path; + } + if ($query_string) + { + $page .= '?' . $query_string; + } // The script path from the webroot to the current directory (for example: /phpBB3/adm/) : always prefixed with / and ends in / - $script_path = trim(str_replace('\\', '/', dirname($script_name))); + $script_path = $symfony_request->getBasePath(); // The script path from the webroot to the phpBB root (for example: /phpBB3/) $script_dirs = explode('/', $script_path); From 9c535da52888d60aecef9799062974e375f22f82 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Tue, 17 Sep 2013 22:00:06 -0500 Subject: [PATCH 2/9] [ticket/11850] page_name contains controller request rather than query string Fixing tests PHPBB3-11850 --- phpBB/phpbb/session.php | 11 ++-- tests/session/extract_page_test.php | 58 +++++++++++++++---- tests/session/testable_facade.php | 16 ----- .../phpbb_session_test_case.php | 7 +++ 4 files changed, 59 insertions(+), 33 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 52f621dbf6..1752291cf2 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -87,6 +87,12 @@ class phpbb_session $page_name = (substr($script_name, -1, 1) == '/') ? '' : basename($script_name); $page_name = urlencode(htmlspecialchars($page_name)); + $symfony_request_path = $phpbb_filesystem->clean_path($symfony_request->getPathInfo()); + if ($symfony_request_path !== '/') + { + $page_name .= $symfony_request_path; + } + // current directory within the phpBB root (for example: adm) $root_dirs = explode('/', str_replace('\\', '/', phpbb_realpath($root_path))); $page_dirs = explode('/', str_replace('\\', '/', phpbb_realpath('./'))); @@ -103,12 +109,7 @@ class phpbb_session } // Current page from phpBB root (for example: adm/index.php?i=10&b=2) - $symfony_request_path = $phpbb_filesystem->clean_path($symfony_request->getPathInfo()); $page = (($page_dir) ? $page_dir . '/' : '') . $page_name; - if ($symfony_request_path !== '/') - { - $page .= $symfony_request_path; - } if ($query_string) { $page .= '?' . $query_string; diff --git a/tests/session/extract_page_test.php b/tests/session/extract_page_test.php index f4ae8de021..d1a49ed3ef 100644 --- a/tests/session/extract_page_test.php +++ b/tests/session/extract_page_test.php @@ -24,6 +24,7 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case '/phpBB/index.php', '', '/phpBB/', + '/', array( 'page_name' => 'index.php', 'page_dir' => '', @@ -38,7 +39,8 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case './', '/phpBB/ucp.php', 'mode=login', - '/phpBB/ucp.php?mode=login', + '/phpBB/', + '/', array( 'page_name' => 'ucp.php', 'page_dir' => '', @@ -53,7 +55,8 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case './', '/phpBB/ucp.php', 'mode=register', - '/phpBB/ucp.php?mode=register', + '/phpBB/', + '/', array( 'page_name' => 'ucp.php', 'page_dir' => '', @@ -68,7 +71,8 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case './', '/phpBB/ucp.php', 'mode=register', - '/phpBB/ucp.php?mode=register', + '/phpBB/', + '/', array( 'page_name' => 'ucp.php', 'page_dir' => '', @@ -83,30 +87,60 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case './../', '/phpBB/adm/index.php', 'sid=e7215d958cdd41a6fc13509bebe53e42', - '/phpBB/adm/index.php?sid=e7215d958cdd41a6fc13509bebe53e42', + '/phpBB/adm/', + '/', array( 'page_name' => 'index.php', //'page_dir' => 'adm', // ^-- Ignored because .. returns different directory in live vs testing 'query_string' => '', 'script_path' => '/phpBB/adm/', - 'root_script_path' => '/phpBB/', + //'root_script_path' => '/phpBB/', //'page' => 'adm/index.php', 'forum' => 0, ), ), + array( + './', + '/phpBB/adm/app.php', + 'page=1&test=2', + '/phpBB/', + '/foo/bar', + array( + 'page_name' => 'app.php/foo/bar', + 'page_dir' => '', + 'query_string' => 'page=1&test=2', + 'script_path' => '/phpBB/', + 'root_script_path' => '/phpBB/', + 'page' => 'app.php/foo/bar?page=1&test=2', + 'forum' => 0, + ), + ), ); } /** @dataProvider extract_current_page_data */ - function test_extract_current_page($root_path, $php_self, $query_string, $request_uri, $expected) + function test_extract_current_page($root_path, $getScriptName, $getQueryString, $getBasePath, $getPathInfo, $expected) { - $output = $this->session_facade->extract_current_page( - $root_path, - $php_self, - $query_string, - $request_uri - ); + global $symfony_request; + + $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + new phpbb_mock_request(), + )); + $symfony_request->expects($this->any()) + ->method('getScriptName') + ->will($this->returnValue($getScriptName)); + $symfony_request->expects($this->any()) + ->method('getQueryString') + ->will($this->returnValue($getQueryString)); + $symfony_request->expects($this->any()) + ->method('getBasePath') + ->will($this->returnValue($getBasePath)); + $symfony_request->expects($this->any()) + ->method('getPathInfo') + ->will($this->returnValue($getPathInfo)); + + $output = phpbb_session::extract_current_page($root_path); // This compares the result of the output. // Any keys that are not in the expected array are overwritten by the output (aka not checked). diff --git a/tests/session/testable_facade.php b/tests/session/testable_facade.php index 9f0a3c5f59..e9d16999ce 100644 --- a/tests/session/testable_facade.php +++ b/tests/session/testable_facade.php @@ -33,21 +33,6 @@ class phpbb_session_testable_facade $this->session_factory = $session_factory; } - function extract_current_page( - $root_path, - $php_self, - $query_string, - $request_uri - ) - { - $this->session_factory->get_session($this->db); - global $request; - $request->overwrite('PHP_SELF', $php_self, phpbb_request_interface::SERVER); - $request->overwrite('QUERY_STRING', $query_string, phpbb_request_interface::SERVER); - $request->overwrite('REQUEST_URI', $request_uri, phpbb_request_interface::SERVER); - return phpbb_session::extract_current_page($root_path); - } - function extract_current_hostname( $host, $server_name_config, @@ -139,4 +124,3 @@ class phpbb_session_testable_facade return $session->validate_referer($check_script_path); } } - diff --git a/tests/test_framework/phpbb_session_test_case.php b/tests/test_framework/phpbb_session_test_case.php index e6a2b03bba..cfa8399c96 100644 --- a/tests/test_framework/phpbb_session_test_case.php +++ b/tests/test_framework/phpbb_session_test_case.php @@ -19,6 +19,13 @@ abstract class phpbb_session_test_case extends phpbb_database_test_case function setUp() { parent::setUp(); + + global $symfony_request, $phpbb_filesystem, $request, $phpbb_root_path, $phpEx; + $symfony_request = new phpbb_symfony_request( + new phpbb_mock_request() + ); + $phpbb_filesystem = new phpbb_filesystem($symfony_request, $phpbb_root_path, $phpEx); + $this->session_factory = new phpbb_session_testable_factory; $this->db = $this->new_dbal(); $this->session_facade = From d85ae0f7bc0bd9663eda83c6713f0ef71289b0f4 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Wed, 18 Sep 2013 09:25:58 -0500 Subject: [PATCH 3/9] [ticket/11850] Add test for outside of the phpBB directory PHPBB3-11850 --- tests/session/extract_page_test.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/session/extract_page_test.php b/tests/session/extract_page_test.php index d1a49ed3ef..1834eddebc 100644 --- a/tests/session/extract_page_test.php +++ b/tests/session/extract_page_test.php @@ -116,6 +116,22 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case 'forum' => 0, ), ), + array( + './../phpBB/', + '/test/test.php', + 'page=1&test=2', + '/test/', + '', + array( + 'page_name' => 'test.php', + //'page_dir' => '', + 'query_string' => 'page=1&test=2', + 'script_path' => '/test/', + //'root_script_path' => '../phpBB/', + //'page' => '../test/test.php/foo/bar?page=1&test=2', + 'forum' => 0, + ), + ), ); } From 2ee22d4615a86a33d3c65f94d7a9c4b99dfb1b7e Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Thu, 19 Sep 2013 11:49:13 -0500 Subject: [PATCH 4/9] [ticket/11850] Fix extract current page test PHPBB3-11850 --- tests/security/base.php | 18 ++++++++- tests/security/extract_current_page_test.php | 40 +++++++++++++++----- 2 files changed, 47 insertions(+), 11 deletions(-) diff --git a/tests/security/base.php b/tests/security/base.php index 08878ad60d..c7dbbb550a 100644 --- a/tests/security/base.php +++ b/tests/security/base.php @@ -14,7 +14,7 @@ abstract class phpbb_security_test_base extends phpbb_test_case */ protected function setUp() { - global $user, $phpbb_root_path, $request; + global $user, $phpbb_root_path, $phpEx, $request, $symfony_request, $phpbb_filesystem; // Put this into a global function being run by every test to init a proper user session $server['HTTP_HOST'] = 'localhost'; @@ -37,6 +37,22 @@ abstract class phpbb_security_test_base extends phpbb_test_case */ $request = new phpbb_mock_request(array(), array(), array(), $server); + $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $request, + )); + $symfony_request->expects($this->any()) + ->method('getScriptName') + ->will($this->returnValue($server['SCRIPT_NAME'])); + $symfony_request->expects($this->any()) + ->method('getQueryString') + ->will($this->returnValue($server['QUERY_STRING'])); + $symfony_request->expects($this->any()) + ->method('getBasePath') + ->will($this->returnValue($server['REQUEST_URI'])); + $symfony_request->expects($this->any()) + ->method('getPathInfo') + ->will($this->returnValue('/')); + $phpbb_filesystem = new phpbb_filesystem($symfony_request, $phpbb_root_path, $phpEx); // Set no user and trick a bit to circumvent errors $user = new phpbb_user(); diff --git a/tests/security/extract_current_page_test.php b/tests/security/extract_current_page_test.php index d77cbbcaf3..2c69e7955b 100644 --- a/tests/security/extract_current_page_test.php +++ b/tests/security/extract_current_page_test.php @@ -26,13 +26,23 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base */ public function test_query_string_php_self($url, $query_string, $expected) { - global $request; + global $symfony_request, $request; - $request->merge(phpbb_request_interface::SERVER, array( - 'PHP_SELF' => $url, - 'QUERY_STRING' => $query_string, + $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $request, )); - + $symfony_request->expects($this->any()) + ->method('getScriptName') + ->will($this->returnValue($url)); + $symfony_request->expects($this->any()) + ->method('getQueryString') + ->will($this->returnValue($query_string)); + $symfony_request->expects($this->any()) + ->method('getBasePath') + ->will($this->returnValue($server['REQUEST_URI'])); + $symfony_request->expects($this->any()) + ->method('getPathInfo') + ->will($this->returnValue('/')); $result = phpbb_session::extract_current_page('./'); $label = 'Running extract_current_page on ' . $query_string . ' with PHP_SELF filled.'; @@ -44,12 +54,23 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base */ public function test_query_string_request_uri($url, $query_string, $expected) { - global $request; + global $symfony_request, $request; - $request->merge(phpbb_request_interface::SERVER, array( - 'PHP_SELF' => $url, - 'QUERY_STRING' => $query_string, + $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $request, )); + $symfony_request->expects($this->any()) + ->method('getScriptName') + ->will($this->returnValue($url)); + $symfony_request->expects($this->any()) + ->method('getQueryString') + ->will($this->returnValue($query_string)); + $symfony_request->expects($this->any()) + ->method('getBasePath') + ->will($this->returnValue($server['REQUEST_URI'])); + $symfony_request->expects($this->any()) + ->method('getPathInfo') + ->will($this->returnValue('/')); $result = phpbb_session::extract_current_page('./'); @@ -57,4 +78,3 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base $this->assertEquals($expected, $result['query_string'], $label); } } - From 85ae55ca2d6a99bfc0eaf66bf7bb710050b0cb1e Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Mon, 30 Sep 2013 18:31:26 -0500 Subject: [PATCH 5/9] [ticket/11850] Update for namespaces PHPBB3-11850 --- tests/security/extract_current_page_test.php | 2 +- tests/session/extract_page_test.php | 2 +- tests/test_framework/phpbb_session_test_case.php | 11 ++++++++--- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/security/extract_current_page_test.php b/tests/security/extract_current_page_test.php index a7560f0d15..9980530dbe 100644 --- a/tests/security/extract_current_page_test.php +++ b/tests/security/extract_current_page_test.php @@ -43,7 +43,7 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base $symfony_request->expects($this->any()) ->method('getPathInfo') ->will($this->returnValue('/')); - $result = phpbb_session::extract_current_page('./'); + $result = \phpbb\session::extract_current_page('./'); $label = 'Running extract_current_page on ' . $query_string . ' with PHP_SELF filled.'; $this->assertEquals($expected, $result['query_string'], $label); diff --git a/tests/session/extract_page_test.php b/tests/session/extract_page_test.php index 1834eddebc..123ae591c6 100644 --- a/tests/session/extract_page_test.php +++ b/tests/session/extract_page_test.php @@ -156,7 +156,7 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case ->method('getPathInfo') ->will($this->returnValue($getPathInfo)); - $output = phpbb_session::extract_current_page($root_path); + $output = \phpbb\session::extract_current_page($root_path); // This compares the result of the output. // Any keys that are not in the expected array are overwritten by the output (aka not checked). diff --git a/tests/test_framework/phpbb_session_test_case.php b/tests/test_framework/phpbb_session_test_case.php index cfa8399c96..36b4ff025f 100644 --- a/tests/test_framework/phpbb_session_test_case.php +++ b/tests/test_framework/phpbb_session_test_case.php @@ -20,11 +20,16 @@ abstract class phpbb_session_test_case extends phpbb_database_test_case { parent::setUp(); - global $symfony_request, $phpbb_filesystem, $request, $phpbb_root_path, $phpEx; - $symfony_request = new phpbb_symfony_request( + global $symfony_request, $phpbb_path_helper, $request, $phpbb_root_path, $phpEx; + $symfony_request = new \phpbb\symfony_request( new phpbb_mock_request() ); - $phpbb_filesystem = new phpbb_filesystem($symfony_request, $phpbb_root_path, $phpEx); + $phpbb_path_helper = new \phpbb\path_helper( + $symfony_request, + new \phpbb\filesystem(), + $phpbb_root_path, + $phpEx + ); $this->session_factory = new phpbb_session_testable_factory; $this->db = $this->new_dbal(); From 1440517b78e51aa00c36dbc4f09c2df2efafe6d3 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Mon, 30 Sep 2013 18:44:02 -0500 Subject: [PATCH 6/9] [ticket/11850] Need symfony request and filesystem setup globally for session PHPBB3-11850 --- phpBB/common.php | 2 ++ phpBB/install/index.php | 2 ++ 2 files changed, 4 insertions(+) diff --git a/phpBB/common.php b/phpBB/common.php index 6bb3509ea1..b1da2215fb 100644 --- a/phpBB/common.php +++ b/phpBB/common.php @@ -115,6 +115,8 @@ set_config(null, null, null, $config); set_config_count(null, null, null, $config); $phpbb_log = $phpbb_container->get('log'); +$symfony_request = $phpbb_container->get('symfony_request'); +$phpbb_filesystem = $phpbb_container->get('filesystem'); $phpbb_path_helper = $phpbb_container->get('path_helper'); // load extensions diff --git a/phpBB/install/index.php b/phpBB/install/index.php index 161dc78173..c9bf76bf04 100644 --- a/phpBB/install/index.php +++ b/phpBB/install/index.php @@ -244,6 +244,8 @@ $config = new \phpbb\config\config(array( 'load_tplcompile' => '1' )); +$symfony_request = $phpbb_container->get('symfony_request'); +$phpbb_filesystem = $phpbb_container->get('filesystem'); $phpbb_path_helper = $phpbb_container->get('path_helper'); $template = new \phpbb\template\twig\twig($phpbb_path_helper, $config, $user, new \phpbb\template\context()); $paths = array($phpbb_root_path . 'install/update/new/adm/style', $phpbb_admin_path . 'style'); From 870c293bab260e1941c6c2e5fea1c11aa80472ec Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Mon, 30 Sep 2013 20:04:32 -0500 Subject: [PATCH 7/9] [ticket/11850] Fix tests PHPBB3-11850 --- tests/test_framework/phpbb_session_test_case.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_framework/phpbb_session_test_case.php b/tests/test_framework/phpbb_session_test_case.php index 36b4ff025f..0a2f767845 100644 --- a/tests/test_framework/phpbb_session_test_case.php +++ b/tests/test_framework/phpbb_session_test_case.php @@ -20,13 +20,14 @@ abstract class phpbb_session_test_case extends phpbb_database_test_case { parent::setUp(); - global $symfony_request, $phpbb_path_helper, $request, $phpbb_root_path, $phpEx; + global $symfony_request, $phpbb_filesystem, $phpbb_path_helper, $request, $phpbb_root_path, $phpEx; $symfony_request = new \phpbb\symfony_request( new phpbb_mock_request() ); + $phpbb_filesystem = new \phpbb\filesystem(); $phpbb_path_helper = new \phpbb\path_helper( $symfony_request, - new \phpbb\filesystem(), + $phpbb_filesystem, $phpbb_root_path, $phpEx ); From b81d0bc2284faee73d5ca6ca74ae5f7f1eb3f574 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Mon, 30 Sep 2013 20:38:41 -0500 Subject: [PATCH 8/9] [ticket/11850] More namespaces PHPBB3-11850 --- tests/security/base.php | 2 +- tests/security/extract_current_page_test.php | 4 ++-- tests/session/extract_page_test.php | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/security/base.php b/tests/security/base.php index 26f267745c..ff92a7e2a3 100644 --- a/tests/security/base.php +++ b/tests/security/base.php @@ -37,7 +37,7 @@ abstract class phpbb_security_test_base extends phpbb_test_case */ $request = new phpbb_mock_request(array(), array(), array(), $server); - $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $symfony_request = $this->getMock("\phpbb\symfony_request", array(), array( $request, )); $symfony_request->expects($this->any()) diff --git a/tests/security/extract_current_page_test.php b/tests/security/extract_current_page_test.php index 9980530dbe..1284aab94c 100644 --- a/tests/security/extract_current_page_test.php +++ b/tests/security/extract_current_page_test.php @@ -28,7 +28,7 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base { global $symfony_request, $request; - $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $symfony_request = $this->getMock("\phpbb\symfony_request", array(), array( $request, )); $symfony_request->expects($this->any()) @@ -56,7 +56,7 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base { global $symfony_request, $request; - $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $symfony_request = $this->getMock("\phpbb\symfony_request", array(), array( $request, )); $symfony_request->expects($this->any()) diff --git a/tests/session/extract_page_test.php b/tests/session/extract_page_test.php index 123ae591c6..6e137e28b8 100644 --- a/tests/session/extract_page_test.php +++ b/tests/session/extract_page_test.php @@ -140,7 +140,7 @@ class phpbb_session_extract_page_test extends phpbb_session_test_case { global $symfony_request; - $symfony_request = $this->getMock("phpbb_symfony_request", array(), array( + $symfony_request = $this->getMock("\phpbb\symfony_request", array(), array( new phpbb_mock_request(), )); $symfony_request->expects($this->any()) From 51c0aec066a4029f626ee300b3a34a0cc97c6031 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Mon, 30 Sep 2013 20:42:45 -0500 Subject: [PATCH 9/9] [ticket/11850] More namespaces PHPBB3-11850 --- tests/security/base.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/security/base.php b/tests/security/base.php index ff92a7e2a3..3ab2d1cfec 100644 --- a/tests/security/base.php +++ b/tests/security/base.php @@ -52,7 +52,7 @@ abstract class phpbb_security_test_base extends phpbb_test_case $symfony_request->expects($this->any()) ->method('getPathInfo') ->will($this->returnValue('/')); - $phpbb_filesystem = new phpbb_filesystem($symfony_request, $phpbb_root_path, $phpEx); + $phpbb_filesystem = new \phpbb\filesystem($symfony_request, $phpbb_root_path, $phpEx); // Set no user and trick a bit to circumvent errors $user = new \phpbb\user();