This repository was archived by the owner on May 29, 2026. It is now read-only.
fix(security): wave-7 critical C1 (RevertHandler RBAC) + C2 (@self injection allowlist)#2008
Merged
Merged
Conversation
…jection allowlist) 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)
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR closes two wave-7 CRITICAL security findings from the deep team-reviewer pass (2026-05-27). See
project_deep-review-findings-2026-05-27.mdin project memory.CRITICAL #1 — RevertHandler bypasses RBAC + multitenancy
File:
lib/Service/Object/RevertHandler.phpVulnerability:
revert()calledfindAcrossAllSources(_rbac: false, _multitenancy: false), allowing any authenticated user to revert any object in any tenant.NotAuthorizedExceptionwas imported but never thrown.Fix:
PermissionHandlerdependency_rbac: false/_multitenancy: false— the find is now tenant-scoped with RBAC enforcedpermissionHandler->hasPermission(schema, 'update', object)is checked; throwsNotAuthorizedExceptionon denial (correctly caught byRevertControllerfor a 403)CRITICAL #2 — Client-supplied
@selfoverwrites server-controlled security fieldsFiles:
lib/Service/Object/SaveObject.php,lib/Db/MagicMapper.phpVulnerability: A client POST with
{"@self": {"owner": "admin"}}set the object owner to an arbitrary value. For authenticated REST requestsapplyOwnerAttribution()overrode this — but for background/system contexts (OpenConnector sync, cron) with noIUserSessionuser,applyOwnerAttributiononly fills owner when empty, so the client-controlled value persisted.Fix (two layers):
Layer 1 —
SaveObject::setSelfMetadata(primary): Removed theownersetter from the method.applyOwnerAttribution()is now the sole authoritative owner setter.Layer 2 —
MagicMapper::prepareObjectDataForTable(defense-in-depth): Stripsownerandauthorizationfrom the@selfblock before DB write. Makesregisterandschemaoverride unconditional (was conditional-on-empty), preventing identity field injection.Tests
Four test files updated to invert owner-injection assertions (owner-is-set → owner-is-null/ignored):
tests/Unit/Service/Object/SaveObjectDeepTest.phptests/Unit/Service/Object/SaveObjectTest.phptests/Unit/Service/ObjectHandlers/SaveObjectCoverageTest.phptests/Unit/Service/MagicMapperTest.phpGate output:
php -l: clean on all 7 changed filesphpstan: no new errors (2 pre-existing test-file errors unchanged)phpunit: 457/457 pass on all modified test files