From 4a58d24096263180cc99b3eb60364b37c9ae43d4 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 15 Mar 2024 11:37:16 +0900 Subject: [PATCH 01/12] test: add tests for URI paths --- tests/system/HTTP/URITest.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/system/HTTP/URITest.php b/tests/system/HTTP/URITest.php index 1e1fbc4d1a87..f644c13995ef 100644 --- a/tests/system/HTTP/URITest.php +++ b/tests/system/HTTP/URITest.php @@ -484,18 +484,26 @@ public static function providePathGetsFiltered(): iterable './path/to/nowhere', '/path/to/nowhere', ], - 'start-double' => [ + 'start-double-dot' => [ '../path/to/nowhere', '/path/to/nowhere', ], 'decoded' => [ - '../%41path', + '/%41path', '/Apath', ], - 'encoded' => [ + 'encode-unreserved-chars' => [ '/path^here', '/path%5Ehere', ], + 'encode-invalid-percent-encoding' => [ + '/pa%2-th', + '/pa%252-th', + ], + 'encode-multibyte-chars' => [ + '/あいう', + '/%E3%81%82%E3%81%84%E3%81%86', + ], ]; } From 7f0cab1e6cd22cac0ab2c6cec85111f5a816d4ae Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 15 Mar 2024 14:33:58 +0900 Subject: [PATCH 02/12] fix: Language does not throw exception We have code like this: throw PageNotFoundException::forMethodNotFound($this->method); and public static function forMethodNotFound(string $method) { return new static(self::lang('HTTP.methodNotFound', [$method])); } If Language throws an Exception, the first Exception will be hidden in the error message. --- system/Language/Language.php | 23 ++++++++++++++++++++--- tests/system/Language/LanguageTest.php | 15 +++++---------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/system/Language/Language.php b/system/Language/Language.php index 6d4105861fd7..873da415a0fc 100644 --- a/system/Language/Language.php +++ b/system/Language/Language.php @@ -12,7 +12,7 @@ namespace CodeIgniter\Language; use Config\Services; -use InvalidArgumentException; +use IntlException; use MessageFormatter; /** @@ -194,9 +194,26 @@ protected function formatMessage($message, array $args = []) $formatted = MessageFormatter::formatMessage($this->locale, $message, $args); if ($formatted === false) { - throw new InvalidArgumentException( - lang('Language.invalidMessageFormat', [$message, implode(',', $args)]) + // Format again to get the error message. + try { + $fmt = new MessageFormatter($this->locale, $message); + $formatted = $fmt->format($args); + $fmtError = '"' . $fmt->getErrorMessage() . '" (' . $fmt->getErrorCode() . ')'; + } catch (IntlException $e) { + $fmtError = '"' . $e->getMessage() . '" (' . $e->getCode() . ')'; + } + + $argsString = implode(',', $args); + + log_message( + 'error', + 'Language.invalidMessageFormat: $message: "' . $message + . '", $args: "' . $argsString . '"' + . ' (urlencoded: ' . rawurlencode($argsString) . '),' + . ' MessageFormatter Error: ' . $fmtError ); + + return $message . "\n【Warning】Also, invalid string(s) was passed to the Language class. See log file for details."; } return $formatted; diff --git a/tests/system/Language/LanguageTest.php b/tests/system/Language/LanguageTest.php index b235ebfa6d41..6419d7cdf4d3 100644 --- a/tests/system/Language/LanguageTest.php +++ b/tests/system/Language/LanguageTest.php @@ -14,7 +14,6 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockLanguage; use Config\Services; -use InvalidArgumentException; use MessageFormatter; use Tests\Support\Language\SecondMockLanguage; @@ -137,18 +136,14 @@ public function testGetLineInvalidFormatMessage(): void $this->markTestSkipped('No intl support.'); } - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage( - 'Invalid message format: "تم الكشف عن كلمة المرور {0} بسبب اختراق البيانات وشوهدت {1 ، عدد} مرة في {2} في كلمات المرور المخترقة.", args: "password,hits,wording"' - ); - $this->lang->setLocale('ar'); - $this->lang->setData('Auth', [ - 'errorPasswordPwned' => 'تم الكشف عن كلمة المرور {0} بسبب اختراق البيانات وشوهدت {1 ، عدد} مرة في {2} في كلمات المرور المخترقة.', - ]); + $line = 'تم الكشف عن كلمة المرور {0} بسبب اختراق البيانات وشوهدت {1 ، عدد} مرة في {2} في كلمات المرور المخترقة.'; + $this->lang->setData('Auth', ['errorPasswordPwned' => $line]); + + $output = $this->lang->getLine('Auth.errorPasswordPwned', ['password', 'hits', 'wording']); - $this->lang->getLine('Auth.errorPasswordPwned', ['password', 'hits', 'wording']); + $this->assertSame($line . "\n【Warning】Also, invalid string(s) was passed to the Language class. See log file for details.", $output); } /** From 4d6b45b76989404bf1a9f6459f5cba2a3bda911f Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 15 Mar 2024 14:23:30 +0900 Subject: [PATCH 03/12] fix: add missing URI Security and Conig\App::$permittedURIChars --- app/Config/App.php | 24 ++++++++++++++ .../HTTP/Exceptions/BadRequestException.php | 28 ++++++++++++++++ system/Router/Router.php | 31 +++++++++++++++++ tests/system/Router/RouterTest.php | 18 ++++++++++ user_guide_src/source/changelogs/v4.4.7.rst | 9 +++++ user_guide_src/source/concepts/security.rst | 1 + user_guide_src/source/general/urls.rst | 33 +++++++++++++++++++ .../source/installation/upgrade_447.rst | 15 +++++++++ 8 files changed, 159 insertions(+) create mode 100644 system/HTTP/Exceptions/BadRequestException.php diff --git a/app/Config/App.php b/app/Config/App.php index 21b4df205286..b761da772ad1 100644 --- a/app/Config/App.php +++ b/app/Config/App.php @@ -59,6 +59,30 @@ class App extends BaseConfig */ public string $uriProtocol = 'REQUEST_URI'; + /* + |-------------------------------------------------------------------------- + | Allowed URL Characters + |-------------------------------------------------------------------------- + | + | 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~%.:_-` + | + | Set an empty string to allow all characters -- but only if you are insane. + | + | The configured value is actually a regular expression character group + | and it will be used as: '/\A[]+\z/iu' + | + | DO NOT CHANGE THIS UNLESS YOU FULLY UNDERSTAND THE REPERCUSSIONS!! + | + */ + public string $permittedURIChars = 'a-z 0-9~%.:_\-'; + /** * -------------------------------------------------------------------------- * Default Locale diff --git a/system/HTTP/Exceptions/BadRequestException.php b/system/HTTP/Exceptions/BadRequestException.php new file mode 100644 index 000000000000..e68e14f053e7 --- /dev/null +++ b/system/HTTP/Exceptions/BadRequestException.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\HTTP\Exceptions; + +use CodeIgniter\Exceptions\HTTPExceptionInterface; +use RuntimeException; + +/** + * 400 Bad Request + */ +class BadRequestException extends RuntimeException implements HTTPExceptionInterface +{ + /** + * HTTP status code for Bad Request + * + * @var int + */ + protected $code = 400; // @phpstan-ignore-line +} diff --git a/system/Router/Router.php b/system/Router/Router.php index 8f03b4dcaad2..1f1008f7e655 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -13,6 +13,7 @@ use Closure; use CodeIgniter\Exceptions\PageNotFoundException; +use CodeIgniter\HTTP\Exceptions\BadRequestException; use CodeIgniter\HTTP\Exceptions\RedirectException; use CodeIgniter\HTTP\Request; use CodeIgniter\HTTP\ResponseInterface; @@ -120,11 +121,23 @@ class Router implements RouterInterface protected ?AutoRouterInterface $autoRouter = null; + /** + * Permitted URI chars + * + * The default value is `''` (do not check) for backward compatibility. + */ + protected string $permittedURIChars = ''; + /** * Stores a reference to the RouteCollection object. */ public function __construct(RouteCollectionInterface $routes, ?Request $request = null) { + $config = config(App::class); + if (isset($config->permittedURIChars)) { + $this->permittedURIChars = $config->permittedURIChars; + } + $this->collection = $routes; // These are only for auto-routing @@ -179,6 +192,8 @@ public function handle(?string $uri = null) // Decode URL-encoded string $uri = urldecode($uri); + $this->checkDisallowedChars($uri); + // Restart filterInfo $this->filterInfo = null; $this->filtersInfo = []; @@ -676,4 +691,20 @@ protected function setMatchedRoute(string $route, $handler): void $this->matchedRouteOptions = $this->collection->getRoutesOptions($route); } + + /** + * Checks disallowed characters + */ + private function checkDisallowedChars(string $uri): void + { + foreach (explode('/', $uri) as $segment) { + if ($segment !== '' && $this->permittedURIChars !== '' + && preg_match('/\A[' . $this->permittedURIChars . ']+\z/iu', $segment) !== 1 + ) { + throw new BadRequestException( + 'The URI you submitted has disallowed characters: "' . $segment . '"' + ); + } + } + } } diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index c5a05fb36ace..521dcdb68913 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -13,10 +13,12 @@ use CodeIgniter\Config\Services; use CodeIgniter\Exceptions\PageNotFoundException; +use CodeIgniter\HTTP\Exceptions\BadRequestException; use CodeIgniter\HTTP\Exceptions\RedirectException; use CodeIgniter\HTTP\IncomingRequest; use CodeIgniter\Router\Exceptions\RouterException; use CodeIgniter\Test\CIUnitTestCase; +use Config\App; use Config\Modules; use Config\Routing; use Tests\Support\Filters\Customfilter; @@ -87,6 +89,16 @@ public function testZeroAsURIPath(): void $router->handle('0'); } + public function testNotPermittedChars(): void + { + $router = new Router($this->collection, $this->request); + + $this->expectException(BadRequestException::class); + $this->expectExceptionMessage('The URI you submitted has disallowed characters: ""'); + + $router->handle('test/%3Ca%3E'); + } + public function testURIMapsToController(): void { $router = new Router($this->collection, $this->request); @@ -783,6 +795,9 @@ public function testAutoRouteMethodEmpty(): void */ public function testRegularExpressionWithUnicode(): void { + $config = config(App::class); + $config->permittedURIChars = 'a-z 0-9~%.:_\-\x{0980}-\x{09ff}'; + $this->collection->get('news/([a-z0-9\x{0980}-\x{09ff}-]+)', 'News::view/$1'); $router = new Router($this->collection, $this->request); @@ -802,6 +817,9 @@ public function testRegularExpressionWithUnicode(): void */ public function testRegularExpressionPlaceholderWithUnicode(): void { + $config = config(App::class); + $config->permittedURIChars = 'a-z 0-9~%.:_\-\x{0980}-\x{09ff}'; + $this->collection->addPlaceholder('custom', '[a-z0-9\x{0980}-\x{09ff}-]+'); $this->collection->get('news/(:custom)', 'News::view/$1'); diff --git a/user_guide_src/source/changelogs/v4.4.7.rst b/user_guide_src/source/changelogs/v4.4.7.rst index 93193d0a1cc9..6e09061c5178 100644 --- a/user_guide_src/source/changelogs/v4.4.7.rst +++ b/user_guide_src/source/changelogs/v4.4.7.rst @@ -10,6 +10,15 @@ Release Date: Unreleased :local: :depth: 3 +******** +SECURITY +******** + +- **URI Security:** The feature to check if URIs do not contain not permitted + strings has been added. This check is equivalent to the URI Security found in + CodeIgniter 3. This is enabled by default, but upgraded users need to add + a setting to enable it. See :ref:`urls-uri-security` for details. + ******** BREAKING ******** diff --git a/user_guide_src/source/concepts/security.rst b/user_guide_src/source/concepts/security.rst index 208131703f74..df1bd2b34827 100644 --- a/user_guide_src/source/concepts/security.rst +++ b/user_guide_src/source/concepts/security.rst @@ -38,6 +38,7 @@ OWASP recommendations CodeIgniter provisions ====================== +- :ref:`urls-uri-security` - :ref:`invalidchars` filter - :doc:`../libraries/validation` library - :doc:`HTTP library <../incoming/incomingrequest>` provides for :ref:`input field filtering ` & content metadata diff --git a/user_guide_src/source/general/urls.rst b/user_guide_src/source/general/urls.rst index 963b7706e9b7..afbc1316986a 100644 --- a/user_guide_src/source/general/urls.rst +++ b/user_guide_src/source/general/urls.rst @@ -58,6 +58,39 @@ Route path /blog/news/2022/10 The URI path relative to the Bas Query page=2 ========== ==================================== ========================================= +.. _urls-uri-security: + +URI Security +============ + +.. versionadded:: 4.4.7 + +.. important:: + Users upgrading from versions prior to v4.4.7 will need to add the following + to **app/Config/App.php** in order to use this feature:: + + public string $permittedURIChars = 'a-z 0-9~%.:_\-'; + +CodeIgniter is fairly restrictive regarding which characters it allows in your +URI strings (Route path) in order to help minimize the possibility that malicious +data can be passed to your application. URIs may only contain the following: + +- Alpha-numeric text (latin characters only) +- Tilde: ``~`` +- Percent sign: ``%`` +- Period: ``.`` +- Colon: ``:`` +- Underscore: ``_`` +- Dash: ``-`` +- Space: `` `` + +This setting can be changed by ``Config\App::$permittedURIChars``. + +.. note:: + This check is performed by the ``Router``. The Router takes the URL-encoded + value held by the ``SiteURI`` class, decodes it, and then checks that it + does not contain not permitted strings. + .. _urls-remove-index-php: Removing the index.php file diff --git a/user_guide_src/source/installation/upgrade_447.rst b/user_guide_src/source/installation/upgrade_447.rst index dbb679a3ad2d..9a28c06bb543 100644 --- a/user_guide_src/source/installation/upgrade_447.rst +++ b/user_guide_src/source/installation/upgrade_447.rst @@ -16,6 +16,18 @@ Please refer to the upgrade instructions corresponding to your installation meth Mandatory File Changes ********************** +URI Security +============ + +The feature to check if URIs do not contain not permitted strings has been added. +This check is equivalent to the URI Security found in CodeIgniter 3. + +We recommend you enable this feature. Add the following to **app/Config/App.php**:: + + public string $permittedURIChars = 'a-z 0-9~%.:_\-';. + +See :ref:`urls-uri-security` for details. + Error Files =========== @@ -66,6 +78,9 @@ and it is recommended that you merge the updated versions with your application: Config ------ +- app/Config/App.php + - The property ``$permittedURIChars`` was added. See :ref:`urls-uri-security` + for details. - @TODO All Changes From 99daaa449b03b4d4edc8c4dff907526fcacbff01 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 17 Mar 2024 16:52:21 +0900 Subject: [PATCH 04/12] docs: add link to Security Advisory on GitHub --- user_guide_src/source/changelogs/v4.4.7.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.4.7.rst b/user_guide_src/source/changelogs/v4.4.7.rst index 6e09061c5178..7f6c5d2ab146 100644 --- a/user_guide_src/source/changelogs/v4.4.7.rst +++ b/user_guide_src/source/changelogs/v4.4.7.rst @@ -14,6 +14,9 @@ Release Date: Unreleased SECURITY ******** +- *Language class DoS Vulnerability* was fixed. + See the `Security advisory GHSA-39fp-mqmm-gxj6 `_ + for more information. - **URI Security:** The feature to check if URIs do not contain not permitted strings has been added. This check is equivalent to the URI Security found in CodeIgniter 3. This is enabled by default, but upgraded users need to add From 1c65aa11c9144c7abc0f54cee353fd769e19a8b6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 22 Mar 2024 10:07:33 +0900 Subject: [PATCH 05/12] fix: improve log message --- system/Language/Language.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/system/Language/Language.php b/system/Language/Language.php index 873da415a0fc..6e41285e90a7 100644 --- a/system/Language/Language.php +++ b/system/Language/Language.php @@ -203,13 +203,20 @@ protected function formatMessage($message, array $args = []) $fmtError = '"' . $e->getMessage() . '" (' . $e->getCode() . ')'; } - $argsString = implode(',', $args); + $argsString = implode( + ', ', + array_map(static fn ($element) => '"' . $element . '"', $args) + ); + $argsUrlEncoded = implode( + ', ', + array_map(static fn ($element) => '"' . rawurlencode($element) . '"', $args) + ); log_message( 'error', 'Language.invalidMessageFormat: $message: "' . $message - . '", $args: "' . $argsString . '"' - . ' (urlencoded: ' . rawurlencode($argsString) . '),' + . '", $args: ' . $argsString + . ' (urlencoded: ' . $argsUrlEncoded . '),' . ' MessageFormatter Error: ' . $fmtError ); From 03ec3ff566fefcdbb17be2fea2f9c718ecdd64ff Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 24 Mar 2024 09:43:39 +0900 Subject: [PATCH 06/12] refactor: use \A and \z It is better to use always \A and \z if you try to check a string. Because `$` matches the end of a line, and matches a newline charactor. --- system/Router/Router.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Router/Router.php b/system/Router/Router.php index 1f1008f7e655..3f1ffc20a979 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -448,7 +448,7 @@ protected function checkRoutes(string $uri): bool }, is_array($handler) ? key($handler) : $handler); throw new RedirectException( - preg_replace('#^' . $routeKey . '$#u', $redirectTo, $uri), + preg_replace('#\A' . $routeKey . '\z#u', $redirectTo, $uri), $this->collection->getRedirectCode($routeKey) ); } @@ -502,7 +502,7 @@ protected function checkRoutes(string $uri): bool } // Using back-references - $handler = preg_replace('#^' . $routeKey . '$#u', $handler, $uri); + $handler = preg_replace('#\A' . $routeKey . '\z#u', $handler, $uri); } $this->setRequest(explode('/', $handler)); From ecc3a1e175cd7b426552fca0281fbf9b540066c6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 24 Mar 2024 10:38:41 +0900 Subject: [PATCH 07/12] docs: add sub section "Adding Permitted Characters" --- user_guide_src/source/general/urls.rst | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/user_guide_src/source/general/urls.rst b/user_guide_src/source/general/urls.rst index afbc1316986a..8c869cd0818f 100644 --- a/user_guide_src/source/general/urls.rst +++ b/user_guide_src/source/general/urls.rst @@ -84,13 +84,22 @@ data can be passed to your application. URIs may only contain the following: - Dash: ``-`` - Space: `` `` -This setting can be changed by ``Config\App::$permittedURIChars``. - .. note:: This check is performed by the ``Router``. The Router takes the URL-encoded value held by the ``SiteURI`` class, decodes it, and then checks that it does not contain not permitted strings. +Adding Permitted Characters +--------------------------- + +The permitted characters can be changed by ``Config\App::$permittedURIChars``. + +If you want to use Unicode for URI paths, modify it to allow the characters to +be used. For example, if you want to use Bengali, you will need to set the +following value in **app/Config/App.php**:: + + public string $permittedURIChars = 'a-z 0-9~%.:_\-\x{0980}-\x{09ff}'; + .. _urls-remove-index-php: Removing the index.php file From 7ef2161ccf08ed8eb2dc9abf4a99b61f26ebfae9 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 24 Mar 2024 10:47:23 +0900 Subject: [PATCH 08/12] docs: add `**Language:**` for consistency --- user_guide_src/source/changelogs/v4.4.7.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/user_guide_src/source/changelogs/v4.4.7.rst b/user_guide_src/source/changelogs/v4.4.7.rst index 7f6c5d2ab146..94d5a7bd5703 100644 --- a/user_guide_src/source/changelogs/v4.4.7.rst +++ b/user_guide_src/source/changelogs/v4.4.7.rst @@ -14,7 +14,7 @@ Release Date: Unreleased SECURITY ******** -- *Language class DoS Vulnerability* was fixed. +- **Language:** *Language class DoS Vulnerability* was fixed. See the `Security advisory GHSA-39fp-mqmm-gxj6 `_ for more information. - **URI Security:** The feature to check if URIs do not contain not permitted From 01a562423827ce2d143ac227287890b8340485a1 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 24 Mar 2024 08:50:23 +0900 Subject: [PATCH 09/12] test: add test for URI path --- tests/system/HTTP/URITest.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/system/HTTP/URITest.php b/tests/system/HTTP/URITest.php index f644c13995ef..95ba148a05eb 100644 --- a/tests/system/HTTP/URITest.php +++ b/tests/system/HTTP/URITest.php @@ -473,8 +473,8 @@ public static function providePathGetsFiltered(): iterable { return [ 'dot-segment' => [ - '/./path/to/nowhere', - '/path/to/nowhere', + '/./path/to/nowhere', // path + '/path/to/nowhere', // expectedPath ], 'double-dots' => [ '/../path/to/nowhere', @@ -488,22 +488,26 @@ public static function providePathGetsFiltered(): iterable '../path/to/nowhere', '/path/to/nowhere', ], - 'decoded' => [ + 'decode-percent-encoded-chars' => [ '/%41path', '/Apath', ], + 'decode-slash' => [ + '/a%2Fb', + '/a/b', + ], 'encode-unreserved-chars' => [ '/path^here', '/path%5Ehere', ], - 'encode-invalid-percent-encoding' => [ - '/pa%2-th', - '/pa%252-th', - ], 'encode-multibyte-chars' => [ '/あいう', '/%E3%81%82%E3%81%84%E3%81%86', ], + 'encode-invalid-percent-encoding' => [ + '/pa%2-th', + '/pa%252-th', + ], ]; } From 8745b20dbd9b7ea1f5394c5ed88e47edbe137699 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 24 Mar 2024 09:07:47 +0900 Subject: [PATCH 10/12] fix: Filters does not decode URI path Router does decode it, so Filters also should decode. --- system/CodeIgniter.php | 2 + system/Filters/Filters.php | 7 ++- tests/system/Filters/FiltersTest.php | 46 +++++++++++++++++++ user_guide_src/source/changelogs/v4.4.7.rst | 2 + user_guide_src/source/incoming/filters.rst | 10 ++++ .../source/installation/upgrade_447.rst | 34 ++++++++++++++ 6 files changed, 99 insertions(+), 2 deletions(-) diff --git a/system/CodeIgniter.php b/system/CodeIgniter.php index be917d2f494c..bfc1154e9ff3 100644 --- a/system/CodeIgniter.php +++ b/system/CodeIgniter.php @@ -449,6 +449,7 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache $routeFilter = $this->tryToRouteIt($routes); + // $uri is URL-encoded. $uri = $this->determinePath(); if ($this->enableFilters) { @@ -813,6 +814,7 @@ protected function tryToRouteIt(?RouteCollectionInterface $routes = null) // $routes is defined in Config/Routes.php $this->router = Services::router($routes, $this->request); + // $path is URL-encoded. $path = $this->determinePath(); $this->benchmark->stop('bootstrap'); diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php index e7cabc87435d..c3e5aa5cbfaa 100644 --- a/system/Filters/Filters.php +++ b/system/Filters/Filters.php @@ -245,6 +245,9 @@ public function initialize(?string $uri = null) return $this; } + // Decode URL-encoded string + $uri = urldecode($uri); + $this->processGlobals($uri); $this->processMethods(); $this->processFilters($uri); @@ -639,7 +642,7 @@ private function checkExcept(string $uri, $paths): bool /** * Check the URI path as pseudo-regex * - * @param string $uri URI path relative to baseURL (all lowercase) + * @param string $uri URI path relative to baseURL (all lowercase, URL-decoded) * @param array $paths The except path patterns */ private function checkPseudoRegex(string $uri, array $paths): bool @@ -652,7 +655,7 @@ private function checkPseudoRegex(string $uri, array $paths): bool $path = strtolower(str_replace('*', '.*', $path)); // Does this rule apply here? - if (preg_match('#^' . $path . '$#', $uri, $match) === 1) { + if (preg_match('#\A' . $path . '\z#u', $uri, $match) === 1) { return true; } } diff --git a/tests/system/Filters/FiltersTest.php b/tests/system/Filters/FiltersTest.php index 8e80e4d5de1d..ff63d577e0da 100644 --- a/tests/system/Filters/FiltersTest.php +++ b/tests/system/Filters/FiltersTest.php @@ -1056,6 +1056,52 @@ public function testMatchesURICaseInsensitively(): void $this->assertSame($expected, $filters->initialize($uri)->getFilters()); } + public function testMatchesURIWithUnicode(): void + { + $_SERVER['REQUEST_METHOD'] = 'GET'; + + $config = [ + 'aliases' => [ + 'foo' => '', + 'bar' => '', + 'frak' => '', + 'baz' => '', + ], + 'globals' => [ + 'before' => [ + 'foo' => ['except' => '日本語/*'], + 'bar', + ], + 'after' => [ + 'foo' => ['except' => '日本語/*'], + 'baz', + ], + ], + 'filters' => [ + 'frak' => [ + 'before' => ['日本語/*'], + 'after' => ['日本語/*'], + ], + ], + ]; + $filtersConfig = $this->createConfigFromArray(FiltersConfig::class, $config); + $filters = $this->createFilters($filtersConfig); + + // URIs passed to Filters are URL-encoded. + $uri = '%E6%97%A5%E6%9C%AC%E8%AA%9E/foo/bar'; + $expected = [ + 'before' => [ + 'bar', + 'frak', + ], + 'after' => [ + 'baz', + 'frak', + ], + ]; + $this->assertSame($expected, $filters->initialize($uri)->getFilters()); + } + /** * @see https://github.com/codeigniter4/CodeIgniter4/issues/1907 */ diff --git a/user_guide_src/source/changelogs/v4.4.7.rst b/user_guide_src/source/changelogs/v4.4.7.rst index 94d5a7bd5703..31ecb0cda2ab 100644 --- a/user_guide_src/source/changelogs/v4.4.7.rst +++ b/user_guide_src/source/changelogs/v4.4.7.rst @@ -21,6 +21,8 @@ SECURITY strings has been added. This check is equivalent to the URI Security found in CodeIgniter 3. This is enabled by default, but upgraded users need to add a setting to enable it. See :ref:`urls-uri-security` for details. +- **Filters:** A bug where URI paths processed by Filters were not URL-decoded + has been fixed. See :ref:`upgrade-447-filter-paths` for details. ******** BREAKING diff --git a/user_guide_src/source/incoming/filters.rst b/user_guide_src/source/incoming/filters.rst index cee8515dcd54..138bb03116e1 100644 --- a/user_guide_src/source/incoming/filters.rst +++ b/user_guide_src/source/incoming/filters.rst @@ -140,6 +140,11 @@ an array with the ``except`` key and a URI path (relative to BaseURL) to match a .. literalinclude:: filters/006.php +.. Warning:: Prior to v4.4.7, due to a bug, the URI paths processed by the filter + were not URL-decoded. In other words, the URI paths specified in the routing + and the URI paths specified in the filter could be different. + See :ref:`upgrade-447-filter-paths` for details. + Any place you can use a URI path (relative to BaseURL) in the filter settings, you can use a regular expression or, like in this example, use an asterisk (``*``) for a wildcard that will match all characters after that. In this example, any URI path starting with ``api/`` would be exempted from CSRF protection, but the site's forms would all be protected. @@ -175,6 +180,11 @@ a list of URI path (relative to BaseURL) patterns that filter should apply to: .. literalinclude:: filters/009.php +.. Warning:: Prior to v4.4.7, due to a bug, the URI paths processed by the filter + were not URL-decoded. In other words, the URI paths specified in the routing + and the URI paths specified in the filter could be different. + See :ref:`upgrade-447-filter-paths` for details. + .. _filters-filters-filter-arguments: Filter Arguments diff --git a/user_guide_src/source/installation/upgrade_447.rst b/user_guide_src/source/installation/upgrade_447.rst index 9a28c06bb543..53ad1974fb5d 100644 --- a/user_guide_src/source/installation/upgrade_447.rst +++ b/user_guide_src/source/installation/upgrade_447.rst @@ -40,6 +40,40 @@ The error page has been updated. Please update the following files: Breaking Changes **************** +.. _upgrade-447-filter-paths: + +Paths in Controller Filters +=========================== + +A bug where URI paths processed by :doc:`../incoming/filters` were not URL-decoded has been fixed. + +.. note:: Note that :doc:`Router <../incoming/routing>` processes URL-decoded URI paths. + +``Config\Filters`` has some places to specify the URI paths. If the paths have +different values when URL-decoded, change them to the URL-decoded values. + +E.g.,: + +.. code-block:: php + + public array $globals = [ + 'before' => [ + 'csrf' => ['except' => '%E6%97%A5%E6%9C%AC%E8%AA%9E/*'], + ], + // ... + ]; + +↓ + +.. code-block:: php + + public array $globals = [ + 'before' => [ + 'csrf' => ['except' => '日本語/*'], + ], + // ... + ]; + Time::difference() and DST ========================== From eaecd084688a39796b1b5755db63ee163137440b Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Mar 2024 09:53:52 +0900 Subject: [PATCH 11/12] =?UTF-8?q?docs:=20add=20link=20to=20Wikipedia?= =?UTF-8?q?=E2=80=99s=20Unicode=20block?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- user_guide_src/source/general/urls.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/user_guide_src/source/general/urls.rst b/user_guide_src/source/general/urls.rst index 8c869cd0818f..bdb10508393f 100644 --- a/user_guide_src/source/general/urls.rst +++ b/user_guide_src/source/general/urls.rst @@ -100,6 +100,10 @@ following value in **app/Config/App.php**:: public string $permittedURIChars = 'a-z 0-9~%.:_\-\x{0980}-\x{09ff}'; +A full list of Unicode ranges can be found at Wikipedia's `Unicode block`_. + +.. _Unicode block: https://en.wikipedia.org/wiki/Unicode_block + .. _urls-remove-index-php: Removing the index.php file From a3572a792274d0591896f8de4dce4c2c74b76ea9 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Mar 2024 15:50:31 +0900 Subject: [PATCH 12/12] chore: update phpstan-baseline.php --- phpstan-baseline.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpstan-baseline.php b/phpstan-baseline.php index dc1e4cbd9b05..3380da0b8733 100644 --- a/phpstan-baseline.php +++ b/phpstan-baseline.php @@ -13713,7 +13713,7 @@ ]; $ignoreErrors[] = [ 'message' => '#^Assigning \'GET\' directly on offset \'REQUEST_METHOD\' of \\$_SERVER is discouraged\\.$#', - 'count' => 35, + 'count' => 36, 'path' => __DIR__ . '/tests/system/Filters/FiltersTest.php', ]; $ignoreErrors[] = [