fix(security): wave-7 critical C1 (RevertHandler RBAC) + C2 (@self injection allowlist)

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 = [

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');

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']);

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.

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

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

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());
     }
 
     /**