fix(security): tenant-scope object-name resolution — closes gate-7 (SEC-CTRL-2 step 2)

[rubenvdlinde]

What this closes

development's last failing gate: gate-7 no-admin-idor — 2 method(s) (NamesController::index and ::create). The finding is true, not a false positive, and it is closed by fixing the defect — no @no-admin-idor-exempt tag, and lib/Controller/NamesController.php is byte-identical to development apart from two stale TODO(SEC-CTRL-2) comments. The fix lives in the collaborator, which is where the leak was.

This is SEC-CTRL-2 step 2, written down in the repo's own CODE-REVIEW-IMPROVEMENT-PLAN.md. Step 1 (#2523) removed show/stats/warmup and dropped @PublicPage; it left an authenticated cross-tenant name disclosure open.

The defect, measured

CacheHandler::getMultipleObjectNames() / getAllObjectNames() resolved names through three paths with no tenant dimension at all —

• OrganisationMapper::findMultipleByUuid() / findAllWithUserCount() (every organisation),
• MagicMapper::findMultiple() → findMultipleAcrossAllMagicTables(), a raw UNION ALL over every magic table with no organisation predicate and no flag to switch on,
• loadNamesFromMagicTables() / queryTableForNames(), which selected _uuid, _name and never looked at _organisation,

— and then cached the answer in a process-wide map that any later caller could read, plus a distributed cache shared by every tenant on the instance.

What this does

1. A per-object authorisation predicate. hasOrganisationAccess() mirrors Db\MultiTenancyTrait::applyOrganisationFilter() exactly: multitenancy switched off, or an admin under an enabled (non-SaaS) override, is unrestricted; otherwise the caller sees the active organisation plus its parents; no active organisation means no names (the trait's 1 = 0 arm). An entity whose owning organisation cannot be established is refused — absence of tenancy is not permission to disclose.

2. The magic-table queries now select _organisation. That column is the tenancy oracle for every object-backed name, because — measured while writing this — MagicMapper::rowToObjectEntity() never populates organisation on the entities it builds, so filtering on $object->getOrganisation() alone would have filtered on null for every row read out of a magic table. Left alone deliberately: populating it would switch on PermissionHandler::filterUuidsForPermissions()'s currently-dead organisation branch, which is its own change. Recorded in the plan.

3. The cache KEY is deliberately unchanged; the tenancy rides in the VALUE. In memory as a parallel nameOrganisations map, in the distributed cache as a {n, o} envelope under the same name_<identifier> key.

   The dispatch's suggested mechanism was to change the key. This is better and the reason is the failure mode the previous attempt hit: prefixing the key forces every invalidation site to learn the prefix, and it orphans an entry under a stale prefix whenever an object moves organisation — a stale name servable to the old tenant forever. Keeping the key means all six invalidation sites keep matching untouched:

   #	site	still correct because
   1	invalidateForObjectChange() create/update	rewrites the same key, now with organisation:
   2	invalidateForObjectChange() delete	unsets the same keys, plus the new organisation map
   3	clearObjectNameFromCache() (BUG-OBJ-7)	same keys, plus the organisation map
   4	clearAllCaches()	clears everything, plus the organisation map
   5	clearNameCache()	clears everything, plus the organisation map
   6	persistNameCacheToDistributed()	same keys, now writing envelopes

   A value written before this change is a bare string with no tenancy and is read as a MISS, not served unscoped — the fail-closed direction across a deploy.

4. The scope is memoised for one call and dropped at the top of every public reader, so a cron worker or any long-lived process serving several callers cannot carry one caller's scope into the next.

Also fixed on the way, both reachable now: batchLoadNamesFromMagicTables() fataled on a null registerMapper, and getSingleObjectName() passed a null name into a non-nullable parameter for an entity with neither name nor uuid.

RED-then-green control

tests/Unit/Service/Object/CacheHandlerTenantScopeTest.php, committed first, measured against the pre-fix implementation:

Tests: 8, Assertions: 10, Failures: 6      <- before the fix
Tests: 9, Assertions: 15, Failures: 0      <- after

The six that were RED: the object-mapper path, the organisation path, a cache warmed as tenant A served to tenant B, getAllObjectNames() handing over the whole warm cache, the magic-table warmup ignoring _organisation, and a name with unresolvable tenancy being served anyway. The two that were GREEN throughout are functional controls (in-scope names must still resolve; an empty request answers empty) — they are what a fix that simply returned nothing would fail.

Gate-7, before and after

Gate package ConductionNL/.github@742f370e (today's main), the gate's own helper, 139 controller files scanned on both sides:

origin/development @ 1749c1d3b  -> 2 findings   NamesController.php:115 index
                                                NamesController.php:264 create
this branch        @ 969d97c33  -> 0 findings

The base count reproduces CI's gate-7 no-admin-idor — 2 method(s) exactly, which is the calibration. Positive control on the same package and tree: an unguarded #[NoAdminRequired] show(string $id) planted in lib/Controller/ is reported (1 finding), removed again → 0. (The first probe I planted read as clean — it called a *Mapper, which Pattern 2 exempts inside this repo. Worth knowing.)

The clearing mechanism is the gate's Pattern 4: CacheHandler is a typed collaborator of NamesController, and its name readers now call an authorisation predicate the gate reads out of CacheHandler's own source. The controller itself is unchanged.

Regression evidence

Full unit suite, same container, same command, base vs head:

origin/development @ 1749c1d3b   Tests: 16657, Errors: 8, Failures: 0
this branch        @ 969d97c33   Tests: 16665, Errors: 8, Failures: 0

The 8 errors are the identical pre-existing ones in IconControllerTest (3), GraphQLControllerExplorerTest (3) and WebPushControllerTest (2) — local-environment, present on the base. +8 tests are the new controls. phpcs and phpmd are clean on all three changed lib/ files. phpstan/psalm cannot run in a worktree here (the vendored quality-config resolves a path that does not exist locally) — CI is the measurement for those.

The pre-existing CacheHandler suites are now pinned to multitenancy.enabled = false — the configuration they were actually written against — and say so in a docblock. Nothing is left unmeasured: the scoped behaviour has its own nine controls.

The six internal callers

Every one consumes array<uuid, string> and already tolerates an absent key; the only change they can observe is that the map is now partial. Pinned by testPartialMapContractHoldsForCallersThatIndexTheResult:

caller	handles absence by
Controller\ObjectsController::collectNamesForResponse	returns the map; an absent key renders as the raw UUID. Only ever asks for UUIDs inside an object the caller already read, so its ids are in scope by construction
Service\ObjectService::collectNamesForResults	returns the map
Service\ExportService::resolveUuidNameMap	array_merge onto a pre-seeded map
Service\Object\SaveObject\MetadataHydrationHandler	empty($names[$uuid]) === false, else falls back to the UUID
Service\Object\PerformanceHandler	assigns the map to relatedNames
Db\MagicMapper\MagicFacetHandler	isset($names[$value]) at both single-value sites (falls back to a shortened UUID); its batch site foreaches over exactly what came back

Plus the three external setObjectName() writers in SaveObject.php, all now passing organisation:.

What this does NOT close

• RBAC is not evaluated. Only the organisation dimension is. A caller in the right organisation still resolves a name for an object whose register/schema authorization.read would refuse them. Closing that needs a Schema the name cache does not hold — PermissionHandler::hasPermission() requires one — and is a separate change.
• getSingleObjectName()'s database lookup is still _rbac: false, _multitenancy: false. It is the ANSWER that is gated, not the query. The method has no caller in lib/; its docblock now says exactly this rather than the old "deliberately unscoped".
• MagicMapper::rowToObjectEntity() still drops _organisation. Recorded, not fixed.

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/openregister @ 8b27f23

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	❌
psalm	✅
phpstan	❌
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
composer			✅	✅ 175/175
npm			✅	✅ 528/528
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					❌
Playwright					❌
Hydra gates					❌

---

Quality workflow — 2026-08-17 07:47 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openregister @ 0c1e5e0

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
composer			✅	✅ 175/175
npm			✅	✅ 528/528
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					✅
Hydra gates					✅

---

Quality workflow — 2026-08-17 08:00 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/openregister @ 6041f64

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
composer			✅	✅ 175/175
npm			✅	✅ 528/528
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					✅

---

Quality workflow — 2026-08-17 08:27 UTC

Download the full PDF report from the workflow artifacts.