Merge pull request from GHSA-39fp-mqmm-gxj6

fix: Language, Router, and Filters
codeigniter4 · Mar 29, 2024 · fa851ac · fa851ac
2 parents 404e50b + a3572a7
commit fa851ac
Show file tree

Hide file tree

Showing 16 changed files with 327 additions and 24 deletions.
diff --git 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[<permittedURIChars>]+\z/iu'
+    |
+    | DO NOT CHANGE THIS UNLESS YOU FULLY UNDERSTAND THE REPERCUSSIONS!!
+    |
+    */
+    public string $permittedURIChars = 'a-z 0-9~%.:_\-';
+
     /**
      * --------------------------------------------------------------------------
      * Default Locale

diff --git 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[] = [

diff --git 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
@@ -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/system/HTTP/Exceptions/BadRequestException.php b/system/HTTP/Exceptions/BadRequestException.php
@@ -0,0 +1,28 @@
+<?php
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <[email protected]>
+ *
+ * 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/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,33 @@ 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(
+                ', ',
+                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: ' . $argsUrlEncoded . '),'
+                . ' 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/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 = [];
@@ -433,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)
                     );
                 }
@@ -487,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));
@@ -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/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/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',
@@ -484,18 +484,30 @@ public static function providePathGetsFiltered(): iterable
                 './path/to/nowhere',
                 '/path/to/nowhere',
             ],
-            'start-double' => [
+            'start-double-dot' => [
                 '../path/to/nowhere',
                 '/path/to/nowhere',
             ],
-            'decoded' => [
-                '../%41path',
+            'decode-percent-encoded-chars' => [
+                '/%41path',
                 '/Apath',
             ],
-            'encoded' => [
+            'decode-slash' => [
+                '/a%2Fb',
+                '/a/b',
+            ],
+            'encode-unreserved-chars' => [
                 '/path^here',
                 '/path%5Ehere',
             ],
+            'encode-multibyte-chars' => [
+                '/あいう',
+                '/%E3%81%82%E3%81%84%E3%81%86',
+            ],
+            'encode-invalid-percent-encoding' => [
+                '/pa%2-th',
+                '/pa%252-th',
+            ],
         ];
     }
 

diff --git 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);
     }
 
     /**