diff options
| -rw-r--r-- | contributing.md | 6 | ||||
| -rw-r--r-- | system/core/Router.php | 13 | ||||
| -rw-r--r-- | system/core/URI.php | 14 | ||||
| -rw-r--r-- | tests/codeigniter/core/URI_test.php | 23 | ||||
| -rw-r--r-- | user_guide_src/source/changelog.rst | 2 | ||||
| -rw-r--r-- | user_guide_src/source/installation/upgrade_300.rst | 28 |
6 files changed, 45 insertions, 41 deletions
diff --git a/contributing.md b/contributing.md index 99ed85909..126bdc74f 100644 --- a/contributing.md +++ b/contributing.md @@ -79,7 +79,9 @@ Hard way The best way to contribute is to "clone" your fork of CodeIgniter to yo The Reactor Engineers will now be alerted about the change and at least one of the team will respond. If your change fails to meet the guidelines it will be bounced, or feedback will be provided to help you improve it. -Once the Reactor Engineer handling your pull request is happy with it they will merge it into develop and your patch will be part of the next release. Keeping your fork up-to-date +Once the Reactor Engineer handling your pull request is happy with it they will merge it into develop and your patch will be part of the next release. + +### Keeping your fork up-to-date Unlike systems like Subversion, Git can have multiple remotes. A remote is the name for a URL of a Git repository. By default your fork will have a remote named "origin" which points to your fork, but you can add another remote named "codeigniter" which points to `git://github.com/bcit-ci/CodeIgniter.git`. This is a read-only remote but you can pull from this develop branch to update your own. @@ -89,4 +91,4 @@ If you are using command-line you can do the following: 2. `git pull codeigniter develop` 3. `git push origin develop` -Now your fork is up to date. This should be done regularly, or before you send a pull request at least.
\ No newline at end of file +Now your fork is up to date. This should be done regularly, or before you send a pull request at least. diff --git a/system/core/Router.php b/system/core/Router.php index 7f18adbf5..d86735f5f 100644 --- a/system/core/Router.php +++ b/system/core/Router.php @@ -171,18 +171,21 @@ class CI_Router { $_d = isset($_GET[$_d]) ? trim($_GET[$_d], " \t\n\r\0\x0B/") : ''; if ($_d !== '') { - $this->set_directory($this->uri->filter_uri($_d)); + $this->uri->filter_uri($_d); + $this->set_directory($_d); } - $_c = $this->config->item('controller_trigger'); + $_c = trim($this->config->item('controller_trigger')); if ( ! empty($_GET[$_c])) { - $this->set_class(trim($this->uri->filter_uri(trim($_GET[$_c])))); + $this->uri->filter_uri($_GET[$_c]); + $this->set_class($_GET[$_c]); - $_f = $this->config->item('function_trigger'); + $_f = trim($this->config->item('function_trigger')); if ( ! empty($_GET[$_f])) { - $this->set_method(trim($this->uri->filter_uri($_GET[$_f]))); + $this->uri->filter_uri($_GET[$_f]); + $this->set_method($_GET[$_f]); } $this->uri->rsegments = array( diff --git a/system/core/URI.php b/system/core/URI.php index 1817374b7..790910169 100644 --- a/system/core/URI.php +++ b/system/core/URI.php @@ -173,8 +173,9 @@ class CI_URI { // Populate the segments array foreach (explode('/', trim($this->uri_string, '/')) as $val) { + $val = trim($val); // Filter segments for security - $val = trim($this->filter_uri($val)); + $this->filter_uri($val); if ($val !== '') { @@ -318,21 +319,14 @@ class CI_URI { * Filters segments for malicious characters. * * @param string $str - * @return string + * @return void */ - public function filter_uri($str) + public function filter_uri(&$str) { if ( ! empty($str) && ! empty($this->_permitted_uri_chars) && ! preg_match('/^['.$this->_permitted_uri_chars.']+$/i'.(UTF8_ENABLED ? 'u' : ''), $str)) { show_error('The URI you submitted has disallowed characters.', 400); } - - // Convert programatic characters to entities and return - return str_replace( - array('$', '(', ')', '%28', '%29'), // Bad - array('$', '(', ')', '(', ')'), // Good - $str - ); } // -------------------------------------------------------------------- diff --git a/tests/codeigniter/core/URI_test.php b/tests/codeigniter/core/URI_test.php index b05a385d6..50663d54a 100644 --- a/tests/codeigniter/core/URI_test.php +++ b/tests/codeigniter/core/URI_test.php @@ -119,26 +119,12 @@ class URI_test extends CI_TestCase { */ // -------------------------------------------------------------------- - public function test_filter_uri() + public function test_filter_uri_passing() { $this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-'); - $str_in = 'abc01239~%.:_-'; - $str = $this->uri->filter_uri($str_in); - - $this->assertEquals($str, $str_in); - } - - // -------------------------------------------------------------------- - - public function test_filter_uri_escaping() - { - // ensure escaping even if dodgey characters are permitted - $this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-()$'); - - $str = $this->uri->filter_uri('$destroy_app(foo)'); - - $this->assertEquals($str, '$destroy_app(foo)'); + $str = 'abc01239~%.:_-'; + $this->uri->filter_uri($str); } // -------------------------------------------------------------------- @@ -149,7 +135,8 @@ class URI_test extends CI_TestCase { $this->uri->config->set_item('enable_query_strings', FALSE); $this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-'); - $this->uri->filter_uri('$this()'); + $segment = '$this()'; // filter_uri() accepts by reference + $this->uri->filter_uri($segment); } // -------------------------------------------------------------------- diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst index 57a7dc770..431016d6a 100644 --- a/user_guide_src/source/changelog.rst +++ b/user_guide_src/source/changelog.rst @@ -57,6 +57,7 @@ Release Date: Not Released - Added support for changing the file extension of log files using ``$config['log_file_extension']``. - Added support for turning newline standardization on/off via ``$config['standardize_newlines']`` and set it to FALSE by default. - Added configuration setting ``$config['composer_autoload']`` to enable loading of a `Composer <https://getcomposer.org/>`_ auto-loader. + - Removed the automatic conversion of 'programmatic characters' to HTML entities from the :doc:`URI Library <libraries/uri>`. - Helpers @@ -446,6 +447,7 @@ Release Date: Not Released - Added conditional PCRE UTF-8 support to the "invalid URI characters" check and removed the ``preg_quote()`` call from it to allow more flexibility. - Renamed method ``_filter_uri()`` to ``filter_uri()``. + - Changed method ``filter_uri()`` to accept by reference and removed its return value. - Changed private methods to protected so that MY_URI can override them. - Renamed internal method ``_parse_cli_args()`` to ``_parse_argv()``. - Renamed internal method ``_detect_uri()`` to ``_parse_request_uri()``. diff --git a/user_guide_src/source/installation/upgrade_300.rst b/user_guide_src/source/installation/upgrade_300.rst index ef85106b7..2e9ee4e72 100644 --- a/user_guide_src/source/installation/upgrade_300.rst +++ b/user_guide_src/source/installation/upgrade_300.rst @@ -223,8 +223,24 @@ Otherwise however, please review your usage of the following functions: ``$_COOKIE`` and ``$_SERVER`` superglobals are no longer automatically overwritten when global XSS filtering is turned on. +************************************************* +Step 12: Check for potential XSS issues with URIs +************************************************* + +The :doc:`URI Library <../libraries/uri>` used to automatically convert +a certain set of "programmatic characters" to HTML entities when they +are encountered in a URI segment. + +This was aimed at providing some automatic XSS prodection, in addition +to the ``$config['permitted_uri_chars']`` setting, but has proven to be +problematic and is now removed in CodeIgniter 3.0. + +If your application has relied on this feature, you should update it to +filter URI segments through ``$this->security->xss_clean()`` whenever you +output them. + ******************************************************** -Step 12: Update usage of Input Class's get_post() method +Step 13: Update usage of Input Class's get_post() method ******************************************************** Previously, the :doc:`Input Class <../libraries/input>` method ``get_post()`` @@ -235,14 +251,14 @@ A method has been added, ``post_get()``, which searches in POST then in GET, as ``get_post()`` was doing before. *********************************************************************** -Step 13: Update usage of Directory Helper's directory_map() function +Step 14: 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 14: Update usage of Database Forge's drop_table() method +Step 15: 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 @@ -264,7 +280,7 @@ If your application relies on IF EXISTS, you'll have to change its usage. all drivers with the exception of ODBC. *********************************************************** -Step 15: Change usage of Email library with multiple emails +Step 16: Change usage of Email library with multiple emails *********************************************************** The :doc:`Email Library <../libraries/email>` will automatically clear the @@ -279,7 +295,7 @@ pass FALSE as the first parameter in the ``send()`` method: } *************************************************** -Step 16: Update your Form_validation language lines +Step 17: Update your Form_validation language lines *************************************************** Two improvements have been made to the :doc:`Form Validation Library @@ -310,7 +326,7 @@ files and error messages format: later. **************************************************************** -Step 17: Remove usage of (previously) deprecated functionalities +Step 18: Remove usage of (previously) deprecated functionalities **************************************************************** In addition to the ``$autoload['core']`` configuration setting, there's a |