Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter exec order #7955

Merged
merged 10 commits into from
Oct 3, 2023
Merged

fix: filter exec order #7955

merged 10 commits into from
Oct 3, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 20, 2023

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 kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.5 labels Sep 20, 2023
@kenjis kenjis marked this pull request as draft September 20, 2023 08:12
@kenjis kenjis marked this pull request as ready for review September 21, 2023 01:50
@kenjis kenjis marked this pull request as draft September 22, 2023 04:34
@kenjis kenjis marked this pull request as ready for review September 22, 2023 07:42
@kenjis
Copy link
Member Author

kenjis commented Sep 22, 2023

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.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kenjis
Copy link
Member Author

kenjis commented Sep 27, 2023

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

@MGatner
Copy link
Member

MGatner commented Sep 27, 2023

@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 github-actions bot added the stale Pull requests with conflicts label Sep 28, 2023
@github-actions
Copy link

👋 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 kenjis removed the stale Pull requests with conflicts label Sep 29, 2023
@github-actions github-actions bot added the stale Pull requests with conflicts label Sep 30, 2023
@github-actions
Copy link

👋 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
Copy link
Member Author

kenjis commented Oct 1, 2023

Rebased to resolve a conflict.

@kenjis kenjis removed the stale Pull requests with conflicts label Oct 2, 2023
@kenjis kenjis merged commit 05bbe66 into codeigniter4:4.5 Oct 3, 2023
54 checks passed
@kenjis kenjis deleted the fix-filter-order branch October 3, 2023 23:15
@kenjis
Copy link
Member Author

kenjis commented Oct 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants