Skip to content

Commit f0b7298

Browse files
javiereguiluzjaviereguiluz
committed
bug #7556 Improve security of batchDelete() action (javiereguiluz)
This PR was squashed before being merged into the 4.x branch. Discussion ---------- Improve security of batchDelete() action Commits ------- 4cde4cd Improve security of batchDelete() action
2 parents f278004 + 4cde4cd commit f0b7298

5 files changed

Lines changed: 142 additions & 8 deletions

File tree

‎src/Controller/AbstractCrudController.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,10 +435,18 @@ public function batchDelete(AdminContext $context, BatchActionDto $batchActionDt
435435
return $event->getResponse();
436436
}
437437

438-
if (!$this->isCsrfTokenValid('ea-batch-action-'.Action::BATCH_DELETE, $batchActionDto->getCsrfToken())) {
438+
if (!$this->isCsrfTokenValid('ea-batch-action-'.Action::BATCH_DELETE.'-'.$batchActionDto->getEntityFqcn(), $batchActionDto->getCsrfToken())) {
439439
return $this->redirectToRoute($context->getDashboardRouteName());
440440
}
441441

442+
// the entity FQCN in the batch action DTO comes from the POST body and is
443+
// attacker-controlled. The FQCN in the admin context comes from the URL-routed
444+
// CRUD controller. If they disagree, an admin with batch-delete rights on one
445+
// CRUD could delete rows of any other Doctrine entity. Reject the mismatch.
446+
if ($batchActionDto->getEntityFqcn() !== $context->getEntity()->getFqcn()) {
447+
throw new BadRequestHttpException();
448+
}
449+
442450
/** @var EntityManagerInterface $entityManager */
443451
$entityManager = $this->container->get('doctrine')->getManagerForClass($batchActionDto->getEntityFqcn());
444452
$repository = $entityManager->getRepository($batchActionDto->getEntityFqcn());

‎src/Dto/EntityDto.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,11 @@ public function isEmbeddedClassProperty(string $propertyName): bool
306306
*/
307307
public function setInstance(?object $newEntityInstance): void
308308
{
309-
if (null !== $this->instance && null !== $newEntityInstance && !$newEntityInstance instanceof $this->fqcn) {
309+
// the instanceof guard must run even when $this->instance is null. Otherwise
310+
// a caller can store an instance whose class does not match $this->fqcn, and
311+
// downstream code (authorization, DB operations) that trusts either side of
312+
// that pair may be redirected to the wrong entity (this is a CWE-441 (Confused Deputy) attack vector)
313+
if (null !== $newEntityInstance && !$newEntityInstance instanceof $this->fqcn) {
310314
throw new \InvalidArgumentException(sprintf('The new entity instance must be of the same type as the previous instance (original instance: "%s", new instance: "%s").', $this->fqcn, $newEntityInstance::class));
311315
}
312316

@@ -331,7 +335,12 @@ public function newWithInstance(/* object */ $newEntityInstance): self
331335
);
332336
}
333337

334-
if (null !== $this->instance && !$newEntityInstance instanceof $this->fqcn) {
338+
// the instanceof guard must run even when $this->instance is null. Otherwise
339+
// a caller that wraps an entity into a DTO whose $fqcn was set from a different
340+
// source (e.g. batch actions, where the FQCN comes from the admin context but
341+
// the instance comes from a repository lookup) can silently produce a DTO
342+
// whose $fqcn does not match its $instance (this is a CWE-441 (Confused Deputy) attack vector).
343+
if (!$newEntityInstance instanceof $this->fqcn) {
335344
throw new \InvalidArgumentException(sprintf('The new entity instance must be of the same type as the previous instance (original instance: "%s", new instance: "%s").', $this->fqcn, $newEntityInstance::class));
336345
}
337346

‎src/Factory/ActionFactory.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,9 @@ private function processAction(string $pageName, ActionDto $actionDto, ?EntityDt
312312

313313
if ($actionDto->isBatchAction()) {
314314
$batchActionAttributes = [
315-
'data-action-csrf-token' => $this->csrfTokenManager?->getToken('ea-batch-action-'.$actionDto->getName()),
315+
// the token id is bound to the entity FQCN so a token minted for
316+
// one CRUD cannot be replayed against another CRUD's batch action.
317+
'data-action-csrf-token' => $this->csrfTokenManager?->getToken('ea-batch-action-'.$actionDto->getName().'-'.$adminContext->getCrud()->getEntityFqcn()),
316318
'data-action-batch' => 'true',
317319
'data-entity-fqcn' => $adminContext->getCrud()->getEntityFqcn(),
318320
'data-action-url' => $actionDto->getLinkUrl(),

‎tests/Functional/BatchActions/BatchDeleteTest.php‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use EasyCorp\Bundle\EasyAdminBundle\Test\AbstractCrudTestCase;
88
use EasyCorp\Bundle\EasyAdminBundle\Tests\Functional\Apps\DefaultApp\Controller\DashboardController;
99
use EasyCorp\Bundle\EasyAdminBundle\Tests\Functional\Apps\DefaultApp\Controller\Synthetic\BatchActionTestEntityCrudController;
10+
use EasyCorp\Bundle\EasyAdminBundle\Tests\Functional\Apps\DefaultApp\Entity\Category;
1011
use EasyCorp\Bundle\EasyAdminBundle\Tests\Functional\Apps\DefaultApp\Entity\Synthetic\BatchActionTestEntity;
1112
use EasyCorp\Bundle\EasyAdminBundle\Tests\Functional\Apps\DefaultApp\Kernel;
1213
use Symfony\Component\DomCrawler\Crawler;
@@ -140,6 +141,31 @@ public function testBatchDeleteWithNonExistentEntityId(): void
140141
self::assertSame($initialCount, $finalCount, 'No entities should have been deleted with non-existent ID');
141142
}
142143

144+
public function testBatchDeleteRejectsMismatchedEntityFqcn(): void
145+
{
146+
$categoryRepository = $this->entityManager->getRepository(Category::class);
147+
$initialCategoryCount = \count($categoryRepository->findAll());
148+
self::assertGreaterThan(0, $initialCategoryCount, 'Need at least 1 Category fixture for this test');
149+
150+
$targetCategoryId = $categoryRepository->findAll()[0]->getId();
151+
$initialBatchEntityCount = \count($this->repository->findAll());
152+
153+
// simulate an attacker that can batchDelete on BatchActionTestEntityCrudController
154+
// but targets a different Doctrine entity (Category) by overriding entityFqcn in
155+
// the POST body. The attack must be blocked (either by the FQCN-bound CSRF token
156+
// or by the explicit mismatch check) and no rows of any entity may be deleted.
157+
$this->submitBatchDelete([$targetCategoryId], entityFqcnOverride: Category::class);
158+
159+
$this->entityManager->clear();
160+
161+
self::assertNotNull(
162+
$categoryRepository->find($targetCategoryId),
163+
'Category row must not be deleted by a cross-entity batchDelete attempt'
164+
);
165+
self::assertCount($initialCategoryCount, $categoryRepository->findAll());
166+
self::assertCount($initialBatchEntityCount, $this->repository->findAll());
167+
}
168+
143169
public function testBatchDeleteRedirectsToIndexPage(): void
144170
{
145171
// get first entity ID
@@ -163,12 +189,14 @@ public function testBatchDeleteRedirectsToIndexPage(): void
163189
* 2. Extracting the data attributes needed for submission
164190
* 3. Submitting a POST request with the same parameters the JavaScript would send
165191
*
166-
* @param array $entityIds The entity IDs to delete
167-
* @param bool $useValidCsrfToken Whether to use a valid CSRF token (false to test CSRF protection)
192+
* @param array $entityIds The entity IDs to delete
193+
* @param bool $useValidCsrfToken Whether to use a valid CSRF token (false to test CSRF protection)
194+
* @param string|null $entityFqcnOverride When set, replaces the body's entityFqcn with this class (used to
195+
* simulate a cross-entity authorization-bypass attempt)
168196
*
169197
* @return Crawler The crawler after submitting the batch action
170198
*/
171-
private function submitBatchDelete(array $entityIds, bool $useValidCsrfToken = true): Crawler
199+
private function submitBatchDelete(array $entityIds, bool $useValidCsrfToken = true, ?string $entityFqcnOverride = null): Crawler
172200
{
173201
$crawler = $this->client->request('GET', $this->generateIndexUrl());
174202
self::assertResponseIsSuccessful();
@@ -179,7 +207,7 @@ private function submitBatchDelete(array $entityIds, bool $useValidCsrfToken = t
179207

180208
$csrfToken = $useValidCsrfToken ? $batchDeleteButton->attr('data-action-csrf-token') : 'invalid-csrf-token';
181209
$batchActionUrl = $batchDeleteButton->attr('data-action-url');
182-
$entityFqcn = $batchDeleteButton->attr('data-entity-fqcn');
210+
$entityFqcn = $entityFqcnOverride ?? $batchDeleteButton->attr('data-entity-fqcn');
183211

184212
// submit the batch delete action
185213
return $this->client->request('POST', $batchActionUrl, [

‎tests/Unit/Dto/EntityDtoTest.php‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
<?php
2+
3+
namespace EasyCorp\Bundle\EasyAdminBundle\Tests\Unit\Dto;
4+
5+
use Doctrine\ORM\Mapping\ClassMetadata;
6+
use EasyCorp\Bundle\EasyAdminBundle\Dto\EntityDto;
7+
use PHPUnit\Framework\TestCase;
8+
9+
class EntityDtoTest extends TestCase
10+
{
11+
public function testNewWithInstanceAcceptsMatchingInstance(): void
12+
{
13+
$dto = $this->createEntityDto(\stdClass::class);
14+
15+
$newDto = $dto->newWithInstance($instance = new \stdClass());
16+
17+
self::assertNotSame($dto, $newDto);
18+
self::assertSame($instance, $newDto->getInstance());
19+
self::assertSame(\stdClass::class, $newDto->getFqcn());
20+
}
21+
22+
public function testNewWithInstanceRejectsMismatchedInstanceOnEmptyDto(): void
23+
{
24+
// a DTO whose $instance is null must still reject a mismatched instance;
25+
// before the fix, the instanceof guard was gated on "null !== $this->instance"
26+
// and this call silently produced a DTO whose $fqcn did not match its $instance
27+
// (CWE-441 Confused Deputy, exploited by the batchDelete cross-entity bypass).
28+
$dto = $this->createEntityDto(\stdClass::class);
29+
30+
$this->expectException(\InvalidArgumentException::class);
31+
32+
$dto->newWithInstance(new \DateTime());
33+
}
34+
35+
public function testNewWithInstanceRejectsMismatchedInstanceOnPopulatedDto(): void
36+
{
37+
$dto = $this->createEntityDto(\stdClass::class, new \stdClass());
38+
39+
$this->expectException(\InvalidArgumentException::class);
40+
41+
$dto->newWithInstance(new \DateTime());
42+
}
43+
44+
public function testSetInstanceAcceptsMatchingInstance(): void
45+
{
46+
$dto = $this->createEntityDto(\stdClass::class);
47+
48+
$dto->setInstance($instance = new \stdClass());
49+
50+
self::assertSame($instance, $dto->getInstance());
51+
}
52+
53+
public function testSetInstanceAcceptsNull(): void
54+
{
55+
$dto = $this->createEntityDto(\stdClass::class, new \stdClass());
56+
57+
$dto->setInstance(null);
58+
59+
self::assertNull($dto->getInstance());
60+
}
61+
62+
public function testSetInstanceRejectsMismatchedInstanceOnEmptyDto(): void
63+
{
64+
// same defense as in newWithInstance: the instanceof guard must run even when
65+
// $this->instance is null, so that a fresh DTO cannot be populated with an
66+
// instance whose class does not match its $fqcn.
67+
$dto = $this->createEntityDto(\stdClass::class);
68+
69+
$this->expectException(\InvalidArgumentException::class);
70+
71+
$dto->setInstance(new \DateTime());
72+
}
73+
74+
public function testSetInstanceRejectsMismatchedInstanceOnPopulatedDto(): void
75+
{
76+
$dto = $this->createEntityDto(\stdClass::class, new \stdClass());
77+
78+
$this->expectException(\InvalidArgumentException::class);
79+
80+
$dto->setInstance(new \DateTime());
81+
}
82+
83+
private function createEntityDto(string $fqcn, ?object $instance = null): EntityDto
84+
{
85+
return new EntityDto($fqcn, $this->createStub(ClassMetadata::class), null, $instance);
86+
}
87+
}

0 commit comments

Comments
 (0)