Merge pull request #1102 from imkingdavid/ticket/11215

[ticket/11215] Correct paths when path info is used for controller access

phpBB/.htaccess

@@ -1,12 +1,30 @@
+<IfModule mod_rewrite.c>
+RewriteEngine on
+
 #
 # Uncomment the statement below if you want to make use of
 # HTTP authentication and it does not already work.
 # This could be required if you are for example using PHP via Apache CGI.
 #
-#<IfModule mod_rewrite.c>
-#RewriteEngine on
 #RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization},L]
-#</IfModule>
+
+#
+# The following 3 lines will rewrite URLs passed through the front controller
+# to not require app.php in the actual URL. In other words, a controller is
+# by default accessed at /app.php/my/controller, but can also be accessed at
+# /my/controller
+#
+RewriteCond %{REQUEST_FILENAME} !-f
+RewriteCond %{REQUEST_FILENAME} !-d
+RewriteRule ^(.*)$ app.php [QSA,L]
+
+#
+# If symbolic links are not already being followed,
+# uncomment the line below.
+# http://anothersysadmin.wordpress.com/2008/06/10/mod_rewrite-forbidden-403-with-apache-228/
+#
+#Options +FollowSymLinks
+</IfModule>
 
 <Files "config.php">
 Order Allow,Deny

phpBB/app.php

@@ -24,7 +24,6 @@ $user->session_begin();
 $auth->acl($user->data);
 $user->setup('app');
 
-$symfony_request = phpbb_create_symfony_request($request);
 $http_kernel = $phpbb_container->get('http_kernel');
 $response = $http_kernel->handle($symfony_request);
 $response->send();

phpBB/common.php

@@ -109,6 +109,9 @@ $db			= $phpbb_container->get('dbal.conn');
 // make sure request_var uses this request instance
 request_var('', 0, false, false, $request); // "dependency injection" for a function
 
+// Create a Symfony Request object from our phpbb_request object
+$symfony_request = phpbb_create_symfony_request($request);
+
 // Grab global variables, re-cache if necessary
 $config = $phpbb_container->get('config');
 set_config(null, null, null, $config);

phpBB/config/services.yml

@@ -90,6 +90,7 @@ services:
         arguments:
             - @template
             - @user
+            - @request
             - %core.root_path%
             - %core.php_ext%
 

phpBB/includes/functions.php

@@ -2413,6 +2413,7 @@ function append_sid($url, $params = false, $is_amp = true, $session_id = false)
 {
 	global $_SID, $_EXTRA_URL, $phpbb_hook;
 	global $phpbb_dispatcher;
+	global $symfony_request, $phpbb_root_path;
 
 	if ($params === '' || (is_array($params) && empty($params)))
 	{
@@ -2420,6 +2421,12 @@ function append_sid($url, $params = false, $is_amp = true, $session_id = false)
 		$params = false;
 	}
 
+	$corrected_path = $symfony_request !== null ? phpbb_get_web_root_path($symfony_request, $phpbb_root_path) : '';
+	if ($corrected_path)
+	{
+		$url = substr($corrected_path . $url, strlen($phpbb_root_path));
+	}
+
 	$append_sid_overwrite = false;
 
 	/**
@@ -5051,7 +5058,7 @@ function phpbb_build_hidden_fields_for_query_params($request, $exclude = null)
 function page_header($page_title = '', $display_online_list = true, $item_id = 0, $item = 'forum')
 {
 	global $db, $config, $template, $SID, $_SID, $_EXTRA_URL, $user, $auth, $phpEx, $phpbb_root_path;
-	global $phpbb_dispatcher, $request, $phpbb_container;
+	global $phpbb_dispatcher, $request, $phpbb_container, $symfony_request;
 
 	if (defined('HEADER_INC'))
 	{
@@ -5208,7 +5215,11 @@ function page_header($page_title = '', $display_online_list = true, $item_id = 0
 
 	// Determine board url - we may need it later
 	$board_url = generate_board_url() . '/';
-	$web_path = (defined('PHPBB_USE_BOARD_URL_PATH') && PHPBB_USE_BOARD_URL_PATH) ? $board_url : $phpbb_root_path;
+	// This path is sent with the base template paths in the assign_vars()
+	// call below. We need to correct it in case we are accessing from a
+	// controller because the web paths will be incorrect otherwise.
+	$corrected_path = $symfony_request !== null ? phpbb_get_web_root_path($symfony_request, $phpbb_root_path) : '';
+	$web_path = (defined('PHPBB_USE_BOARD_URL_PATH') && PHPBB_USE_BOARD_URL_PATH) ? $board_url : $corrected_path;
 
 	// Send a proper content-language to the output
 	$user_lang = $user->lang['USER_LANG'];
@@ -5682,6 +5693,16 @@ function phpbb_convert_30_dbms_to_31($dbms)
 */
 function phpbb_create_symfony_request(phpbb_request $request)
 {
+	// If we have already gotten it, don't go back through all the trouble of
+	// creating it again; instead, just return it. This allows multiple calls
+	// of this method so we don't have to globalize $symfony_request in other
+	// functions.
+	static $symfony_request;
+	if (null !== $symfony_request)
+	{
+		return $symfony_request;
+	}
+
 	// This function is meant to sanitize the global input arrays
 	$sanitizer = function(&$value, $key) {
 		$type_cast_helper = new phpbb_request_type_cast_helper();
@@ -5701,21 +5722,39 @@ function phpbb_create_symfony_request(phpbb_request $request)
 	array_walk_recursive($get_parameters, $sanitizer);
 	array_walk_recursive($post_parameters, $sanitizer);
 
-	// Until we fix the issue with relative paths, we have to fake path info
-	// to allow urls like app.php?controller=foo/bar
-	$controller = $request->variable('controller', '');
-	$path_info = '/' . $controller;
-	$request_uri = $server_parameters['REQUEST_URI'];
+	$symfony_request = new Request($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters);
+	return $symfony_request;
+}
 
-	// Remove the query string from REQUEST_URI
-	if ($pos = strpos($request_uri, '?'))
+/**
+* Get a relative root path from the current URL
+*
+* @param Request $symfony_request Symfony Request object
+*/
+function phpbb_get_web_root_path(Request $symfony_request, $phpbb_root_path = '')
 {
-		$request_uri = substr($request_uri, 0, $pos);
+	static $path;
+	if (null !== $path)
+	{
+		return $path;
 	}
 
-	// Add the path info (i.e. controller route) to the REQUEST_URI
-	$server_parameters['REQUEST_URI'] = $request_uri . $path_info;
-	$server_parameters['SCRIPT_NAME'] = '';
-
-	return new Request($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters);
+	$path_info = $symfony_request->getPathInfo();
+	if ($path_info === '/')
+	{
+		$path = $phpbb_root_path;
+		return $path;
+	}
+
+	$corrections = substr_count($path_info, '/');
+
+	// When URL Rewriting is enabled, app.php is optional. We have to
+	// correct for it not being there
+	if (strpos($symfony_request->getRequestUri(), $symfony_request->getScriptName()) === false)
+	{
+		$corrections -= 1;
+	}
+
+	$path = $phpbb_root_path . str_repeat('../', $corrections);
+	return $path;
 }

phpBB/phpbb/controller/helper.php

@@ -35,6 +35,12 @@ class phpbb_controller_helper
 	*/
 	protected $user;
 
+	/**
+	* Request object
+	* @var phpbb_request
+	*/
+	protected $request;
+
 	/**
 	* phpBB root path
 	* @var string
@@ -55,10 +61,11 @@ class phpbb_controller_helper
 	* @param string $phpbb_root_path phpBB root path
 	* @param string $php_ext PHP extension
 	*/
-	public function __construct(phpbb_template $template, phpbb_user $user, $phpbb_root_path, $php_ext)
+	public function __construct(phpbb_template $template, phpbb_user $user, phpbb_request_interface $request, $phpbb_root_path, $php_ext)
 	{
 		$this->template = $template;
 		$this->user = $user;
+		$this->request = $request;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->php_ext = $php_ext;
 	}
@@ -102,22 +109,16 @@ class phpbb_controller_helper
 			$route = substr($route, 0, $route_delim);
 		}
 
-		if (is_array($params) && !empty($params))
-		{
-			$params = array_merge(array(
-				'controller' => $route,
-			), $params);
-		}
-		else if (is_string($params) && $params)
-		{
-			$params = 'controller=' . $route . (($is_amp) ? '&' : '&') . $params;
-		}
-		else
-		{
-			$params = array('controller' => $route);
-		}
+		$request_uri = $this->request->variable('REQUEST_URI', '', false, phpbb_request::SERVER);
+		$script_name = $this->request->variable('SCRIPT_NAME', '', false, phpbb_request::SERVER);
 
-		return append_sid($this->phpbb_root_path . 'app.' . $this->php_ext . $route_params, $params, $is_amp, $session_id);
+		// If the app.php file is being used (no rewrite) keep it in the URL.
+		// Otherwise, don't include it.
+		$route_prefix = $this->phpbb_root_path;
+		$parts = explode('/', $script_name);
+		$route_prefix .= strpos($request_uri, $script_name) === 0 ? array_pop($parts) . '/' : '';
+
+		return append_sid($route_prefix . "$route" . $route_params, $params, $is_amp, $session_id);
 	}
 
 	/**

tests/controller/helper_url_test.php

@@ -15,28 +15,28 @@ class phpbb_controller_helper_url_test extends phpbb_test_case
 	public function helper_url_data()
 	{
 		return array(
-			array('foo/bar?t=1&f=2', false, true, false, 'app.php?t=1&f=2&controller=foo/bar', 'parameters in url-argument'),
-			array('foo/bar', 't=1&f=2', true, false, 'app.php?controller=foo/bar&t=1&f=2', 'parameters in params-argument using amp'),
-			array('foo/bar', 't=1&f=2', false, false, 'app.php?controller=foo/bar&t=1&f=2', 'parameters in params-argument using &'),
-			array('foo/bar', array('t' => 1, 'f' => 2), true, false, 'app.php?controller=foo/bar&t=1&f=2', 'parameters in params-argument as array'),
+			array('foo/bar?t=1&f=2', false, true, false, 'foo/bar?t=1&f=2', 'parameters in url-argument'),
+			array('foo/bar', 't=1&f=2', true, false, 'foo/bar?t=1&f=2', 'parameters in params-argument using amp'),
+			array('foo/bar', 't=1&f=2', false, false, 'foo/bar?t=1&f=2', 'parameters in params-argument using &'),
+			array('foo/bar', array('t' => 1, 'f' => 2), true, false, 'foo/bar?t=1&f=2', 'parameters in params-argument as array'),
 
 			// Custom sid parameter
-			array('foo/bar', 't=1&f=2', true, 'custom-sid', 'app.php?controller=foo/bar&t=1&f=2&sid=custom-sid', 'using session_id'),
+			array('foo/bar', 't=1&f=2', true, 'custom-sid', 'foo/bar?t=1&f=2&sid=custom-sid', 'using session_id'),
 
 			// Testing anchors
-			array('foo/bar?t=1&f=2#anchor', false, true, false, 'app.php?t=1&f=2&controller=foo/bar#anchor', 'anchor in url-argument'),
-			array('foo/bar', 't=1&f=2#anchor', true, false, 'app.php?controller=foo/bar&t=1&f=2#anchor', 'anchor in params-argument'),
-			array('foo/bar', array('t' => 1, 'f' => 2, '#' => 'anchor'), true, false, 'app.php?controller=foo/bar&t=1&f=2#anchor', 'anchor in params-argument (array)'),
+			array('foo/bar?t=1&f=2#anchor', false, true, false, 'foo/bar?t=1&f=2#anchor', 'anchor in url-argument'),
+			array('foo/bar', 't=1&f=2#anchor', true, false, 'foo/bar?t=1&f=2#anchor', 'anchor in params-argument'),
+			array('foo/bar', array('t' => 1, 'f' => 2, '#' => 'anchor'), true, false, 'foo/bar?t=1&f=2#anchor', 'anchor in params-argument (array)'),
 
 			// Anchors and custom sid
-			array('foo/bar?t=1&f=2#anchor', false, true, 'custom-sid', 'app.php?t=1&f=2&controller=foo/bar&sid=custom-sid#anchor', 'anchor in url-argument using session_id'),
-			array('foo/bar', 't=1&f=2#anchor', true, 'custom-sid', 'app.php?controller=foo/bar&t=1&f=2&sid=custom-sid#anchor', 'anchor in params-argument using session_id'),
-			array('foo/bar', array('t' => 1, 'f' => 2, '#' => 'anchor'), true, 'custom-sid', 'app.php?controller=foo/bar&t=1&f=2&sid=custom-sid#anchor', 'anchor in params-argument (array) using session_id'),
+			array('foo/bar?t=1&f=2#anchor', false, true, 'custom-sid', 'foo/bar?t=1&f=2&sid=custom-sid#anchor', 'anchor in url-argument using session_id'),
+			array('foo/bar', 't=1&f=2#anchor', true, 'custom-sid', 'foo/bar?t=1&f=2&sid=custom-sid#anchor', 'anchor in params-argument using session_id'),
+			array('foo/bar', array('t' => 1, 'f' => 2, '#' => 'anchor'), true, 'custom-sid', 'foo/bar?t=1&f=2&sid=custom-sid#anchor', 'anchor in params-argument (array) using session_id'),
 
 			// Empty parameters should not append the &
-			array('foo/bar', false, true, false, 'app.php?controller=foo/bar', 'no params using bool false'),
-			array('foo/bar', '', true, false, 'app.php?controller=foo/bar', 'no params using empty string'),
-			array('foo/bar', array(), true, false, 'app.php?controller=foo/bar', 'no params using empty array'),
+			array('foo/bar', false, true, false, 'foo/bar', 'no params using bool false'),
+			array('foo/bar', '', true, false, 'foo/bar', 'no params using empty string'),
+			array('foo/bar', array(), true, false, 'foo/bar', 'no params using empty array'),
 		);
 	}
 
@@ -51,7 +51,9 @@ class phpbb_controller_helper_url_test extends phpbb_test_case
 		$this->user = $this->getMock('phpbb_user');
 		$this->template = new phpbb_template_twig($phpbb_root_path, $phpEx, $config, $this->user, new phpbb_template_context());
 
-		$helper = new phpbb_controller_helper($this->template, $this->user, '', 'php');
+
+		$request = new phpbb_mock_request($_GET, $_POST, $_COOKIE, $_SERVER, false, $_FILES);
+		$helper = new phpbb_controller_helper($this->template, $this->user, $request, '', 'php');
 		$this->assertEquals($helper->url($route, $params, $is_amp, $session_id), $expected);
 	}
 }

tests/functional/extension_controller_test.php

@@ -52,7 +52,7 @@ class phpbb_functional_extension_controller_test extends phpbb_functional_test_c
 	public function test_foo_bar()
 	{
 		$this->phpbb_extension_manager->enable('foo/bar');
-		$crawler = self::request('GET', 'app.php?controller=foo/bar', array(), false);
+		$crawler = self::request('GET', 'app.php/foo/bar', array(), false);
 		self::assert_response_status_code();
 		$this->assertContains("foo/bar controller handle() method", $crawler->filter('body')->text());
 		$this->phpbb_extension_manager->purge('foo/bar');
@@ -64,7 +64,7 @@ class phpbb_functional_extension_controller_test extends phpbb_functional_test_c
 	public function test_controller_with_template()
 	{
 		$this->phpbb_extension_manager->enable('foo/bar');
-		$crawler = self::request('GET', 'app.php?controller=foo/template');
+		$crawler = self::request('GET', 'app.php/foo/template');
 		$this->assertContains("I am a variable", $crawler->filter('#content')->text());
 		$this->phpbb_extension_manager->purge('foo/bar');
 	}
@@ -76,7 +76,7 @@ class phpbb_functional_extension_controller_test extends phpbb_functional_test_c
 	public function test_missing_argument()
 	{
 		$this->phpbb_extension_manager->enable('foo/bar');
-		$crawler = self::request('GET', 'app.php?controller=foo/baz', array(), false);
+		$crawler = self::request('GET', 'app.php/foo/baz', array(), false);
 		$this->assert_response_html(500);
 		$this->assertContains('Missing value for argument #1: test in class phpbb_ext_foo_bar_controller:baz', $crawler->filter('body')->text());
 		$this->phpbb_extension_manager->purge('foo/bar');
@@ -88,7 +88,7 @@ class phpbb_functional_extension_controller_test extends phpbb_functional_test_c
 	public function test_exception_should_result_in_500_status_code()
 	{
 		$this->phpbb_extension_manager->enable('foo/bar');
-		$crawler = self::request('GET', 'app.php?controller=foo/exception', array(), false);
+		$crawler = self::request('GET', 'app.php/foo/exception', array(), false);
 		$this->assert_response_html(500);
 		$this->assertContains('Exception thrown from foo/exception route', $crawler->filter('body')->text());
 		$this->phpbb_extension_manager->purge('foo/bar');
@@ -105,7 +105,7 @@ class phpbb_functional_extension_controller_test extends phpbb_functional_test_c
 	*/
 	public function test_error_ext_disabled_or_404()
 	{
-		$crawler = self::request('GET', 'app.php?controller=does/not/exist', array(), false);
+		$crawler = self::request('GET', 'app.php/does/not/exist', array(), false);
 		$this->assert_response_html(404);
 		$this->assertContains('No route found for "GET /does/not/exist"', $crawler->filter('body')->text());
 	}