diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 1c5e0b63a1..485f5487cc 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -19,121 +19,53 @@ if (!defined('IN_PHPBB')) // Common global functions /** -* set_var +* Wrapper function of phpbb_request::variable which exists for backwards compatability. +* See {@link phpbb_request_interface::variable phpbb_request_interface::variable} for +* documentation of this function's use. * -* Set variable, used by {@link request_var the request_var function} +* @param mixed $var_name The form variable's name from which data shall be retrieved. +* If the value is an array this may be an array of indizes which will give +* direct access to a value at any depth. E.g. if the value of "var" is array(1 => "a") +* then specifying array("var", 1) as the name will return "a". +* If you pass an instance of {@link phpbb_request_interface phpbb_request_interface} +* as this parameter it will overwrite the current request class instance. If you do +* not do so, it will create its own instance (but leave superglobals enabled). +* @param mixed $default A default value that is returned if the variable was not set. +* This function will always return a value of the same type as the default. +* @param bool $multibyte If $default is a string this paramater has to be true if the variable may contain any UTF-8 characters +* Default is false, causing all bytes outside the ASCII range (0-127) to be replaced with question marks +* @param bool $cookie This param is mapped to phpbb_request_interface::COOKIE as the last param for +* phpbb_request_interface::variable for backwards compatability reasons. * -* @access private -*/ -function set_var(&$result, $var, $type, $multibyte = false) -{ - settype($var, $type); - $result = $var; - - if ($type == 'string') - { - $result = trim(htmlspecialchars(str_replace(array("\r\n", "\r", "\0"), array("\n", "\n", ''), $result), ENT_COMPAT, 'UTF-8')); - - if (!empty($result)) - { - // Make sure multibyte characters are wellformed - if ($multibyte) - { - if (!preg_match('/^./u', $result)) - { - $result = ''; - } - } - else - { - // no multibyte, allow only ASCII (0-127) - $result = preg_replace('/[\x80-\xFF]/', '?', $result); - } - } - - $result = (STRIP) ? stripslashes($result) : $result; - } -} - -/** -* request_var -* -* Used to get passed variable +* @return mixed The value of $_REQUEST[$var_name] run through {@link set_var set_var} to ensure that the type is the +* the same as that of $default. If the variable is not set $default is returned. */ function request_var($var_name, $default, $multibyte = false, $cookie = false) { - if (!$cookie && isset($_COOKIE[$var_name])) + // This is all just an ugly hack to add "Dependency Injection" to a function + // the only real code is the function call which maps this function to a method. + static $request = null; + + if ($var_name instanceof phpbb_request_interface) { - if (!isset($_GET[$var_name]) && !isset($_POST[$var_name])) - { - return (is_array($default)) ? array() : $default; - } - $_REQUEST[$var_name] = isset($_POST[$var_name]) ? $_POST[$var_name] : $_GET[$var_name]; + $request = $var_name; } - $super_global = ($cookie) ? '_COOKIE' : '_REQUEST'; - if (!isset($GLOBALS[$super_global][$var_name]) || is_array($GLOBALS[$super_global][$var_name]) != is_array($default)) + // no request class set, create a temporary one ourselves to keep backwards compatability + if ($request === null) { - return (is_array($default)) ? array() : $default; - } - - $var = $GLOBALS[$super_global][$var_name]; - if (!is_array($default)) - { - $type = gettype($default); + $tmp_request = new phpbb_request(); + // enable super globals, so the magically created request class does not + // make super globals inaccessible everywhere outside this function. + $tmp_request->enable_super_globals(); } else { - list($key_type, $type) = each($default); - $type = gettype($type); - $key_type = gettype($key_type); - if ($type == 'array') - { - reset($default); - $default = current($default); - list($sub_key_type, $sub_type) = each($default); - $sub_type = gettype($sub_type); - $sub_type = ($sub_type == 'array') ? 'NULL' : $sub_type; - $sub_key_type = gettype($sub_key_type); - } + // otherwise use the static injected instance + $tmp_request = $request; } - if (is_array($var)) - { - $_var = $var; - $var = array(); - - foreach ($_var as $k => $v) - { - set_var($k, $k, $key_type); - if ($type == 'array' && is_array($v)) - { - foreach ($v as $_k => $_v) - { - if (is_array($_v)) - { - $_v = null; - } - set_var($_k, $_k, $sub_key_type, $multibyte); - set_var($var[$k][$_k], $_v, $sub_type, $multibyte); - } - } - else - { - if ($type == 'array' || is_array($v)) - { - $v = null; - } - set_var($var[$k], $v, $type, $multibyte); - } - } - } - else - { - set_var($var, $var, $type, $multibyte); - } - - return $var; + return $tmp_request->variable($var_name, $default, $multibyte, ($cookie) ? phpbb_request_interface::COOKIE : phpbb_request_interface::REQUEST); } /** diff --git a/phpBB/includes/request/deactivated_super_global.php b/phpBB/includes/request/deactivated_super_global.php new file mode 100644 index 0000000000..9152d10811 --- /dev/null +++ b/phpBB/includes/request/deactivated_super_global.php @@ -0,0 +1,121 @@ +request = $request; + $this->name = $name; + $this->super_global = $super_global; + } + + /** + * Calls trigger_error with the file and line number the super global was used in. + */ + private function error() + { + $file = ''; + $line = 0; + + $message = 'Illegal use of $' . $this->name . '. You must use the request class or request_var() to access input data. Found in %s on line %d. This error message was generated'; + + $backtrace = debug_backtrace(); + if (isset($backtrace[1])) + { + $file = $backtrace[1]['file']; + $line = $backtrace[1]['line']; + } + trigger_error(sprintf($message, $file, $line), E_USER_ERROR); + } + + /** + * Redirects isset to the correct request class call. + * + * @param string $offset The key of the super global being accessed. + * + * @return bool Whether the key on the super global exists. + */ + public function offsetExists($offset) + { + return $this->request->is_set($offset, $this->super_global); + } + + /**#@+ + * Part of the ArrayAccess implementation, will always result in a FATAL error. + */ + public function offsetGet($offset) + { + $this->error(); + } + + public function offsetSet($offset, $value) + { + $this->error(); + } + + public function offsetUnset($offset) + { + $this->error(); + } + /**#@-*/ + + /** + * Part of the Countable implementation, will always result in a FATAL error + */ + public function count() + { + $this->error(); + } + + /** + * Part of the Traversable/IteratorAggregate implementation, will always result in a FATAL error + */ + public function getIterator() + { + $this->error(); + } +} + diff --git a/phpBB/includes/request/request.php b/phpBB/includes/request/request.php new file mode 100644 index 0000000000..dbed95546f --- /dev/null +++ b/phpBB/includes/request/request.php @@ -0,0 +1,412 @@ + '_POST', + phpbb_request_interface::GET => '_GET', + phpbb_request_interface::REQUEST => '_REQUEST', + phpbb_request_interface::COOKIE => '_COOKIE' + ); + + /** + * @var array Stores original contents of $_REQUEST array. + */ + protected $original_request = null; + + /** + * @var + */ + protected $super_globals_disabled = false; + + /** + * @var array An associative array that has the value of super global constants as keys and holds their data as values. + */ + protected $input; + + /** + * @var string Whether slashes need to be stripped from input + */ + protected $strip; + + /** + * Initialises the request class, that means it stores all input data in {@link $input input} + * and then calls {@link phpbb_deactivated_super_global phpbb_deactivated_super_global} + */ + public function __construct() + { + if (version_compare(PHP_VERSION, '6.0.0-dev', '>=')) + { + $this->strip = false; + } + else + { + $this->strip = (@get_magic_quotes_gpc()) ? true : false; + } + + foreach (self::$super_globals as $const => $super_global) + { + $this->input[$const] = isset($GLOBALS[$super_global]) ? $GLOBALS[$super_global] : array(); + } + + // simulate request_order = GP + $this->original_request = $this->input[phpbb_request_interface::REQUEST]; + $this->input[phpbb_request_interface::REQUEST] = $this->input[phpbb_request_interface::POST] + $this->input[phpbb_request_interface::GET]; + + $this->disable_super_globals(); + } + + /** + * Getter for $super_globals_disabled + * + * @return bool Whether super globals are disabled or not. + */ + public function super_globals_disabled() + { + return $this->super_globals_disabled; + } + + /** + * Disables access of super globals specified in $super_globals. + * This is achieved by overwriting the super globals with instances of {@link phpbb_deactivated_super_global phpbb_deactivated_super_global} + */ + public function disable_super_globals() + { + if (!$this->super_globals_disabled) + { + foreach (self::$super_globals as $const => $super_global) + { + unset($GLOBALS[$super_global]); + $GLOBALS[$super_global] = new phpbb_deactivated_super_global($this, $super_global, $const); + } + + $this->super_globals_disabled = true; + } + } + + /** + * Enables access of super globals specified in $super_globals if they were disabled by {@link disable_super_globals disable_super_globals}. + * This is achieved by making the super globals point to the data stored within this class in {@link $input input}. + */ + public function enable_super_globals() + { + if ($this->super_globals_disabled) + { + foreach (self::$super_globals as $const => $super_global) + { + $GLOBALS[$super_global] = $this->input[$const]; + } + + $GLOBALS['_REQUEST'] = $this->original_request; + + $this->super_globals_disabled = false; + } + } + + /** + * Recursively applies addslashes to a variable. + * + * @param mixed &$var Variable passed by reference to which slashes will be added. + */ + public static function addslashes_recursively(&$var) + { + if (is_string($var)) + { + $var = addslashes($var); + } + else if (is_array($var)) + { + $var_copy = $var; + $var = array(); + foreach ($var_copy as $key => $value) + { + if (is_string($key)) + { + $key = addslashes($key); + } + $var[$key] = $value; + + self::addslashes_recursively($var[$key]); + } + } + } + + /** + * This function allows overwriting or setting a value in one of the super global arrays. + * + * Changes which are performed on the super globals directly will not have any effect on the results of + * other methods this class provides. Using this function should be avoided if possible! It will + * consume twice the the amount of memory of the value + * + * @param string $var_name The name of the variable that shall be overwritten + * @param mixed $value The value which the variable shall contain. + * If this is null the variable will be unset. + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * Specifies which super global shall be changed + */ + public function overwrite($var_name, $value, $super_global = phpbb_request_interface::REQUEST) + { + if (!isset(self::$super_globals[$super_global])) + { + return; + } + + if ($this->strip) + { + self::addslashes_recursively($value); + } + + // setting to null means unsetting + if ($value === null) + { + unset($this->input[$super_global][$var_name]); + if (!$this->super_globals_disabled()) + { + unset($GLOBALS[self::$super_globals[$super_global]][$var_name]); + } + } + else + { + $this->input[$super_global][$var_name] = $value; + if (!self::super_globals_disabled()) + { + $GLOBALS[self::$super_globals[$super_global]][$var_name] = $value; + } + } + + if (!self::super_globals_disabled()) + { + unset($GLOBALS[self::$super_globals[$super_global]][$var_name]); + $GLOBALS[self::$super_globals[$super_global]][$var_name] = $value; + } + } + + /** + * Set variable $result to a particular type. + * + * @param mixed &$result The variable to fill + * @param mixed $var The contents to fill with + * @param mixed $type The variable type. Will be used with {@link settype()} + * @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. + */ + public function set_var(&$result, $var, $type, $multibyte = false) + { + settype($var, $type); + $result = $var; + + if ($type == 'string') + { + $result = trim(htmlspecialchars(str_replace(array("\r\n", "\r", "\0"), array("\n", "\n", ''), $result), ENT_COMPAT, 'UTF-8')); + + if (!empty($result)) + { + // Make sure multibyte characters are wellformed + if ($multibyte) + { + if (!preg_match('/^./u', $result)) + { + $result = ''; + } + } + else + { + // no multibyte, allow only ASCII (0-127) + $result = preg_replace('/[\x80-\xFF]/', '?', $result); + } + } + + $result = ($this->strip) ? stripslashes($result) : $result; + } + } + + /** + * Recursively sets a variable to a given type using {@link set_var set_var} + * This function is only used from within {@link phpbb_request::variable phpbb_request::variable}. + * + * @param string $var The value which shall be sanitised (passed by reference). + * @param mixed $default Specifies the type $var shall have. + * If it is an array and $var is not one, then an empty array is returned. + * Otherwise var is cast to the same type, and if $default is an array all + * keys and values are cast recursively using this function too. + * @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. + */ + protected function recursive_set_var(&$var, $default, $multibyte) + { + if (is_array($var) !== is_array($default)) + { + $var = (is_array($default)) ? array() : $default; + return; + } + + if (!is_array($default)) + { + $type = gettype($default); + $this->set_var($var, $var, $type, $multibyte); + } + else + { + // make sure there is at least one key/value pair to use get the + // types from + if (empty($default)) + { + $var = array(); + return; + } + + list($default_key, $default_value) = each($default); + $value_type = gettype($default_value); + $key_type = gettype($default_key); + + $_var = $var; + $var = array(); + + foreach ($_var as $k => $v) + { + $this->set_var($k, $k, $key_type, $multibyte, $multibyte); + + $this->recursive_set_var($v, $default_value, $multibyte); + $this->set_var($var[$k], $v, $value_type, $multibyte); + } + } + } + + /** + * Central type safe input handling function. + * All variables in GET or POST requests should be retrieved through this function to maximise security. + * + * @param string|array $var_name The form variable's name from which data shall be retrieved. + * If the value is an array this may be an array of indizes which will give + * direct access to a value at any depth. E.g. if the value of "var" is array(1 => "a") + * then specifying array("var", 1) as the name will return "a". + * @param mixed $default A default value that is returned if the variable was not set. + * This function will always return a value of the same type as the default. + * @param bool $multibyte If $default is a string this paramater has to be true if the variable may contain any UTF-8 characters + * Default is false, causing all bytes outside the ASCII range (0-127) to be replaced with question marks + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * Specifies which super global should be used + * + * @return mixed The value of $_REQUEST[$var_name] run through {@link set_var set_var} to ensure that the type is the + * the same as that of $default. If the variable is not set $default is returned. + */ + public function variable($var_name, $default, $multibyte = false, $super_global = phpbb_request_interface::REQUEST) + { + $path = false; + + // deep direct access to multi dimensional arrays + if (is_array($var_name)) + { + $path = $var_name; + // make sure at least the variable name is specified + if (empty($path)) + { + return (is_array($default)) ? array() : $default; + } + // the variable name is the first element on the path + $var_name = array_shift($path); + } + + if (!isset($this->input[$super_global][$var_name])) + { + return (is_array($default)) ? array() : $default; + } + $var = $this->input[$super_global][$var_name]; + + if ($path) + { + // walk through the array structure and find the element we are looking for + foreach ($path as $key) + { + if (is_array($var) && isset($var[$key])) + { + $var = $var[$key]; + } + else + { + return (is_array($default)) ? array() : $default; + } + } + } + + self::recursive_set_var($var, $default, $multibyte); + + return $var; + } + + /** + * Checks whether a certain variable was sent via POST. + * To make sure that a request was sent using POST you should call this function + * on at least one variable. + * + * @param string $name The name of the form variable which should have a + * _p suffix to indicate the check in the code that creates the form too. + * + * @return bool True if the variable was set in a POST request, false otherwise. + */ + public function is_set_post($name) + { + return $this->is_set($name, phpbb_request_interface::POST); + } + + /** + * Checks whether a certain variable is set in one of the super global + * arrays. + * + * @param string $var Name of the variable + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * Specifies the super global which shall be checked + * + * @return bool True if the variable was sent as input + */ + public function is_set($var, $super_global = phpbb_request_interface::REQUEST) + { + return isset($this->input[$super_global][$var]); + } + + /** + * Returns all variable names for a given super global + * + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * The super global from which names shall be taken + * + * @return array All variable names that are set for the super global. + * Pay attention when using these, they are unsanitised! + */ + public function variable_names($super_global = phpbb_request_interface::REQUEST) + { + if (!isset($this->input[$super_global])) + { + return array(); + } + + return array_keys($this->input[$super_global]); + } +} diff --git a/phpBB/includes/request/request_interface.php b/phpBB/includes/request/request_interface.php new file mode 100644 index 0000000000..7b5b600100 --- /dev/null +++ b/phpBB/includes/request/request_interface.php @@ -0,0 +1,103 @@ + "a") + * then specifying array("var", 1) as the name will return "a". + * @param mixed $default A default value that is returned if the variable was not set. + * This function will always return a value of the same type as the default. + * @param bool $multibyte If $default is a string this paramater has to be true if the variable may contain any UTF-8 characters + * Default is false, causing all bytes outside the ASCII range (0-127) to be replaced with question marks + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * Specifies which super global should be used + * + * @return mixed The value of $_REQUEST[$var_name] run through {@link set_var set_var} to ensure that the type is the + * the same as that of $default. If the variable is not set $default is returned. + */ + public function variable($var_name, $default, $multibyte = false, $super_global = phpbb_request_interface::REQUEST); + + /** + * Checks whether a certain variable was sent via POST. + * To make sure that a request was sent using POST you should call this function + * on at least one variable. + * + * @param string $name The name of the form variable which should have a + * _p suffix to indicate the check in the code that creates the form too. + * + * @return bool True if the variable was set in a POST request, false otherwise. + */ + public function is_set_post($name); + + /** + * Checks whether a certain variable is set in one of the super global + * arrays. + * + * @param string $var Name of the variable + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * Specifies the super global which shall be checked + * + * @return bool True if the variable was sent as input + */ + public function is_set($var, $super_global = phpbb_request_interface::REQUEST); + + /** + * Returns all variable names for a given super global + * + * @param phpbb_request_interface::POST|GET|REQUEST|COOKIE $super_global + * The super global from which names shall be taken + * + * @return array All variable names that are set for the super global. + * Pay attention when using these, they are unsanitised! + */ + public function variable_names($super_global = phpbb_request_interface::REQUEST); +} diff --git a/tests/request/all_tests.php b/tests/request/all_tests.php index 1ee3029b36..6757d463c5 100644 --- a/tests/request/all_tests.php +++ b/tests/request/all_tests.php @@ -15,6 +15,8 @@ if (!defined('PHPUnit_MAIN_METHOD')) require_once 'test_framework/framework.php'; require_once 'PHPUnit/TextUI/TestRunner.php'; +require_once 'request/deactivated_super_global.php'; +require_once 'request/request.php'; require_once 'request/request_var.php'; class phpbb_request_all_tests @@ -28,6 +30,8 @@ class phpbb_request_all_tests { $suite = new PHPUnit_Framework_TestSuite('phpBB Request Parameter Handling'); + $suite->addTestSuite('phpbb_request_deactivated_super_global_test'); + $suite->addTestSuite('phpbb_request_test'); $suite->addTestSuite('phpbb_request_request_var_test'); return $suite; diff --git a/tests/request/deactivated_super_global.php b/tests/request/deactivated_super_global.php new file mode 100644 index 0000000000..dcf17b0a0e --- /dev/null +++ b/tests/request/deactivated_super_global.php @@ -0,0 +1,25 @@ +setExpectedTriggerError(E_USER_ERROR); + $obj = new phpbb_deactivated_super_global($this->getMock('phpbb_request_interface'), 'obj', phpbb_request_interface::POST); + $obj->offsetSet(0, 0); + } +} diff --git a/tests/request/request.php b/tests/request/request.php new file mode 100644 index 0000000000..1376d0665a --- /dev/null +++ b/tests/request/request.php @@ -0,0 +1,89 @@ +request = new phpbb_request(); + } + + public function test_toggle_super_globals() + { + $this->assertTrue($this->request->super_globals_disabled(), 'Superglobals were not disabled'); + + $this->request->enable_super_globals(); + + $this->assertFalse($this->request->super_globals_disabled(), 'Superglobals were not enabled'); + + $this->assertEquals(1, $_POST['test'], 'Checking $_POST after enable_super_globals'); + $this->assertEquals(2, $_GET['test'], 'Checking $_GET after enable_super_globals'); + $this->assertEquals(3, $_COOKIE['test'], 'Checking $_COOKIE after enable_super_globals'); + $this->assertEquals(3, $_REQUEST['test'], 'Checking $_REQUEST after enable_super_globals'); + + $_POST['x'] = 2; + $this->assertEquals($_POST, $GLOBALS['_POST'], 'Checking whether $_POST can still be accessed via $GLOBALS[\'_POST\']'); + } + + /** + * Checks that directly accessing $_POST will trigger + * an error. + */ + public function test_disable_post_super_global() + { + $this->setExpectedTriggerError(E_USER_ERROR); + $_POST['test'] = 3; + } + + public function test_is_set_post() + { + $this->assertTrue($this->request->is_set_post('test')); + $this->assertFalse($this->request->is_set_post('unset')); + } + + public function test_addslashes_recursively() + { + $data = array('some"string' => array('that"' => 'really"', 'needs"' => '"escaping')); + $expected = array('some\\"string' => array('that\\"' => 'really\\"', 'needs\\"' => '\\"escaping')); + + phpbb_request::addslashes_recursively($data); + + $this->assertEquals($expected, $data); + } + + public function test_variable_names() + { + $expected = array('test', 'unset'); + $result = $this->request->variable_names(); + $this->assertEquals($expected, $result); + } + + /** + * Makes sure super globals work properly after these tests + */ + protected function tearDown() + { + $this->request->enable_super_globals(); + } +} diff --git a/tests/request/request_class.php b/tests/request/request_class.php deleted file mode 100644 index e8c2154bab..0000000000 --- a/tests/request/request_class.php +++ /dev/null @@ -1,74 +0,0 @@ -assertEquals(1, $_POST['test'], 'Checking $_POST toggling via request::dis/enable_super_globals'); - $this->assertEquals(2, $_GET['test'], 'Checking $_GET toggling via request::dis/enable_super_globals'); - $this->assertEquals(3, $_COOKIE['test'], 'Checking $_COOKIE toggling via request::dis/enable_super_globals'); - $this->assertEquals(3, $_REQUEST['test'], 'Checking $_REQUEST toggling via request::dis/enable_super_globals'); - - $_POST['x'] = 2; - $this->assertEquals($_POST, $GLOBALS['_POST'], 'Checking whether $_POST can still be accessed via $GLOBALS[\'_POST\']'); - } - - /** - * Checks that directly accessing $_POST will trigger - * an error. - */ - public function test_disable_post_super_global() - { - request::disable_super_globals(); - - $this->setExpectedTriggerError(E_USER_ERROR); - $_POST['test'] = 3; - } - - public function test_is_set_post() - { - $_GET['unset'] = ''; - request::reset(); - - $this->assertTrue(request::is_set_post('test')); - $this->assertFalse(request::is_set_post('unset')); - } - - /** - * Makes sure super globals work properly after these tests - */ - protected function tearDown() - { - request::enable_super_globals(); - request::reset(); - } -} \ No newline at end of file diff --git a/tests/request/request_var.php b/tests/request/request_var.php index 0f24d77034..50c6ffd44b 100644 --- a/tests/request/request_var.php +++ b/tests/request/request_var.php @@ -8,6 +8,9 @@ */ require_once 'test_framework/framework.php'; +require_once '../phpBB/includes/request/request_interface.php'; +require_once '../phpBB/includes/request/deactivated_super_global.php'; +require_once '../phpBB/includes/request/request.php'; require_once '../phpBB/includes/functions.php'; class phpbb_request_request_var_test extends phpbb_test_case