Skip to content

fix(scope): make {register} a hard boundary on every path that names one - #2774

Merged
rubenvdlinde merged 7 commits into
developmentfrom
fix/aggregation-register-scope
Aug 22, 2026
Merged

fix(scope): make {register} a hard boundary on every path that names one#2774
rubenvdlinde merged 7 commits into
developmentfrom
fix/aggregation-register-scope

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Third and final site of the slug-collision class (after #2694 fixed schemas#show, and nextcloud-vue#725 made the client send the scope).

Measured live on a fleet instance — same register, same slug, same filter:

  • GET /api/objects/aggregations/hrmq/TimeEntry/value?metric=sum&field=hours&filter[userId]=admin returned 0
  • GET /api/objects/hrmq/TimeEntry?userId=admin returned 2 rows totalling 168 hours

The aggregation path resolved the schema slug instance-wide and landed on another app's schema (planix's TimeEntry #161 instead of hrmq's #9466), so a dashboard stat widget silently counted another app's rows. Single-app instances and CI cannot reproduce it — it needs a fleet-shaped box.

Rather than patching the one endpoint I reported, this generalises the rule: a path that names a register never falls back to instance-wide resolution, via a shared RegisterScopedSchemaResolver applied across every affected controller and service.

Found while building humaniq's personal dashboard, whose two stat widgets are the first consumers.

🤖 Generated with Claude Code

Found live on a fleet instance 2026-08-21, on the endpoint the dashboard
`stat` widget calls: GET /api/objects/aggregations/{register}/{schema}/value
accepted the {register} path segment and never used it. AggregationRunner::run()
and ::runAdhocByRef() both opened with a GLOBAL SchemaMapper::find($schemaRef)
and loaded the register afterwards, so by the time the register was known the
schema had already been matched against every register and every app on the
instance. `TimeEntry` resolved to planix's schema 161 instead of hrmq's 9466,
and `Expense` to pipelinq's 507 instead of hrmq's 5026 — a widget aggregating
another app's rows. Single-app instances and CI cannot reproduce it.

This is the defect class #2694 fixed for schemas#show. That fix was written
INLINE and PRIVATE in SchemasController, so none of the dozen other sites that
hold a register ref could reuse it, and each went on resolving globally. The
resolution is therefore extracted, not copied:

  lib/Service/RegisterScopedSchemaResolver.php — resolveRegister() /
  resolveSchemaWithin() / resolvePair(), with #2694's exact semantics: every
  identifier form (numeric id, uuid, case-insensitive slug) matched only among
  the register's carried ids via SchemaMapper::findInIds(), an unresolvable
  register refused with RegisterNotFoundException instead of widening back to
  global, and a scoped miss refused with SchemaNotInRegisterException carrying
  the register, the same-slug candidate count and the relink-schemas command.

Sites converted to it, all previously resolving instance-wide with a register
in hand:

  - AggregationRunner::run/runAdhocByRef/findSchema — aggregate, value, grouped,
    timeseries. The mis-resolution drove the x-openregister-aggregations
    annotation AND the RBAC `list` gate, not just the number.
  - ObjectService::setRegister/setSchema — scoping applied to SLUGS only (a
    numeric id or uuid still resolved globally) and only when setRegister ran
    FIRST. The inverted order is the shape of the copy-pasted validateObject()
    helper in ~24 link controllers and of every resolution in FilesController,
    so setRegister() now re-resolves a pending schema ref: the boundary holds
    whichever way round the setters are called, fixed in one place.
  - MigrationService::resolveRegisterAndSchema — status + MigrateStorageCommand.
  - QualityStatisticsService / DuplicateDetectionService loadAnnotation() — the
    object set was loaded with both refs while the annotation drifted, so
    another app's thresholds and match rules judged this register's rows.
  - ContextsController::schema — a published, cacheable JSON-LD @context.
  - TmloController exportSingle/exportBatch/summary — archival metadata.
  - TablesController::sync/syncAll — DDL against
    oc_openregister_table_<registerId>_<schemaId>.
  - ObjectsController::export — reuses the already-scoped entities instead of
    re-resolving globally for the filename.
  - SchemasController::resolveSchemaInRegister — now delegates, so one
    implementation owns the wording and the identifier forms.

Behaviour is unchanged for every caller that names no register: findSchema()
without a registerRef, the cross-schema `from:` target (a schema-author ref that
may legitimately point at another register), and ObjectService with no register
context all keep global resolution.

AggregationRunner::loadRegister() is removed rather than left behind: a path
that resolves a register WITHOUT then bounding a schema by it is the shape this
change deletes. Its metadata-read policy (_multitenancy: false) is unchanged,
moved into the resolver, and still locked by
AggregationRunnerTest::testRegisterLoadPassesMultitenancyFalse.

No spec Scenario is added — gate-19 requires an e2e reference for a new one, and
#2694 set the precedent of documenting in docblocks instead. Every touched
method carries @SPEC openspec/specs/register-scoped-slug-resolution/spec.md.

Tests: new AggregationRegisterScopeTest mirrors #2694's style — scoped hit
(global resolver asserted never to run), scoped miss with the slug existing
elsewhere, unknown register, numeric id outside the register, empty schemas
list, and both halves of the control (register-less callers keep global
resolution; a named register never reaches it).
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 182cfad

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-22 01:10 UTC

Download the full PDF report from the workflow artifacts.

Completes the sweep: ObjectService::setRegister() re-resolves a pending
schema ref (which fixes the ~45 call sites that call setSchema before
setRegister in one place instead of 45 edits), plus MigrationService and
the two quality services, with the matching controller/service tests.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ b9f9e6c

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-22 06:24 UTC

Download the full PDF report from the workflow artifacts.

Conduction Release Bot added 3 commits August 22, 2026 08:26
…rg host

gate-94 (retired-git-host-metadata) flags six URLs in appinfo/info.xml —
website, bugs, repository and three screenshots — that still point at
codeberg.org. Codeberg is retired for this fleet; GitHub is the only
host, issues included. Screenshots move to raw.githubusercontent.com,
GitHub's equivalent of the raw branch path.
…r test

The controller now resolves a schema within the register's carried ids
(findInIds) rather than instance-wide, so the three tests that stubbed
only find() fell through to an unresolved schema. testConditionalGetReturns304
surfaced it as a TypeError — the error response carries no ETag, and the
test assigns that null into a typed string property. That test skips
outside the Nextcloud container, which is why CI caught it and a
standalone clone could not.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ ab3bc1a

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-22 06:44 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 4009229

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-22 07:00 UTC

Download the full PDF report from the workflow artifacts.

…iers

The boundary was applied to every identifier form, which is stricter than
the problem it solves and broke real callers: POST /api/objects/{registerId}/{schemaId}
began returning 404 whenever the register's schemas array did not list the
schema — measured as 10 failed assertions in the Newman suite, where the
first failed create cascaded into every later request.

A slug is ambiguous instance-wide (several registers legitimately carry a
TimeEntry) and that ambiguity is the entire bug: resolving one globally
served another app's schema into a leaf app's forms and aggregations. A
numeric id or uuid is unique by construction, so scoping it protects
nothing and can only refuse a caller whose membership list is stale. Unique
identifiers therefore resolve globally, and the list is treated as the
cache it is; a genuinely absent identifier still refuses, because the
widening is for ambiguity and never for absence.

Two tests that pinned the over-strict rule now pin the corrected one, and
a dedicated resolver suite covers both halves.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 318593b

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-22 08:08 UTC

Download the full PDF report from the workflow artifacts.

The test refused a numeric schema id, which the narrowed rule now
resolves globally — a numeric id is unique by construction, so scoping
it protects nothing and only punishes a stale membership list. The
refusal it means to prove is a slug refusal, so it now uses one.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ eeaedc0

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-22 08:23 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 5e7471b into development Aug 22, 2026
48 of 76 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/aggregation-register-scope branch August 22, 2026 08:24
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Possible fallout: buildiq's Docudesk-template E2E went red on development

Flagging rather than asserting — I have a coherent causal chain but not a bisect.

The fleet drift sweep (run 32565741872) found buildiq red:

✘ tests/e2e/spec-coverage/docudesk-document-templates.spec.ts:179
  REQ-DDT-002 — attach a Docudesk template via the Documents section
  Test timeout of 30000ms exceeded.
  waiting for getByRole('option').filter({ hasText: /Bevestigingsbrief/i })

The template option never appears in the picker.

Why this PR is the leading suspect

buildiq's own tree has not moved, and the same SHA passed 8 hours earlier:

buildiq SHA 6c190470 passed 01:56, failed 09:44 and again on re-run
ConductionNL/docudesk @ main (its other sibling app) unchanged since 08-04
openregister @ development (cloned by that job) moved — this PR landed 08:24

So the variable is openregister's development, and this PR is the change on it that touches exactly what the test does: reading objects that belong to another app's register. The test attaches a Docudesk template from buildiq, which is a cross-register read; making {register} a hard boundary is the one change today that would alter whether such a lookup resolves.

Two other object fixes landed in the same window (bulk-save #2779, pending schema ref #2790), so I am not ruling those out.

What I have not done

Bisected it, or touched anything. This is your change, live-verified against a real fleet instance, and the failing assertion may equally be buildiq's test relying on the global resolution that was the bug. Worth a look from whoever owns the boundary decision — if the cross-register read is now intentionally refused, buildiq's spec needs to name the register rather than the fix being reverted.

Context: the rest of the fleet is green — 19 of 21 apps passed the same sweep. The other failure (integriq) is unrelated and reported on ConductionNL/integriq#1526.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Correction and a sharper signal

My earlier comment guessed this was a blocked cross-register read. That guess had a missing link I had not checked: buildiq fetches templates from docudesk's own endpoint (/apps/docudesk/api/templates), not from openregister's object API. The real signal is upstream of the picker, in buildiq's global-setup:

[globalSetup] docudesk template "Bevestigingsbrief" create returned 500
[globalSetup] docudesk template "Besluit" create returned 500

POST /apps/docudesk/api/templatesHTTP 500. That path lands in TemplateService::createTemplate(), which calls openregister's ObjectService::saveObject() after RegisterResolver::getRegisterAndSchema(). The empty picker and the Playwright timeout are downstream consequences, not the fault.

buildiq's own log then reported templates ready … (already present) despite both creates failing, which is why this took a while to see — fixed separately in ConductionNL/buildiq#329.

This may already be fixed

The scoping chain continued after my report: #2790, then #2792 ("make the pending schema ref single-use so it cannot leak"), whose own message records repair steps across two apps being refused with a slug from an unrelated operation. Both of my buildiq runs (09:44 and 11:01) predate #2792 at 11:09.

So the 500 is plausibly the same leak, and plausibly already closed. I have re-run buildiq's and integriq's E2E against the current development HEAD (1ebd732a) to find out, and will report the result here either way.

Not asking for action yet — just correcting the record, since my first comment pointed at the wrong mechanism.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

The regression window, pinned

The 500 is new, and the window is narrow. Same buildiq SHA 6c190470 throughout:

run seed result
01:56 docudesk templates ready: … (created Bevestigingsbrief, Besluit)creates succeeded
09:44 create returned 500 (both)
11:01 create returned 500
11:17 (after #2792) create returned 500

buildiq's tree did not move. ConductionNL/docudesk@main — the sibling app it clones — has not moved since 08-04. The only thing that changed is openregister@development, which between 02:09 and 09:44 gained #2774, #2777 and #2780.

POST /apps/docudesk/api/templatesTemplateService::createTemplate()ObjectService::saveObject(object:, register: 19, schema: 18). Both are numeric ids, and the seed verifies them on read-back immediately before.

What I ruled out

  • saveObject's signature — byte-identical between aa886cb3 (01:51) and 1ebd732a (now).
  • The scoped resolver refusing numeric idsresolveSchemaWithin() falls back to a global lookup for unique identifiers, exactly as its comment says.
  • fix(scope): make the pending schema ref single-use so it cannot leak between operations #2792 — the 500 persists on a run that cloned it.
  • A stale vendored contract in docudesk — docudesk@main ships no hydra-gates and has zero OpenRegister\Contract references.

What I cannot rule out

My own #2777 is in that window — it bumped openregister to hydra-gates v1.9.0 and nc-vue 2.11.1. openregister's own CI was green on it (43 checks), but nothing in that suite exercises a sibling app writing through ObjectService. If the scoping changes turn out innocent, that is the next place to look and I will own it.

What I need from someone with the instance

The server-side exception. The E2E job does not dump nextcloud.log, so the 500's message never reaches CI output — I have the request and the response code and nothing between them.

Instrument side is fixed either way: buildiq's seeder announced templates ready … (already present) while both creates were failing, which is why this read as a flaky picker for several hours (ConductionNL/buildiq#329).

rubenvdlinde added a commit to ConductionNL/buildiq that referenced this pull request Aug 22, 2026
…#329)

The drift sweep found buildiq red on two docudesk-template specs, and the seed
log had already said why — phrased as success:

    [globalSetup] docudesk template "Bevestigingsbrief" create returned 500
    [globalSetup] docudesk template "Besluit" create returned 500
    [globalSetup] docudesk templates ready: Bevestigingsbrief, Besluit (already present)

Nothing was created and nothing was present. The summary read
`created.length ? '(created …)' : '(already present)'`, and `created` is empty
in BOTH the nothing-to-do case and the everything-failed case, so the message
picked the innocent reading of the ambiguity.

The cost is not the wrong word. It is that the failure then resurfaces 30
minutes later as

    Test timeout of 30000ms exceeded.
    waiting for getByRole('option').filter({ hasText: /Bevestigingsbrief/i })

— a Playwright timeout on an empty picker, which reads as a flaky UI test and
sent me looking at nc-vue, at openregister's register-scope change, and at the
sibling app's branch before I re-read the seeding log I already had.

Now a failed create is tracked and the summary says NOT ready, names the
failures with their status codes, and says where to look. A run that seeded
nothing can no longer announce that it seeded everything.

The 500 itself is a separate, live defect in docudesk's template-create path
against the current openregister development — reported on
ConductionNL/openregister#2774. This change is only about the instrument.

Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

A second app fails the same way — integriq, at its object-write step

Adding this because it changes the shape of the evidence from one app to a pattern.

integriq's flow-native-sync spec drives a generated flow and asserts every step executes:

executed: trigger, fetch, explode, map, contract, target-uuid, write
missing:  synced-id, commit, sweep, end

It halts immediately after write — the step that writes objects into the target register. The generator itself is fine: it emits all eleven steps, and I originally mis-attributed this to the PR that touched the generator (ConductionNL/integriq#1526) before reading it properly. Corrected there.

The pattern

app surface fails at
buildiq POST /apps/docudesk/api/templates → 500 ObjectService::saveObject() via docudesk
integriq flow run halts after write the object-write step of a generated flow

Two apps, two unrelated surfaces, both failing at the point where they write objects belonging to another app's register, both starting in the window that contains #2774 / #2790 / #2792, and neither app's own tree having moved. buildiq's is provable to the minute: the same SHA created its fixtures successfully at 01:56 and gets 500 from 09:44 onward.

Still not proved, and here is the missing piece

Neither failure's server-side reason reaches CI, because the shared E2E job discards nextcloud.log. That is fixed in ConductionNL/.github#551... — correction, ConductionNL/.github#554 — which captures every level>=3 entry with its exception on failure. Once that merges, the next failing run on either app should name the cause instead of leaving us to infer it.

I am not asking for a revert on inference. If the boundary is behaving as intended and these two writes were always illegitimate, that is a fair answer — it just needs to be the answer someone gives deliberately, because right now two apps' E2E are red on development and the reason is only visible from outside.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

The server-side cause, at last: the magic table does not exist

ConductionNL/.github#554 landed, so the E2E job now keeps nextcloud.log. First failing run with it (integriq, flow-native-sync) names the failure outright:

POST /index.php/apps/openregister/api/objects/15/72

[MagicMapper] Failed to find object in register+schema table
  identifier: fns-mt4ddzfd-ddr0-alpha
  tableName:  openregister_table_15_72
  error:      SQLSTATE[42P01]: Undefined table: 7
              ERROR: relation "oc_openregister_table_15_72" does not exist
  at lib/Db/MagicMapper.php:5397

Ten occurrences in one run, plus 4× [RelationHandler] Failed to get used by and 4× Failed to get uses alongside.

That is exactly where integriq's flow stops: the run executes trigger → fetch → explode → map → contract → target-uuid → write and then halts, with synced-id, commit, sweep, end never running. synced-id is the step that reads the object back after write.

Why I think this belongs to the scoping chain

#2774's own commit message describes the mirror image of this:

register document (id 6) carried an empty schemas list while nine docudesk-owned schemas shared the slug anonymizationLink; the fallback resolved to id 5084, which has no table under register 6 at all

Before, a schema ref could resolve to a register whose (register, schema) table exists. Now the pair is honoured strictly — and for 15/72 the table has never been created. So the question is whether the write path still creates the magic table lazily for a newly-scoped pair, or whether provisioning now happens somewhere the flow never reaches.

I am not proposing the fix: whether 15/72 should exist, or whether the flow should be targeting a different pair, is a call for whoever owns the boundary.

buildiq is very likely the same

Its symptom is POST /apps/docudesk/api/templates → 500, also a cross-app object write. Its capture-enabled run is finishing now and I will post its log here too, so both can be judged from the server's own account rather than from my inference — which was wrong twice before this.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Walking part of that back before it misleads anyone

I posted the MagicMapper / missing-table entries as "the server-side cause". On reading further I do not think that is established, and I would rather say so than let it stand.

findInRegisterSchemaTable() catches the 42P01 and rethrows it as DoesNotExistException (lib/Db/MagicMapper.php:5390-5404). A missing table therefore surfaces to the caller as "this object does not exist" — which is a perfectly ordinary answer on the first write to a (register, schema) pair, before ensureTableForRegisterSchema() creates it. The logger->error on that path makes normal first-write behaviour look like a failure in the log.

Supporting that reading: the captured log has no level-4 entries at all. 498 level-3, of which 448 are jsresourceloader missing-asset noise and 10 are these. Nothing in the file is fatal.

So what I actually have is:

  • integriq's flow reliably halts after write, with synced-id, commit, sweep, end never executing
  • buildiq's POST /apps/docudesk/api/templates reliably returns 500
  • both are cross-app object writes, both started in the window containing the scoping chain, neither app's own tree moved
  • and the server log for integriq contains no fatal, which makes the halt harder to explain, not easier

The missing table for 15/72 may still be relevant — it is at minimum surprising that the pair has no table while a write is being attempted against it. But I cannot call it the cause, and I should not have.

buildiq's capture-enabled run is still going. A 500 must leave something behind, so that log is the more promising of the two; I will post it here as raw evidence rather than as a conclusion.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

The 500, in the server's own words — and it is the leak #2792 was meant to close

buildiq's capture-enabled run finally produced it:

POST /index.php/apps/docudesk/api/templates

Failed to create template: Schema slug "application" is not carried by register
"docudesk" (id 19), which carries 9 schema(s). 1 schema(s) elsewhere on this
instance carry this slug; none of them is served here, because naming a register
makes it a boundary.

Docudesk is creating a template. Its configured pair is register 19 / schema 18 (template) — the seed writes exactly that and verifies it on read-back immediately before. The slug being resolved is application, which is openbuild's schema, from an entirely unrelated operation.

That is the same shape #2792 describes:

a pipelinq repair step seeding trustConfiguration was told posJournalEntryOutbound is not carried by register trust-configuration — a slug from an entirely unrelated operation

This run started 12:26 UTC; #2792 merged 11:09 UTC. So the pending-ref leak survives that fix, at least on this path.

Why the boundary message is itself doing its job

The refusal is correct and the error text is genuinely good — it names the register, the count it carries, and that a match exists elsewhere. The bug is upstream: the resolver is being handed a slug the caller never asked for.

Correcting myself, twice

Earlier in this thread I posted the MagicMapper missing-table entries as the cause, then walked that back. They were a red herring — findInRegisterSchemaTable() catches 42P01 and rethrows DoesNotExistException, which is ordinary first-write behaviour. And before that I attributed integriq's failure to its own generator PR. Both wrong; this one is the server saying it outright.

integriq's flow halting after write is plausibly the same leak on a different surface, but I am not asserting that again without its own message.

Reproduction

Any buildiq E2E run on development: tests/e2e/global-setup.ts POSTs two templates during seeding, both 500. The server log is now attached to failing E2E runs as the nextcloud-log-e2e artifact (ConductionNL/.github#554).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant