Skip to content

Commit

Permalink
Enhance CSRF handling
Browse files Browse the repository at this point in the history
  • Loading branch information
cedric-anne authored Jan 22, 2025
1 parent 77cac14 commit 823b4eb
Show file tree
Hide file tree
Showing 12 changed files with 184 additions and 45 deletions.
12 changes: 11 additions & 1 deletion js/modules/GlpiInstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
* ---------------------------------------------------------------------
*/

/* global getAjaxCsrfToken */

import { ProgressBar } from './ProgressBar.js';

export async function init_database(progress_key)
Expand Down Expand Up @@ -74,7 +76,15 @@ export async function init_database(progress_key)
}, 1500);

try {
await fetch(`${CFG_GLPI.root_doc}/install/init_database`, {method: 'POST'});
await fetch(`${CFG_GLPI.root_doc}/install/init_database`, {
method: 'POST',
headers: {
'Accept': 'application/json',
'Content-Type': 'application/x-www-form-urlencoded;',
'X-Requested-With': 'XMLHttpRequest',
'X-Glpi-Csrf-Token': getAjaxCsrfToken(),
},
});
} catch {
// DB installation is really long and can result in a `Proxy timeout` error.
// It does not mean that the process is killed, it just mean that the proxy did not wait for the response
Expand Down
3 changes: 2 additions & 1 deletion js/modules/ObjectLock.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ class ObjectLock {
headers: {
'Accept': 'application/json',
'Content-Type': 'application/x-www-form-urlencoded;',
'X-Glpi-Csrf-Token': getAjaxCsrfToken()
'X-Requested-With': 'XMLHttpRequest',
'X-Glpi-Csrf-Token': getAjaxCsrfToken(),
},
body: `unlock=1&id=${this.lock.id}`
}).catch(() => {
Expand Down
2 changes: 2 additions & 0 deletions src/Glpi/Controller/ApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
use Glpi\Http\HeaderlessStreamedResponse;
use Glpi\Http\JSONResponse;
use Glpi\Http\Request;
use Glpi\Security\Attribute\DisableCsrfChecks;
use Glpi\Security\Attribute\SecurityStrategy;
use Symfony\Component\HttpFoundation\Request as SymfonyRequest;
use Symfony\Component\HttpFoundation\Response as SymfonyResponse;
Expand All @@ -55,6 +56,7 @@ final class ApiController extends AbstractController
'request_parameters' => '.*',
]
)]
#[DisableCsrfChecks()]
#[SecurityStrategy(Firewall::STRATEGY_NO_CHECK)]
public function __invoke(SymfonyRequest $request): SymfonyResponse
{
Expand Down
2 changes: 2 additions & 0 deletions src/Glpi/Controller/ApiRestController.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
use Glpi\Application\ErrorHandler;
use Glpi\Http\Firewall;
use Glpi\Http\HeaderlessStreamedResponse;
use Glpi\Security\Attribute\DisableCsrfChecks;
use Glpi\Security\Attribute\SecurityStrategy;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
Expand All @@ -52,6 +53,7 @@ final class ApiRestController extends AbstractController
'request_parameters' => '.*',
]
)]
#[DisableCsrfChecks()]
#[SecurityStrategy(Firewall::STRATEGY_NO_CHECK)]
public function __invoke(Request $request): Response
{
Expand Down
2 changes: 2 additions & 0 deletions src/Glpi/Controller/CaldavController.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

use Glpi\Http\Firewall;
use Glpi\Http\HeaderlessStreamedResponse;
use Glpi\Security\Attribute\DisableCsrfChecks;
use Glpi\Security\Attribute\SecurityStrategy;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
Expand All @@ -50,6 +51,7 @@ final class CaldavController extends AbstractController
'request_parameters' => '.*',
]
)]
#[DisableCsrfChecks()]
#[SecurityStrategy(Firewall::STRATEGY_NO_CHECK)]
public function __invoke(Request $request): Response
{
Expand Down
17 changes: 0 additions & 17 deletions src/Glpi/Controller/IndexController.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,10 @@
use Glpi\Security\Attribute\SecurityStrategy;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\Routing\Attribute\Route;

final class IndexController extends AbstractController
{
public function __construct(private HttpKernelInterface $http_kernel)
{
}

#[Route(
[
"base" => "/",
Expand All @@ -67,18 +62,6 @@ public function __construct(private HttpKernelInterface $http_kernel)
#[SecurityStrategy(Firewall::STRATEGY_NO_CHECK)]
public function __invoke(Request $request): Response
{
if (
$request->isMethod('POST')
&& !$request->request->has('totp_code')
&& $request->getContent() !== ''
) {
// POST request from the inventory agent, forward it to the inventory controller.
$sub_request = $request->duplicate(
attributes: ['_controller' => InventoryController::class . '::index']
);
return $this->http_kernel->handle($sub_request, HttpKernelInterface::SUB_REQUEST);
}

return new HeaderlessStreamedResponse($this->call(...));
}

Expand Down
3 changes: 3 additions & 0 deletions src/Glpi/Controller/InventoryController.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
use Glpi\Http\Firewall;
use Symfony\Component\HttpFoundation\Request;
use Glpi\Inventory\Conf;
use Glpi\Security\Attribute\DisableCsrfChecks;
use Glpi\Security\Attribute\SecurityStrategy;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Response;
Expand All @@ -47,6 +48,8 @@
final class InventoryController extends AbstractController
{
public static bool $is_running = false;

#[DisableCsrfChecks()]
#[SecurityStrategy(Firewall::STRATEGY_NO_CHECK)]
#[Route("/Inventory", name: "glpi_inventory", methods: ['GET', 'POST'])]
#[Route("/front/inventory.php", name: "glpi_inventory_legacy", methods: ['GET', 'POST'])]
Expand Down
68 changes: 68 additions & 0 deletions src/Glpi/Http/Listener/CatchInventoryAgentRequestListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

/**
* ---------------------------------------------------------------------
*
* GLPI - Gestionnaire Libre de Parc Informatique
*
* http://glpi-project.org
*
* @copyright 2015-2025 Teclib' and contributors.
* @licence https://www.gnu.org/licenses/gpl-3.0.html
*
* ---------------------------------------------------------------------
*
* LICENSE
*
* This file is part of GLPI.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*
* ---------------------------------------------------------------------
*/

namespace Glpi\Http\Listener;

use Glpi\Controller\InventoryController;
use Glpi\Kernel\ListenersPriority;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

final readonly class CatchInventoryAgentRequestListener implements EventSubscriberInterface
{
public static function getSubscribedEvents(): array
{
return [
KernelEvents::REQUEST => ['onKernelRequest', ListenersPriority::REQUEST_LISTENERS_PRIORITIES[self::class]],
];
}

public function onKernelRequest(RequestEvent $event): void
{
if (!$event->isMainRequest()) {
return;
}
$request = $event->getRequest();

if (
$request->getPathInfo() === '/'
&& $request->isMethod('POST')
&& !$request->request->has('totp_code')
&& $request->getContent() !== ''
) {
$request->attributes->set('_controller', InventoryController::class . '::index');
}
}
}
30 changes: 21 additions & 9 deletions src/Glpi/Http/Listener/CheckCsrfListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,35 +34,47 @@

namespace Glpi\Http\Listener;

use Glpi\Security\Attribute\DisableCsrfChecks;
use Session;
use Glpi\Kernel\ListenersPriority;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Event\ControllerEvent;
use Symfony\Component\HttpKernel\KernelEvents;

final readonly class CheckCsrfListener implements EventSubscriberInterface
{
public static function getSubscribedEvents(): array
{
return [
KernelEvents::REQUEST => ['onKernelRequest', ListenersPriority::REQUEST_LISTENERS_PRIORITIES[self::class]],
];
return [KernelEvents::CONTROLLER => 'onKernelController'];
}

public function onKernelRequest(RequestEvent $event): void
public function onKernelController(ControllerEvent $event): void
{
if (!$event->isMainRequest()) {
return;
}

/** @var DisableCsrfChecks[] $attributes */
$attributes = $event->getAttributes(DisableCsrfChecks::class);
if (\count($attributes) > 0) {
// CSRF checks are explicitely disabled for this controller.
return;
}

$request = $event->getRequest();

// Security : check CSRF token
if (isAPI() || !$request->request->count()) {
$bodyless_methods = [
Request::METHOD_GET,
Request::METHOD_HEAD,
Request::METHOD_OPTIONS,
Request::METHOD_TRACE,
];
if (in_array($request->getRealMethod(), $bodyless_methods)) {
// No CSRF checks if method is not supposed to have a body.
return;
}

if (preg_match('~(/(plugins|marketplace)/[^/]*|)/ajax/~', $request->getPathInfo()) === 1) {
if ($request->isXmlHttpRequest()) {
// Keep CSRF token as many AJAX requests may be made at the same time.
// This is due to the fact that read operations are often made using POST method (see #277).
define('GLPI_KEEP_CSRF_TOKEN', true);
Expand Down
3 changes: 2 additions & 1 deletion src/Glpi/Kernel/ListenersPriority.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ final class ListenersPriority

HttpListener\CheckMaintenanceListener::class => 430,

HttpListener\CheckCsrfListener::class => 420,
// This listener will forward to the inventory controller any inventory agent requests made on the index endpoint.
HttpListener\CatchInventoryAgentRequestListener::class => 420,

// Executes the legacy controller scripts (`/ajax/*.php` or `/front/*.php` scripts) whenever the
// requested URI matches an existing file.
Expand Down
40 changes: 40 additions & 0 deletions src/Glpi/Security/Attribute/DisableCsrfChecks.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

/**
* ---------------------------------------------------------------------
*
* GLPI - Gestionnaire Libre de Parc Informatique
*
* http://glpi-project.org
*
* @copyright 2015-2025 Teclib' and contributors.
* @licence https://www.gnu.org/licenses/gpl-3.0.html
*
* ---------------------------------------------------------------------
*
* LICENSE
*
* This file is part of GLPI.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*
* ---------------------------------------------------------------------
*/

namespace Glpi\Security\Attribute;

#[\Attribute(\Attribute::TARGET_METHOD)]
final readonly class DisableCsrfChecks
{
}
47 changes: 31 additions & 16 deletions tests/cypress/support/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -428,14 +428,21 @@ Cypress.Commands.add('enableDebugMode', () => {
return;
}

cy.request({
method: 'POST',
url: '/ajax/switchdebug.php',
body: {
'debug': 'on',
},
}).then(() => {
cy.reload();
cy.getCsrfToken().then((csrf) => {
cy.request({
method: 'POST',
url: '/ajax/switchdebug.php',
headers: {
'Content-Type': 'application/x-www-form-urlencoded',
'X-Requested-With': 'XMLHttpRequest',
'X-Glpi-Csrf-Token': csrf,
},
body: {
'debug': 'on',
},
}).then(() => {
cy.reload();
});
});
});

Expand All @@ -448,14 +455,22 @@ Cypress.Commands.add('disableDebugMode', () => {
if (Cypress.$('#debug-toolbar-applet').length === 0) {
return;
}
cy.request({
method: 'POST',
url: '/ajax/switchdebug.php',
body: {
'debug': 'off',
},
}).then(() => {
cy.reload();

cy.getCsrfToken().then((csrf) => {
cy.request({
method: 'POST',
url: '/ajax/switchdebug.php',
headers: {
'Content-Type': 'application/x-www-form-urlencoded',
'X-Requested-With': 'XMLHttpRequest',
'X-Glpi-Csrf-Token': csrf,
},
body: {
'debug': 'off',
},
}).then(() => {
cy.reload();
});
});
});

Expand Down

0 comments on commit 823b4eb

Please sign in to comment.