fix(users) : guard anti-bypass sur sites null dans UserRbacProcessor

restoreAbsentCollections utilisait `array_key_exists('sites', $payload)`
qui retourne true pour une valeur null, sautant la restauration.

API Platform rejette deja `sites: null` au denormalize (400 type mismatch),
donc le bypass n'est pas reellement exploitable aujourd'hui via l'API HTTP.
Mais le test `&& is_array(...)` reste une defense-in-depth si la config
denormalizer change un jour, et rend l'intention explicite.

Test de regression : PATCH {sites: null} -> 400 + collection sites intacte
en DB (aucune trace de l'echec).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-04-22 19:35:48 +02:00
parent e0624eace0
commit 277178cf19
2 changed files with 46 additions and 1 deletions

View File

@@ -250,7 +250,12 @@ final class UserRbacProcessor implements ProcessorInterface
}
foreach (self::COLLECTION_MAP as $jsonKey => $accessors) {
if (array_key_exists($jsonKey, $payload)) {
// La garde ne doit sauter la restauration que si le payload fournit
// un VRAI tableau pour cette cle. Un `null`, un scalaire ou un autre
// type doivent etre traites comme "cle absente" : sinon un payload
// `{"sites": null}` contourne la restauration et laisse API Platform
// vider la collection silencieusement (bypass de la garde).
if (array_key_exists($jsonKey, $payload) && is_array($payload[$jsonKey])) {
continue;
}

View File

@@ -139,6 +139,46 @@ final class UserRbacSitesApiTest extends AbstractApiTestCase
self::assertSame('Chatellerault', $reloaded->getCurrentSite()->getName());
}
/**
* Defense-in-depth contre un bypass hypothetique de restoreAbsentCollections.
*
* API Platform rejette deja `sites: null` au denormalize (400 Bad Request,
* type mismatch). Le guard `is_array` dans UserRbacProcessor est une
* deuxieme ligne de defense si la config denormalizer change un jour.
*
* Ce test verrouille deux garanties :
* 1. `{"sites": null}` → 400 (pas un 500, pas un 200 silencieux)
* 2. La collection sites d'alice est intacte apres l'echec
*/
public function testRbacPatchWithNullSitesReturns400AndDoesNotWipeSitesCollection(): void
{
$em = $this->getEm();
$alice = $em->getRepository(User::class)->findOneBy(['username' => 'alice']);
$aliceId = $alice->getId();
self::assertCount(1, $alice->getSites(), 'Pre-condition : alice doit avoir exactement 1 site');
$em->clear();
$client = $this->authenticatedClient('admin', 'admin');
$client->request('PATCH', '/api/users/'.$aliceId.'/rbac', [
'headers' => ['Content-Type' => 'application/merge-patch+json'],
'json' => [
'isAdmin' => false,
'sites' => null,
],
]);
self::assertResponseStatusCodeSame(400);
$em = $this->getEm();
$em->clear();
$reloaded = $em->getRepository(User::class)->find($aliceId);
self::assertCount(
1,
$reloaded->getSites(),
'Un payload `sites: null` rejete en 400 ne doit laisser aucune trace en DB.',
);
}
public function testRbacPatchWithoutSitesFieldDoesNotChangeCurrentSite(): void
{
// Garde structurelle : si le payload /rbac ne contient pas le champ