fix(gate-9): the admin rule matched no real isAdmin() call, and prose bought the self-auth exemption - #198
Merged
rubenvdlinde merged 2 commits intoAug 7, 2026
Conversation
added 2 commits
August 7, 2026 08:17
… bought the self-auth exemption Three defects, found while working two openconnector findings. 1. `no-admin-required-annotation-with-admin-body` was blind to the only form the guard is ever written in. The condition matcher used `\bisAdmin\b[^)]*===\s*false`, and `isAdmin($this->userId) === false` puts a `)` between the name and the comparison, so the character class could not reach it. The same over-restrictive-class mistake as the `[^}]*` body regex this module was written to replace (W28). The condition slice is already bounded by its own matching paren, so `.*?` is safe there; the Yoda form is matched too. That one character class was hiding 25 findings across 10 repos — #[NoAdminRequired] on methods that enforce admin in the body, i.e. endpoints whose attribute says the opposite of what they do. Every one was reported PASS by every run of this gate until now. Sampled and confirmed real: openregister FederatedConfigController::trust/setTrust, softwarecatalog SettingsController ×8, pipelinq CtiController ×4. 2. The self-auth exemption was matched against raw source, so a docblock describing a credential check exempted a method that performed none. The 2026-08-06 gate-64 shape, where a commented-out call counted as a real one. Matched against comment-stripped source now. Comments only — NOT string literals. `'Bearer '`, `'HTTP_AUTHORIZATION'` and the header name passed to getHeader() are literals in every real handler, and blanking those would manufacture exactly the false positives this gate was rewritten to stop. 3. Two endpoints were passing only BECAUSE of (2), and would have turned red the moment it was fixed: hermiq McpRunController::handle and EgressAuthorizeController::authorize resolve their credential through a helper (`bearerToken()`) passed as a named argument (`token:`), neither of which the six-verb pattern list covered. It only ever matched the word "bearer" in their comments. Widened to any `->…Token…(` call and to named credential arguments. Also adds username/password credentials to the exemption. A login endpoint is #[PublicPage] by necessity — the caller has no session yet, that is what it is asking for — and answers 401 when the password is wrong. The token-only list reported openconnector UserController::login as an unsourced denial although it resolves $username/$password from the request and calls checkPassword() in the body; there was no correct action a developer could take on that finding. Measured over 761 fleet controllers, old gate vs new: 2 false positives removed, 25 previously invisible true positives revealed. Adds test_check_semantic_auth.py — the module had none, and it is picked up automatically by the discovery runner. 21 assertions, of which 9 are failure-demonstrating: every exemption is paired with the same PHP shape minus the one thing that earns it, so the suite cannot go quiet by accident. Three tests specifically pin (2) and (3) against each other — prose must not exempt, literals must still count.
…d to it
The previous commit said this module shipped without tests and wrote a new
file. It did not — 252 lines of suite already existed, and the write
replaced them. Restored verbatim and extended instead.
What came back matters more than what I added. `RemediationTextIsSafe`
asserts on the FINDING STRING itself: that the advice never tells a
developer to remove #[PublicPage] or delete the auth check, and that it
names the request-borne alternative. That is the assertion standing between
this gate and its own history of telling people to open an endpoint.
`GateIsNotBlind` asserts the scanner reads methods at all, without which
every `assertEqual([])` in the file passes on nothing.
Added on top, one class per defect the previous commit fixed:
AdminGuardsWithArguments isAdmin($uid) === false, and the Yoda form,
against a non-admin predicate that must stay
clean
PasswordsAreCredentialsToo login, password_verify share unlock, and the
helper-resolved token passed as a named
argument
ProseDoesNotEarnTheExemption a docblock and a commented-out check must
still fire
StringLiteralsStillCount 'Bearer ', $_SERVER['HTTP_…'] and a `//`
inside a URL must still count
The last two are each other's control: strip too little and prose exempts,
strip too much and correct handlers become findings.
24 tests, up from 13.
This was referenced Aug 7, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Aug 8, 2026
…o full of markup, and three reported PASS over a crashed checker (#272) * fix(gates 35,40,42,44): four a11y gates excused themselves from a repo full of markup, and three reported PASS over a crashed checker Measured at package sha cdfbd7a against opencatalogi (93 .vue) and nldesign (zero .vue, one PHP template), one textbook true positive planted per gate in BOTH — the asymmetry that made #225/#261 possible. All 11 gates in the 34-44 band fired and named the plant in both arms, and all returned to their exact prior verdict on removal. Two defects survive that. 1. FOUR GATES GO `na` ON A TEMPLATES-ONLY REPO Gates 35, 40, 42 and 44 still guarded on `[ -d src ]` while 34/36/37/39/43 had moved to `_a11y_has_markup_dir`, and the central applicability table listed the whole family under `[ -d src ]`. On a repo with a `templates/` full of markup and no `src/`, same run, same files: gate-34/36/37/38/39/41/43 ran; four of them FAILED on the plants gate-35/40/42/44 NOT APPLICABLE — "this repo ships no frontend, so there is no .vue/.js/.ts source for this gate to inspect" `na` is the one verdict that removes a gate from coverage accounting, and the reason was contradicted by the same run's own output three lines above it. No fleet app is templates-only today; nldesign is one `rm` away, since its `src/` holds a single `manifest.json` — the exact shape that made twelve gates pass over nothing in #225. The guards now call `_a11y_has_markup_dir`, and the applicability declaration calls THE SAME FUNCTION rather than restating it, so the two cannot drift again. No third scope definition was added. 2. A CRASHED CHECKER REPORTED PASS (#147 / #249) — gates 40, 42, 44 With a `python3` on PATH that exits 1 on every call, run against opencatalogi: gate-40 PASS gate-42 PASS gate-44 PASS <- the three inline ones gate-34/37/38/39/41/43 SKIPPED (wiring) <- the six behind a helper gate-40 printed PASS over the 13 real findings it had reported one run earlier. gate-40 discarded its status with `2>/dev/null || true`; 42 and 44 ran per-file inline heredocs and never had one. 42 and 44 move to scripts/lib/check_link_text.py and scripts/lib/check_autocomplete.py — one interpreter for the whole file set, findings on stdout, exit code as a status — and 40 gains the same return-code guard. FOUND WHILE WRITING THE TESTS * gate-44 judged an input on the FIRST of name/id/v-model and stopped, so `<input id="e" type="text" name="email">` — the plainest textbook case this gate has — passed. Fleet effect, measured across 15 repos: openregister 0 -> 1 (an OpenAI Organization ID field), pipelinq 4 -> 5 (a "Colleague email" field). Both genuine, nothing lost. * gates 35, 36 and 44 read attribute values out of DOUBLE QUOTES ONLY. `tabindex='5'`, `alt=''` and `name='telephone'` render identically and reported PASS in both arms. Zero occurrences in the fleet today, which is why they could sit there indefinitely. * `[^>]*` in gates 42 and 44: a `>` inside an attribute value is not the end of a tag — the parse that hid 19 buttons from gate-39 (#259, #198, #236). * gates 42 and 44 scanned RAW text, so a commented-out `<a>click here</a>` or `<input name="email">` counted. That is gate-64's defect (#184), the one gate-38 (#247) and gate-41 (#266) each shipped a fix for. MEASURED AFTER, NOT ONLY BEFORE * 15 repos, gates 34-44, before vs after: every verdict and every finding count identical except the two new gate-44 true positives above. The rewrites of 42 and 44 removed nothing. * opencatalogi and nldesign return to their exact pre-plant baselines. * ARM 4 of test_gate_a11y_markup_scope.sh was mutation-checked: reverting gate-42's guard to `[ -d src ]` turns it red with the finding it was written for. TESTS * scripts/lib/test_check_link_text.py, test_check_autocomplete.py — 32 assertions; every relaxation ships with the true positive it must not swallow, comment/script exclusions ship with their positive control, and each ends with the whole PRE-FIX checker replayed as the mutant, asserting it answers DIFFERENTLY on every fixture. * test_gate_a11y_helper_wiring.sh gains gates 39, 40, 42, 44 (39 was wired correctly but never listed, so nothing held it to that) — 70 assertions. * test_gate_a11y_markup_scope.sh gains ARM 4, the templates-only repo. * Full discovered suite: 49 passed, 0 failed, 2 pre-existing quarantines. tests/test-hydra-gates-bin.sh: 59 passed, 0 failed. * fix(test): SC2194 — the case word was the constant, not the subject `case " 38 45 " in *" ${_g} "*)` matches a constant against a pattern built from the variable, which is the comparison written backwards. It happened to work, and ShellCheck is right that it reads as a mistake. Verified with shellcheck 0.10.0 at full severity: clean.
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.
Found while clearing two gate-9 findings on openconnector
development. Both findings were false positives; chasing why turned up a third, larger problem underneath.1. The admin rule has never matched a real guard
no-admin-required-annotation-with-admin-bodytested theifcondition with:isAdmin()is essentially always called with a UID —$this->groupManager->isAdmin($this->userId) === false— and that)stops[^)]*before the comparison. It is the same over-restrictive character class as the[^}]*body regex this module was written to replace (W28).The condition slice is already bounded by its own matching paren, so
.*?cannot run past the condition. The Yoda form (false === …isAdmin(…)) is matched too.That one character class was hiding 25 findings across 10 repos —
#[NoAdminRequired]on methods that enforce admin in the body, i.e. endpoints whose attribute states the opposite of what they do. Every run of this gate reported PASS on all of them. Sampled and confirmed real:FederatedConfigController::trust,::setTrust,CredentialController::registerAppSettingsController×8CtiController×4,NotesController::deleteAll,CallbackController::reassign,KassakoppelingAuditController::exportRoutingController::reroute,InspectionChecklistController::submitResult2. Prose bought the self-auth exemption
_SELF_AUTH_REwas matched against raw source, so a docblock reading "callers must present a bearer token" exempted a method containing no such check — the 2026-08-06 gate-64 shape, where a commented-out call counted as a real one. Now matched against comment-stripped source.Comments only, not string literals.
'Bearer ','HTTP_AUTHORIZATION'and the header name handed togetHeader()live in literals in every real handler; blanking those would manufacture exactly the false positives this gate was rewritten to stop.3. Two endpoints were passing only because of (2)
hermiq
McpRunController::handleandEgressAuthorizeController::authorizeresolve their credential via a helper passed as a named argument:Neither
bearerToken()nortoken:was in the six-verb pattern list — the only thing matching was the word bearer in their own comments. Fixing (2) alone would have turned two correct endpoints red. Widened to any->…Token…(call and to named credential arguments.Plus: a password is a credential
A login endpoint is
#[PublicPage]by necessity — the caller has no session yet, that is what it is asking for — and answers 401 when the password is wrong. openconnectorUserController::loginresolves$username/$passwordfrom the request and callscheckPassword()in its body, and the token-only list still called it an unsourced denial. There was no correct action a developer could take on that finding, which is the standard this gate already sets for itself.Measured impact
Old gate vs new, over 761 fleet controllers:
UserController::login, hermiqMcpRunController::handleRepos go red only where their diff touches those files (ADR-020 diff scoping).
Tests
The module shipped with none.
test_check_semantic_auth.pyis picked up automatically by the discovery runner — 21 assertions, 9 of them failure-demonstrating: every exemption is paired with the same PHP shape minus the one thing that earns it, so the suite cannot go quiet by accident. Three tests pin (2) and (3) against each other — prose must not exempt, literals must still count, and a//inside a URL must not swallow the rest of the method.Full helper-suite run: 26 passed, 2 quarantined as documented, 0 failed.