Skip to content

SECURITY: @no-admin-idor-exempt on requirementReport states a reason that is false — character schema declares no authorization block (gate-7 reports 0; .github#365) #310

Description

@rubenvdlinde

Summary

GET /api/characters/{id}/requirement-report carries a
@no-admin-idor-exempt annotation whose stated reason is false, on a route
whose sibling in the same file was hardened for exactly this bug (#205).

One endpoint, low-stakes data — but it is the cleanest governance failure
found in an 18-app fleet gate-7 re-audit, because the exemption actively
suppresses the gate rather than merely evading it.

The finding

lib/Controller/CharactersController.php:235requirementReport:

:216-220  docblock: "Authorization is delegated to OpenRegister via the fetch —
                     a user who cannot read the character via the OR-backed
                     fetch gets a 404"
:245      @no-admin-idor-exempt OR-delegated read via RegisterObjectFetcher::getObject (ADR-022)

Both halves of that reason are false.

The fetch is RegisterObjectFetcher.php:400:

$object = $mapper->find($idLower);

a raw MagicMapper::find() against the character schema. _rbac defaults to
true, but lib/Settings/larpingapp_register.json:39-41 declares no
authorization key at all
on character. Per
openregister/lib/Service/Object/PermissionHandler.php:1237:

if (empty($authorization) === true || $publicOptIn === true) { … return true; }

so hasGroupPermission() grants unconditionally and find() never returns
404
for a character the caller may not read. The only two authorization
blocks in the whole app are xpAward and attendance, both gamemaster
write-locks.

Same-file positive control — the app already knows this is an IDOR class

CharactersController::downloadPdf (:150):

if ($this->groupManager->isAdmin($user->getUID()) === false) { return … Http::STATUS_FORBIDDEN; }
// comment: "Closes #205 (Character PDF IDOR). Per-player self-access requires the
//           character schema to include a `player` ownership field so the controller
//           can verify ownership — that is a follow-up schema change."

So: the app identified character reads as an IDOR class, guarded one route
with a blunt admin check, wrote down that the real fix needs a player ownership
field on the schema — and left the other route reading the same object by the
same caller-supplied id, under an exemption asserting it is safe.

Why gate-7 never caught it

Two layers, either of which alone would have sufficed:

  1. .github#365 — gate-7 accepts a no user → 401 preamble as an authorisation
    guard. 3 of larpingapp's 6 controller files carry it.
  2. The @no-admin-idor-exempt tag makes the method skip the check entirely
    (check_no_admin_idor.py exemption 4).
checker findings
canonical (what CI reports today) 0
same run, UNAUTHORIZED/401 dropped from _GUARD_BODY_RE 3

1 takes a caller-supplied id; hand-verified REAL.

Suggested remediation

Two parts, and please do not do only the first:

  1. Remove the false @no-admin-idor-exempt at :245 and the :216-220
    docblock claim.
    This is unambiguous and small — the reason names a state of
    the world ("OR delegates authorization") which is measurably not the case.
    ⚠️ Note that removing it will not turn gate-7 red today, because the 401
    preamble silences it anyway (.github#365) — so do not treat a green gate
    after the removal as evidence the endpoint is fine.
  2. #205's own note names the real fix: add a player ownership field to the
    character schema and verify ownership, then apply it to both
    downloadPdf and requirementReport. Declaring an authorization block on
    character (wave-12 follow-up: fleet sweep of *_register.json to add authorization blocks ahead of default-closed flip openregister#2011) would additionally make the
    original exemption's reason true rather than merely deleted.

🔑 Generalisable: an exemption's reason is a testable claim. This one names a
state of the world rather than a test artifact, and it rotted — the schema never
had the block the reason assumes.


Confirmed by: canonical ConductionNL/.github @ 112d4c9 (post-#368);
larpingapp @ origin/development 526bbc5f. Full harness:
/home/rubenlinde/fleet-board/findings/gate7-fleet-reaudit.md.
Related: ConductionNL/.github#365, #372, ConductionNL/openregister#1955, #2011,
and this repo's own #205.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions