diff --git a/system/Router/Router.php b/system/Router/Router.php index f0f9555ad1ac..5b6e877e7834 100644 --- a/system/Router/Router.php +++ b/system/Router/Router.php @@ -759,6 +759,14 @@ protected function setMatchedRoute(string $route, $handler): void */ private function checkDisallowedChars(string $uri): void { + // Decode the URI until it no longer changes. A URI can be encoded + // multiple times, and decoding it only once would allow a + // double-encoded URI to bypass this check when it is decoded again + // by another component. + while (($decoded = urldecode($uri)) !== $uri) { + $uri = $decoded; + } + foreach (explode('/', $uri) as $segment) { if ($segment !== '' && $this->permittedURIChars !== '' && preg_match('/\A[' . $this->permittedURIChars . ']+\z/iu', $segment) !== 1 diff --git a/tests/system/Router/RouterTest.php b/tests/system/Router/RouterTest.php index 6e22c95ed4f9..af1d99944bf9 100644 --- a/tests/system/Router/RouterTest.php +++ b/tests/system/Router/RouterTest.php @@ -120,6 +120,29 @@ public function testNotPermittedChars(): void $router->handle('test/%3Ca%3E'); } + public function testDoubleEncodedNotPermittedChars(): void + { + $router = new Router($this->collection, $this->request); + + $this->expectException(BadRequestException::class); + $this->expectExceptionMessage('The URI you submitted has disallowed characters: ""'); + + $router->handle('test/%253Ca%253E'); + } + + public function testPermittedCharsWithPercentSign(): void + { + $this->collection->get('encoded/(:any)', 'News::view/$1'); + + $router = new Router($this->collection, $this->request); + + $router->handle('encoded/100%25'); + + $this->assertSame('\News', $router->controllerName()); + $this->assertSame('view', $router->methodName()); + $this->assertSame(['100%'], $router->params()); + } + public function testURIMapsToController(): void { $router = new Router($this->collection, $this->request); diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 8739e9546d4c..9b7142387c68 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -38,6 +38,9 @@ Bugs Fixed is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. +- **Router:** Fixed a bug where a double-encoded URI could bypass the + disallowed character check when the URI was decoded again by another + component. - **Security:** Fixed a bug where a valid JSON request body that is not an object (for example a JSON array) was corrupted after CSRF verification when the token was sent via the ``X-CSRF-TOKEN`` header. diff --git a/user_guide_src/source/general/urls.rst b/user_guide_src/source/general/urls.rst index bdb10508393f..5681a5f039aa 100644 --- a/user_guide_src/source/general/urls.rst +++ b/user_guide_src/source/general/urls.rst @@ -86,8 +86,9 @@ data can be passed to your application. URIs may only contain the following: .. 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. + value held by the ``SiteURI`` class, decodes it repeatedly until it no + longer changes, and then checks that it does not contain not permitted + strings. Adding Permitted Characters ---------------------------