Fix #346

When ['global_xss_filtering'] was turned on, the , , & superglobals were automatically overwritten. This resulted in one of the following problems: - xss_clean() being called twice - Inability to retrieve the original (not filtered) value XSS filtering is now only applied on demand by the Input class, and the default value for the parameter in CI_Input methods is changed to NULL. Unless a boolean value is passed to them, whether XSS filtering is applied depends on the ['global_xss_filtering'] value.
bcit-ci · Jan 8, 2014 · 80a16b1 · 80a16b1 · mpmont · Jan 8, 2014
1 parent fb61447
commit 80a16b1
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 22 deletions.
diff --git a/system/core/Input.php b/system/core/Input.php
@@ -151,8 +151,10 @@ public function __construct()
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	protected function _fetch_from_array(&$array, $index = '', $xss_clean = FALSE)
+	protected function _fetch_from_array(&$array, $index = '', $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		if (isset($array[$index]))
 		{
 			$value = $array[$index];
@@ -197,8 +199,10 @@ protected function _fetch_from_array(&$array, $index = '', $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function get($index = NULL, $xss_clean = FALSE)
+	public function get($index = NULL, $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		// Check if a field has been provided
 		if ($index === NULL)
 		{
@@ -229,8 +233,10 @@ public function get($index = NULL, $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function post($index = NULL, $xss_clean = FALSE)
+	public function post($index = NULL, $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		// Check if a field has been provided
 		if ($index === NULL)
 		{
@@ -261,8 +267,10 @@ public function post($index = NULL, $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function post_get($index = '', $xss_clean = FALSE)
+	public function post_get($index = '', $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		return isset($_POST[$index])
 			? $this->post($index, $xss_clean)
 			: $this->get($index, $xss_clean);
@@ -277,8 +285,10 @@ public function post_get($index = '', $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function get_post($index = '', $xss_clean = FALSE)
+	public function get_post($index = '', $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		return isset($_GET[$index])
 			? $this->get($index, $xss_clean)
 			: $this->post($index, $xss_clean);
@@ -293,8 +303,10 @@ public function get_post($index = '', $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function cookie($index = '', $xss_clean = FALSE)
+	public function cookie($index = '', $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		return $this->_fetch_from_array($_COOKIE, $index, $xss_clean);
 	}
 
@@ -307,8 +319,10 @@ public function cookie($index = '', $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function server($index = '', $xss_clean = FALSE)
+	public function server($index = '', $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		return $this->_fetch_from_array($_SERVER, $index, $xss_clean);
 	}
 
@@ -323,8 +337,10 @@ public function server($index = '', $xss_clean = FALSE)
 	 * @param	bool	$xss_clean	Whether to apply XSS filtering
 	 * @return	mixed
 	 */
-	public function input_stream($index = '', $xss_clean = FALSE)
+	public function input_stream($index = '', $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = $this->_enable_xss;
+
 		// The input stream can only be read once, so we'll need to check
 		// if we have already done that first.
 		if (is_array($this->_input_stream))
@@ -760,12 +776,6 @@ protected function _clean_input_data($str)
 		// Remove control characters
 		$str = remove_invisible_characters($str, FALSE);
 
-		// Should we filter the input data?
-		if ($this->_enable_xss === TRUE)
-		{
-			$str = $this->security->xss_clean($str);
-		}
-
 		// Standardize newlines if needed
 		if ($this->_standardize_newlines === TRUE)
 		{

diff --git a/system/helpers/cookie_helper.php b/system/helpers/cookie_helper.php
@@ -74,8 +74,9 @@ function set_cookie($name, $value = '', $expire = '', $domain = '', $path = '/',
 	 * @param	bool
 	 * @return	mixed
 	 */
-	function get_cookie($index, $xss_clean = FALSE)
+	function get_cookie($index, $xss_clean = NULL)
 	{
+		is_bool($xss_clean) OR $xss_clean = (config_item('global_xss_filtering') === TRUE);
 		$prefix = isset($_COOKIE[$index]) ? '' : config_item('cookie_prefix');
 		return get_instance()->input->cookie($prefix.$index, $xss_clean);
 	}

diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst
@@ -402,6 +402,7 @@ Release Date: Not Released
       -  Changed method ``valid_ip()`` to use PHP's native ``filter_var()`` function.
       -  Changed internal method ``_sanitize_globals()`` to skip enforcing reversal of *register_globals* in PHP 5.4+, where this functionality no longer exists.
       -  Changed methods ``get()``, ``post()``, ``get_post()``, ``cookie()``, ``server()``, ``user_agent()`` to return NULL instead of FALSE when no value is found.
+      -  Changed default value of the ``$xss_clean`` parameter to NULL for all methods that utilize it, the default value is now determined by the ``$config['global_xss_filtering']`` setting.
       -  Added method ``post_get()`` and changed ``get_post()`` to search in GET data first. Both methods' names now properly match their GET/POST data search priorities.
       -  Changed method ``_fetch_from_array()`` to parse array notation in field name.
       -  Added an option for ``_clean_input_keys()`` to return FALSE instead of terminating the whole script.
@@ -646,6 +647,7 @@ Bug fixes for 3.0
 -  Fixed a bug (#2143) - :doc:`Form Validation Library <libraries/form_validation>` didn't check for rule groups named in a *controller/method* manner when trying to load from a config file.
 -  Fixed a bug (#2762) - :doc:`Hooks Class <general/hooks>` didn't properly check if the called class/function exists.
 -  Fixed a bug (#148) - while sanitizing input data, ``CI_Input::_clean_input_data()`` assumed that it is URL-encoded, stripping certain character sequences from it.
+-  Fixed a bug (#346) - with ``$config['global_xss_filtering']`` turned on, the ``$_GET``, ``$_POST``, ``$_COOKIE`` and ``$_SERVER`` superglobals were overwritten during initialization time, resulting in XSS filtering being either performed twice or there was no possible way to get the original data, even though options for this do exist.
 
 Version 2.1.4
 =============

diff --git a/user_guide_src/source/installation/upgrade_300.rst b/user_guide_src/source/installation/upgrade_300.rst
@@ -188,8 +188,39 @@ Many methods and functions now return NULL instead of FALSE when the required it
    - element()
    - elements()
 
+*******************************
+Step 11: Usage of XSS filtering
+*******************************
+
+Many functions in CodeIgniter allow you to use its XSS filtering feature
+on demand by passing a boolean parameter. The default value of that
+parameter used to be boolean FALSE, but it is now changed to NULL and it
+will be dynamically determined by your ``$config['global_xss_filtering']``
+value.
+
+If you used to manually pass a boolean value for the ``$xss_filter``
+parameter or if you've always had ``$config['global_xss_filtering']`` set
+to FALSE, then this change doesn't concern you.
+
+Otherwise however, please review your usage of the following functions:
+
+ - :doc:`Input Library <../libraries/input>`
+
+   - input->get()
+   - input->post()
+   - input->get_post()
+   - input->cookie()
+   - input->server()
+   - input->input_stream()
+
+ - :doc:`Cookie Helper <../helpers/cookie_helper>` :func:`get_cookie()`
+
+.. important:: Another related change is that the ``$_GET``, ``$_POST``,
+	``$_COOKIE`` and ``$_SERVER`` superglobals are no longer
+	automatically overwritten when global XSS filtering is turned on.
+
 ********************************************************
-Step 11: Update usage of Input Class's get_post() method
+Step 12: Update usage of Input Class's get_post() method
 ********************************************************
 
 Previously, the :doc:`Input Class <../libraries/input>` method ``get_post()``
@@ -200,14 +231,14 @@ A method has been added, ``post_get()``, which searches in POST then in GET, as
 ``get_post()`` was doing before.
 
 ***********************************************************************
-Step 12: Update usage of Directory Helper's directory_map() function
+Step 13: Update usage of Directory Helper's directory_map() function
 ***********************************************************************
 
 In the resulting array, directories now end with a trailing directory
 separator (i.e. a slash, usually).
 
 *************************************************************
-Step 13: Update usage of Database Forge's drop_table() method
+Step 14: Update usage of Database Forge's drop_table() method
 *************************************************************
 
 Up until now, ``drop_table()`` added an IF EXISTS clause by default or it didn't work
@@ -229,7 +260,7 @@ If your application relies on IF EXISTS, you'll have to change its usage.
 	all drivers with the exception of ODBC.
 
 ***********************************************************
-Step 14: Change usage of Email library with multiple emails
+Step 15: Change usage of Email library with multiple emails
 ***********************************************************
 
 The :doc:`Email Library <../libraries/email>` will automatically clear the
@@ -244,7 +275,7 @@ pass FALSE as the first parameter in the ``send()`` method:
  	}
 
 ***************************************************
-Step 15: Update your Form_validation language lines
+Step 16: Update your Form_validation language lines
 ***************************************************
 
 Two improvements have been made to the :doc:`Form Validation Library
@@ -275,7 +306,7 @@ files and error messages format:
 	later.
 
 ****************************************************************
-Step 16: Remove usage of (previously) deprecated functionalities
+Step 17: Remove usage of (previously) deprecated functionalities
 ****************************************************************
 
 In addition to the ``$autoload['core']`` configuration setting, there's a
@@ -491,7 +522,7 @@ CodeIgniter 3.1+.
 	sooner rather than later.
 
 ***********************************************************
-Step 17: Check your usage of Text helper highlight_phrase()
+Step 18: Check your usage of Text helper highlight_phrase()
 ***********************************************************
 
 The default HTML tag used by :doc:`Text Helper <../helpers/text_helper>` function