From bc1f27994006ca20d581f0d29cd9f7f0370c8a98 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 15 Aug 2023 19:12:00 +0900 Subject: [PATCH 01/10] test: add test for except empty array --- tests/system/Filters/FiltersTest.php | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/system/Filters/FiltersTest.php b/tests/system/Filters/FiltersTest.php index 5c0fc2efabdf..b78ea8b3a03d 100644 --- a/tests/system/Filters/FiltersTest.php +++ b/tests/system/Filters/FiltersTest.php @@ -639,6 +639,40 @@ public function testBeforeExceptInapplicable(): void $this->assertSame($expected, $filters->initialize($uri)->getFilters()); } + public function testBeforeExceptEmptyArray(): void + { + $_SERVER['REQUEST_METHOD'] = 'GET'; + + $config = [ + 'aliases' => [ + 'foo' => '', + 'bar' => '', + 'baz' => '', + ], + 'globals' => [ + 'before' => [ + 'foo' => ['except' => []], + 'bar', + ], + 'after' => [ + 'baz', + ], + ], + ]; + $filtersConfig = $this->createConfigFromArray(FiltersConfig::class, $config); + $filters = $this->createFilters($filtersConfig); + + $uri = 'admin/foo/bar'; + $expected = [ + 'before' => [ + 'foo', + 'bar', + ], + 'after' => ['baz'], + ]; + $this->assertSame($expected, $filters->initialize($uri)->getFilters()); + } + public function testAfterExceptString(): void { $_SERVER['REQUEST_METHOD'] = 'GET'; From e7297f1a9a555faa7be9266a8b0933f05cbc9672 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 15 Aug 2023 19:12:48 +0900 Subject: [PATCH 02/10] fix: "except empty" means "except all" This behavior is unexpected and not good for security. If a dev removes all items in `except` key, the filter is disabled. Now "except empty" means "except nothing". --- system/Filters/Filters.php | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php index fd15ba445838..f7052486bc74 100644 --- a/system/Filters/Filters.php +++ b/system/Filters/Filters.php @@ -416,7 +416,7 @@ protected function processGlobals(?string $uri = null) if (isset($rules['except'])) { // grab the exclusion rules $check = $rules['except']; - if ($this->pathApplies($uri, $check)) { + if ($this->checkExcept($uri, $check)) { $keep = false; } } @@ -550,4 +550,39 @@ private function pathApplies(string $uri, $paths) return false; } + + /** + * Check except paths + * + * @param string $uri URI to check + * @param array|string $paths The except path patterns + * + * @return bool True if the URI matches except paths. + */ + private function checkExcept(string $uri, $paths): bool + { + // empty path does not match anything + if (empty($paths)) { + return false; + } + + // make sure the paths are iterable + if (is_string($paths)) { + $paths = [$paths]; + } + + // treat each paths as pseudo-regex + foreach ($paths as $path) { + // need to escape path separators + $path = str_replace('/', '\/', trim($path, '/ ')); + // need to make pseudo wildcard real + $path = strtolower(str_replace('*', '.*', $path)); + // Does this rule apply here? + if (preg_match('#^' . $path . '$#', $uri, $match) === 1) { + return true; + } + } + + return false; + } } From fc3ae4c3962956d86a7578999a75ac8b33d6e06f Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 15 Aug 2023 19:36:34 +0900 Subject: [PATCH 03/10] test: fix test case --- tests/system/Filters/FiltersTest.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/system/Filters/FiltersTest.php b/tests/system/Filters/FiltersTest.php index b78ea8b3a03d..95378dbcc832 100644 --- a/tests/system/Filters/FiltersTest.php +++ b/tests/system/Filters/FiltersTest.php @@ -215,15 +215,23 @@ public static function provideProcessMethodProcessGlobalsWithExcept(): iterable ['admin/*'], ], [ - [], + ['admin/*', 'foo/*'], + ], + [ + ['*'], + ], + [ + 'admin/*', ], ]; } /** * @dataProvider provideProcessMethodProcessGlobalsWithExcept + * + * @param array|string $except */ - public function testProcessMethodProcessGlobalsWithExcept(array $except): void + public function testProcessMethodProcessGlobalsWithExcept($except): void { $_SERVER['REQUEST_METHOD'] = 'GET'; From b6a3c7d4219e0749c8095acf7c8dafe8dd0d07e8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 15 Aug 2023 19:37:01 +0900 Subject: [PATCH 04/10] refactor: extract method --- system/Filters/Filters.php | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php index f7052486bc74..b3ad411f649e 100644 --- a/system/Filters/Filters.php +++ b/system/Filters/Filters.php @@ -536,19 +536,7 @@ private function pathApplies(string $uri, $paths) $paths = [$paths]; } - // treat each paths as pseudo-regex - foreach ($paths as $path) { - // need to escape path separators - $path = str_replace('/', '\/', trim($path, '/ ')); - // need to make pseudo wildcard real - $path = strtolower(str_replace('*', '.*', $path)); - // Does this rule apply here? - if (preg_match('#^' . $path . '$#', $uri, $match) === 1) { - return true; - } - } - - return false; + return $this->checkPseudoRegex($uri, $paths); } /** @@ -571,12 +559,21 @@ private function checkExcept(string $uri, $paths): bool $paths = [$paths]; } - // treat each paths as pseudo-regex + return $this->checkPseudoRegex($uri, $paths); + } + + /** + * Check the URI path as pseudo-regex + */ + private function checkPseudoRegex(string $uri, array $paths): bool + { + // treat each path as pseudo-regex foreach ($paths as $path) { // need to escape path separators $path = str_replace('/', '\/', trim($path, '/ ')); // need to make pseudo wildcard real $path = strtolower(str_replace('*', '.*', $path)); + // Does this rule apply here? if (preg_match('#^' . $path . '$#', $uri, $match) === 1) { return true; From 459ea264033f4372a09891eddcdeb96314b579df Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 16 Aug 2023 11:37:54 +0900 Subject: [PATCH 05/10] docs: add/fix PHPDoc types --- system/Filters/Filters.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php index b3ad411f649e..5777f75b3f93 100644 --- a/system/Filters/Filters.php +++ b/system/Filters/Filters.php @@ -156,7 +156,9 @@ public function setResponse(ResponseInterface $response) * Runs through all of the filters for the specified * uri and position. * - * @return mixed|RequestInterface|ResponseInterface + * @param string $uri URI path relative to baseURL + * + * @return RequestInterface|ResponseInterface|string|null * * @throws FilterException */ @@ -221,6 +223,8 @@ public function run(string $uri, string $position = 'before') * run through both a before and after and don't want to double * process the rows. * + * @param string|null $uri URI path relative to baseURL (all lowercase) + * * @return Filters */ public function initialize(?string $uri = null) @@ -391,7 +395,7 @@ public function getArguments(?string $key = null) /** * Add any applicable (not excluded) global filter settings to the mix. * - * @param string $uri + * @param string|null $uri URI path relative to baseURL (all lowercase) * * @return void */ @@ -454,7 +458,7 @@ protected function processMethods() /** * Add any applicable configured filters to the mix. * - * @param string $uri + * @param string|null $uri URI path relative to baseURL (all lowercase) * * @return void */ From f9ecd03c9976655796e3ef83a05f20ea04c6ded7 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 16 Aug 2023 13:04:10 +0900 Subject: [PATCH 06/10] test: use dataProvider --- tests/system/Filters/FiltersTest.php | 104 ++++++++++----------------- 1 file changed, 36 insertions(+), 68 deletions(-) diff --git a/tests/system/Filters/FiltersTest.php b/tests/system/Filters/FiltersTest.php index 95378dbcc832..d4ae24d1386b 100644 --- a/tests/system/Filters/FiltersTest.php +++ b/tests/system/Filters/FiltersTest.php @@ -580,7 +580,12 @@ public function testOtherResult(): void $this->assertSame('This is curious', $response); } - public function testBeforeExceptString(): void + /** + * @dataProvider provideBeforeExcept + * + * @param array|string $except + */ + public function testBeforeExcept(string $uri, $except, array $expected): void { $_SERVER['REQUEST_METHOD'] = 'GET'; @@ -592,7 +597,7 @@ public function testBeforeExceptString(): void ], 'globals' => [ 'before' => [ - 'foo' => ['except' => 'admin/*'], + 'foo' => ['except' => $except], 'bar', ], 'after' => [ @@ -603,82 +608,45 @@ public function testBeforeExceptString(): void $filtersConfig = $this->createConfigFromArray(FiltersConfig::class, $config); $filters = $this->createFilters($filtersConfig); - $uri = 'admin/foo/bar'; - $expected = [ - 'before' => [ - 'bar', - ], - 'after' => ['baz'], - ]; $this->assertSame($expected, $filters->initialize($uri)->getFilters()); } - public function testBeforeExceptInapplicable(): void + public static function provideBeforeExcept(): iterable { - $_SERVER['REQUEST_METHOD'] = 'GET'; - - $config = [ - 'aliases' => [ - 'foo' => '', - 'bar' => '', - 'baz' => '', - ], - 'globals' => [ - 'before' => [ - 'foo' => ['except' => 'george/*'], - 'bar', - ], - 'after' => [ - 'baz', + return [ + 'string exclude' => [ + 'admin/foo/bar', + 'admin/*', + [ + 'before' => [ + 'bar', + ], + 'after' => ['baz'], ], ], - ]; - $filtersConfig = $this->createConfigFromArray(FiltersConfig::class, $config); - $filters = $this->createFilters($filtersConfig); - - $uri = 'admin/foo/bar'; - $expected = [ - 'before' => [ - 'foo', - 'bar', - ], - 'after' => ['baz'], - ]; - $this->assertSame($expected, $filters->initialize($uri)->getFilters()); - } - - public function testBeforeExceptEmptyArray(): void - { - $_SERVER['REQUEST_METHOD'] = 'GET'; - - $config = [ - 'aliases' => [ - 'foo' => '', - 'bar' => '', - 'baz' => '', - ], - 'globals' => [ - 'before' => [ - 'foo' => ['except' => []], - 'bar', - ], - 'after' => [ - 'baz', + 'string not exclude' => [ + 'admin/foo/bar', + 'george/*', + [ + 'before' => [ + 'foo', + 'bar', + ], + 'after' => ['baz'], ], ], - ]; - $filtersConfig = $this->createConfigFromArray(FiltersConfig::class, $config); - $filters = $this->createFilters($filtersConfig); - - $uri = 'admin/foo/bar'; - $expected = [ - 'before' => [ - 'foo', - 'bar', + 'empty array not exclude' => [ + 'admin/foo/bar', + [], + [ + 'before' => [ + 'foo', + 'bar', + ], + 'after' => ['baz'], + ], ], - 'after' => ['baz'], ]; - $this->assertSame($expected, $filters->initialize($uri)->getFilters()); } public function testAfterExceptString(): void From 2ad0864ed1408805c4985503ab18d64f2f21407b Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 16 Aug 2023 13:05:28 +0900 Subject: [PATCH 07/10] test: add test cases for except --- tests/system/Filters/FiltersTest.php | 46 ++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/system/Filters/FiltersTest.php b/tests/system/Filters/FiltersTest.php index d4ae24d1386b..ae75b14af835 100644 --- a/tests/system/Filters/FiltersTest.php +++ b/tests/system/Filters/FiltersTest.php @@ -646,6 +646,52 @@ public static function provideBeforeExcept(): iterable 'after' => ['baz'], ], ], + 'empty string not exclude' => [ + 'admin/foo/bar', + // The URI path '' means the baseURL. + '', + [ + 'before' => [ + 'foo', + 'bar', + ], + 'after' => ['baz'], + ], + ], + 'empty string exclude' => [ + // The URI path '' means the baseURL. + '', + // So this setting excludes `foo` filter only to the baseURL. + '', + [ + 'before' => [ + 'bar', + ], + 'after' => ['baz'], + ], + ], + 'slash not exclude' => [ + 'admin/foo/bar', + '/', + [ + 'before' => [ + 'foo', + 'bar', + ], + 'after' => ['baz'], + ], + ], + 'slash exclude' => [ + // The URI path '' means the baseURL. + '', + '/', + [ + 'before' => [ + 'bar', + ], + 'after' => ['baz'], + ], + ], ]; } From 79aaefbef794debdd04f08ce0f8f4abfe90344f5 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 16 Aug 2023 13:10:18 +0900 Subject: [PATCH 08/10] fix: empty string should mean except only baseURL The URI path '' means the baseURL. So ['except' => ''] should mean that except for the baseURL only. --- system/Filters/Filters.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php index 5777f75b3f93..feba435471c3 100644 --- a/system/Filters/Filters.php +++ b/system/Filters/Filters.php @@ -553,8 +553,8 @@ private function pathApplies(string $uri, $paths) */ private function checkExcept(string $uri, $paths): bool { - // empty path does not match anything - if (empty($paths)) { + // empty array does not match anything + if ($paths === []) { return false; } From 9b93434b841b475723f236472b0fe8453d747599 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 16 Aug 2023 13:31:47 +0900 Subject: [PATCH 09/10] docs: add/update PHPDoc types --- system/Filters/Filters.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/system/Filters/Filters.php b/system/Filters/Filters.php index feba435471c3..e4070b9eeccc 100644 --- a/system/Filters/Filters.php +++ b/system/Filters/Filters.php @@ -546,7 +546,7 @@ private function pathApplies(string $uri, $paths) /** * Check except paths * - * @param string $uri URI to check + * @param string $uri URI path relative to baseURL (all lowercase) * @param array|string $paths The except path patterns * * @return bool True if the URI matches except paths. @@ -568,6 +568,9 @@ 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 array $paths The except path patterns */ private function checkPseudoRegex(string $uri, array $paths): bool { From e510da9563a6f25251c8afd2c6fa8de15a1c65c9 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 16 Aug 2023 13:28:18 +0900 Subject: [PATCH 10/10] docs: add changelog --- user_guide_src/source/changelogs/v4.3.8.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.3.8.rst b/user_guide_src/source/changelogs/v4.3.8.rst index b4c8e4da4a69..d62e5a5ef65b 100644 --- a/user_guide_src/source/changelogs/v4.3.8.rst +++ b/user_guide_src/source/changelogs/v4.3.8.rst @@ -24,6 +24,12 @@ Deprecations Bugs Fixed ********** +- **Controller Filters:** In previous versions, ``['except' => []]`` or ``['except' => '']`` + meant "except all". The bug has been fixed, and now + + - ``['except' => []]`` means to exclude nothing. + - ``['except' => '']`` means to exclude the baseURL only. + See the repo's `CHANGELOG.md `_ for a complete list of bugs fixed.