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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9d26d21
Filters's is execute order fixed.
c4user Apr 3, 2023
3b0ec99
test: update test expectations
kenjis Sep 20, 2023
b2998ba
feat: add feature flag for filter execution order
kenjis Sep 21, 2023
d101e35
docs: add about Filter Execution Order
kenjis Sep 21, 2023
42c02ea
docs: add changelog and ugrading
kenjis Sep 21, 2023
72968b7
test: add tests for filter order
kenjis Sep 21, 2023
ac743b2
fix: change filter order in route filter and filters filter
kenjis Sep 22, 2023
47b8493
refactor: rename variable name
kenjis Sep 22, 2023
5deb11f
docs: update docs
kenjis Sep 22, 2023
7687855
test: update assertion
kenjis Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/Config/Feature.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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