Skip to content

fix(pdf): validate the template id before probing for DocuDesk, and seed a player for character.ocName - #281

Merged
rubenvdlinde merged 3 commits into
developmentfrom
fix/newman-character-fixture-and-guard-order
Aug 7, 2026
Merged

fix(pdf): validate the template id before probing for DocuDesk, and seed a player for character.ocName#281
rubenvdlinde merged 3 commits into
developmentfrom
fix/newman-character-fixture-and-guard-order

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Newman on development (run 31110917018, job 92648343763):
1 collection · 27 requests · 43 assertions · 33 passed · 10 FAILED.

Two root causes account for all ten.

1. The collection sent a free-text ocName (8 failures — fixture wrong)

The character schema declares ocName as
{ "$ref": "player", "format": "uuid", "x-allow-create": true } — it is the
PLAYER RELATION, not an out-of-character display name. The collection sent
"ocName": "NEWMAN character".

Measured live against the dev instance, status code printed:

POST .../objects/larpingapp/character  {"ocName":"NEWMAN character"}
-> HTTP 400 "Property 'ocName' should match format 'uuid' but
   'NEWMAN character' does not."

The app is CORRECT; the fixture was not. x-allow-create is not an
OpenRegister annotation at all (grep -rn x-allow-create over openregister
returns nothing), so it is a silently-ignored hint and does not permit a bare
name on write.

Every other object create in the collection (item, skill, condition, effect,
event, player) passed, because none of them carries a relation property. The
failing create left charId undefined, which cascaded into the 404s on read /
update / delete and put the literal null into the PDF URLs.

Fix: a 0. Setup folder seeds a player and captures setupPlayerId BEFORE
section 1, and the character create/update bodies reference it. Section 2's
own player create is untouched, so domain-object coverage is unchanged; a
matching teardown removes the seeded player.

The negative test was passing for the wrong reason

Create character ERROR: missing required ocName (400, not 500) asserted only
within(400, 499). While ocName was malformed the endpoint answered 400 to
EVERY request, so that test would have passed even if the endpoint did nothing
at all. It is now pinned to the missing-required branch by name — the body must
mention required and ocName, and must NOT be the uuid-format rejection,
which is also a 400. Measured, the two branches are distinguishable:

{"description": "..."}  -> 400 "The required properties (name, ocName) are missing."
{"ocName": "not-a-uuid"} -> 400 "Property 'ocName' should match format 'uuid'"

This is a strengthening, not a relaxation.

2. The DocuDesk probe ran before input validation (1 failure — app bug)

downloadPdf ERROR: invalid (non-UUID) template -> 400 not 500 got 424.

CharactersController::downloadPdf() and EventsController::downloadRunsheet()
both checked isDocuDeskAvailable() BEFORE normaliseTemplateId(). CI does not
install DocuDesk alongside LarpingApp, so the availability probe answered 424 to
every request and the documented 400 contract was unreachable — a crafted
template value was never rejected on its own merits.

Input validation is a property of the REQUEST and must not depend on which
optional apps are installed. normaliseTemplateId() is a pure regex with no
DocuDesk dependency, so the two guards are now in the correct order in both
controllers.

Failure proof

No existing test covered the combination that matters — DocuDesk ABSENT and a
non-UUID template. Every case fixed one guard while leaving the other in its
passing state, so BOTH orders satisfied all of them, which is why the bug
survived. The two tests added here cover exactly that combination. Measured:

ARM A (original order):  2 failures — "Failed asserting that 424 is identical to 400"
ARM B (this change):     OK (27 tests, 62 assertions)
full suite:              OK (175 tests, 581 assertions)

Three existing tests passed a non-UUID template while asserting 424; they now
pass a well-formed UUID so the 424 branch is genuinely reached rather than
being short-circuited by the 400.

Not fixed here — needs a product decision

Create character AUTHZ: anonymous write returns 401 still fails. Measured:

POST (no auth) .../objects/larpingapp/character
-> HTTP 403 {"error":"User 'Anonymous' does not have permission to
   'create' objects in schema 'Character'"}

LarpingApp's own /api/settings correctly answers 401 to an anonymous caller.
This 403 comes from OpenRegister's RBAC layer, not from LarpingApp, so the
assertion is left honest and failing rather than repointed at 403. Whether an
anonymous caller should get 401 (not authenticated) or 403 (operation
forbidden) — and whether naming the schema to an anonymous caller is an
acceptable disclosure — is an OpenRegister decision.

No test was skipped, disabled, relaxed or deleted.

…eed a player for character.ocName

Newman on `development` (run 31110917018, job 92648343763):
1 collection · 27 requests · 43 assertions · 33 passed · 10 FAILED.

Two root causes account for all ten.

## 1. The collection sent a free-text `ocName` (8 failures — fixture wrong)

The character schema declares `ocName` as
`{ "$ref": "player", "format": "uuid", "x-allow-create": true }` — it is the
PLAYER RELATION, not an out-of-character display name. The collection sent
`"ocName": "NEWMAN character"`.

Measured live against the dev instance, status code printed:

    POST .../objects/larpingapp/character  {"ocName":"NEWMAN character"}
    -> HTTP 400 "Property 'ocName' should match format 'uuid' but
       'NEWMAN character' does not."

The app is CORRECT; the fixture was not. `x-allow-create` is not an
OpenRegister annotation at all (`grep -rn x-allow-create` over openregister
returns nothing), so it is a silently-ignored hint and does not permit a bare
name on write.

Every other object create in the collection (item, skill, condition, effect,
event, player) passed, because none of them carries a relation property. The
failing create left `charId` undefined, which cascaded into the 404s on read /
update / delete and put the literal `null` into the PDF URLs.

Fix: a `0. Setup` folder seeds a player and captures `setupPlayerId` BEFORE
section 1, and the character create/update bodies reference it. Section 2's
own player create is untouched, so domain-object coverage is unchanged; a
matching teardown removes the seeded player.

### The negative test was passing for the wrong reason

`Create character ERROR: missing required ocName (400, not 500)` asserted only
`within(400, 499)`. While `ocName` was malformed the endpoint answered 400 to
EVERY request, so that test would have passed even if the endpoint did nothing
at all. It is now pinned to the missing-required branch by name — the body must
mention `required` and `ocName`, and must NOT be the uuid-format rejection,
which is also a 400. Measured, the two branches are distinguishable:

    {"description": "..."}  -> 400 "The required properties (name, ocName) are missing."
    {"ocName": "not-a-uuid"} -> 400 "Property 'ocName' should match format 'uuid'"

This is a strengthening, not a relaxation.

## 2. The DocuDesk probe ran before input validation (1 failure — app bug)

`downloadPdf ERROR: invalid (non-UUID) template -> 400 not 500` got 424.

`CharactersController::downloadPdf()` and `EventsController::downloadRunsheet()`
both checked `isDocuDeskAvailable()` BEFORE `normaliseTemplateId()`. CI does not
install DocuDesk alongside LarpingApp, so the availability probe answered 424 to
every request and the documented 400 contract was unreachable — a crafted
template value was never rejected on its own merits.

Input validation is a property of the REQUEST and must not depend on which
optional apps are installed. `normaliseTemplateId()` is a pure regex with no
DocuDesk dependency, so the two guards are now in the correct order in both
controllers.

### Failure proof

No existing test covered the combination that matters — DocuDesk ABSENT and a
non-UUID template. Every case fixed one guard while leaving the other in its
passing state, so BOTH orders satisfied all of them, which is why the bug
survived. The two tests added here cover exactly that combination. Measured:

    ARM A (original order):  2 failures — "Failed asserting that 424 is identical to 400"
    ARM B (this change):     OK (27 tests, 62 assertions)
    full suite:              OK (175 tests, 581 assertions)

Three existing tests passed a non-UUID template while asserting 424; they now
pass a well-formed UUID so the 424 branch is genuinely reached rather than
being short-circuited by the 400.

## Not fixed here — needs a product decision

`Create character AUTHZ: anonymous write returns 401` still fails. Measured:

    POST (no auth) .../objects/larpingapp/character
    -> HTTP 403 {"error":"User 'Anonymous' does not have permission to
       'create' objects in schema 'Character'"}

LarpingApp's own `/api/settings` correctly answers 401 to an anonymous caller.
This 403 comes from OpenRegister's RBAC layer, not from LarpingApp, so the
assertion is left honest and failing rather than repointed at 403. Whether an
anonymous caller should get 401 (not authenticated) or 403 (operation
forbidden) — and whether naming the schema to an anonymous caller is an
acceptable disclosure — is an OpenRegister decision.

No test was skipped, disabled, relaxed or deleted.
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/larpingapp @ 3ac363d

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 101/101
npm ✅ 537/537
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-07 08:20 UTC

Download the full PDF report from the workflow artifacts.

…ed admin (gate-9)

Touching downloadPdf() pulled it into gate-9's ADR-020 diff scope and exposed a
pre-existing contradiction: the method carried `#[NoAdminRequired]` /
`@NoAdminRequired` while its body requires an administrator — the `isAdmin()`
guard added by #205 to close the character-PDF IDOR. Anyone reading the
attribute would conclude any logged-in user may call it.

    lib/Controller/CharactersController.php:131 method=downloadPdf
    rule=no-admin-required-annotation-with-admin-body

The attribute is removed, so Nextcloud's middleware rejects a non-admin before
the controller runs. The in-body guard stays as defence in depth for direct
invocation. Anonymous callers are unaffected: SecurityMiddleware raises
NotLoggedIn (401) before the admin check, so the collection's
`downloadPdf AUTHZ: anonymous -> 401` still holds.

Why this was not caught before: my first local gate run reported gate-9 PASS.
That local checkout of ConductionNL/.github predated
`1558036 fix(gate-9): the admin rule matched no real isAdmin() call`, so the
gate I ran had never matched anything — a dead gate reads exactly like a
passing one. Re-running the version CI actually uses (hydra-gates @ main)
reproduced the FAIL locally, which is also the failure proof:

    with    #[NoAdminRequired]: [gate-9] semantic-auth: FAIL — 1 mismatch
    without #[NoAdminRequired]: [gate-9] semantic-auth: PASS
    gate-5 route-auth: PASS, gate-7 no-admin-idor: PASS
    unit suite: OK (175 tests, 581 assertions)

Per-player self-access remains a follow-up needing a `player` ownership field
on the character schema; when that lands the attribute returns alongside a real
ownership check.
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/larpingapp @ 2b3e803

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 101/101
npm ✅ 537/537
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-07 08:29 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/larpingapp @ 2b3e803

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 101/101
npm ✅ 537/537
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-07 08:34 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/larpingapp @ 3ac363d

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 101/101
npm ✅ 537/537
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-07 08:38 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

⛔ Holding this merge — blocked by an external break, not by this PR

What this PR achieved (measured, run 31160287191, commit a027e029, 08:05 UTC)

collections requests assertions passed failed
development (run 31110917018) 1 27 43 33 10
this PR 1 29 48 47 1

The single remaining failure is anonymous OR write rejected 401 (OpenRegister answers 403), left deliberately failing — see the PR body; it is an OpenRegister product decision, not a LarpingApp defect.

Hydra Gates went green on the follow-up commit 30f689c3 (gate-9 semantic-auth).

Why it is not merged

At 08:18 UTC — between my two CI runs — openregister@development landed
8d5181f7a chore: require Nextcloud 32, which is where the ContextChat interfaces exist (#2378),
making appinfo/info.xml declare <nextcloud min-version="32" max-version="34"/>.

LarpingApp's workflow sets nextcloud-test-refs: '["stable31", "stable32"]'. On the stable31 leg openregister can no longer be enabled, so every /apps/openregister/... call returns Nextcloud's HTML 404 page. Consequences on the latest run:

  • Newman: 26 failures, every new one a 404 on the OpenRegister object API (the collection never reaches a live store);
  • E2E: seed aborts with OpenRegister registers endpoint returned HTTP 404.

Controlled experiment proving this is external: I re-ran the E2E job of run 31160287191 — the same commit a027e029, unchanged. It passed at 08:05 UTC and failed at 08:35 UTC. No code of mine changed between the two; only openregister's minimum version did.

So the current red cannot be attributed to this PR, and a strict-subset comparison against development is not measurable until the dependency is resolved. development will show the same red on its next run.

Decision needed from a human

openregister flip-flopped on this within a day — 42cfbd459 fix(contextchat): OCP\ContextChat is NC 33+, but this app supports NC 28+ (#2372) (08-06) supported NC 28+, then #2378 (08-07) required NC 32. Either:

  1. openregister relaxes min-version back and guards the ContextChat interfaces at runtime, or
  2. every leaf app that pins stable31 drops that leg.

This affects far more than LarpingApp — decidesk (stable31, stable32) and shillinq (stable31, stable32, stable33) are hit the same way. docudesk is immune only because it tests stable32 only.

Once that is settled, this PR should merge on a re-run with no further changes.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Follow-up: development's own baseline re-measured under the current openregister

To make the comparison like-for-like I re-ran development's own Newman job (run 31110917018, job 92648343763) just now — same commit as yesterday, same stable31 leg, only openregister changed underneath it:

measurement when assertions passed failed
development 2026-08-06 14:27 UTC 43 33 10
development, re-run unchanged 2026-08-07 ~09:2x UTC 32 10 22

The same commit went from 10 failures to 22 with no code change at all. That is the openregister min-version=32 break (8d5181f7a) against this repo's stable31 leg, and it is why neither development nor this PR is measurable right now.

For the record, this PR's verified result before the break (run 31160287191, commit a027e029, 08:05 UTC) was 48 assertions / 47 passed / 1 failed, against development's 43 / 33 / 10.

Tracked as ConductionNL/openregister#2379. This PR should be re-run and merged once that is resolved; no further changes are expected here.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/larpingapp @ a298413

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 101/101
npm ✅ 537/537
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-07 12:51 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Landing — re-measured against development today, post-revert

The earlier hold rested on evidence that turned out to be an artifact. Re-measured now.

Why the old evidence was wrong

This PR's previous CI run was 2026-08-07T08:24:46Z (run 31161582668). The shared quality.yml clones openregister via additional-apps at ref: development, resolved at job start — so what a run tested depends on when it started, not on this PR. That 08:24 run picked up openregister while its min-version="32" floor was in place, so openregister could not install and the data layer was simply absent. openregister#2380 reverted that floor at 10:02:44Z, and this repo's own #284 restored its floor at 11:08:23Z. Both post-date the run the hold was based on.

Rebased onto development (4e35caaf) and re-run: 31178917398.

Newman — 10 → 1, and the 1 is pre-existing

Both runs completed (status == "completed", no cancellations, grep -c "Failed to resolve action download info" = 0 on both, so no platform outage).

requests assertions failed
development @ 4e35caaf (run 31172875749, 11:25Z) 27 43 10
this PR @ 2bf671c6 (run 31178917398, 12:38Z) 29 48 1

The denominator moved up (43 → 48 assertions), because the fixture adds a seeded player request — so this is not a case of a collection aborting early and shrinking its own total. Failures were compared by assertion name, not by count:

The single remaining PR failure:

Character (OR object CRUD, ADR-022) / Create character AUTHZ: anonymous write returns 401
  → AssertionError: anonymous OR write rejected 401

That exact assertion is item 8 of development's own 10. PR failures not present in development: 0.

The 9 this PR clears, all by name:

Create character (201/200)                    :: character created (2xx)
Create character (201/200)                    :: returns an id
Create character (201/200)                    :: echoes the name
Read character (200)                          :: character read (200)
Read character (200)                          :: id matches created
Update character (200)                        :: character updated (200)
Update character (200)                        :: description updated
downloadPdf ERROR: invalid (non-UUID) template -> 400 not 500 :: invalid template rejected 400
Teardown / Delete character                   :: character deleted (2xx)

The two defects

  1. Fixture. character.ocName is a relation — {"$ref":"player","format":"uuid"} — not a display name. The collection was posting a string, so every create 4xx'd and the whole CRUD chain plus teardown fell over behind it. Fixed by seeding a player and referencing its uuid.
  2. A real app fix. In downloadPdf, the DocuDesk availability probe ran before UUID validation. On CI DocuDesk is absent, so every request — including a deliberately malformed template id — short-circuited to 424, and the documented 400 was unreachable by construction. Validating the id first restores it. This is an ordering bug in shipped code, not a test accommodation.

Full job-name comparison

job development this PR
Frontend Check (test:l10n) fail fail
Integration Tests (Newman) fail fail (1 vs 10)
Quality Report fail fail
E2E Tests (Playwright) pass pass
everything else pass pass

The failing set is a strict subset by name. test:l10n was diffed line-for-line between the two runs and is identical — the same 56 lines, the same 26 missing backend keys per locale; only PIDs and runner temp paths differ. Playwright failed on the stale 08:24 run and passes now, which was the same openregister artifact.

Nothing was waived, skipped or baselined; no threshold moved.

@rubenvdlinde
rubenvdlinde merged commit 6eeb3db into development Aug 7, 2026
30 of 33 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/newman-character-fixture-and-guard-order branch August 7, 2026 12:59
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