fix(gate-7): a delegated authentication check is not an authorization guard - #516
Merged
rubenvdlinde merged 3 commits intoAug 20, 2026
Merged
Conversation
rubenvdlinde
pushed a commit
to ConductionNL/decidesk
that referenced
this pull request
Aug 20, 2026
The hydra gate-7 checker treated a DELEGATED authentication helper as an authorization guard: `$this->requireUserOr401()` cleared a method on its NAME alone. It also assumed any collaborator reaching OpenRegister's ObjectService was RBAC-guarded — untrue when the schema declares no `authorization` block, because in OpenRegister an absent block leaves the schema OPEN. Fixed upstream in ConductionNL/.github#516; this commit closes what that fix revealed in decidesk. Measured fleet-wide: decidesk was the only affected app (all 17 others 0 -> 0). Real defects closed, per controller: - ConflictOfInterest (3): declare/forMember/recordAction took a caller- supplied id with zero caller scoping. `recordAction` is now chair/ secretary only — recording the mitigating action is a presiding-officer act, so the declaring member deliberately does not pass. The schema also fell back to the register baseline's PUBLIC read, so declarations of financial interests and personal relationships were readable by any unauthenticated caller through OR's generic object API; read/list are now `authenticated` only. - BoardEvaluation (3): close/publish reuse the chair-or-secretary rule the schema already declares on its `lifecycle` property, so no second rule can drift. `publish` was the worst: the service swallowed OR's refusal and still answered HTTP 200, making denial indistinguishable from success. `report` is deliberately WIDER (any body member) — narrowing it would have removed a capability members have in the UI today (ADR-044). - EIDASSignature (2 + 1 exempt): `finalize` let any authenticated user set version=signed, QES level, archive ref and hash on any Minutes UUID and resolve its signature stage to adopted. `verify` returns the signer's certificate thumbprint, which identifies a natural person. `certStatus` is genuinely app-wide (it takes a cert thumbprint, not an object id) and carries a reason-bearing exemption tag plus a test pinning that a non-signatory still gets 200. - Integration (2): `getOutcome`'s docblock claimed "per-object read access is enforced by OpenRegister ObjectService RBAC". That claim was false — precisely because the `decision` schema declared no authorization block. `subscribe` writes `outcomeCallbackUrl`, a single scalar, so an unauthorised write REPLACED the raising consumer's delivery target: envelope hijack and denial in one write. Owner-or-admin; the `isPublished === 'public'` read arm is deliberately NOT honoured on the write path, since public readability is not a write grant. - Engagement (1): `index` returned every participant's speech log, question count and derived engagementScore for any meeting UUID. Now admin/chair/secretary read the whole meeting; anyone else reads only their own participant's records. - ProxyVote (1): `index` took `meetingId` from the request, so any authenticated user could enumerate meetings and read who delegated their vote to whom. Deliberately NOT restricted, with evidence rather than silence: submitProposal, castAdvisoryVote and submitReaction are citizen- participation intake and are correctly open to any authenticated user — identity was already session-derived, never taken from the body. Their session provenance is now visible at the endpoint, which is what the gate could not see before. One real fail-open was found there anyway: the advisory-vote window guard sat inside a nested `if`, so a proposal whose round could not be resolved accepted votes indefinitely, past the deadline, on a healthy 201. Schema authorization added for `decision` (anonymous read/list narrowed to isPublished === 'public') and `conflict-of-interest`, both with version bumps — an annotation-only schema change never deploys. The RegisterAuthorizationTest sentinel moves 24 -> 26 deliberately. Every guard is proven in BOTH directions, and each was mutation-tested: the guard was removed, the specific test observed going red, then restored. One such mutation exposed a weak instrument — an `expects($this->never())` sitting behind a catch-all `\Throwable` reports as a 500, so the assertion fires but its message never reaches the report; a write-counting test was added that nothing can swallow. 1150 tests green (0 failures), phpcs 0 errors on all changed files, phpstan/psalm/phpmd clean, spec anchors resolve, and gate-7 now reports 0 findings across all 40 decidesk controllers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The blind spot
#365established thatif ($user === null) { return 401; }isauthentication, not authorisation, and stopped it clearing a method when
written inline. The same clause one call-hop away — behind a helper whose
name merely begins with
require/ensure/check— still cleared the methodon the helper's name alone.
Two independent paths let an unguarded id through:
_GUARD_BODY_RE/_HELPER_GUARD_BODY_REmatched->require*(,->ensure*(,->check*(by shape.requireUserOr401(),checkLoggedIn(),ensureAuthenticated()all matched — every one of theman authentication check.
reaching
ObjectServiceis RBAC-guarded. That is only true when the schemathe collaborator touches actually declares an
authorizationblock —and in OpenRegister an absent block leaves the schema open
(~99.3% of ~1.02M rows, measured previously).
How it was found
decidesk
ConflictOfInterestController— three#[NoAdminRequired]routestake a caller-supplied
$idstraight into a service with no caller scoping.Gate-7 reported PASS.
Positive control: a bare IDOR probe was reported; adding only
$this->requireUserOr401(session: $this->userSession);silenced it. So thegate could see the shape — the helper name was doing the clearing.
Confirmed the assumption is unsound rather than merely unproven: the
conflict-of-interestschema declares noauthorizationblock(
has authorization block: False).The fix
require/ensure/check+LoggedIn|Login|UserSession|User|Session| Authenticated|Authentication|Auth(optionallyOr\d{3}) no longer clears.Genuine authorisation spellings —
requireOwner,requireUserIsOwner,ensureAccess,authorizePermission— still clear, and there is a testpinning each one.
_or_clear_is_credible()gates the deep OR-delegation clear: a class thatnames schema slugs only earns the clear if at least one named schema
declares
authorizationinlib/Settings/*register*.jsonorregister.d/*.json. A class naming no schema is unchanged (still cleared) —the change is deliberately narrow.
Note it reads source with
keep_strings=True; the slug lives in a stringliteral, so the usual comment-stripping pass would erase the evidence.
Measured fleet impact
This turns decidesk gate-7 red. The 15 are real findings, not noise: 3 in
ConflictOfInterestController(being fixed now), plusBoardEvaluationController3,EIDASSignatureController3,EngagementController1,IntegrationController1,ParticipationBudgetController2,ParticipationController1,ProxyVoteController1. No other app in the fleet moves.Tests
171 passed— 169 pre-existing plus 2 new, covering both directions: anauthentication-only helper must be reported, and the four genuine
authorisation spellings must still clear. A gate fix without a test proving
the gate can fail is exactly what let this through in the first place.