Skip to content

fix(gates): widen six pattern-matchers that were failing toward "no match" (gate-7 ×6, gate-48 ×2) - #482

Merged
rubenvdlinde merged 3 commits into
mainfrom
S42/gate-checker-pattern-matching-widenings
Aug 16, 2026
Merged

fix(gates): widen six pattern-matchers that were failing toward "no match" (gate-7 ×6, gate-48 ×2)#482
rubenvdlinde merged 3 commits into
mainfrom
S42/gate-checker-pattern-matching-widenings

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

One PR, one defect class

Every gap below is a pattern-matching instrument failing toward "no match"
and a pattern that matches nothing prints nothing, which reads exactly like a
clean run. That family has cost this fleet six separate defects today. They are
fixed here as a class, with a test that asserts a checker's own pattern matches
the shapes it documents.

Each change carries a positive control (it still catches what it caught) and
a negative control (it no longer flags the false shape), measured on real
origin/development trees. Every measurement states the file count.


gate-7 — check_no_admin_idor.py

1. _OR_IMPORT_RE could not see ObjectServiceInterface

ObjectService\b does not match before the I\b needs a non-word
character. So ADR-084 adoption CREATED gate-7 findings: the delegation the
gate exists to recognise became invisible the moment a repo did what the ADR
told it to.

tree files before after
decidesk 5d6de54 40 12 5
decidesk at 46bc39a^ (pre-ADR-084) 40 5 5

Positive control: decidesk's finding set on today's tip is now identical
(file + method + rule) to its pre-ADR-084 tree's — the exact expectation S13
measured, reproduced.
Negative control: ObjectServiceHelper, ObjectServiceInterfaceFactory and
a leaf app's own OCA\ZaakAfhandelApp\Service\ObjectService still do not match.

What it stops catching: 7 decidesk + 2 shillinq findings. Verified by hand —
e.g. VotingController::proxyProxyDelegationService, which declares
use OCA\OpenRegister\Contract\ObjectServiceInterface and calls
$objectService->find(...) / saveObject(...) with RBAC left on. That is
Pattern 2b's own definition of a delegation.

2. Only use/quoted FQCNs counted, never a type-position one

private \OCA\OpenRegister\Service\ObjectService $objectService; — 16
occurrences in zaakafhandelapp's ZGW services, zero use lines. The three
alternatives collapse to one FQCN matcher, still anchored on
OCA\OpenRegister\, still read from comment-free source so prose cannot qualify
a file.

3. _OR_FACADE_CALL_RE required the facade to be the next -> receiver

The fleet does not write it that way:
$orService = $this->mapperService->getOpenRegisters();,
return $this->container->get('OCA\OpenRegister\Service\ObjectService');,
$objectService = $this->settingsService->getObjectService();.

tree files before after
procest 862c8f8 94 27 25
zaakafhandelapp 01dacc7 25 81 70

->objectService and $objectService still require the arrow — a name is not
a call. getObjectService() and container->get(<OR FQCN>) are calls that
return the facade, so the call is the evidence.

4. The collaborator pass walked ONE class out; the real chains are three

procest  TemplateController::activate -> TemplateLibraryService::activateTemplate
                                      -> SettingsService::getObjectService() -> OR
zaakafhandelapp ZakenController::index -> App\ObjectService::getResultArrayForRequest
                                       -> MapperService::getOpenRegisters()  -> OR

The middle class names OpenRegister nowhere. A wider type match does not reach
it either — procest's accessors are declared ?object. What generalises is
following the CALL, bounded: depth 3, cycle-guarded, fail-closed, and also
following use <Trait>; (procest's OpenRegister bridge is a trait used by 89
classes).

tree files before after
procest 94 25 5
zaakafhandelapp 01dacc7 25 70 30

Bound control: depth 3 / 4 / 5 give identical counts (procest 5, zaa 30), so
the bound sits above the fleet's longest real chain rather than truncating it.
Negative control, same file: TemplateController::indexlistTemplates()
reads a filesystem catalogue and stays a finding; ::activate (two hops to
OpenRegister) clears. Runtime cost on the largest tree (openregister, 139 files):
11.6s → 19.7s.

⚠️ Writing this exposed a latent hole and it is fixed here. _rbac: false
vetoed the SEED of the delegation set but not the CLOSURE, so
softwarecatalog's GebruikService::getGebruiken()
searchObjectsPaginated(query: $options, _rbac: false, _multitenancy: false)
entered the set via a helper and cleared the controller above it. The veto now
applies wherever a name enters the set, and that endpoint reports again
(softwarecatalog 1 → 1). Caught by measurement, not by review.

5. An inline getUser() === null silently defeated Pattern 3

#365 blanks authentication-only clauses so they cannot be mistaken for a
guard. Pattern 3's condition 3 is not a guard test — it asks whether the method
references a caller identity at all — but it read the blanked text, so:

$user = $this->userSession->getUser();
if ($user === null) { return 401; }
return $this->service->findPendingForCurrentUser();   // CLEARED

if ($this->userSession->getUser() === null) { return 401; }
return $this->service->findPendingForCurrentUser();   // FINDING

Two spellings of one preamble, two verdicts. Condition 3 now reads the
comment-free body (cleaned, so a docblock still cannot satisfy it); conditions
1 and 2 are untouched. procest 5 → 4.
Negative controls: a comment naming the session does not clear; a method
taking a parameter still reports; a method reading the request still reports.
Stated rather than hidden: Pattern 3 has no mutation veto, so a zero-input
mutation with only an authentication preamble clears — that was already true via
the assignment spelling; this makes the two agree rather than introducing it.

6. Pattern 6 could not see an identity written INTO a tainted array

softwarecatalog GebruikController::getGebruikenForDeelnemer:

$options = $this->request->getParams();
$options['participants'] = [$orgUuid];   // forced AFTER getParams()
$this->gebruikService->getGebruiken(options: $options);

The one argument carries both the caller's values and an identity the caller
cannot override, and Pattern 6 saw only the first half — so it reported "a
caller-controlled value reaching an unscoped call" about a call that is scoped.
Negative controls: the key filled from the caller's own value still reports;
an identity DERIVED from caller input still reports; a call with nothing written
in still reports.


gate-48

8. check_csrf_removal.py could not tell a DELETED method from a STRIPPED annotation

A -U0 diff gives the same evidence for both. Added a post-image test: a
removal is out of scope when the method it annotated no longer exists at HEAD.
Line numbers come from the hunk headers (five identical - * @NoCSRFRequired
lines cannot be told apart by content), and the located line is verified against
the removed text, so a misparsed offset fails closed.

Proof pair, one repo:

PR what it did before after
zaakafhandelapp#371 deleted 5 DashboardController methods 5 0
zaakafhandelapp#380 stripped the annotation from 10 surviving methods 10 10

Without --repo/--base the helper behaves exactly as before (5 and 10), so an
unreadable image is never a reason to drop a security finding. The runner passes
the merge base, not BASE_REF, because the diff is three-dot.

10. MUTATING_METHOD matched only a quoted literal

const method = isNew ? 'POST' : 'PUT'fetch(url, { method }) was
invisible, and invisible is indistinguishable from protected in this helper's
output.

tree before after
zaakafhandelapp d7cea2a 15 27

Positive control: all 15 originals are still reported (comm -23 = 0
missing). The 12 new ones are the create-or-update handlers — the most
CSRF-relevant calls in the app.
Fail-closed rule: a method value not proven to be GET/HEAD/OPTIONS
counts as mutating. An identifier resolves to its nearest preceding binding —
whole-file resolution produced a false positive on a store module holding both
const method = 'GET' and const method = isNew ? 'POST' : 'PUT'.
Negative controls: method: 'GET', no method key, and a binding proven
safe all stay silent.


The class-level test

scripts/lib/test_checker_patterns_match_documented_shapes.py — a registry of
(pattern, shapes it must match, shapes it must not), plus a meta-test that a
renamed pattern fails loudly rather than silently stopping being covered,
plus the word-boundary property stated directly.

Fail-first control: run against the pre-fix checkers it reports
10 failures + 8 errors. Against these, 0.


Predicted per-repo effect — all 18 development tips, measured

repo files gate-7 before after
decidesk 40 12 4
procest 94 27 4
shillinq 83 6 4
softwarecatalog 21 1 1
openregister 139 8 8
openconnector 46 1 1
opencatalogi 24 · docudesk 32 · nldesign 12 · launchpad 45 · larpingapp 6 · zaakafhandelapp 25 · pipelinq 82 · scholiq 22 · portaliq 11 · openbuild 21 · doriath 49 · hermiq 43 0 0
fleet total 55 21

gate-48 changes no repo's verdict on its own: it changes what a diff is judged
to have done. check_csrf_callers.py will report more call sites wherever a
repo computes its verb — zaakafhandelapp is the measured case (15 → 27 on the
pre-#373 tree; 0 → 0 on today's tip, which already carries tokens).


Deliberately NOT done, with the measurement that blocked each

  • gap 7 — Pattern 3b's request-input veto. opencatalogi a5295b3a measures
    0 gate-7 findings over 24 controller files on both the old and the new
    checker, so the shape is gone from the tree and there is no positive control
    available. Relaxing a security clear on argument rather than measurement is
    what §2 L1 forbids.
  • defect 9 — endpoint-scoping the caller check. Built, then withdrawn
    because its own control failed. On zaakafhandelapp d7cea2a, routes read from
    appinfo/routes.php: repo-wide 27, scoped to DashboardController
    11, scoped to ZakenController 11 — the identical set. An identical
    count across two unrelated controllers is a property of the instrument, not of
    the inputs; and among the 16 it ruled out for ZakenController was
    src/store/modules/zaken.ts:128 — fetch() DELETE, a real zaken caller, dropped
    because the route table says /api/zaken while the store calls
    /api/zrc/zaken. Defect 8 alone clears fix(gate-61): an empty scope has two causes and only one of them is a diff (.github#347) #371 honestly (5 → 0), so the outcome
    is delivered without it. The analysis is recorded in the helper's own
    commentary so the next attempt starts from the measurement.

Blast radius

All 18 repos run this suite from .github@main. Package invariants must stay
green — it is the suite that guards the suite. Locally: test-hydra-gates-bin.sh
70 passed / 0 failed; test_check_no_admin_idor.py 169 passed;
test_check_csrf_callers.py + test_check_csrf_removal.py 46 passed;
the new suite 11 passed. test_gate_45_to_55_acceptance.sh refuses to run
locally without ajv (CI installs it first, by design).

Conduction Release Bot added 3 commits August 16, 2026 21:06
…atch"

Every gap fixed here is the same defect: an instrument that matches nothing
prints nothing, and nothing reads as a clean run. Measured on real
origin/development trees, each with a positive AND a negative control.

gate-7 check_no_admin_idor.py
  1. _OR_IMPORT_RE could not see `ObjectServiceInterface` — `\b` does not
     match before the `I`. ADR-084 adoption therefore CREATED findings:
     decidesk 5 -> 12 with no controller edit. Fixed: decidesk 12 -> 5, and
     its finding set is identical (file+method+rule) to its pre-ADR-084 tree.
  2. The same pattern saw only `use`/quoted FQCNs, never a type-position one
     (`private \OCA\OpenRegister\Service\ObjectService $svc;` — 16 occurrences
     in zaakafhandelapp, zero `use` lines). Collapsed to one FQCN alternative,
     still anchored on `OCA\OpenRegister\`.
  3. _OR_FACADE_CALL_RE required the facade to be the receiver of the next
     `->`, so `$svc = $this->settingsService->getObjectService()` and
     `$c->get('OCA\OpenRegister\Service\ObjectService')` did not count as
     reaching it. procest 27 -> 25, zaakafhandelapp@01dacc7 81 -> 70.
  4. The collaborator pass walked ONE class out; the fleet's chains are three,
     and part of one lives in `use <Trait>;`. Added a bounded (depth 3, cycle-
     guarded, fail-closed) transitive pass. procest 25 -> 5, zaakafhandelapp
     @01dacc7 70 -> 30. Depth 3/4/5 give identical counts, so the bound is
     above the longest real chain. Negative control: procest
     TemplateController::index (a filesystem catalogue) stays a finding while
     ::activate (2 hops to OpenRegister) clears.
     ⚠️ Writing (4) exposed a latent hole and it is fixed here: `_rbac: false`
     vetoed the SEED but not the CLOSURE, so softwarecatalog's
     `searchObjectsPaginated(_rbac: false, _multitenancy: false)` chain
     cleared. It reports again.
  5. Pattern 3's condition 3 read the auth-BLANKED body, so an inline
     `if ($this->userSession->getUser() === null)` and the assignment spelling
     of the same preamble got opposite verdicts. Condition 3 now reads the
     comment-free body; conditions 1 and 2 are untouched. procest 5 -> 4.
  6. Pattern 6 could not see an identity written INTO a tainted array
     (`$options['participants'] = [$orgUuid]` after `getParams()`).
     softwarecatalog 1 -> 0.

gate-48 check_csrf_removal.py / check_csrf_callers.py
  8. A `-U0` diff cannot tell a DELETED method from a STRIPPED annotation.
     Added a post-image test: a removal is out of scope when the method it
     annotated no longer exists at HEAD. Positive/negative pair in one repo —
     zaakafhandelapp#371 (5 methods deleted) 5 -> 0; #380 (10 surviving
     methods stripped) 10 -> 10. Without --repo/--base the behaviour is
     unchanged, so an unreadable image never drops a security finding.
 10. MUTATING_METHOD matched only a quoted literal, so
     `const method = isNew ? 'POST' : 'PUT'` … `{ method }` was invisible.
     zaakafhandelapp@d7cea2a 15 -> 27 reported, and all 15 originals are still
     reported. Fail-closed: a `method` value not PROVEN to be GET/HEAD/OPTIONS
     counts as mutating; an identifier resolves to its nearest preceding
     binding.

Plus the class-level test this whole family needed:
test_checker_patterns_match_documented_shapes.py asserts that each registered
pattern matches every spelling it is responsible for and none it disclaims,
and that a renamed pattern fails loudly rather than silently stops being
covered. Against the pre-fix checkers it reports 10 failures + 8 errors;
against these, 0.

Fleet gate-7 effect, measured on all 18 development tips:
decidesk 12->4, procest 27->4, shillinq 6->4, softwarecatalog 1->1,
every other repo unchanged. Total 55 -> 21.

Deliberately NOT done, with the measurement that blocked each:
  - gap 7 (Pattern 3b's request-input veto): opencatalogi measures 0 gate-7
    findings today, so there is no positive control on a real tree for
    relaxing a security clear.
  - defect 9 (endpoint-scoping the caller check): built, then withdrawn — it
    scored 27 -> 11 for DashboardController AND 27 -> 11 for ZakenController,
    the IDENTICAL set, and among the 16 it ruled out for ZakenController was a
    real unprotected zaken DELETE. Defect 8 alone clears #371 honestly.
The package's own acceptance matrix caught this, which is what it is for.

The first version of the facade widening added `->getObjectService()` (no
trailing `->`) as an alternative in `_OR_FACADE_CALL_RE`. `.github#373`'s
planted fixture `comment-silenced-guard/planted` is precisely a SERVICE LOCATOR
with that name:

    if (!class_exists('\OCA\OpenRegister\Service\ObjectService')) { throw ... }
    return $this->themes;                       // a LOCAL service

It NAMES OpenRegister and returns something else. Matching on the name cleared
`ThemeController::show`, and the matrix reported that gate-7 "failed for some
OTHER reason, so this fixture proves nothing" — 165 passed / 1 failed.

The alternative is withdrawn. What stays is the CONTAINER RESOLUTION of
OpenRegister's ObjectService (the app naming the class it obtains, in code),
and the bounded transitive pass, which establishes an accessor's reach from the
accessor's own BODY. procest's `SettingsService::getObjectService()` resolves
through the container and still clears its callers; the fixture's locator does
not and `show()` is a finding again.

Matrix: 165 passed / 1 failed -> 166 passed / 0 failed.
Fleet gate-7 totals are unchanged by this narrowing: 55 -> 21, same four repos.
…layer owns an accessor

The three accessor spellings move from MUST_MATCH to MUST_NOT_MATCH for
_OR_FACADE_CALL_RE, with the reason stated: after the acceptance matrix showed
that matching an accessor by NAME clears .github#373's planted service locator,
the reach of an accessor is _or_delegating_methods_deep's responsibility, and it
establishes it by READING the body.

AccessorReachIsEstablishedFromTheBody pins both directions with two classes that
differ only in what their identically-named getObjectService() actually returns.

228 python assertions pass against these checkers; against the pre-fix ones the
new file alone reports 8 failures + 10 errors.
@rubenvdlinde
rubenvdlinde merged commit f935e2c into main Aug 16, 2026
36 checks passed
@rubenvdlinde
rubenvdlinde deleted the S42/gate-checker-pattern-matching-widenings branch August 16, 2026 19:37
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