fix: filter exec order

[kenjis]

Description
Supersedes #7404

The globals before filters should be applied first.
CSRF filter or Auth filter is often defined as globals before filter, and they should run first.
The current order of execution is different from what developers normally assume, and thus may cause security issues.
See https://forum.codeigniter.com/showthread.php?tid=86619, codeigniter4/shield#798

• change filter exec order
  • (1) filter group order
  • (2) after filter order will be reversed in route filters and filters filters
• add Config\Feature::$oldFilterOrder for backward compatibility

(1)

before filters:

Previous: route → globals → methods → filters Now: globals → methods → filters → route 

after filters:

Previous: route → globals → filters Now: route → filters → globals 

(2)

Configuration

diff --git a/app/Config/Filters.php b/app/Config/Filters.php index 8c02a4acd3..26590b0897 100644 --- a/app/Config/Filters.php +++ b/app/Config/Filters.php @@ -24,6 +24,14 @@ class Filters extends BaseConfig 'honeypot' => Honeypot::class, 'invalidchars' => InvalidChars::class, 'secureheaders' => SecureHeaders::class, + 'global1' => 'Dummy', + 'global2' => 'Dummy', + 'method1' => 'Dummy', + 'method2' => 'Dummy', + 'filter1' => 'Dummy', + 'filter2' => 'Dummy', + 'route1' => 'Dummy', + 'route2' => 'Dummy', ]; /** @@ -35,11 +43,15 @@ class Filters extends BaseConfig */ public array $globals = [ 'before' => [ + 'global1', + 'global2', // 'honeypot', // 'csrf', // 'invalidchars', ], 'after' => [ + 'global2', + 'global1', 'toolbar', // 'honeypot', // 'secureheaders', @@ -57,7 +69,9 @@ class Filters extends BaseConfig * permits any HTTP method to access a controller. Accessing the controller * with a method you don't expect could bypass the filter. */ - public array $methods = []; + public array $methods = [ + 'get' => ['method1', 'method2'], + ]; /** * List of filter aliases that should run on any @@ -66,5 +80,8 @@ class Filters extends BaseConfig * Example: * 'isLoggedIn' => ['before' => ['account/*', 'profiles/*']] */ - public array $filters = []; + public array $filters = [ + 'filter1' => ['before' => '*', 'after' => '*'], + 'filter2' => ['before' => '*', 'after' => '*'], + ]; } diff --git a/app/Config/Routes.php b/app/Config/Routes.php index fc4914a692..95a7d1a150 100644 --- a/app/Config/Routes.php +++ b/app/Config/Routes.php @@ -5,4 +5,4 @@ use CodeIgniter\Router\RouteCollection; /** * @var RouteCollection $routes */ -$routes->get('/', 'Home::index'); +$routes->get('/', 'Home::index', ['filter' => ['route1', 'route2']]);

Previous:

+--------+-------+---------------------------------------------------------------+-------------------------------------------------------+ | Method | Route | Before Filters | After Filters | +--------+-------+---------------------------------------------------------------+-------------------------------------------------------+ | GET | / | route1 route2 global1 global2 method1 method2 filter1 filter2 | route1 route2 global2 global1 filter1 filter2 toolbar | +--------+-------+---------------------------------------------------------------+-------------------------------------------------------+ 

Now:

+--------+-------+---------------------------------------------------------------+-------------------------------------------------------+ | Method | Route | Before Filters | After Filters | +--------+-------+---------------------------------------------------------------+-------------------------------------------------------+ | GET | / | global1 global2 method1 method2 filter1 filter2 route1 route2 | route2 route1 filter2 filter1 global2 global1 toolbar | +--------+-------+---------------------------------------------------------------+-------------------------------------------------------+ 

Ref #6262

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

For Route filters and Filters filters that specify before and after at the same time, the order of execution in the after filters has been reversed.
But Globals filters are not reversed.

[MGatner]

Thanks for making this a feature flag. I think we should move away from the "Filters include" way of configuring these.

[kenjis]

@MGatner What do you mean by the "Filters include" way?

[MGatner]

@kenjis see Filters::discoverFilters(), where we essentially use a route-like file include to run imperative code from Config namespaces that isn't classed or anything. It's a fragile system and not friendly to static analysis or testing, and it is unnecessary: we have other perfectly good ways of defining and discovering components now that should be used instead.

[github-actions]

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[github-actions]

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

Rebased to resolve a conflict.

[kenjis]

@MGatner Sent PR #8014 to make Filters::discoverFilters() deprecated.