Skip to content

Commit bc9e857

Browse files
trasherjjLDN
trasher
and
jjLDN
committed
Fix session expiration handling on POST requests
When the session has expired, a POST request (e.g. profile change from the profile selector) failed with a generic "Action not possible" error instead of redirecting the user to the login page: - the CSRF check was executed before the firewall, and failed because there is no CSRF token in an expired session; the firewall is now executed first. Controller listeners priorities are now centralized in `ListenersPriority::CONTROLLER_LISTENERS_PRIORITIES`; - after the login, the user was redirected with a GET method to the original URL, that may not accept it (405 error); the original URL is now kept as redirection target only for GET/HEAD requests. Fixes #25619 Co-authored-by: jjLDN <1943244+jjLDN@users.noreply.github.com>
1 parent bfffe53 commit bc9e857

6 files changed

Lines changed: 272 additions & 4 deletions

File tree

‎src/Glpi/Kernel/Listener/ControllerListener/CheckCsrfListener.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
namespace Glpi\Kernel\Listener\ControllerListener;
3636

3737
use Glpi\Http\SessionManager;
38+
use Glpi\Kernel\ListenersPriority;
3839
use Session;
3940
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
4041
use Symfony\Component\HttpFoundation\Request;
@@ -49,7 +50,9 @@ public function __construct(
4950

5051
public static function getSubscribedEvents(): array
5152
{
52-
return [KernelEvents::CONTROLLER => 'onKernelController'];
53+
return [
54+
KernelEvents::CONTROLLER => ['onKernelController', ListenersPriority::CONTROLLER_LISTENERS_PRIORITIES[self::class]],
55+
];
5356
}
5457

5558
public function onKernelController(ControllerEvent $event): void

‎src/Glpi/Kernel/Listener/ControllerListener/FirewallStrategyListener.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636

3737
use Glpi\Http\Firewall;
3838
use Glpi\Http\SessionManager;
39+
use Glpi\Kernel\ListenersPriority;
3940
use Glpi\Security\Attribute\SecurityStrategy;
4041
use RuntimeException;
4142
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -51,7 +52,9 @@ public function __construct(
5152

5253
public static function getSubscribedEvents(): array
5354
{
54-
return [KernelEvents::CONTROLLER => 'onKernelController'];
55+
return [
56+
KernelEvents::CONTROLLER => ['onKernelController', ListenersPriority::CONTROLLER_LISTENERS_PRIORITIES[self::class]],
57+
];
5558
}
5659

5760
public function onKernelController(ControllerEvent $event): void

‎src/Glpi/Kernel/Listener/ExceptionListener/AccessErrorListener.php‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
use Glpi\Http\RedirectResponse;
4343
use Session;
4444
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
45+
use Symfony\Component\HttpFoundation\Request;
4546
use Symfony\Component\HttpFoundation\Response;
4647
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
4748
use Symfony\Component\HttpKernel\KernelEvents;
@@ -85,12 +86,18 @@ public function onKernelException(ExceptionEvent $event): void
8586
$http_exception->setMessageToDisplay(__('Your session has expired. Please log in again.'));
8687
throw $http_exception;
8788
} else {
88-
// For HTML requests, redirect to login page (existing behavior)
89+
// For HTML requests, redirect to login page (existing behavior).
90+
// The original URL is kept as redirection target only for safe methods, as replaying it with
91+
// a `GET` method after the login may target a route that does not accept this method.
92+
// The `redirect` parameter must not be empty, as it is required to display the "session expired" message.
93+
$redirect = in_array($request->getRealMethod(), [Request::METHOD_GET, Request::METHOD_HEAD], true)
94+
? $request->getPathInfo() . '?' . $request->getQueryString()
95+
: '/';
8996
$response = new RedirectResponse(
9097
sprintf(
9198
'%s/?redirect=%s&error=3',
9299
$request->getBasePath(),
93-
\rawurlencode($request->getPathInfo() . '?' . $request->getQueryString())
100+
\rawurlencode($redirect)
94101
)
95102
);
96103
}

‎src/Glpi/Kernel/ListenersPriority.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434

3535
namespace Glpi\Kernel;
3636

37+
use Glpi\Kernel\Listener\ControllerListener\CheckCsrfListener;
38+
use Glpi\Kernel\Listener\ControllerListener\FirewallStrategyListener;
3739
use Glpi\Kernel\Listener\PostBootListener\BootPlugins;
3840
use Glpi\Kernel\Listener\PostBootListener\CheckPluginsStates;
3941
use Glpi\Kernel\Listener\PostBootListener\CustomObjectsAutoloaderRegistration;
@@ -139,5 +141,16 @@ final class ListenersPriority
139141
SessionVariables::class => 0,
140142
];
141143

144+
public const CONTROLLER_LISTENERS_PRIORITIES = [
145+
// Applies the security strategy of the controller (authentication, profile rights, ...).
146+
// Must be executed before the `CheckCsrfListener`: when the session has expired, the CSRF token
147+
// no longer exists in the session, and the CSRF check would fail with a generic "access denied" error
148+
// instead of letting the firewall throw a `SessionExpiredException` that redirects the user to the login page.
149+
FirewallStrategyListener::class => 10,
150+
151+
// Validates the CSRF token of requests that are supposed to have a body.
152+
CheckCsrfListener::class => 0,
153+
];
154+
142155
private function __construct() {}
143156
}

‎tests/functional/Glpi/Kernel/Listener/ControllerListener/ControllerListenersPriorityTest.php‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
<?php
2+
3+
/**
4+
* ---------------------------------------------------------------------
5+
*
6+
* GLPI - Gestionnaire Libre de Parc Informatique
7+
*
8+
* http://glpi-project.org
9+
*
10+
* @copyright 2015-2026 Teclib' and contributors.
11+
* @licence https://www.gnu.org/licenses/gpl-3.0.html
12+
*
13+
* ---------------------------------------------------------------------
14+
*
15+
* LICENSE
16+
*
17+
* This file is part of GLPI.
18+
*
19+
* This program is free software: you can redistribute it and/or modify
20+
* it under the terms of the GNU General Public License as published by
21+
* the Free Software Foundation, either version 3 of the License, or
22+
* (at your option) any later version.
23+
*
24+
* This program is distributed in the hope that it will be useful,
25+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
26+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
27+
* GNU General Public License for more details.
28+
*
29+
* You should have received a copy of the GNU General Public License
30+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
31+
*
32+
* ---------------------------------------------------------------------
33+
*/
34+
35+
namespace tests\units\Glpi\Kernel\Listener\ControllerListener;
36+
37+
use Glpi\Controller\Session\ChangeProfileController;
38+
use Glpi\Exception\Http\AccessDeniedHttpException;
39+
use Glpi\Exception\SessionExpiredException;
40+
use Glpi\Http\Firewall;
41+
use Glpi\Http\SessionManager;
42+
use Glpi\Kernel\Listener\ControllerListener\CheckCsrfListener;
43+
use Glpi\Kernel\Listener\ControllerListener\FirewallStrategyListener;
44+
use Glpi\Tests\DbTestCase;
45+
use Session;
46+
use Symfony\Component\EventDispatcher\EventDispatcher;
47+
use Symfony\Component\HttpFoundation\Request;
48+
use Symfony\Component\HttpKernel\Event\ControllerEvent;
49+
use Symfony\Component\HttpKernel\HttpKernelInterface;
50+
use Symfony\Component\HttpKernel\KernelEvents;
51+
52+
final class ControllerListenersPriorityTest extends DbTestCase
53+
{
54+
public function testFirewallIsExecutedBeforeCsrfCheck(): void
55+
{
56+
$session_manager = new SessionManager();
57+
$csrf_listener = new CheckCsrfListener($session_manager);
58+
$firewall_listener = new FirewallStrategyListener(new Firewall(), $session_manager);
59+
60+
$dispatcher = new EventDispatcher();
61+
// Register the listeners in the reverse order of the expected execution order, to be sure that
62+
// the execution order does not depend on the registration order.
63+
$dispatcher->addSubscriber($csrf_listener);
64+
$dispatcher->addSubscriber($firewall_listener);
65+
66+
$this->assertSame(
67+
[
68+
[$firewall_listener, 'onKernelController'],
69+
[$csrf_listener, 'onKernelController'],
70+
],
71+
$dispatcher->getListeners(KernelEvents::CONTROLLER)
72+
);
73+
}
74+
75+
public function testPostRequestWithExpiredSessionThrowsSessionExpiredException(): void
76+
{
77+
// No session, thus no CSRF token.
78+
$this->assertFalse(Session::getLoginUserID());
79+
$this->assertArrayNotHasKey('glpicsrftokens', $_SESSION);
80+
81+
$this->expectException(SessionExpiredException::class);
82+
83+
$this->dispatch(Request::create('/Session/ChangeProfile', 'POST', ['id' => 1]));
84+
}
85+
86+
public function testPostRequestWithInvalidCsrfTokenIsStillDenied(): void
87+
{
88+
$this->login();
89+
90+
$this->expectException(AccessDeniedHttpException::class);
91+
92+
$this->dispatch(Request::create('/Session/ChangeProfile', 'POST', ['id' => 1, '_glpi_csrf_token' => 'invalid']));
93+
}
94+
95+
public function testPostRequestWithValidCsrfTokenIsAccepted(): void
96+
{
97+
$this->login();
98+
99+
$token = Session::getNewCSRFToken();
100+
101+
$this->dispatch(Request::create('/Session/ChangeProfile', 'POST', ['id' => 1, '_glpi_csrf_token' => $token]));
102+
103+
// Token has been validated and consumed.
104+
$this->assertArrayNotHasKey($token, $_SESSION['glpicsrftokens']);
105+
}
106+
107+
private function getDispatcher(): EventDispatcher
108+
{
109+
$session_manager = new SessionManager();
110+
111+
$dispatcher = new EventDispatcher();
112+
$dispatcher->addSubscriber(new CheckCsrfListener($session_manager));
113+
$dispatcher->addSubscriber(new FirewallStrategyListener(new Firewall(), $session_manager));
114+
115+
return $dispatcher;
116+
}
117+
118+
private function dispatch(Request $request): void
119+
{
120+
// The controller is not executed, only its attributes are read by the firewall.
121+
$event = new ControllerEvent(
122+
$this->createStub(HttpKernelInterface::class),
123+
new ChangeProfileController(),
124+
$request,
125+
HttpKernelInterface::MAIN_REQUEST
126+
);
127+
128+
$this->getDispatcher()->dispatch($event, KernelEvents::CONTROLLER);
129+
}
130+
}

‎tests/functional/Glpi/Kernel/Listener/ExceptionListener/AccessErrorListenerTest.php‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
<?php
2+
3+
/**
4+
* ---------------------------------------------------------------------
5+
*
6+
* GLPI - Gestionnaire Libre de Parc Informatique
7+
*
8+
* http://glpi-project.org
9+
*
10+
* @copyright 2015-2026 Teclib' and contributors.
11+
* @licence https://www.gnu.org/licenses/gpl-3.0.html
12+
*
13+
* ---------------------------------------------------------------------
14+
*
15+
* LICENSE
16+
*
17+
* This file is part of GLPI.
18+
*
19+
* This program is free software: you can redistribute it and/or modify
20+
* it under the terms of the GNU General Public License as published by
21+
* the Free Software Foundation, either version 3 of the License, or
22+
* (at your option) any later version.
23+
*
24+
* This program is distributed in the hope that it will be useful,
25+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
26+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
27+
* GNU General Public License for more details.
28+
*
29+
* You should have received a copy of the GNU General Public License
30+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
31+
*
32+
* ---------------------------------------------------------------------
33+
*/
34+
35+
namespace tests\units\Glpi\Kernel\Listener\ExceptionListener;
36+
37+
use Glpi\Exception\SessionExpiredException;
38+
use Glpi\Http\RedirectResponse;
39+
use Glpi\Kernel\Listener\ExceptionListener\AccessErrorListener;
40+
use Glpi\Tests\DbTestCase;
41+
use PHPUnit\Framework\Attributes\DataProvider;
42+
use Symfony\Component\HttpFoundation\Request;
43+
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
44+
use Symfony\Component\HttpKernel\HttpKernelInterface;
45+
46+
final class AccessErrorListenerTest extends DbTestCase
47+
{
48+
/**
49+
* @return iterable<string, array{method: string, uri: string, redirect: string}>
50+
*/
51+
public static function sessionExpiredProvider(): iterable
52+
{
53+
yield 'GET' => [
54+
'method' => 'GET',
55+
'uri' => '/front/computer.form.php?id=5',
56+
'redirect' => '/front/computer.form.php?id=5',
57+
];
58+
yield 'HEAD' => [
59+
'method' => 'HEAD',
60+
'uri' => '/front/computer.form.php?id=5',
61+
'redirect' => '/front/computer.form.php?id=5',
62+
];
63+
// The original URL must not be used as redirection target, as it will be requested using the `GET` method.
64+
yield 'POST' => [
65+
'method' => 'POST',
66+
'uri' => '/Session/ChangeProfile',
67+
'redirect' => '/',
68+
];
69+
yield 'POST with query string' => [
70+
'method' => 'POST',
71+
'uri' => '/front/computer.form.php?id=5',
72+
'redirect' => '/',
73+
];
74+
yield 'PUT' => [
75+
'method' => 'PUT',
76+
'uri' => '/front/computer.form.php?id=5',
77+
'redirect' => '/',
78+
];
79+
yield 'DELETE' => [
80+
'method' => 'DELETE',
81+
'uri' => '/front/computer.form.php?id=5',
82+
'redirect' => '/',
83+
];
84+
}
85+
86+
#[DataProvider('sessionExpiredProvider')]
87+
public function testSessionExpiredRedirection(string $method, string $uri, string $redirect): void
88+
{
89+
$this->login();
90+
91+
$request = Request::create($uri, $method);
92+
93+
$event = new ExceptionEvent(
94+
$this->createStub(HttpKernelInterface::class),
95+
$request,
96+
HttpKernelInterface::MAIN_REQUEST,
97+
new SessionExpiredException()
98+
);
99+
100+
(new AccessErrorListener())->onKernelException($event);
101+
102+
$response = $event->getResponse();
103+
$this->assertInstanceOf(RedirectResponse::class, $response);
104+
$this->assertSame(
105+
\sprintf('/?redirect=%s&error=3', \rawurlencode($redirect)),
106+
$response->getTargetUrl()
107+
);
108+
109+
// Session must have been destroyed.
110+
$this->assertEmpty($_SESSION);
111+
}
112+
}

0 commit comments

Comments
 (0)