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

Enhance CSRF handling #18723

Merged
merged 2 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading