mirror/php-phpbb
1
0
Fork 0
mirror of https://github.com/phpbb/phpbb.git synced 2025-04-21 16:22:22 +02:00
Code Issues Releases Wiki Activity

[ticket/13280] Output escaping for the symfony request object

Browse Source 
PHPBB3-13280
This commit is contained in:
Tristan Darricau 2014-11-12 11:44:56 +01:00
parent 526a97db7c
commit 0dfe1d0d8b
12 changed files with 116 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
phpBB/config/services.yml
View File

@ -75,6 +75,7 @@ services:
- @controller.provider
- @ext.manager
- @symfony_request
- @request
- @filesystem
- %core.root_path%
- %core.php_ext%
@ -155,6 +156,8 @@ services:
- null
- %core.disable_super_globals%
# WARNING: The Symfony request does not escape the input and should be used very carefully
# prefer the phpbb request (service @request) as possible
symfony_request:
class: phpbb\symfony_request
arguments:

11
phpBB/phpbb/controller/helper.php
View File

@ -44,6 +44,9 @@ class helper
/* @var \phpbb\symfony_request */
protected $symfony_request;
/* @var \phpbb\request\request_interface */
protected $request;
/**
* @var \phpbb\filesystem The filesystem object
*/
@ -70,16 +73,18 @@ class helper
* @param \phpbb\controller\provider $provider Path provider
* @param \phpbb\extension\manager $manager Extension manager object
* @param \phpbb\symfony_request $symfony_request Symfony Request object
* @param \phpbb\request\request_interface $request phpBB request object
* @param \phpbb\filesystem $filesystem The filesystem object
* @param string $phpbb_root_path phpBB root path
* @param string $php_ext PHP file extension
*/
public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext)
public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\request\request_interface $request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext)
{
$this->template = $template;
$this->user = $user;
$this->config = $config;
$this->symfony_request = $symfony_request;
$this->request = $request;
$this->filesystem = $filesystem;
$this->phpbb_root_path = $phpbb_root_path;
$this->php_ext = $php_ext;
@ -153,7 +158,7 @@ class helper
}
}
$base_url = $this->filesystem->clean_path($base_url);
$base_url = $this->request->escape($this->filesystem->clean_path($base_url), true);
$context->setBaseUrl($base_url);
@ -197,6 +202,6 @@ class helper
*/
public function get_current_url()
{
return generate_board_url(true) . $this->symfony_request->getRequestUri();
return generate_board_url(true) . $this->request->escape($this->symfony_request->getRequestUri(), true);
}
}

6
phpBB/phpbb/path_helper.php
View File

@ -154,6 +154,7 @@ class path_helper
return $this->web_root_path;
}
// We do not need to escape $path_info, $request_uri and $script_name because we can not find their content in the result.
// Path info (e.g. /foo/bar)
$path_info = $this->filesystem->clean_path($this->symfony_request->getPathInfo());
@ -203,9 +204,12 @@ class path_helper
*/
if ($this->request->is_ajax() && $this->symfony_request->get('_referer'))
{
// We need to escape $absolute_board_url because it can be partially concatenated to the result.
$absolute_board_url = $this->request->escape($this->symfony_request->getSchemeAndHttpHost() . $this->symfony_request->getBasePath(), true);
$referer_web_root_path = $this->get_web_root_path_from_ajax_referer(
$this->symfony_request->get('_referer'),
$this->symfony_request->getSchemeAndHttpHost() . $this->symfony_request->getBasePath()
$absolute_board_url
);
return $this->web_root_path = $this->phpbb_root_path . $referer_web_root_path;
}

23
phpBB/phpbb/request/request.php
View File

@ -416,4 +416,27 @@ class request implements \phpbb\request\request_interface
{
return $this->input[$super_global];
}
/**
* {@inheritdoc}
*/
public function escape($var, $multibyte)
{
if (is_array($var))
{
$result = array();
foreach ($var as $key => $value)
{
$this->type_cast_helper->set_var($key, $key, gettype($key), $multibyte);
$result[$key] = $this->escape($value, $multibyte);
}
$var = $result;
}
else
{
$this->type_cast_helper->set_var($var, $var, 'string', $multibyte);
}
return $var;
}
}

10
phpBB/phpbb/request/request_interface.php
View File

@ -142,4 +142,14 @@ interface request_interface
* @return array The original array of the requested super global.
*/
public function get_super_global($super_global = \phpbb\request\request_interface::REQUEST);
/**
* Escape a string variable.
*
* @param mixed $value The contents to fill with
* @param bool $multibyte Indicates whether string values may contain UTF-8 characters.
* Default is false, causing all bytes outside the ASCII range (0-127) to be replaced with question marks.
* @return string|array
*/
public function escape($value, $multibyte);
}

17
phpBB/phpbb/session.php
View File

@ -31,10 +31,11 @@ class session
var $update_session_page = true;
/**
* Extract current session page
*
* @param string $root_path current root path (phpbb_root_path)
*/
* Extract current session page
*
* @param string $root_path current root path (phpbb_root_path)
* @return array
*/
static function extract_current_page($root_path)
{
global $request, $symfony_request, $phpbb_filesystem;
@ -42,8 +43,8 @@ class session
$page_array = array();
// First of all, get the request uri...
$script_name = $symfony_request->getScriptName();
$args = explode('&', $symfony_request->getQueryString());
$script_name = $request->escape($symfony_request->getScriptName(), true);
$args = $request->escape(explode('&', $symfony_request->getQueryString()), true);
// 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)
@ -61,8 +62,8 @@ class session
// Since some browser do not encode correctly we need to do this with some "special" characters...
// " -> %22, ' => %27, < -> %3C, > -> %3E
$find = array('"', "'", '<', '>');
$replace = array('%22', '%27', '%3C', '%3E');
$find = array('"', "'", '<', '>', '&quot;', '&lt;', '&gt;');
$replace = array('%22', '%27', '%3C', '%3E', '%22', '%3C', '%3E');
foreach ($args as $key => $argument)
{

16
phpBB/phpbb/symfony_request.php
View File

@ -15,6 +15,10 @@ namespace phpbb;
use Symfony\Component\HttpFoundation\Request;
/**
* WARNING: The Symfony request does not escape the input and should be used very carefully
* prefer the phpbb request as possible
*/
class symfony_request extends Request
{
/**
@ -24,24 +28,12 @@ class symfony_request extends Request
*/
public function __construct(\phpbb\request\request_interface $phpbb_request)
{
// This function is meant to sanitize the global input arrays
$sanitizer = function(&$value, $key) {
$type_cast_helper = new \phpbb\request\type_cast_helper();
$type_cast_helper->set_var($value, $value, gettype($value), true);
};
$get_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::GET);
$post_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::POST);
$server_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::SERVER);
$files_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::FILES);
$cookie_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::COOKIE);
array_walk_recursive($get_parameters, $sanitizer);
array_walk_recursive($post_parameters, $sanitizer);
array_walk_recursive($server_parameters, $sanitizer);
array_walk_recursive($files_parameters, $sanitizer);
array_walk_recursive($cookie_parameters, $sanitizer);
parent::__construct($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters);
}
}

32
tests/controller/common_helper_route.php
View File

@ -63,21 +63,21 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
protected function generate_route_objects()
{
$request = new phpbb_mock_request();
$request->overwrite('SCRIPT_NAME', $this->get_uri(), \phpbb\request\request_interface::SERVER);
$request->overwrite('SCRIPT_FILENAME', $this->get_script_name(), \phpbb\request\request_interface::SERVER);
$request->overwrite('REQUEST_URI', $this->get_uri(), \phpbb\request\request_interface::SERVER);
$request->overwrite('SERVER_NAME', 'localhost', \phpbb\request\request_interface::SERVER);
$request->overwrite('SERVER_PORT', '80', \phpbb\request\request_interface::SERVER);
$this->request = new phpbb_mock_request();
$this->request->overwrite('SCRIPT_NAME', $this->get_uri(), \phpbb\request\request_interface::SERVER);
$this->request->overwrite('SCRIPT_FILENAME', $this->get_script_name(), \phpbb\request\request_interface::SERVER);
$this->request->overwrite('REQUEST_URI', $this->get_uri(), \phpbb\request\request_interface::SERVER);
$this->request->overwrite('SERVER_NAME', 'localhost', \phpbb\request\request_interface::SERVER);
$this->request->overwrite('SERVER_PORT', '80', \phpbb\request\request_interface::SERVER);
$this->symfony_request = new \phpbb\symfony_request(
$request
$this->request
);
$this->filesystem = new \phpbb\filesystem();
$this->phpbb_path_helper = new \phpbb\path_helper(
$this->symfony_request,
$this->filesystem,
$this->getMock('\phpbb\request\request'),
$this->request,
$phpbb_root_path,
$phpEx
);
@ -130,7 +130,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
*/
public function test_helper_url_no_rewrite($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id));
}
@ -170,7 +170,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id));
}
@ -210,7 +210,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_absolute($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '0'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::ABSOLUTE_URL));
}
@ -250,7 +250,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_relative_path($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '0'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::RELATIVE_PATH));
}
@ -290,7 +290,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_network($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '0'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::NETWORK_PATH));
}
//TODO
@ -330,7 +330,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_absolute_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::ABSOLUTE_URL));
}
@ -370,7 +370,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_relative_path_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::RELATIVE_PATH));
}
@ -410,7 +410,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case
public function test_helper_url_network_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description)
{
$this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1'));
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/');
$this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::NETWORK_PATH));
}
}

3
tests/mock/controller_helper.php
View File

@ -13,12 +13,13 @@
class phpbb_mock_controller_helper extends \phpbb\controller\helper
{
public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext, $phpbb_root_path_ext)
public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\request\request_interface $request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext, $phpbb_root_path_ext)
{
$this->template = $template;
$this->user = $user;
$this->config = $config;
$this->symfony_request = $symfony_request;
$this->request = $request;
$this->filesystem = $filesystem;
$this->phpbb_root_path = $phpbb_root_path;
$this->php_ext = $php_ext;

21
tests/mock/request.php
View File

@ -114,4 +114,25 @@ class phpbb_mock_request implements \phpbb\request\request_interface
{
$this->data[$super_global] = array_merge($this->data[$super_global], $values);
}
public function escape($var, $multibyte)
{
$type_cast_helper = new \phpbb\request\type_cast_helper();
if (is_array($var))
{
$result = array();
foreach ($var as $key => $value)
{
$type_cast_helper->set_var($key, $key, gettype($key), $multibyte);
$result[$key] = $this->escape($value, $multibyte);
}
$var = $result;
}
else
{
$type_cast_helper->set_var($var, $var, 'string', $multibyte);
}
return $var;
}
}

2
tests/pagination/pagination_test.php
View File

@ -57,7 +57,7 @@ class phpbb_pagination_pagination_test extends phpbb_template_template_test_case
$request
);
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $provider, $manager, $symfony_request, $filesystem, '', 'php', dirname(__FILE__) . '/');
$this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $provider, $manager, $symfony_request, $request, $filesystem, '', 'php', dirname(__FILE__) . '/');
$this->pagination = new \phpbb\pagination($this->template, $this->user, $this->helper, $phpbb_dispatcher);
}

28
tests/security/extract_current_page_test.php
View File

@ -1,15 +1,15 @@
<?php
/**
*
* This file is part of the phpBB Forum Software package.
*
* @copyright (c) phpBB Limited <https://www.phpbb.com>
* @license GNU General Public License, version 2 (GPL-2.0)
*
* For full copyright and license information, please see
* the docs/CREDITS.txt file.
*
*/
*
* This file is part of the phpBB Forum Software package.
*
* @copyright (c) phpBB Limited <https://www.phpbb.com>
* @license GNU General Public License, version 2 (GPL-2.0)
*
* For full copyright and license information, please see
* the docs/CREDITS.txt file.
*
*/
require_once dirname(__FILE__) . '/base.php';
@ -27,8 +27,8 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base
}
/**
* @dataProvider security_variables
*/
* @dataProvider security_variables
*/
public function test_query_string_php_self($query_string, $expected)
{
global $symfony_request, $request;
@ -46,8 +46,8 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base
}
/**
* @dataProvider security_variables
*/
* @dataProvider security_variables
*/
public function test_query_string_request_uri($query_string, $expected)
{
global $symfony_request, $request;