Skill-requirement enforcement never runs on the OpenRegister REST write path — violating characters persist with HTTP 201/200

[rubenvdlinde]

Summary

LarpingApp's server-authoritative skill-requirement enforcement never runs for writes made through the OpenRegister REST objects API — the path the SPA and every REST client use. A character that violates a declared requiredSkills[] prerequisite is created and updated successfully.

CharacterRequirementListener, SkillRequirementService and SkillRequirementChecker (1001 lines) are correct and registered. The problem is upstream: OpenRegister does not dispatch the vetoable pre-write event on that write path, so the listener is never invoked.

This makes the spec's first scenario — "Direct API write bypassing the UI is rejected" — currently false, and 16 skill-requirement-enforcement scenarios untestable as written. They are the largest gate-19 cluster (50 findings total).

Reproduction (measured, not inferred)

Isolated rig: NC 34.0.0, openregister 0.2.17-unstable.36 (healthy 75-entry vendor/), larpingapp 0.1.39, register provisioned by this app's own tests/e2e/ci-seed.sh, firstrunwizard disabled.

Skill PROBE-adv declares requiredSkills: [PROBE-basic]. Character does not have PROBE-basic:

write	required by spec	observed
POST /apps/openregister/api/objects/{register}/{characterSchema} with skills:[adv]	rejected before persistence	HTTP 201, persisted
PUT …/{characterSchema}/{id} adding skills:[adv]	rejected before persistence	HTTP 200, persisted

The listener is not failing open either — it catches \Throwable and logs, and:

grep -c 'skill-requirement validation errored' data/nextcloud.log  ->  0

It is never called at all.

Root cause

ObjectsController saves through objectService->saveObject(...). In openregister/lib/Service/Object/SaveObjects.php:

grep -c 'ObjectCreatingEvent|ObjectUpdatingEvent'   -> 0          # the VETOABLE pre-write events
grep -n  'ObjectCreatedEvent|ObjectUpdatedEvent'    -> 72,73,1687 # positive control: the grep works
grep -n  '->insert(|->update('                      -> (none)

The vetoable events are dispatched only from MagicMapper::insertObjectEntity() (l.6764) and the two ObjectUpdatingEvent sites (l.6919, l.7158), reached via MagicMapper::insert() (l.8803). SaveObjects never calls those.

Those dispatch sites do honour isPropagationStopped() and getErrors(), so the veto contract itself is implemented — the REST path simply does not traverse it. Note the asymmetry: the post-write events fire on both paths (SaveObjects.php:1687 and MagicMapper.php:8832), the pre-write events on only one.

Why this is easy to miss

• The app's unit tests exercise the service and the listener directly, so they are green.
• gate-61 only inspects post-event listeners, so it cannot see this.
• Any e2e test written in the acceptance direction ("this valid assignment succeeds") passes with the enforcement layer entirely absent.

That last point is not hypothetical. I wrote 13 full-stack tests for these scenarios. 10 failed. The 3 that passed all asserted that a write is accepted — and would pass with enforcement removed, exactly the shallowness #301 describes. They were deliberately not shipped: anchoring all 13 would have moved gate-19 from 50 to 37 while proving nothing.

Suggested resolution

1. Raise on OpenRegister: SaveObjects must dispatch ObjectCreatingEvent/ObjectUpdatingEvent and honour stopPropagation() + getErrors(), as MagicMapper already does — otherwise the vetoable-event contract is not available to any consuming app on the REST path.
2. Until then, the 16 skill-requirement-enforcement scenarios should stay as open gate-19 findings. They must not be closed with @e2e exclude: a browser can observe this perfectly well, and the reason would be "the feature does not work", which is a defect report, not an exemption.
3. When it is fixed, the rejection-direction tests must land first — they are the only ones that can fail for the right reason.

Related

• 20 @e2e tags point at openspec/changes/ and have never been counted — and retargeting them alone would manufacture a false green #301 — 20 inert @e2e tags, and the warning against retargeting tags onto tests that do not perform their scenario. Same standard applied here.
• gate-19 is larpingapp's only failing gate (50 findings, full scope, run 31490125155).

[rubenvdlinde]

Refutation — the REST objects API does dispatch the vetoable pre-write events, and a veto returns 422, not 201

I was sent to verify this and fix it. The headline claim does not hold, and because it has been propagated as "every OpenRegister pre-write guard in the fleet is inert on the path SPAs actually use", the correction matters more than the fix would have.

The observation in the issue is correct — about the wrong file

lib/Service/Object/SaveObjects.php (plural) really does have 0 references to ObjectCreatingEvent/ObjectUpdatingEvent while dispatching the post-write pair at :1687/:1702. Positive control on the same file: ObjectCreatedEvent → 4 hits.

But SaveObjects.php is the bulk path. It is not what ObjectsController::create()/update() save through.

The REST path, traced hop by hop

ObjectsController.php:2622   create()                  [route objects#create, appinfo/routes.php:791]
  -> :2715   $objectService->saveObject(...)
ObjectService.php:1189       saveObject()
  -> :1342   $this->saveHandler->saveObject(...)
lib/Service/Object/SaveObject.php:2798   saveObject()      (SINGULAR — a different file)
  -> :3038   handleObjectCreation(...)
  -> :3413   $this->unifiedObjectMapper->insert(...)    // declared MagicMapper at :323
MagicMapper.php:8803         insert()
  -> :8818   insertObjectEntity(...)                    // $dispatchEvents defaults TRUE (:6759)
MagicMapper.php:6755         insertObjectEntity()
  -> :6764   dispatch ObjectCreatingEvent               <- PRE-WRITE, VETOABLE
  -> :6767   if isPropagationStopped() -> throw HookStoppedException (:6769)
  -> :8832   dispatch ObjectCreatedEvent                <- POST-WRITE
ObjectsController.php:2739   catch HookStoppedException -> JSONResponse(statusCode: 422)

Verified by reading each hop, not inferred. MagicMapper::insert() calls insertObjectEntity() without dispatchEvents, so the = true default applies and the dispatch is unconditional.

The update path is symmetric: ObjectsController.php:2809 → SaveObject.php:3301 → MagicMapper.php:8973 → ObjectUpdatingEvent at :6919 → 422 at :2985. There are five HookStoppedException catches in ObjectsController (:2739, :2985, :3218, :3368, :3452), all returning 422.

All three pre-write events implement StoppableEventInterface with setErrors()/setModifiedData(), and 14 listeners are registered across them in lib/AppInfo/Application.php:2512-2702 (FlowNodePreflightListener, LifecycleValidationListener, ApprovalChainGateListener, CalculationOnSaveListener, QualityScoreOnSaveListener, SurvivorshipRecomputeListener, HookListener, …). tests/Unit/Event/ObjectStoppableEventsTest.php covers the veto mechanism.

So: pre-write guards on the REST path are live, and a violating object does not persist at 201.

---

The real defect, which is narrower and still worth fixing

The BULK path bypasses every guard, and it is @NoAdminRequired.

POST /api/bulk/{register}/{schema}/save (appinfo/routes.php:810, BulkController.php:399-406) calls:

lib/Controller/BulkController.php:369-377

$savedObjects = $this->objectService->saveObjects(
    objects: $objects, register: $register, schema: $schema,
    _rbac: true, _multitenancy: true, validation: true,
    events: false
);

• No pre-write event, ever. SaveObjects.php:1843 → MagicMapper::ultraFastBulkSave() (:9212), which never reaches insertObjectEntity(). The file's own comment at :1602-1605 says the bulk path "bypasses the per-row event/audit hooks the single-object insert/update apply, so the bulk path replays them here" — and it replays only the post-write pair.
• And with events: false, not even those fire. _events also defaults to false on saveObjects() (SaveObjects.php:305), so any caller that does not opt in gets nothing.

Consequence: a caller can write through /api/bulk/.../save an object that /api/objects/... would refuse with 422 — the approval-chain gate, lifecycle validation and flow preflight are all skipped. Schema validation still runs (validation: true); listener-based guards do not.

Other callers of the same path: lib/Service/ImportService.php:1290, :1534, lib/Service/Object/ValidationHandler.php:658.

I did not fix this. Dispatching per row through the bulk path is a direct trade against the reason ultraFastBulkSave exists, and the right answer (dispatch per row / dispatch per chunk / require an explicit events: true from BulkController / refuse bulk for guarded schemas) is a product decision, not a code cleanup.

Two smaller real defects found en route

1. WebhookEventListener has dead pre-write branches. It handles ObjectCreatingEvent/ObjectUpdatingEvent/ObjectDeletingEvent at lib/Listener/WebhookEventListener.php:169/180/193, but Application.php registers it only on ObjectCreatedEvent (:2711) and ObjectUpdatedEvent (:2712). Those branches can never execute.

2. Batched soft delete treats a veto as continue. MagicMapper.php:7158-7167 dispatches ObjectUpdatingEvent and, on stopPropagation(), logs at debug and skips the row — diverging from the HookStoppedException contract at :6769/:6924/:7019. So a bulk delete reports success for objects a guard rejected, with no signal to the caller.

For completeness: suppressLifecycleEvents() (MagicMapper.php:9029) gates only the post-write dispatches (:8829, :8988). Pre-write vetoes still fire under SystemOperationContext, which is the correct design.

[rubenvdlinde]

Which tree I measured, since the flow engine has been moving underneath this issue.

Everything above was read at openregister@9628dc5d0, the current tip of development — which already contains the two recent flow-engine commits ee3448fea ("run a flow synchronously, and take its history with it on delete") and a4dd90679. So this is not a pre-flow-work reading that the flow work has since invalidated; it is the post-flow-work state.

That matters here specifically because one of the listeners on the pre-write events is FlowNodePreflightListener (Application.php:2512-2513), i.e. flow-owned. It is still registered on ObjectCreatingEvent and ObjectUpdatingEvent at that commit, and MagicMapper::insertObjectEntity() still dispatches them unconditionally with $dispatchEvents defaulting to true.

Separately, and not affecting the above: development is currently red on phpmd / phpstan / PHPUnit, all introduced by ee3448fea — filed as openregister#2429, with the phpstan and PHPUnit halves fixed in openregister#2427. Worth knowing if you measure this repo in the next while, because inherited red is easy to mistake for your own.

[rubenvdlinde]

The root cause is in larpingapp, not OpenRegister — and it is one line

The symptom in this issue reproduces exactly on a fresh single-owner rig
(NC 34.0.0, openregister 0.2.17-unstable.36, larpingapp 0.1.39, register
provisioned by tests/e2e/ci-seed.sh): a character carrying a skill whose
requiredSkills[] is unmet is created at HTTP 201 and persists, and a PUT
adding that skill returns HTTP 200, with zero listener errors in
nextcloud.log.

The stated cause is wrong. OpenRegister does dispatch the vetoable
pre-write event on the REST path.

What the evidence in this issue actually landed on

SaveObjects.php (plural) is OpenRegister's bulk handler, reached only
from BulkController. ObjectsController create/update/patch go through
SaveObject.php (singular), which persists via MagicMapper::insert() /
::update() — and those do dispatch ObjectCreatingEvent /
ObjectUpdatingEvent with full veto handling (MagicMapper.php:6764 and
:6919, $dispatchEvents defaulting to true). A one-character difference in
the filename produced a fully-evidenced wrong conclusion.

Measured on the rig — the listeners registered for ObjectCreatingEvent:

OCA\LarpingApp\Listener\CharacterRequirementListener     <- ours, FIRST in the chain
OCA\OpenRegister\Listener\FlowNodePreflightListener
OCA\OpenRegister\Listener\LifecycleInitialStateListener
OCA\OpenRegister\Listener\CalculationOnSaveListener
OCA\OpenRegister\Listener\QualityScoreOnSaveListener
OCA\OpenRegister\Listener\SurvivorshipRecomputeListener
OCA\OpenRegister\Listener\HookListener

So registration is fine, dispatch is fine, and SkillRequirementService::validate()
is fine too — called directly with the violating candidate it returns
{"valid":false,"requirements":[{…"status":"unmet"…}]}, and with a clean
candidate {"valid":true} (positive control).

The actual defect

CharacterRequirementListener::isCharacterSchema() probed
method_exists($entity, 'getSchema').

OCA\OpenRegister\Db\ObjectEntity declares exactly one accessor of its own,
getObject(). Every other accessor — getSchema() included — is resolved at
runtime by OCP\AppFramework\Db\Entity::__call(). method_exists() inspects the
declared method table, so:

method_exists($entity, 'getSchema')  -> false      (magic)
is_callable([$entity, 'getSchema'])  -> true
$entity->getSchema()                 -> "17"
method_exists($entity, 'getObject')  -> true       (declared — positive control)

$schema therefore stayed '', the method returned false for every
character write, handle() returned early before reaching the validator, and no
veto was ever raised. Nothing threw, so there was no log line either — the
catch (\Throwable) fallback never fired, which is why
grep -c 'skill-requirement validation errored' was 0.

Verified fix

method_exists(...) → is_callable([$entity, 'getSchema']). Black-box on the
same rig, before → after, one line changed:

write	before	after
POST a character carrying a skill whose requiredSkills[] is unmet	201, persisted	422, itemised requirements
PUT adding that skill to a clean character	200, persisted	422
POST a character with no unmet requirement (control)	201	201

Why the unit suite was green over this the whole time

tests/unit/Listener/CharacterRequirementListenerTest.php's FakeObjectEntity
declared getSchema(). So method_exists() answered true in the suite and
false in production — the fake was shaped to what the caller calls, not to
what the collaborator is, and could never have failed for this reason. The fake
now mirrors the real class (getObject() declared, getSchema() via __call());
with the fix reverted, exactly the three rejection-asserting tests go red.

A sibling defect of the same family was found next door and fixed in the same PR:
DeepLinkRegistrationListener guarded on method_exists($event, 'registerDeepLink')
— a method DeepLinkRegistrationEvent has never declared (the API is register())
— so it registered zero deep links, and the guard was the only thing hiding a
call that would otherwise have fatalled.

Fixed in #311.

Scope note

The narrow OpenRegister defect that does survive is the bulk endpoint
(POST /api/bulk/.../save passes events: false). larpingapp has zero exposure
there: a grep for bulk across src/ lib/ appinfo/ tests/e2e/ — run after a
positive control confirming the search works — matches only one prose comment.