fix(gate-7): an authentication check is not an authorisation guard (#365)

[rubenvdlinde]

Closes #365.

Note on wording: this description deliberately avoids reproducing verdict-shaped
lines. The workflow exports the PR body into the gates log, so a pasted verdict
line reappears there and reads as a finding of the very run meant to prove the
fix. Counts below are given in prose and in a table instead.

What was wrong

gate-7 reported zero in all eighteen fleet apps. Every one of those zeros came
from a checker that accepts "is anyone logged in?" as an answer to "may this
caller touch this object?".

Reproduced before touching anything, with a three-arm committed-plant control —
same file, same commit, same run, byte-identical data-access bodies, only the
preamble differing:

arm	canonical gate-7
bare unguarded method	1
the same body, preceded only by a no user -> 401 preamble	0
the same body, plus a real per-object ownership check	0 (correct)

Scale, measured against the canonical package at 57bcb2b: zero reported
fleet-wide, while 453 of 791 controller files carry that preamble.

Positive-controlled in all 18 apps. A zero from a gate looks exactly like an
empty scope, so a bare #[NoAdminRequired] IDOR was planted into each extracted
tree and the canonical checker re-run: it moved from 0 to 1 in every single one,
then every plant was removed and verified absent. Each zero is a measurement.

Under #[NoAdminRequired] Nextcloud's middleware has already rejected the
anonymous caller before the method runs, so that preamble cannot even fail. It
was nonetheless silencing the gate, through three separate guard patterns.

Why the fix is not "drop 401 from the regex"

That is what #365 proposes, and it is wrong in both directions. Both are now
pinned by tests.

Too wide. A genuine per-object decision that answers with the wrong status —

if ($account['ownerId'] !== $user->getUID()) {
    return new JSONResponse([], Http::STATUS_UNAUTHORIZED);
}

— is a real guard. Deleting the token turns it into a false positive, and false
positives (#353, #360) are precisely how gate-7 lost its credibility and how its
silences came to be believed.

Too narrow. The same authentication clause answering 403 keeps clearing.
One token of edit, made by someone chasing a green cell, buys the silence back by
making the response worse.

So this treats authentication-ness as a property of the condition, not of the
status code. An if whose condition tests only whether a caller identity is
absent, and whose consequent refuses, is blanked to same-length whitespace
before any guard pattern runs. Byte offsets, spans and reported line numbers are
unchanged; only the text the guard patterns get to see is.

Three controls keep the blanking narrow, each closing a way it could have eaten a
real guard:

1. Polarity — absence tests only. if ($user !== null) { ...whole body... }
   is a wrapper, and blanking it would erase every guard inside.
2. Argument-free operands only — $this->access->canAccess($id, $user) and
   $account['ownerId'] can never be read as an identity, however they are
   spelled. The first is a guard; the second is object data. This one was caught
   by the existing suite, which went red on the first draft.
3. The consequent must refuse. A conditional that merely computes is left
   alone.

Identity recognition is deliberately spelling-agnostic. #365's own re-audit built
the opposite mistake into its triage tool — three enumerated spellings of "who is
the caller", sessionUserId() matching none — and manufactured 19 false positives
from that gap.

Two companion patterns, and why they ship in the same PR

Removing the clear is what exposes them. Shipping without them would have
reported 45 findings in doriath alone, an app whose real gate-7 exposure was
hand-verified as zero.

• Pattern 6 — session-identity hand-off. Every call that receives a
  caller-supplied value must also receive a session-derived identity, so the
  object reference is resolved under a scope the caller cannot forge. An
  ALL-quantifier, not ANY: one unscoped data call still reports, so an audit-log
  line cannot clear a real IDOR. Suppresses 86 fleet-wide.
• Pattern 7 — in-body ownership comparison, whatever status it answers with,
  including the 404 chosen deliberately so a 403 cannot become an existence
  oracle. That sentence was already printed by this gate's own failure guidance;
  it was advice to a human and is now code. Suppresses 2 fleet-wide, and both
  are endpoints an earlier audit named specifically as over-reports.

Blast radius

Canonical development clones from GitHub, not local checkouts — three local app
directories turned out to have no .git at all and silently resolved the parent
repository's ref.

app	ref	ctrl files	files with the preamble	today	with this PR
zaakafhandelapp	bef3a7c	25	23	0	86
pipelinq	ccf0ff3	82	49	0	52
shillinq	5d31898	83	66	0	40
procest	347f2a4	91	49	0	32
hermiq	7cd256c	43	28	0	20
docudesk	67d5201	32	25	0	19
decidesk	445776b	40	18	0	17
softwarecatalog	ea4dc7e	21	15	0	14
openregister	df815c3	139	42	0	10
opencatalogi	f6e86ab	24	7	0	9
openbuild	6772ff6	21	18	0	9
doriath	bfd6da6	49	31	0	5
scholiq	d2a0a1c	22	17	0	4
launchpad	19e9b49	45	29	0	3
larpingapp	9704abd	6	3	0	3
openconnector	6da5dfb	46	32	0	2
portaliq	65cafbd	10	1	0	2
nldesign	8e49697	12	0	0	0
total		791	453	0	327

This is a candidate count, not an exposure count. An earlier hand-audit of a
similar candidate list found per-app precision ranging from 95% to 0%. Read the
findings; do not quote 327 as a defect count.

Spot-checked that the fix does not swallow the real ones: shillinq's
arbitrary-file-read download endpoint, its SEPA mandate export, pipelinq's
semantic-handoff routes, docudesk's cross-register preview, decidesk's motion
history, opencatalogi's unscoped search, and zaakafhandelapp's object routes are
all still reported. procest's and openconnector's counts are low because both
landed real guards earlier today; the guards were read and confirmed rather than
assumed.

Coverage added

A new authn-vs-authz bundle in the gate-acceptance matrix drives the control
through the real wrapper. Both arms keep the 401 preamble, so the clean arm
pins that the preamble is ignored, not punished — without that, "flag anything
containing a 401" would pass. Its expect.conf requires the finding to name
method=preamble rather than the file path, because the bare arm names the file
under the old checker too; a file-level assertion would have been satisfied by a
checker with the defect intact.

Proven able to fail: running that bundle against the pre-fix checker turns it
red, on exactly that assertion, with the driver reporting that the gate failed for
some other reason and the fixture therefore proves nothing.

Four existing unit tests encoded the defect as correct behaviour and are inverted,
each carrying the reason. The sharpest is the decidesk citizenAction() case: it
sits beside staffAction() in one collaborator, and only staffAction() is an
authorisation guard — the 401 arm was borrowing credibility from the 403 arm next
door.

Verification

• 113 unit tests for this checker, green, up from 95.
• 74 helper suites green, 2 quarantined as documented.
• 61 entry-point tests green.
• One helper suite is red — the gates 45-to-55 acceptance suite — and it is red
  identically on an unmodified canonical clone: ajv is not resolvable in this
  environment and the suite refuses to run rather than emit a false verdict. Not
  caused by this change.

Not verified

• No HTTP probe against a running instance. Every claim about a specific app's
  endpoints here rests on reading source.
• The residual Pattern 6 admits: a callee that accepts the identity argument and
  ignores it. This gate sees one method body by contract, so that residual is
  symmetrical with the guidance already printed with its findings, and the
  ALL-quantifier narrows it rather than removing it.

Merge

Deliberately not merged. It turns a green cell red in all eighteen apps at
once, and the sequencing is Ruben's call.

[rubenvdlinde]

CI settled: 16 pass, 15 skipping, none failing, none pending — count stable at 31 across eleven consecutive polls, since a check list read too early is trivially all-non-pending.

The job that matters is Package invariants, which runs the helper suites: seventy-five green, two quarantined as documented, none red. test_gate_acceptance_matrix.sh is among them, so the new authn-vs-authz bundle is enforced from here on.

One correction to the description: the gates 45-to-55 acceptance suite, reported above as red locally, is green in CI. It refuses to run when ajv is unresolvable rather than emit a verdict it cannot support, so the local red was the machine and not the change — and CI is what establishes that rather than my having said so.

Still deliberately not merged.