CI_URI changes related to the 'permitted_uri_chars' setting

 - Initialize and cache the value in the class constructor instead of searching for it every time
 - Removed the preg_quote() call from _filter_uri() to allow more fine-tuning from configuration
 - Renamed _filter_uri() to filter_uri() - it was public anyway and using it cannot break anything

Related: issue #2799

6 files changed, 52 insertions, 29 deletions

diff --git a/application/config/config.php b/application/config/config.php
index cd2ca479b..5240f6c26 100644
--- a/application/config/config.php
+++ b/application/config/config.php
@@ -141,15 +141,18 @@ $config['subclass_prefix'] = 'MY_';
 | Allowed URL Characters
 |--------------------------------------------------------------------------
 |
-| This lets you specify with a regular expression which characters are permitted
-| within your URLs.  When someone tries to submit a URL with disallowed
-| characters they will get a warning message.
+| This lets you specify which characters are permitted within your URLs.
+| When someone tries to submit a URL with disallowed characters they will
+| get a warning message.
 |
 | As a security measure you are STRONGLY encouraged to restrict URLs to
 | as few characters as possible.  By default only these are allowed: a-z 0-9~%.:_-
 |
 | Leave blank to allow all characters -- but only if you are insane.
 |
+| The configured value is actually a regular expression character group
+| and it will be executed as: ! preg_match('/^[<permitted_uri_chars>]+$/i
+|
 | DO NOT CHANGE THIS UNLESS YOU FULLY UNDERSTAND THE REPERCUSSIONS!!
 |
 */
diff --git a/system/core/Router.php b/system/core/Router.php
index cb44a3ce9..71530ff07 100644
--- a/system/core/Router.php
+++ b/system/core/Router.php
@@ -154,16 +154,16 @@ class CI_Router {
 		{
 			if (isset($_GET[$this->config->item('directory_trigger')]) && is_string($_GET[$this->config->item('directory_trigger')]))
 			{
-				$this->set_directory(trim($this->uri->_filter_uri($_GET[$this->config->item('directory_trigger')])));
+				$this->set_directory(trim($this->uri->filter_uri($_GET[$this->config->item('directory_trigger')])));
 				$segments[] = $this->directory;
 			}
 
-			$this->set_class(trim($this->uri->_filter_uri($_GET[$this->config->item('controller_trigger')])));
+			$this->set_class(trim($this->uri->filter_uri($_GET[$this->config->item('controller_trigger')])));
 			$segments[] = $this->class;
 
 			if ( ! empty($_GET[$this->config->item('function_trigger')]) && is_string($_GET[$this->config->item('function_trigger')]))
 			{
-				$this->set_method(trim($this->uri->_filter_uri($_GET[$this->config->item('function_trigger')])));
+				$this->set_method(trim($this->uri->filter_uri($_GET[$this->config->item('function_trigger')])));
 				$segments[] = $this->method;
 			}
 		}
diff --git a/system/core/URI.php b/system/core/URI.php
index 5e4c80a00..3d6d202c0 100644
--- a/system/core/URI.php
+++ b/system/core/URI.php
@@ -70,6 +70,15 @@ class CI_URI {
 	public $rsegments =	array();
 
 	/**
+	 * Permitted URI chars
+	 *
+	 * PCRE character group allowed in URI segments
+	 *
+	 * @var	string
+	 */
+	protected $_permitted_uri_chars;
+
+	/**
 	 * Class constructor
 	 *
 	 * Simply globalizes the $RTR object. The front
@@ -81,6 +90,12 @@ class CI_URI {
 	public function __construct()
 	{
 		$this->config =& load_class('Config', 'core');
+
+		if ($this->config->item('enable_query_strings') !== TRUE OR is_cli())
+		{
+			$this->_permitted_uri_chars = $this->config->item('permitted_uri_chars');
+		}
+
 		log_message('debug', 'URI Class Initialized');
 	}
 
@@ -303,23 +318,19 @@ class CI_URI {
 	 * @param	string	$str
 	 * @return	string
 	 */
-	public function _filter_uri($str)
+	public function filter_uri($str)
 	{
-		if ($str !== '' && $this->config->item('permitted_uri_chars') != '' && $this->config->item('enable_query_strings') === FALSE)
+		if ( ! empty($str) && ! empty($this->_permitted_uri_chars) && ! preg_match('/^['.$this->_permitted_uri_chars.']+$/i', $str))
 		{
-			// preg_quote() in PHP 5.3 escapes -, so the str_replace() and addition of - to preg_quote() is to maintain backwards
-			// compatibility as many are unaware of how characters in the permitted_uri_chars will be parsed as a regex pattern
-			if ( ! preg_match('|^['.str_replace(array('\\-', '\-'), '-', preg_quote($this->config->item('permitted_uri_chars'), '-')).']+$|i', $str))
-			{
-				show_error('The URI you submitted has disallowed characters.', 400);
-			}
+			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('&#36;', '&#40;', '&#41;', '&#40;', '&#41;'), // Good
-					$str);
+			array('$',     '(',     ')',     '%28',   '%29'),	// Bad
+			array('&#36;', '&#40;', '&#41;', '&#40;', '&#41;'),	// Good
+			$str
+		);
 	}
 
 	// --------------------------------------------------------------------
@@ -365,7 +376,7 @@ class CI_URI {
 		foreach (explode('/', preg_replace('|/*(.+?)/*$|', '\\1', $this->uri_string)) as $val)
 		{
 			// Filter segments for security
-			$val = trim($this->_filter_uri($val));
+			$val = trim($this->filter_uri($val));
 
 			if ($val !== '')
 			{
diff --git a/tests/codeigniter/core/URI_test.php b/tests/codeigniter/core/URI_test.php
index 7fa0e6265..99d79bbd2 100644
--- a/tests/codeigniter/core/URI_test.php
+++ b/tests/codeigniter/core/URI_test.php
@@ -112,11 +112,10 @@ class URI_test extends CI_TestCase {
 
 	public function test_filter_uri()
 	{
-		$this->uri->config->set_item('enable_query_strings', FALSE);
-		$this->uri->config->set_item('permitted_uri_chars', 'a-z 0-9~%.:_\-');
+		$this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-');
 
 		$str_in = 'abc01239~%.:_-';
-		$str = $this->uri->_filter_uri($str_in);
+		$str = $this->uri->filter_uri($str_in);
 
 		$this->assertEquals($str, $str_in);
 	}
@@ -126,11 +125,9 @@ class URI_test extends CI_TestCase {
 	public function test_filter_uri_escaping()
 	{
 		// ensure escaping even if dodgey characters are permitted
+		$this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-()$');
 
-		$this->uri->config->set_item('enable_query_strings', FALSE);
-		$this->uri->config->set_item('permitted_uri_chars', 'a-z 0-9~%.:_\-()$');
-
-		$str = $this->uri->_filter_uri('$destroy_app(foo)');
+		$str = $this->uri->filter_uri('$destroy_app(foo)');
 
 		$this->assertEquals($str, '&#36;destroy_app&#40;foo&#41;');
 	}
@@ -142,8 +139,8 @@ class URI_test extends CI_TestCase {
 		$this->setExpectedException('RuntimeException');
 
 		$this->uri->config->set_item('enable_query_strings', FALSE);
-		$this->uri->config->set_item('permitted_uri_chars', 'a-z 0-9~%.:_\-');
-		$this->uri->_filter_uri('$this()');
+		$this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-');
+		$this->uri->filter_uri('$this()');
 	}
 
 	// --------------------------------------------------------------------
diff --git a/tests/mocks/core/uri.php b/tests/mocks/core/uri.php
index 11078587b..96ec5afa1 100644
--- a/tests/mocks/core/uri.php
+++ b/tests/mocks/core/uri.php
@@ -10,12 +10,23 @@ class Mock_Core_URI extends CI_URI {
 		// set predictable config values
 		$test->ci_set_config(array(
 			'index_page'		=> 'index.php',
-			'base_url'			=> 'http://example.com/',
-			'subclass_prefix'	=> 'MY_'
+			'base_url'		=> 'http://example.com/',
+			'subclass_prefix'	=> 'MY_',
+			'enable_query_strings'	=> FALSE,
+			'permitted_uri_chars'	=> 'a-z 0-9~%.:_\-'
 		));
 
 		$this->config = new $cls;
 
+		if ($this->config->item('enable_query_strings') !== TRUE OR is_cli())
+		{
+			$this->_permitted_uri_chars = $this->config->item('permitted_uri_chars');
+		}
+	}
+
+	public function _set_permitted_uri_chars($value)
+	{
+		$this->_permitted_uri_chars = $value;
 	}
 
 }
\ No newline at end of file
diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst
index 43e7be41f..77378521b 100644
--- a/user_guide_src/source/changelog.rst
+++ b/user_guide_src/source/changelog.rst
@@ -391,6 +391,7 @@ Release Date: Not Released
 
    -  :doc:`URI Library <libraries/uri>` changes include:
 
+      -  Renamed method ``_filter_uri()`` to ``filter_uri()`` and removed the ``preg_quote()`` call from it.
       -  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()``.