Merge remote-tracking branch 'origin/develop' into 4.5

Conflicts: system/CodeIgniter.php system/Filters/Filters.php system/Language/Language.php system/Router/Router.php
codeigniter4 · Mar 29, 2024 · 96ce795 · 96ce795
2 parents 5150c45 + 3503b4d
commit 96ce795
Show file tree

Hide file tree

Showing 19 changed files with 380 additions and 46 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,38 @@
 # Changelog
 
+## [v4.4.7](https://github.com/codeigniter4/CodeIgniter4/tree/v4.4.7) (2024-03-29)
+[Full Changelog](https://github.com/codeigniter4/CodeIgniter4/compare/v4.4.6...v4.4.7)
+
+### SECURITY
+
+* **Language:** *Language class DoS Vulnerability* was fixed. See the
+  [Security advisory](https://github.com/codeigniter4/CodeIgniter4/security/advisories/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
+  a setting to enable it.
+* **Filters:** A bug where URI paths processed by Filters were not URL-decoded
+  has been fixed.
+
+### Breaking Changes
+* fix: Time::difference() DST bug by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8661
+
+### Fixed Bugs
+* fix: [Validation] FileRules cause error if getimagesize() returns false by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8592
+* fix: isWriteType() to recognize CTE; always excluding RETURNING by @markconnellypro in https://github.com/codeigniter4/CodeIgniter4/pull/8599
+* fix: duplicate Cache-Control header with Session by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8601
+* fix: [DebugBar] scroll to top by @ddevsr in https://github.com/codeigniter4/CodeIgniter4/pull/8595
+* fix: Model::shouldUpdate() logic by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8614
+* fix: esc() for 'raw' context by @Cleric-K in https://github.com/codeigniter4/CodeIgniter4/pull/8633
+* docs: fix incorrect CURLRequest allow_redirects description by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8653
+* fix: Model::set() does not accept object by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8670
+
+### Refactoring
+* refactor: replace PHP_VERSION by PHP_VERSION_ID by @justbyitself in https://github.com/codeigniter4/CodeIgniter4/pull/8618
+* refactor: apply early return pattern by @justbyitself in https://github.com/codeigniter4/CodeIgniter4/pull/8621
+* refactor: move footer info to top in error_exception.php by @kenjis in https://github.com/codeigniter4/CodeIgniter4/pull/8626
+
 ## [v4.4.6](https://github.com/codeigniter4/CodeIgniter4/tree/v4.4.6) (2024-02-24)
 [Full Changelog](https://github.com/codeigniter4/CodeIgniter4/compare/v4.4.5...v4.4.6)
 

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/phpdoc.dist.xml b/phpdoc.dist.xml
@@ -10,7 +10,7 @@
         <output>api/build/</output>
         <cache>api/cache/</cache>
     </paths>
-    <version number="4.4.6">
+    <version number="4.4.7">
         <api format="php">
             <source dsn=".">
                 <path>system</path>

diff --git a/phpstan-baseline.php b/phpstan-baseline.php
@@ -13428,7 +13428,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
@@ -56,7 +56,7 @@ class CodeIgniter
     /**
      * The current version of CodeIgniter Framework
      */
-    public const CI_VERSION = '4.4.6';
+    public const CI_VERSION = '4.4.7';
 
     /**
      * App startup time.
@@ -456,6 +456,7 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
 
         $routeFilters = $this->tryToRouteIt($routes);
 
+        // $uri is URL-encoded.
         $uri = $this->request->getPath();
 
         if ($this->enableFilters) {
@@ -823,6 +824,7 @@ protected function tryToRouteIt(?RouteCollectionInterface $routes = null)
         // $routes is defined in Config/Routes.php
         $this->router = Services::router($routes, $this->request);
 
+        // $uri is URL-encoded.
         $uri = $this->request->getPath();
 
         $this->outputBufferingStart();

diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php
@@ -384,6 +384,9 @@ public function initialize(?string $uri = null)
             return $this;
         }
 
+        // Decode URL-encoded string
+        $uri = urldecode($uri);
+
         $oldFilterOrder = config(Feature::class)->oldFilterOrder ?? false;
         if ($oldFilterOrder) {
             $this->processGlobals($uri);
@@ -841,7 +844,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
@@ -854,7 +857,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 <admin@codeigniter.com>
+ *
+ * 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
@@ -13,7 +13,7 @@
 
 namespace CodeIgniter\Language;
 
-use InvalidArgumentException;
+use IntlException;
 use MessageFormatter;
 
 /**
@@ -195,9 +195,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
@@ -15,6 +15,7 @@
 
 use Closure;
 use CodeIgniter\Exceptions\PageNotFoundException;
+use CodeIgniter\HTTP\Exceptions\BadRequestException;
 use CodeIgniter\HTTP\Exceptions\RedirectException;
 use CodeIgniter\HTTP\Method;
 use CodeIgniter\HTTP\Request;
@@ -130,11 +131,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
@@ -187,6 +200,8 @@ public function handle(?string $uri = null)
         // Decode URL-encoded string
         $uri = urldecode($uri);
 
+        $this->checkDisallowedChars($uri);
+
         // Restart filterInfo
         $this->filtersInfo = [];
 
@@ -424,7 +439,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)
                     );
                 }
@@ -484,7 +499,7 @@ protected function checkRoutes(string $uri): bool
 
                     if (config(Routing::class)->multipleSegmentsOneParam === false) {
                         // Using back-references
-                        $segments = explode('/', preg_replace('#^' . $routeKey . '$#u', $handler, $uri));
+                        $segments = explode('/', preg_replace('#\A' . $routeKey . '\z#u', $handler, $uri));
                     } else {
                         if (str_contains($methodAndParams, '/')) {
                             [$method, $handlerParams] = explode('/', $methodAndParams, 2);
@@ -708,4 +723,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
@@ -1058,6 +1058,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
@@ -475,8 +475,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',
@@ -486,18 +486,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',
+            ],
         ];
     }