From 5d2ea5d77fedaaa7f9a398578c901bcb99443669 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 28 May 2026 18:42:47 +0200 Subject: [PATCH] fix(security): wave-7 critical C1 (RevertHandler RBAC) + C2 (@self injection allowlist) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL #1 — RevertHandler RBAC bypass (C1): RevertHandler::revert() called findAcrossAllSources with _rbac:false and _multitenancy:false, letting any authenticated user revert any object in any tenant without permission checks. NotAuthorizedException was imported but never thrown. Fix: - Add missing constructor with AuditTrailMapper, ContainerInterface, IEventDispatcher, MagicMapper, and new PermissionHandler dependency. - Remove _rbac:false / _multitenancy:false flags — use tenant-scoped find. - After the find, call permissionHandler->hasPermission(schema, 'update', object) and throw NotAuthorizedException when denied. CRITICAL #2 — Client-supplied @self overwrites server-controlled fields (C2): Two-layer fix: Layer 1 — SaveObject::setSelfMetadata (primary): The method accepted owner from raw client $selfData and applied it to the entity before applyOwnerAttribution() ran. For logged-in REST requests, applyOwnerAttribution() overrode it; for background/system contexts with no IUserSession user, the client-supplied owner persisted. Removed the owner setter from setSelfMetadata — applyOwnerAttribution() is the sole authoritative owner setter. Layer 2 — MagicMapper::prepareObjectDataForTable (defense-in-depth): Client could inject @self.owner and @self.authorization via the @self block. Added explicit strip of owner and authorization from $metadata before the DB write. Also made the register/schema override unconditional (previously only applied when empty) to prevent identity field injection. Tests updated: SaveObjectDeepTest, SaveObjectTest, SaveObjectCoverageTest (owner-setting assertions inverted to assert null), MagicMapperTest (register/schema values changed to verify server-param override; owner column asserted null). gate output: php -l clean on all 7 files; phpstan no new errors (2 pre-existing errors in test files unchanged); phpunit 457/457 pass on modified test files. References wave-7 deep review (project_deep-review-findings-2026-05-27.md) --- lib/Db/MagicMapper.php | 48 +++++++++++++++---- lib/Service/Object/RevertHandler.php | 47 ++++++++++++++++-- lib/Service/Object/SaveObject.php | 13 +++-- tests/Unit/Service/MagicMapperTest.php | 42 ++++++++++------ .../Service/Object/SaveObjectDeepTest.php | 14 ++++-- tests/Unit/Service/Object/SaveObjectTest.php | 11 +++-- .../ObjectHandlers/SaveObjectCoverageTest.php | 12 +++-- 7 files changed, 147 insertions(+), 40 deletions(-) diff --git a/lib/Db/MagicMapper.php b/lib/Db/MagicMapper.php index bf172e3f97..c9f6015790 100644 --- a/lib/Db/MagicMapper.php +++ b/lib/Db/MagicMapper.php @@ -3062,14 +3062,46 @@ private function prepareObjectDataForTable(array $objectData, Register $register $data = $objectData; unset($data['@self']); - // Ensure register and schema IDs are set correctly. - if (empty($metadata['register']) === true) { - $metadata['register'] = $register->getId(); - } - - if (empty($metadata['schema']) === true) { - $metadata['schema'] = $schema->getId(); - } + // SECURITY (wave-7 CRITICAL C2 — @self allowlist enforcement): + // Clients must not be able to overwrite server-controlled fields via the @self + // block. The primary defence for field-level injection lives in + // SaveObject::setSelfMetadata (which now strips owner/authorization from raw + // client $selfData before applying them to the entity). This layer adds + // defense-in-depth at the DB write boundary for fields that can be reached by + // any code path, not just the REST request path. + // + // Fields handled here: + // + // owner – stripped from client @self to prevent ownership hijacking. + // For the internal entity-serialization path the owner was + // stamped by SaveObject::applyOwnerAttribution before + // jsonSerialize() was called. Stripping it here prevents any + // residual client value from persisting. The DB column will + // receive the value set by the entity's setter (not @self). + // Note: this means that for any path that does NOT go through + // applyOwnerAttribution, _owner will be null. That is safer + // than persisting an attacker-controlled value. + // + // authorization – per-object RBAC rules must not be writable by ordinary + // object-create/update calls. Management of per-object + // authorization is intentionally limited to the dedicated + // authorization management endpoint. + // + // register / schema – always derived from the authoritative $register and + // $schema method parameters. Unconditional override (extends + // the previous conditional-on-empty guard to be absolute). + // + // The following fields are intentionally NOT stripped here because they carry + // legitimate values set by the server before this function is reached: + // version, created, updated, deleted, locked, retention, uuid, slug, uri + // Those fields must be fixed at the SaveObject/controller level if client + // injection is possible for any of them. + unset($metadata['owner'], $metadata['authorization']); + + // Always force register and schema from the authoritative server parameters — + // never accept client-supplied values, even when non-empty. + $metadata['register'] = $register->getId(); + $metadata['schema'] = $schema->getId(); // Map metadata fields with prefix. $metadataFields = [ diff --git a/lib/Service/Object/RevertHandler.php b/lib/Service/Object/RevertHandler.php index 521a6730d3..8451950c21 100644 --- a/lib/Service/Object/RevertHandler.php +++ b/lib/Service/Object/RevertHandler.php @@ -71,6 +71,36 @@ class RevertHandler */ private MagicMapper $objectEntityMapper; + /** + * Permission handler for RBAC enforcement + * + * @var PermissionHandler + */ + private PermissionHandler $permissionHandler; + + /** + * RevertHandler constructor. + * + * @param AuditTrailMapper $auditTrailMapper Audit trail mapper. + * @param ContainerInterface $container DI container. + * @param IEventDispatcher $eventDispatcher Event dispatcher. + * @param MagicMapper $objectEntityMapper Object entity mapper. + * @param PermissionHandler $permissionHandler Permission handler for RBAC. + */ + public function __construct( + AuditTrailMapper $auditTrailMapper, + ContainerInterface $container, + IEventDispatcher $eventDispatcher, + MagicMapper $objectEntityMapper, + PermissionHandler $permissionHandler + ) { + $this->auditTrailMapper = $auditTrailMapper; + $this->container = $container; + $this->eventDispatcher = $eventDispatcher; + $this->objectEntityMapper = $objectEntityMapper; + $this->permissionHandler = $permissionHandler; + }//end __construct() + /** * Revert an object to a previous state * @@ -98,12 +128,10 @@ public function revert( mixed $until, bool $overwriteVersion=false ): ObjectEntity { - // Get the object with context (searches across all magic tables). + // Get the object with RBAC and multitenancy enforced (tenant-scoped find). $context = $this->objectEntityMapper->findAcrossAllSources( identifier: $id, - includeDeleted: false, - _rbac: false, - _multitenancy: false + includeDeleted: false ); $object = $context['object']; $registerEntity = $context['register']; @@ -114,6 +142,17 @@ public function revert( throw new DoesNotExistException('Object not found in specified register/schema'); } + // Enforce RBAC: the caller must have 'update' permission on this object. + if ($this->permissionHandler->hasPermission( + schema: $schemaEntity, + action: 'update', + object: $object + ) === false) { + throw new NotAuthorizedException( + message: 'You do not have permission to revert this object' + ); + } + // Check if the object is locked. if ($object->isLocked() === true) { $userId = $this->container->get('userId'); diff --git a/lib/Service/Object/SaveObject.php b/lib/Service/Object/SaveObject.php index 0d7974b513..111dd866b8 100644 --- a/lib/Service/Object/SaveObject.php +++ b/lib/Service/Object/SaveObject.php @@ -3604,9 +3604,16 @@ private function setSelfMetadata(ObjectEntity $objectEntity, array $selfData, ar $objectEntity->setSlug($slug); } - if (array_key_exists('owner', $selfData) === true && empty($selfData['owner']) === false) { - $objectEntity->setOwner($selfData['owner']); - } + // SECURITY (wave-7 CRITICAL C2): owner must NOT be set from client-supplied + // @self input. The sole authoritative setter is applyOwnerAttribution() which + // stamps the session user's UID (or the configured system identifier for + // background jobs) AFTER this method returns. Accepting an owner value here + // would allow any caller to forge object ownership: + // - For authenticated REST requests applyOwnerAttribution() would override it, + // but the defence-in-depth is still worthwhile. + // - For background / system contexts (no IUserSession user) applyOwnerAttribution + // only fills in owner when it is empty, so a client-supplied value would + // persist — that is the actual attack vector closed by this change. if (array_key_exists('organisation', $selfData) === true && empty($selfData['organisation']) === false) { $objectEntity->setOrganisation($selfData['organisation']); diff --git a/tests/Unit/Service/MagicMapperTest.php b/tests/Unit/Service/MagicMapperTest.php index a4de80ec13..15bcbe48ca 100644 --- a/tests/Unit/Service/MagicMapperTest.php +++ b/tests/Unit/Service/MagicMapperTest.php @@ -594,23 +594,30 @@ public function schemaPropertyMappingProvider(): array public function testObjectDataPreparationForTable(): void { $schema = new TestableSchema(); + $schema->setId(42); $schema->testProperties = [ - 'name' => ['type' => 'string'], - 'age' => ['type' => 'integer'], - 'settings' => ['type' => 'object'] + 'name' => ['type' => 'string'], + 'age' => ['type' => 'integer'], + 'settings' => ['type' => 'object'], ]; $objectData = [ '@self' => [ - 'uuid' => 'test-uuid-123', - 'register' => 'test-register', - 'schema' => 'test-schema', - 'owner' => 'testuser', - 'organisation' => 'test-org' + 'uuid' => 'test-uuid-123', + // register and schema are intentionally supplied with wrong values to + // verify that the security fix forces them from the authoritative + // $register/$schema parameters instead (wave-7 CRITICAL C2). + 'register' => 'client-supplied-register', + 'schema' => 'client-supplied-schema', + // owner is intentionally supplied to verify that the security fix strips + // it — owner must only reach the DB via applyOwnerAttribution, not via + // raw @self input. + 'owner' => 'client-supplied-owner', + 'organisation' => 'test-org', ], - 'name' => 'John Doe', - 'age' => 30, - 'settings' => ['theme' => 'dark', 'language' => 'en'] + 'name' => 'John Doe', + 'age' => 30, + 'settings' => ['theme' => 'dark', 'language' => 'en'], ]; $reflection = new \ReflectionClass($this->magicMapper); @@ -621,9 +628,16 @@ public function testObjectDataPreparationForTable(): void // Verify metadata fields are prefixed. $this->assertEquals('test-uuid-123', $result['_uuid']); - $this->assertEquals('test-register', $result['_register']); - $this->assertEquals('test-schema', $result['_schema']); - $this->assertEquals('testuser', $result['_owner']); + + // SECURITY (wave-7 C2): register and schema must come from the authoritative + // method parameters, NOT from client-supplied @self values. + $this->assertEquals(1, $result['_register']); + $this->assertEquals(42, $result['_schema']); + + // SECURITY (wave-7 C2): owner must be stripped from @self — it is controlled + // exclusively by SaveObject::applyOwnerAttribution, not via raw @self input. + $this->assertNull($result['_owner']); + $this->assertEquals('test-org', $result['_organisation']); // Verify schema properties are included. diff --git a/tests/Unit/Service/Object/SaveObjectDeepTest.php b/tests/Unit/Service/Object/SaveObjectDeepTest.php index f3d245408d..f964e10d75 100644 --- a/tests/Unit/Service/Object/SaveObjectDeepTest.php +++ b/tests/Unit/Service/Object/SaveObjectDeepTest.php @@ -1126,11 +1126,19 @@ public function testSetSelfMetadataDepublishedEmpty(): void $this->assertTrue(true); } - public function testSetSelfMetadataOwner(): void + public function testSetSelfMetadataOwnerIsIgnored(): void { + // SECURITY (wave-7 CRITICAL C2): owner must NOT be settable via client @self input. + // The authoritative setter is applyOwnerAttribution() which stamps the session + // user's UID. Accepting owner from $selfData allowed ownership hijacking in + // background/system contexts where applyOwnerAttribution only fills empty owners. $entity = $this->createObjectEntity(1, 'uuid-1'); - $this->invokePrivateMethod('setSelfMetadata', [$entity, ['owner' => 'admin'], []]); - $this->assertSame('admin', $entity->getOwner()); + // Ensure owner starts as null (default for a new entity). + $this->assertNull($entity->getOwner()); + // Pass a client-supplied owner value — it must be silently ignored. + $this->invokePrivateMethod('setSelfMetadata', [$entity, ['owner' => 'injected-owner'], []]); + // Owner remains null; applyOwnerAttribution() will set it from the session. + $this->assertNull($entity->getOwner()); } public function testSetSelfMetadataOrganisation(): void diff --git a/tests/Unit/Service/Object/SaveObjectTest.php b/tests/Unit/Service/Object/SaveObjectTest.php index 505969e483..f605aa3fe6 100644 --- a/tests/Unit/Service/Object/SaveObjectTest.php +++ b/tests/Unit/Service/Object/SaveObjectTest.php @@ -1718,14 +1718,17 @@ public function testSetSelfMetadataSetsDepublishedToNullWhenMissing(): void $this->assertTrue(true); } - public function testSetSelfMetadataSetsOwner(): void + public function testSetSelfMetadataOwnerIsIgnored(): void { - $entity = new ObjectEntity(); - $selfData = ['owner' => 'admin']; + // SECURITY (wave-7 CRITICAL C2): owner must NOT be settable via client @self input. + // The sole authoritative setter is applyOwnerAttribution() (session user UID). + $entity = new ObjectEntity(); + $selfData = ['owner' => 'injected-owner']; $this->invokePrivateMethod('setSelfMetadata', [$entity, $selfData]); - $this->assertSame('admin', $entity->getOwner()); + // Owner must remain null — client-supplied value is discarded. + $this->assertNull($entity->getOwner()); } public function testSetSelfMetadataSetsOrganisation(): void diff --git a/tests/Unit/Service/ObjectHandlers/SaveObjectCoverageTest.php b/tests/Unit/Service/ObjectHandlers/SaveObjectCoverageTest.php index 441f435d48..0b1ee9d6bc 100644 --- a/tests/Unit/Service/ObjectHandlers/SaveObjectCoverageTest.php +++ b/tests/Unit/Service/ObjectHandlers/SaveObjectCoverageTest.php @@ -1491,18 +1491,22 @@ public function testSetSelfMetadataWithInvalidDepublishedDate(): void } /** - * Test setSelfMetadata sets owner. + * Test setSelfMetadata ignores owner from client @self input. + * + * SECURITY (wave-7 CRITICAL C2): owner must NOT be settable via client @self — + * applyOwnerAttribution() is the sole authoritative setter. * * @return void */ - public function testSetSelfMetadataWithOwner(): void + public function testSetSelfMetadataWithOwnerIsIgnored(): void { - $entity = $this->createObjectEntity(1, 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'); + $entity = $this->createObjectEntity(1, 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'); $selfData = ['owner' => 'user123']; $this->invokePrivate('setSelfMetadata', [$entity, $selfData, []]); - $this->assertSame('user123', $entity->getOwner()); + // owner must remain null — the client-supplied value is discarded. + $this->assertNull($entity->getOwner()); } /**