Skip to content

Commit

Permalink
Merge pull request #7955 from kenjis/fix-filter-order
Browse files Browse the repository at this point in the history 
fix: filter exec order
  • Loading branch information
@kenjis
kenjis authored Oct 3, 2023
2 parents c95a2a5 + 7687855 commit 05bbe66
Show file tree
Hide file tree
Showing 9 changed files with 301 additions and 28 deletions.
5 changes: 5 additions & 0 deletions app/Config/Feature.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,9 @@ class Feature extends BaseConfig
* Use improved new auto routing instead of the default legacy version.
*/
public bool $autoRoutesImproved = false;

/**
* Use filter execution order in 4.4 or before.
*/
public bool $oldFilterOrder = false;
}
14 changes: 10 additions & 4 deletions system/CodeIgniter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use CodeIgniter\Router\Router;
use Config\App;
use Config\Cache;
use Config\Feature;
use Config\Kint as KintConfig;
use Config\Services;
use Exception;
Expand Down Expand Up @@ -446,7 +447,7 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
return $response;
}

$routeFilter = $this->tryToRouteIt($routes);
$routeFilters = $this->tryToRouteIt($routes);

$uri = $this->determinePath();

Expand All @@ -456,9 +457,14 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache

// If any filters were specified within the routes file,
// we need to ensure it's active for the current request
if ($routeFilter !== null) {
$filters->enableFilters($routeFilter, 'before');
$filters->enableFilters($routeFilter, 'after');
if ($routeFilters !== null) {
$filters->enableFilters($routeFilters, 'before');

if (! config(Feature::class)->oldFilterOrder) {
$routeFilters = array_reverse($routeFilters);
}

$filters->enableFilters($routeFilters, 'after');
}

// Run "before" filters
Expand Down
7 changes: 7 additions & 0 deletions system/Commands/Utilities/Routes/FilterFinder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use CodeIgniter\Filters\Filters;
use CodeIgniter\HTTP\Exceptions\RedirectException;
use CodeIgniter\Router\Router;
use Config\Feature;
use Config\Services;

/**
Expand Down Expand Up @@ -52,7 +53,13 @@ public function find(string $uri): array
// Add route filters
try {
$routeFilters = $this->getRouteFilters($uri);

$this->filters->enableFilters($routeFilters, 'before');

if (! config(Feature::class)->oldFilterOrder) {
$routeFilters = array_reverse($routeFilters);
}

$this->filters->enableFilters($routeFilters, 'after');

$this->filters->initialize($uri);
Expand Down
69 changes: 60 additions & 9 deletions system/Filters/Filters.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use CodeIgniter\Filters\Exceptions\FilterException;
use CodeIgniter\HTTP\RequestInterface;
use CodeIgniter\HTTP\ResponseInterface;
use Config\Feature;
use Config\Filters as FiltersConfig;
use Config\Modules;
use Config\Services;
Expand Down Expand Up @@ -245,9 +246,15 @@ public function initialize(?string $uri = null)
return $this;
}

$this->processGlobals($uri);
$this->processMethods();
$this->processFilters($uri);
if (config(Feature::class)->oldFilterOrder) {
$this->processGlobals($uri);
$this->processMethods();
$this->processFilters($uri);
} else {
$this->processFilters($uri);
$this->processMethods();
$this->processGlobals($uri);
}

// Set the toolbar filter to the last position to be executed
if (in_array('toolbar', $this->filters['after'], true)
Expand Down Expand Up @@ -436,6 +443,8 @@ protected function processGlobals(?string $uri = null)
// Add any global filters, unless they are excluded for this URI
$sets = ['before', 'after'];

$filters = [];

foreach ($sets as $set) {
if (isset($this->config->globals[$set])) {
// look at each alias in the group
Expand All @@ -455,11 +464,23 @@ protected function processGlobals(?string $uri = null)
}

if ($keep) {
$this->filters[$set][] = $alias;
$filters[$set][] = $alias;
}
}
}
}

if (isset($filters['before'])) {
if (config(Feature::class)->oldFilterOrder) {
$this->filters['before'] = array_merge($this->filters['before'], $filters['before']);
} else {
$this->filters['before'] = array_merge($filters['before'], $this->filters['before']);
}
}

if (isset($filters['after'])) {
$this->filters['after'] = array_merge($this->filters['after'], $filters['after']);
}
}

/**
Expand All @@ -477,7 +498,11 @@ protected function processMethods()
$method = strtolower($this->request->getMethod()) ?? 'cli';

if (array_key_exists($method, $this->config->methods)) {
$this->filters['before'] = array_merge($this->filters['before'], $this->config->methods[$method]);
if (config(Feature::class)->oldFilterOrder) {
$this->filters['before'] = array_merge($this->filters['before'], $this->config->methods[$method]);
} else {
$this->filters['before'] = array_merge($this->config->methods[$method], $this->filters['before']);
}
}
}

Expand All @@ -497,6 +522,8 @@ protected function processFilters(?string $uri = null)
$uri = strtolower(trim($uri, '/ '));

// Add any filters that apply to this URI
$filters = [];

foreach ($this->config->filters as $alias => $settings) {
// Look for inclusion rules
if (isset($settings['before'])) {
Expand All @@ -506,7 +533,7 @@ protected function processFilters(?string $uri = null)
// Get arguments and clean name
[$name, $arguments] = $this->getCleanName($alias);

$this->filters['before'][] = $name;
$filters['before'][] = $name;

$this->registerArguments($name, $arguments);
}
Expand All @@ -519,14 +546,30 @@ protected function processFilters(?string $uri = null)
// Get arguments and clean name
[$name, $arguments] = $this->getCleanName($alias);

$this->filters['after'][] = $name;
$filters['after'][] = $name;

// The arguments may have already been registered in the before filter.
// So disable check.
$this->registerArguments($name, $arguments, false);
}
}
}

if (isset($filters['before'])) {
if (config(Feature::class)->oldFilterOrder) {
$this->filters['before'] = array_merge($this->filters['before'], $filters['before']);
} else {
$this->filters['before'] = array_merge($filters['before'], $this->filters['before']);
}
}

if (isset($filters['after'])) {
if (! config(Feature::class)->oldFilterOrder) {
$filters['after'] = array_reverse($filters['after']);
}

$this->filters['after'] = array_merge($this->filters['after'], $filters['after']);
}
}

/**
Expand Down Expand Up @@ -563,6 +606,8 @@ private function registerArguments(string $name, array $arguments, bool $check =
*/
protected function processAliasesToClass(string $position)
{
$filtersClass = [];

foreach ($this->filters[$position] as $alias => $rules) {
if (is_numeric($alias) && is_string($rules)) {
$alias = $rules;
Expand All @@ -573,14 +618,20 @@ protected function processAliasesToClass(string $position)
}

if (is_array($this->config->aliases[$alias])) {
$this->filtersClass[$position] = array_merge($this->filtersClass[$position], $this->config->aliases[$alias]);
$filtersClass = array_merge($filtersClass, $this->config->aliases[$alias]);
} else {
$this->filtersClass[$position][] = $this->config->aliases[$alias];
$filtersClass[] = $this->config->aliases[$alias];
}
}

// when using enableFilter() we already write the class name in $filtersClass as well as the
// alias in $filters. This leads to duplicates when using route filters.
if ($position === 'before') {
$this->filtersClass[$position] = array_merge($filtersClass, $this->filtersClass[$position]);
} else {
$this->filtersClass[$position] = array_merge($this->filtersClass[$position], $filtersClass);
}

// Since some filters like rate limiters rely on being executed once a request we filter em here.
$this->filtersClass[$position] = array_values(array_unique($this->filtersClass[$position]));
}
Expand Down
136 changes: 132 additions & 4 deletions tests/system/Commands/Utilities/Routes/FilterFinderTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use CodeIgniter\Router\Router;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\ConfigFromArrayTrait;
use Config\Feature;
use Config\Filters as FiltersConfig;
use Config\Modules;
use Config\Routing;
Expand Down Expand Up @@ -145,7 +146,7 @@ public function testFindGlobalsAndRouteFilters(): void
$filters = $finder->find('admin');

$expected = [
'before' => ['honeypot', 'csrf'],
'before' => ['csrf', 'honeypot'],
'after' => ['honeypot', 'toolbar'],
];
$this->assertSame($expected, $filters);
Expand All @@ -163,7 +164,7 @@ public function testFindGlobalsAndRouteClassnameFilters(): void
$filters = $finder->find('admin');

$expected = [
'before' => [InvalidChars::class, 'csrf'],
'before' => ['csrf', InvalidChars::class],
'after' => [InvalidChars::class, 'toolbar'],
];
$this->assertSame($expected, $filters);
Expand All @@ -181,8 +182,135 @@ public function testFindGlobalsAndRouteMultipleFilters(): void
$filters = $finder->find('admin');

$expected = [
'before' => ['honeypot', InvalidChars::class, 'csrf'],
'after' => ['honeypot', InvalidChars::class, 'toolbar'],
'before' => ['csrf', 'honeypot', InvalidChars::class],
'after' => [InvalidChars::class, 'honeypot', 'toolbar'],
];
$this->assertSame($expected, $filters);
}

public function testFilterOrder()
{
$collection = $this->createRouteCollection([]);
$collection->get('/', ' Home::index', ['filter' => ['route1', 'route2']]);
$router = $this->createRouter($collection);
$filters = $this->createFilters([
'aliases' => [
'global1' => 'Dummy',
'global2' => 'Dummy',
'method1' => 'Dummy',
'method2' => 'Dummy',
'filter1' => 'Dummy',
'filter2' => 'Dummy',
'route1' => 'Dummy',
'route2' => 'Dummy',
],
'globals' => [
'before' => [
'global1',
'global2',
],
'after' => [
'global2',
'global1',
],
],
'methods' => [
'get' => ['method1', 'method2'],
],
'filters' => [
'filter1' => ['before' => '*', 'after' => '*'],
'filter2' => ['before' => '*', 'after' => '*'],
],
]);

$finder = new FilterFinder($router, $filters);

$filters = $finder->find('/');

$expected = [
'before' => [
'global1',
'global2',
'method1',
'method2',
'filter1',
'filter2',
'route1',
'route2',
],
'after' => [
'route2',
'route1',
'filter2',
'filter1',
'global2',
'global1',
],
];
$this->assertSame($expected, $filters);
}

public function testFilterOrderWithOldFilterOrder()
{
$feature = config(Feature::class);
$feature->oldFilterOrder = true;

$collection = $this->createRouteCollection([]);
$collection->get('/', ' Home::index', ['filter' => ['route1', 'route2']]);
$router = $this->createRouter($collection);
$filters = $this->createFilters([
'aliases' => [
'global1' => 'Dummy',
'global2' => 'Dummy',
'method1' => 'Dummy',
'method2' => 'Dummy',
'filter1' => 'Dummy',
'filter2' => 'Dummy',
'route1' => 'Dummy',
'route2' => 'Dummy',
],
'globals' => [
'before' => [
'global1',
'global2',
],
'after' => [
'global1',
'global2',
],
],
'methods' => [
'get' => ['method1', 'method2'],
],
'filters' => [
'filter1' => ['before' => '*', 'after' => '*'],
'filter2' => ['before' => '*', 'after' => '*'],
],
]);

$finder = new FilterFinder($router, $filters);

$filters = $finder->find('/');

$expected = [
'before' => [
'route1',
'route2',
'global1',
'global2',
'method1',
'method2',
'filter1',
'filter2',
],
'after' => [
'route1',
'route2',
'global1',
'global2',
'filter1',
'filter2',
],
];
$this->assertSame($expected, $filters);
}
Expand Down
Loading

0 comments on commit 05bbe66

Please sign in to comment.