From 220286fcfd7e09ea1a37946b743ac588baea94d2 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Sun, 16 Aug 2026 21:06:07 +0200 Subject: [PATCH 1/3] fix(gates): widen six pattern-matchers that were failing toward "no match" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ;`. 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. --- hydra-gates/scripts/lib/check_csrf_callers.py | 156 +++++- hydra-gates/scripts/lib/check_csrf_removal.py | 146 +++++- .../scripts/lib/check_no_admin_idor.py | 483 +++++++++++++++++- ...hecker_patterns_match_documented_shapes.py | 266 ++++++++++ hydra-gates/scripts/run-hydra-gates.sh | 17 +- 5 files changed, 1039 insertions(+), 29 deletions(-) create mode 100644 hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py diff --git a/hydra-gates/scripts/lib/check_csrf_callers.py b/hydra-gates/scripts/lib/check_csrf_callers.py index 635e9a32..ab87a7bd 100644 --- a/hydra-gates/scripts/lib/check_csrf_callers.py +++ b/hydra-gates/scripts/lib/check_csrf_callers.py @@ -69,6 +69,117 @@ r"""method\s*:\s*['"`](?PPOST|PUT|PATCH|DELETE)['"`]""", re.IGNORECASE, ) + +# 🔴 A QUOTED LITERAL IS NOT THE ONLY WAY TO SPELL A VERB — AND THE OTHER WAYS +# WERE INVISIBLE, WHICH MEANS THEY WERE COUNTED AS SAFE. +# +# `MUTATING_METHOD` requires the verb to be a quoted literal sitting directly +# after `method:`. The fleet's create-or-update handlers do not write it that +# way; they compute it: +# +# const method = isNew ? 'POST' : 'PUT' +# await fetch(url, { method, headers, body }) +# +# fetch(url, { method: this.editing ? 'PUT' : 'POST', ... }) +# +# Neither matches, so `verb is None`, so the call is skipped — and skipped is +# indistinguishable from protected in this helper's output. MEASURED on +# zaakafhandelapp: **15 call sites reported, 27 actually unprotected.** The +# twelve invisible ones are exactly the create-or-update handlers, which are +# the most CSRF-relevant calls in the app. +# +# THE RULE IS FAIL-CLOSED, and that is the whole design: a `method` key whose +# value this helper cannot PROVE is a safe verb counts as mutating. Proof is +# narrow on purpose — a quoted `GET`/`HEAD`/`OPTIONS`, or an identifier whose +# every assignment in the file resolves to safe verbs. Anything else (a +# ternary, a template literal, a call, a shorthand `{ method }` whose binding +# cannot be found) is treated as mutating. An unreadable value is not a pass. +METHOD_KEY_VALUE = re.compile(r"""(?[^,}\n]+)""") +# ES6 shorthand: `{ method }` / `{ ..., method, ... }` — the value is the +# binding of the same name, resolved below. +METHOD_SHORTHAND = re.compile(r"""[{,]\s*method\s*(?=[,}])""") +SAFE_VERB_LITERAL = re.compile(r"""^\s*['"`](?:GET|HEAD|OPTIONS)['"`]\s*$""", + re.IGNORECASE) +MUTATING_VERB_ANYWHERE = re.compile(r"""['"`](?:POST|PUT|PATCH|DELETE)['"`]""", + re.IGNORECASE) +BARE_IDENTIFIER = re.compile(r"""^\s*(?P[A-Za-z_$][\w$]*)\s*$""") + + +def _binding_values(text: str, name: str, before: int = None) -> list: + """Right-hand sides assigned to *name*, NEAREST PRECEDING BINDING WINS. + + ⚠️ A WHOLE-FILE SEARCH IS THE WRONG INSTRUMENT HERE, and its error is a + FALSE POSITIVE — which in a security gate is the error that gets the gate + ignored. A store module routinely holds + + const method = 'GET' // in one action + const method = isNew ? 'POST' : 'PUT' // in another + + and answering "can `method` ever be mutating" over the whole file reports + the GET caller too. So the resolution is positional: the last binding + ESTABLISHED BEFORE the call site, which is what a reader resolves. When + *before* is given and no binding precedes it, the list is empty and the + caller fails closed. + """ + pattern = re.compile( + r"""(?:(?:const|let|var)\s+)?(?post|put|patch|delete)\s*\(""", @@ -104,6 +215,36 @@ def _call_text(text: str, open_paren: int) -> str: return text[open_paren:] +# --------------------------------------------------------------------------- +# ⚠️ WHAT IS DELIBERATELY *NOT* HERE: ENDPOINT SCOPING +# --------------------------------------------------------------------------- +# +# gate-48 blocks when an annotation was removed AND any unprotected mutating +# caller exists ANYWHERE under `src/` — without relating the two. That is a +# real defect (zaakafhandelapp#371 was blocked by 15 call sites, byte-identical +# on `origin/development`, none of them targeting `api/dashboard`), and the +# obvious repair is to correlate the call site's URL with the routes of the +# controller that lost its annotation. +# +# IT WAS BUILT AND THEN WITHDRAWN, BECAUSE ITS OWN CONTROL FAILED. Measured on +# zaakafhandelapp at `d7cea2a` with routes read from `appinfo/routes.php`: +# +# repo-wide 27 +# scoped to DashboardController 11 +# scoped to ZakenController 11 <- IDENTICAL SET +# +# An identical count across two unrelated controllers is a property of the +# instrument, not of the diffs. The filter was dominated by the unresolvable +# residue, and among the 16 it ruled out for ZakenController was +# `src/store/modules/zaken.ts:128 — fetch() DELETE` — a zaken caller, dropped +# from the zaken scope, because the route table says `/api/zaken` while the +# store calls `/api/zrc/zaken`. A correlation that drops a true finding is +# worse than the over-blocking it replaces. +# +# `check_csrf_removal.py`'s post-image test (the deleted-vs-stripped fix) +# already clears #371 honestly on its own — measured 5 removals -> 0 — so the +# outcome this scoping was wanted for is delivered without it. Recorded here +# rather than shipped, so the next attempt starts from the measurement. def unprotected_call_sites(app_dir: str) -> list[str]: """Mutating frontend call sites carrying no CSRF-bearing mechanism.""" findings: list[str] = [] @@ -172,18 +313,23 @@ def unprotected_call_sites(app_dir: str) -> list[str]: f"signal and no @nextcloud/axios import" ) - # 2. fetch(...) — mutating iff its init object names a mutating verb. + # 2. fetch(...) — mutating unless its `method` is PROVEN to be a + # safe verb. A `method` key whose value cannot be resolved to + # GET/HEAD/OPTIONS counts as mutating; see the commentary above + # `METHOD_KEY_VALUE`. No `method` key at all is still a GET and + # is still skipped. for m in FETCH_CALL.finditer(text): call = _call_text(text, m.end() - 1) - verb = MUTATING_METHOD.search(call) - if verb is None: + is_mutating, label = _fetch_is_mutating(call, text, m.start()) + if is_mutating is not True: continue if CSRF_SIGNAL.search(call): continue + verb = MUTATING_METHOD.search(call) + shown = verb.group('verb').upper() if verb else label line = text.count('\n', 0, m.start()) + 1 findings.append( - f"{rel}:{line} — fetch() {verb.group('verb').upper()} with no " - f"CSRF signal" + f"{rel}:{line} — fetch() {shown} with no CSRF signal" ) return findings diff --git a/hydra-gates/scripts/lib/check_csrf_removal.py b/hydra-gates/scripts/lib/check_csrf_removal.py index f74170a4..3f1451c4 100644 --- a/hydra-gates/scripts/lib/check_csrf_removal.py +++ b/hydra-gates/scripts/lib/check_csrf_removal.py @@ -55,6 +55,7 @@ from __future__ import annotations import re +import subprocess import sys # `-` then optional whitespace then `#[`, with NoCSRFRequired inside the @@ -70,10 +71,17 @@ # `+++ b/lib/Controller/X.php` — starts a new file's hunks. `+++` must be # tested before the `+` addition branch, exactly as `---` is before `-`. DIFF_FILE_HEADER = re.compile(r'^\+\+\+\s+(?:b/)?(?P\S+)') +# `@@ -12,5 +12,0 @@` — the base-image start line of the hunk. +HUNK_HEADER = re.compile(r'^@@\s+-(?P\d+)(?:,\d+)?\s') def removals(diff: str) -> list[str]: - """Removed lines that genuinely DROPPED CSRF protection. + """Removed lines that genuinely DROPPED CSRF protection (paths dropped).""" + return [line for _path, line, _lineno in removals_with_paths(diff)] + + +def removals_with_paths(diff: str) -> list: + """``(path, line)`` for every removed line that dropped CSRF protection. A REMOVAL PAIRED WITH AN IDENTICAL ADDITION IS A MOVE, NOT A REMOVAL. @@ -127,39 +135,157 @@ def removals(diff: str) -> list[str]: """ # {path: [normalised content of each added line]}, removals in file order. added: dict[str | None, list[str]] = {} - found: list[tuple[str | None, str]] = [] + found: list = [] path: str | None = None + # BASE-IMAGE LINE NUMBER, tracked from the hunk headers. Five identical + # `- * @NoCSRFRequired` lines are indistinguishable by CONTENT, so the + # post-image test below can only be positional. `-U0` emits no context + # lines, so a `-` advances the base cursor and a `+` does not. + base_lineno = 0 for line in diff.splitlines(): header = DIFF_FILE_HEADER.match(line) if header: path = header.group('path') continue + hunk = HUNK_HEADER.match(line) + if hunk: + base_lineno = int(hunk.group('start')) + continue if line.startswith('+'): added.setdefault(path, []).append(line[1:].strip()) continue if not line.startswith('-') or DIFF_HEADER.match(line): + if line.startswith(' '): + base_lineno += 1 continue + here = base_lineno + base_lineno += 1 if ATTRIBUTE_REMOVED.match(line) or DOCBLOCK_TAG_REMOVED.match(line): - found.append((path, line)) + found.append((path, line, here)) - out: list[str] = [] - for file_path, line in found: + out: list = [] + for file_path, line, here in found: pool = added.get(file_path) key = line[1:].strip() if pool is not None and key in pool: # Consume the pairing so a second identical removal still reports. pool.remove(key) continue - out.append(line) + out.append((file_path, line, here)) return out +# --------------------------------------------------------------------------- +# 🔴 DELETING THE METHOD AND STRIPPING ITS ANNOTATION ARE NOT THE SAME CHANGE, +# AND A `-U0` DIFF CANNOT TELL THEM APART +# --------------------------------------------------------------------------- +# +# Everything above reads only `-` lines. A `-U0` diff of +# +# - #[NoCSRFRequired] +# - public function legacyDashboard(): JSONResponse { ... } +# +# and a `-U0` diff of +# +# - #[NoCSRFRequired] +# +# produce the SAME evidence for this helper: one removed attribute line. But +# the first change DELETED the endpoint — there is nothing left for a forged +# request to reach — while the second turned CSRF enforcement ON for a method +# that survives. Only the second is a posture change this gate should judge, +# and only the second can have a frontend counterpart to co-change. +# +# MEASURED on zaakafhandelapp#371: base declared `#[NoCSRFRequired]` six times +# on `DashboardController`, the branch declares it once on the surviving +# `page()`, and the other five methods are GONE. gate-48 reported five +# removals. The PR spent a coordinator decision, a security warning and an +# exclusion marker on a finding about endpoints that no longer exist. +# +# THE TEST, and it is a property of the POST-IMAGE rather than of the diff: +# a removal is out of scope when no `function ` survives in that file at +# HEAD, where `` is the method the removed line annotated in the BASE +# image. Both images are read from git, so this needs a repo and a base ref; +# without them the behaviour is unchanged and every removal is reported. THAT +# IS THE FAIL-CLOSED DIRECTION — an unreadable image is never a reason to drop +# a security finding. +_FUNCTION_DECL = re.compile( + r"\bfunction\s+(?P[A-Za-z_][A-Za-z0-9_]*)\s*\(") + + +def _git_show(repo: str, ref: str, path: str): + """File contents at *ref*, or ``None`` when it cannot be read.""" + try: + proc = subprocess.run( + ["git", "-C", repo, "show", f"{ref}:{path}"], + stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, + ) + except OSError: + return None + if proc.returncode != 0: + return None + return proc.stdout.decode("utf-8", "replace") + + +def _annotated_method(image: str, lineno: int, needle: str): + """Name of the method the removed line at 1-based *lineno* annotates. + + An attribute or docblock tag sits ABOVE its declaration, so the method is + the next `function (` at or after that line. ``None`` when the line + is not where the diff said it was — verified against *needle*, so a stale + or misparsed offset cannot silently address a different method. + """ + lines = image.splitlines() + index = lineno - 1 + if index < 0 or index >= len(lines): + return None + if lines[index].strip() != needle: + return None + m = _FUNCTION_DECL.search("\n".join(lines[index:])) + return m.group("name") if m is not None else None + + +def survives_at_head(repo: str, base_ref: str, path: str, line: str, + lineno: int = 0) -> bool: + """True when the method this removal annotated still exists at HEAD. + + Returns True — i.e. "report it" — whenever the question cannot be answered: + no repo, no base ref, an unreadable image, or a line that cannot be located + in the base image. An unanswerable question is not a clean bill of health. + """ + if not repo or not base_ref or not path or not lineno: + return True + head_image = _git_show(repo, "HEAD", path) + if head_image is None: + # The whole controller file is gone at HEAD. Every endpoint in it is + # gone with it, so there is nothing left to protect. + return False + base_image = _git_show(repo, base_ref, path) + if base_image is None: + return True + name = _annotated_method(base_image, lineno, line[1:].strip()) + if name is None: + return True + surviving = {m.group("name") for m in _FUNCTION_DECL.finditer(head_image)} + return name in surviving + + def main(argv: list[str]) -> int: - if len(argv) > 1: - print("usage: check_csrf_removal.py < unified.diff", file=sys.stderr) - return 2 - for line in removals(sys.stdin.read()): + repo = base_ref = None + args = argv[1:] + while args: + head = args.pop(0) + if head == "--repo" and args: + repo = args.pop(0) + elif head == "--base" and args: + base_ref = args.pop(0) + else: + print("usage: check_csrf_removal.py [--repo DIR --base REF] " + "< unified.diff", file=sys.stderr) + return 2 + for path, line, lineno in removals_with_paths(sys.stdin.read()): + if not survives_at_head(repo, base_ref, path or "", line, lineno): + continue print(line) return 0 diff --git a/hydra-gates/scripts/lib/check_no_admin_idor.py b/hydra-gates/scripts/lib/check_no_admin_idor.py index 13a7ba34..2a851f40 100644 --- a/hydra-gates/scripts/lib/check_no_admin_idor.py +++ b/hydra-gates/scripts/lib/check_no_admin_idor.py @@ -1106,6 +1106,112 @@ def _declared_parameter_names(params: str) -> set: return set(re.findall(r"\$([A-Za-z_][A-Za-z0-9_]*)", params)) +# An identity written INTO an array, at any subscript depth: +# +# $options['participants'] = [$orgUuid]; +# $filters['owner'][] = $userId; +# +# The base name is captured; what the key is called is the app's business. +_IDENTITY_INTO_ARRAY_RE = re.compile( + r"\$([A-Za-z_][A-Za-z0-9_]*)\s*(?:\[[^\]\n]{0,120}\]\s*)+=\s*([^;\n]{1,200});") + + +def _expression_carries_identity(expr: str, session: set) -> bool: + """True when *expr* evaluates to — or is a list containing — the caller's identity.""" + expr = expr.strip() + m = re.fullmatch(r"\[(.*)\]", expr, re.S) + if m is not None: + return any(_expression_carries_identity(part, session) + for part in _split_arguments(m.group(1))) + if _SESSION_IDENTITY_RE.search(expr): + return True + m = re.fullmatch(r"\$([A-Za-z_][A-Za-z0-9_]*)", + _strip_leading_scalar_casts(expr)) + return m is not None and m.group(1) in session + + +def _identity_carrier_names(body: str, session: set, declared: set = None) -> set: + """Names of arrays the caller's identity has been written INTO. + + 🔴 PATTERN 6'S BLIND SPOT: AN IDENTITY WRITTEN INTO A TAINTED ARRAY. + + Pattern 6 asks whether every call receiving a caller-supplied value also + receives the session identity. It read that identity only out of a whole- + variable assignment, so the fleet's canonical way of FORCING a scope onto a + caller-supplied query was invisible to it. MEASURED on softwarecatalog + `GebruikController::getGebruikenForDeelnemer`: + + $options = $this->request->getParams(); <- caller-supplied + $options['participants'] = [$orgUuid]; <- session-derived scope, + written AFTER getParams() + so the caller cannot + override it + $this->gebruikService->getGebruiken(options: $options); + + The single argument carries BOTH the caller's values and an identity the + caller cannot forge, 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. A gate that is red for a reason that is false is how reviewers + learn to wave its output through. + + THESE NAMES ARE DELIBERATELY *NOT* SUBTRACTED BY `declared`. That is the + whole shape: the array is tainted AND scoped at once, and the existing + `session - declared` subtraction exists to stop a CALLER-CHOSEN value being + read as an identity — which cannot happen here, because what promotes the + name is an assignment whose right-hand side is itself a session identity. + + The array is treated as the carrier at the same epistemic level as every + other hand-off this pattern recognises: it cannot verify that the callee + honours the key, exactly as it cannot verify that a callee honours a + `userId:` argument. Same bar, same evidence. + + Closed to a fixpoint so `$scope['org'] = $orgUuid; $q['scope'] = $scope;` + is followed. + + ⚠️ IT ALSO HAS TO FOLLOW ONE DERIVATION, or it does not fire on the shape + it was written for. `_is_identity_expression` refuses any call WITH + ARGUMENTS — deliberately, so `canAccess($id, $uid)` can never read as an + identity — and softwarecatalog's org uuid comes from + `getUserValue($user->getUID(), 'core', 'organisation')`. That is a value + DERIVED from the session, and the derivation carries arguments. + + So a local is treated as session-derived when its right-hand side mentions + a session identity AND mentions no caller-supplied name. The second half is + the safety: the moment a declared/tainted name appears in the derivation, + the caller has influenced the value and it is not an identity any more. + """ + out: set = set(session) + declared = declared or set() + tainted_re = re.compile( + "|".join(r"\$" + re.escape(d) + r"\b" for d in sorted(declared)) + ) if declared else None + for _ in range(4): # fixpoint; these chains are one or two links long + grew = False + # (a) `$orgUuid = ` — one derivation hop. + for m in re.finditer( + r"\$([A-Za-z_][A-Za-z0-9_]*)\s*=\s*([^;\n]{1,200});", body): + name, rhs = m.group(1), m.group(2) + if name in out or name in declared: + continue + if tainted_re is not None and tainted_re.search(rhs): + continue + if _expression_carries_identity(rhs, out): + out.add(name) + grew = True + # (b) `$options['participants'] = [$orgUuid];` — the array carrier. + for m in _IDENTITY_INTO_ARRAY_RE.finditer(body): + name, rhs = m.group(1), m.group(2) + if name in out: + continue + if _expression_carries_identity(rhs, out): + out.add(name) + grew = True + if not grew: + break + return out - set(session) + + def _session_identity_names(body: str, declared: set) -> set: """Locals in *body* assigned from an argument-free identity expression.""" out: set = set() @@ -1396,6 +1502,13 @@ def _has_session_identity_handoff(body: str, params, helper_bodies=None, # `#398` half (A): follow the caller's value through the locals it is # laundered into, so half (B) below cannot silently drop a real IDOR. declared = _taint_closure(body, declared, session) + # Arrays the identity was WRITTEN INTO carry it as an argument even though + # they are themselves caller-supplied — see `_identity_carrier_names`. They + # are added AFTER the taint closure on purpose: an array that carries the + # scope is still tainted, so clause 2 keeps asking about it, and the only + # thing this changes is that the answer can now be "yes, and it also + # carries the identity". + carriers = _identity_carrier_names(body, session, declared) saw_scoped_call = False for m in _METHOD_CALL_RE.finditer(body): @@ -1407,7 +1520,8 @@ def _has_session_identity_handoff(body: str, params, helper_bodies=None, if not args: continue identity_here = any( - _argument_is_session_identity(a, declared, session) for a in args + _argument_is_session_identity(a, declared, session | carriers) + for a in args ) caller_value_here = any( re.search(r"\$" + re.escape(p) + r"\b", a) for a in args for p in declared @@ -1713,12 +1827,33 @@ def _or_delegating_methods(cleaned: str, guard_text: str) -> set: The caller is responsible for establishing that this file actually imports ``OCA\\OpenRegister\\…\\ObjectService``; without that check a local class named ObjectService would qualify. + + 🔴 `_rbac: false` VETOES THE CLOSURE, NOT ONLY THE SEED — and it did not. + + The seed test has always withdrawn on `_rbac: false`. The CLOSURE below did + not re-apply it, so a method that reached the facade through a helper kept + the clear even though its own call turns OpenRegister's authorisation off. + That was latent until the seed set grew to include a bare accessor. + MEASURED on softwarecatalog `GebruikController::getGebruikenForDeelnemer`: + + GebruikService::getObjectService() -> seeded (obtains the facade) + GebruikService::getGebruiken() -> calls it, so the closure added it + ... searchObjectsPaginated(query: $options, _rbac: false, + _multitenancy: false) + + and the controller above it cleared — on a query whose own arguments say + OpenRegister is NOT authorising this fetch, which is exactly the case the + leaf app must guard for itself. A widening that un-finds THAT is a + regression, so the veto now applies wherever a name enters the set. """ + def _rbac_off(text: str) -> bool: + return bool(_RBAC_DISABLED_RE.search(text)) + seeds: set = set() spans = list(_all_method_spans(cleaned)) for name, body_start, body_end in spans: body = guard_text[body_start:body_end] - if _OR_FACADE_CALL_RE.search(body) and not _RBAC_DISABLED_RE.search(body): + if _OR_FACADE_CALL_RE.search(body) and not _rbac_off(body): seeds.add(name) changed = True while changed: @@ -1726,14 +1861,207 @@ def _or_delegating_methods(cleaned: str, guard_text: str) -> set: for name, body_start, body_end in spans: if name in seeds: continue - if _calls_guard_helper_before_mutation( - guard_text[body_start:body_end], seeds - ): + body = guard_text[body_start:body_end] + if _rbac_off(body): + continue + if _calls_guard_helper_before_mutation(body, seeds): seeds.add(name) changed = True return seeds +# --------------------------------------------------------------------------- +# Pattern 4c — the delegation chain is longer than ONE class, and part of it +# lives in a TRAIT +# --------------------------------------------------------------------------- +# +# `_collaborator_guard_methods` reads the immediate collaborator's own file and +# stops there. That is one hop, and the fleet's real chains are three: +# +# procest TemplateController::activate +# -> TemplateLibraryService::activateTemplate +# -> SettingsService::getObjectService() +# -> OpenRegister\Service\ObjectService +# +# zaakafhandelapp ZakenController::index +# -> ZaakAfhandelApp\Service\ObjectService::getResultArrayForRequest +# -> MapperService::getOpenRegisters() +# -> OpenRegister\Service\ObjectService +# +# The middle class names OpenRegister nowhere, so a one-hop pass sees an +# ordinary app service and reports the controller as unguarded — for a +# delegation ADR-022 tells the app to write. +# +# ⚠️ AND A WIDER TYPE MATCH DOES NOT REACH IT. The obvious alternative — accept +# a collaborator whose declared type IS OpenRegister's ObjectService — fails on +# the shape the fleet actually ships: procest's accessors are declared +# `getObjectService(): ?object` and zaakafhandelapp's `getOpenRegisters(): +# ?\OCA\OpenRegister\Service\ObjectService`, so the type on the property is +# `object` or absent. What generalises is following the CALL, bounded. +# +# TRAITS ARE PART OF THE SAME PROBLEM. procest puts its OpenRegister bridge in +# `use SearchesObjects;` — 89 classes — and a trait's methods are callable as +# `$this->method()` while living in a different file entirely, so every span +# scan in this module is blind to them. +# +# BOUNDED, and the bound is the point: +# * depth 3 — measured against the two chains above, which are the longest in +# the fleet. An unbounded walk would eventually reach some class that names +# OpenRegister and clear everything above it. +# * a cycle stack, so `A -> B -> A` terminates. +# * a resolved file only — an unresolvable type or trait contributes nothing, +# and the routed method keeps its finding. That is the fail-closed +# direction and it is why this widening cannot go green on ignorance. +# * every hop still requires the hop's OWN file to name OpenRegister's +# ObjectService in CODE (comments stripped), and an `_rbac: false` anywhere +# on the seeding call still withdraws the clear. +_OR_DELEGATION_MAX_DEPTH = 3 + +# Per-(file, depth) cache of the transitively OR-delegating method names. +_OR_DELEGATION_CACHE: dict = {} + +# The first type declaration in the file. Everything before it is a namespace +# import; a `use X;` AFTER it is a trait composition. Distinguishing the two by +# position is what keeps `use OCP\IRequest;` out of the trait resolver. +_TYPE_DECL_RE = re.compile(r"\b(?:class|trait|interface|enum)\s+[A-Za-z_]") + +# `use SearchesObjects;` / `use A, B;` inside a class body. Deliberately +# unqualified-only: a trait imported by FQCN also carries a top-of-file `use`, +# which the class index resolves by short name anyway. +_TRAIT_USE_RE = re.compile( + r"\buse\s+([A-Za-z_][A-Za-z0-9_]*(?:\s*,\s*[A-Za-z_][A-Za-z0-9_]*)*)\s*;" +) + + +# ``(root, short, kind) -> [file, ...]``. The transitive pass asks the same +# question many times over one repo; without this the walk re-reads and +# re-cleans every candidate file on every hop. +_TYPE_RESOLVE_CACHE: dict = {} + + +def _resolve_type_files(short: str, path: str, kind: str = "class") -> list: + """Files under the app's ``lib/`` that actually declare `` ``. + + Shared by the collaborator and trait resolvers so both answer "which file + is this?" identically. An unresolvable name yields ``[]``, which is the + fail-closed answer everywhere it is used. + """ + root = _app_root_for(path) + if root is None: + return [] + key = (root, short, kind) + declaring = _TYPE_RESOLVE_CACHE.get(key) + if declaring is None: + decl_re = re.compile(r"\b(?:abstract\s+|final\s+|readonly\s+)*" + + kind + r"\s+" + re.escape(short) + r"\b") + declaring = [] + for candidate in _class_index(root).get(short, []): + try: + with open(candidate, encoding="utf-8") as fh: + csrc = fh.read() + except OSError: + continue + if decl_re.search(_strip_strings_and_comments(csrc)): + declaring.append(candidate) + _TYPE_RESOLVE_CACHE[key] = declaring + # The self-exclusion is per CALLER, so it must not be cached with the + # declaring set: the same class is "self" for one caller and a resolvable + # collaborator for the next. + here = os.path.abspath(path) + return [c for c in declaring if os.path.abspath(c) != here] + + +def _trait_files(cleaned: str, path: str) -> list: + """Files declaring the traits this class composes with ``use ;``.""" + decl = _TYPE_DECL_RE.search(cleaned) + if decl is None: + return [] + out = [] + for m in _TRAIT_USE_RE.finditer(cleaned, decl.end()): + for name in (n.strip() for n in m.group(1).split(",")): + if not name: + continue + out.extend(_resolve_type_files(name, path, kind="trait")) + return out + + +def _or_delegating_methods_deep(class_file: str, depth: int, + stack: frozenset = frozenset()) -> set: + """Methods of *class_file* that reach OpenRegister's facade within *depth* hops. + + Depth 0 is exactly :func:`_or_delegating_methods` — the class's own bodies, + gated on the class's own file naming OpenRegister's ObjectService. Each + further hop adds: the OR-delegating methods of a RESOLVED typed + collaborator, reachable as ``$this->->(``; and those of a + RESOLVED composed trait, reachable as ``$this->(``. The result is + then closed over same-class calls, so an internal helper chain of any + length inside one file is followed as it already was. + """ + absolute = os.path.abspath(class_file) + key = (absolute, depth) + cached = _OR_DELEGATION_CACHE.get(key) + if cached is not None: + return cached + if absolute in stack: + return set() # cycle — contributes nothing, fails closed + try: + with open(class_file, encoding="utf-8") as fh: + src = fh.read() + except OSError: + _OR_DELEGATION_CACHE[key] = set() + return set() + cleaned = _strip_strings_and_comments(src) + gsrc = _guard_source(src, cleaned) + result: set = set() + # This file's own reach, gated on this file naming OpenRegister in CODE. + if _OR_IMPORT_RE.search(_strip_strings_and_comments(src, keep_strings=True)): + result |= _or_delegating_methods(cleaned, gsrc) + if depth > 0: + deeper = stack | {absolute} + # Composed traits: their methods are called as `$this->method(`, so + # they join this class's own name set directly. + for trait_file in _trait_files(cleaned, class_file): + result |= _or_delegating_methods_deep(trait_file, depth - 1, deeper) + # Typed collaborators: their methods are called as + # `$this->->(`, so they need the property name. + child_map: dict = {} + for type_name, prop in _PROPERTY_DECL_RE.findall(cleaned): + short = type_name.rsplit("\\", 1)[-1] + if short.lower() in _COLLABORATOR_SKIP_TYPES: + continue + for candidate in _resolve_type_files(short, class_file): + child = _or_delegating_methods_deep(candidate, depth - 1, deeper) + if child: + child_map.setdefault(prop, set()).update(child) + if child_map: + for name, body_start, body_end in _all_method_spans(cleaned): + if name in result: + continue + body = gsrc[body_start:body_end] + # `_rbac: false` withdraws the clear at EVERY hop, not only at + # the seed — see the note in `_or_delegating_methods`. + if _RBAC_DISABLED_RE.search(body): + continue + if _calls_collaborator_guard_before_mutation(body, child_map): + result.add(name) + if result: + spans = list(_all_method_spans(cleaned)) + changed = True + while changed: + changed = False + for name, body_start, body_end in spans: + if name in result: + continue + body = gsrc[body_start:body_end] + if _RBAC_DISABLED_RE.search(body): + continue + if _calls_guard_helper_before_mutation(body, result): + result.add(name) + changed = True + _OR_DELEGATION_CACHE[key] = result + return result + + def _collaborator_guard_methods(class_file: str) -> set: """Strict guard-bearing method names declared by the class in *class_file*.""" cached = _COLLABORATOR_GUARD_CACHE.get(class_file) @@ -1766,8 +2094,15 @@ def _collaborator_guard_methods(class_file: str) -> set: # container-resolved file was silently missed. Raw `src` would fix that and # introduce a worse bug: a class merely NAMED in a docblock would qualify # the file — a security gate switched off by prose. - if _OR_IMPORT_RE.search(_strip_strings_and_comments(src, keep_strings=True)): - result = result | _or_delegating_methods(cleaned, gsrc) + # + # `_or_delegating_methods_deep` is the same question asked to a bounded + # depth (Pattern 4c): depth 0 is the previous behaviour exactly, and each + # further hop follows a RESOLVED collaborator or composed trait. It carries + # its own `_OR_IMPORT_RE` gate per hop, so the check that used to guard the + # call now lives inside it. + result = result | _or_delegating_methods_deep( + class_file, _OR_DELEGATION_MAX_DEPTH + ) _COLLABORATOR_GUARD_CACHE[class_file] = result return result @@ -1952,9 +2287,54 @@ def _delegated_guard_methods(cleaned: str, src: str, guard_map: dict) -> set: # on pipelinq 2026-08-14, 75 of 160 service files resolve it that way against # 11 that import it. Recognising only the import declared those 75 unguarded, # which is how this gate reported 50 findings against an app that delegates. +# ⚠️ AND THE CONTRACT SPELLING COUNTS TOO — `ObjectServiceInterface`. +# +# `ObjectService\b` does NOT match `ObjectServiceInterface`: `\b` needs a +# non-word character after the `e`, and `I` is a word character. So the moment +# a repo adopted ADR-084 and replaced the container-string lookup with +# +# use OCA\OpenRegister\Contract\ObjectServiceInterface; +# +# the delegation this pattern exists to recognise became invisible, and the +# gate reported the app as unguarded for doing exactly what the ADR told it to +# do. MEASURED 2026-08-16 on real `origin/development` trees: decidesk went +# **5 -> 12 with no controller edit**, and adding `(?:Interface)?` puts it back +# to 5. This is the third `\b`-against-an-identifier-fragment defect this fleet +# has paid for, and all three failed in the same direction — silently, toward +# "no match". See `ObjectServiceInterfaceIsTheSameContract` in +# test_check_no_admin_idor.py, which pins both halves: the interface spelling +# must match, and `ObjectServiceHelper` / a leaf app's own `ObjectService` +# still must not. +# +# ⚠️ AND A THIRD SPELLING EXISTED ALL ALONG — THE TYPE-POSITION FQCN. +# +# The two alternatives this pattern used to carry were `use ;` and a +# QUOTED `''`. Neither matches the file that skips the import and writes +# the fully-qualified name where the type goes: +# +# private readonly \OCA\OpenRegister\Contract\ObjectServiceInterface $svc, +# $this->container->get(\OCA\OpenRegister\Service\ObjectService::class) +# +# Both name OpenRegister's ObjectService as unambiguously as an import does, +# and the pattern's own stated question — "does this file name OpenRegister's +# ObjectService?" — is answered yes by all three. So the three alternatives +# collapse into one: the FQCN itself, wherever it appears in code. +# +# `\\{1,2}` covers the single backslash of source position and the escaped +# double backslash of a single-quoted PHP string; the interior +# `(?:[A-Za-z0-9_]+\\{1,2})*` covers `Service\`, `Contract\` and any future +# sub-namespace. The trailing `\b` is what keeps `ObjectServiceHelper` and +# `ObjectServiceInterfaceFactory` out. +# +# THE ANCHOR IS THE SAFETY, AND IT IS UNCHANGED: every alternative still +# begins `OCA\OpenRegister\`, so a leaf app's own `OCA\Foo\Service\ +# ObjectService` — which is that app's OWN storage and carries no OR +# authorisation — still contributes nothing. And the caller passes +# COMMENT-FREE source, so a class merely named in a docblock cannot qualify a +# file: a security gate must not be switchable off by prose. _OR_IMPORT_RE = re.compile( - r"\buse\s+OCA\\OpenRegister\\[A-Za-z0-9_\\]*ObjectService\b" - r"|(?:'|\")OCA\\{1,2}OpenRegister\\{1,2}Service\\{1,2}ObjectService(?:'|\")" + r"\bOCA\\{1,2}OpenRegister\\{1,2}(?:[A-Za-z0-9_]+\\{1,2})*" + r"ObjectService(?:Interface)?\b" ) # The OR facade only. Note the absence of a ``*Mapper`` alternative: that is @@ -1964,10 +2344,47 @@ def _delegated_guard_methods(cleaned: str, src: str, guard_map: dict) -> set: # call site reads `$this->getObjectService()->findAll(...)`. Matching only a # property would miss every container-resolved app. Safe because the file must # ALSO name OpenRegister's ObjectService (see _OR_IMPORT_RE). +# +# ⚠️ AND IT MISSED THE TWO SHAPES THE FLEET ACTUALLY WRITES MOST. +# +# The three alternatives above all require the facade to be the RECEIVER of +# the very next `->`. That is one way to reach it, and it is not the common +# one. Measured 2026-08-16 on `origin/development`: +# +# zaakafhandelapp lib/Service/ObjectService.php +# $orService = $this->mapperService->getOpenRegisters(); // then used +# procest lib/Service/SettingsService.php +# return $this->container->get('OCA\OpenRegister\Service\ObjectService'); +# procest 89 classes +# $objectService = $this->settingsService->getObjectService(); +# $this->searchObjectsAsArrays($objectService, …) // PASSED ON +# +# In every one of these the code has OBTAINED OpenRegister's facade — which is +# the question this pattern asks — and then assigned it, returned it, or handed +# it to a helper instead of dereferencing it in place. Requiring the trailing +# `->` answered a narrower question than the one the pattern is named for, and +# answered it "no". +# +# So the accessor alternative drops its trailing `->`, and a container +# resolution of OpenRegister's ObjectService is added. Note what is NOT +# relaxed: `->objectService` and `$objectService` still require the arrow, +# because those are NAMES and a name alone is not a call. `getObjectService()` +# and `container->get()` are calls that return the facade, so the +# call itself is the evidence. +# +# The safety is unchanged and it is upstream of this pattern: a clear also +# requires the FILE to name `OCA\OpenRegister\…\ObjectService` in code +# (`_OR_IMPORT_RE`), and an explicit `_rbac: false` still withdraws it. +_OR_CONTAINER_GET_RE = re.compile( + r"->\s*get\s*\(\s*(?:id\s*:\s*)?['\"]?\\{0,2}OCA\\{1,2}OpenRegister\\{1,2}" + r"(?:[A-Za-z0-9_]+\\{1,2})*ObjectService(?:Interface)?\b" +) + _OR_FACADE_CALL_RE = re.compile( r"->\s*objectService\s*->" r"|\$objectService\s*->" - r"|->\s*getObjectService\s*\(\s*\)\s*->" + r"|->\s*get(?:ObjectService|OpenRegisters?)\s*\(\s*\)" + r"|" + _OR_CONTAINER_GET_RE.pattern ) # ``_rbac: false`` (or ``_rbac : FALSE``) anywhere in the call — the app has @@ -2157,12 +2574,46 @@ def _parameter_list(cleaned: str, sig_start: int): return None -def _is_session_scoped_no_reference(params, body: str) -> bool: +def _is_session_scoped_no_reference(params, body: str, + raw_body: str = None) -> bool: """True when the method has no caller-supplied object reference (Pattern 3). See the Pattern 3 commentary above for why all three conditions are required. *params* is the raw parameter-list text (``None`` when it could not be parsed — treated as "not clearable", fail-closed). + + 🔴 CONDITION 3 READS THE RAW BODY, AND UNTIL NOW IT DID NOT — WHICH MADE + THE VERDICT DEPEND ON THE SPELLING OF THE PREAMBLE. + ------------------------------------------------------------------------ + `#365` blanks authentication-only guard clauses so that "is anyone logged + in" can never be mistaken for an authorisation guard. Condition 3 is not a + guard test: it asks whether the method references a caller identity at all, + as evidence that it is scoped to the caller rather than reading globally. + Running it against the blanked text conflated the two questions, and the + consequence was a verdict that turned on where the author put a variable: + + $user = $this->userSession->getUser(); <- survives the blanking + 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, because the inline form put + the body's ONLY session token inside the clause `#365` deletes. Measured on + procest, where three findings were exactly this and nothing else — and the + right response was never to hoist the assignment, which is a semantic no-op + whose only effect is moving a regex past a checker. + + Conditions 1 and 2 still read the blanked body and are unchanged; they are + the ones that carry the "no caller-controlled value exists" argument. + + ⚠️ WHAT THIS DOES NOT FIX, stated rather than left to be discovered: + Pattern 3 has no mutation veto, so a zero-input method that mutates and + carries only an authentication preamble clears. That was already true via + the assignment spelling; this change makes the inline spelling agree with + it rather than introducing it. Pattern 3b — which does veto mutation — is + unaffected. Closing it is a separate decision about Pattern 3's contract. """ if params is None: return False @@ -2170,7 +2621,8 @@ def _is_session_scoped_no_reference(params, body: str) -> bool: return False if _REQUEST_INPUT_RE.search(body): return False - return bool(_SESSION_IDENTITY_RE.search(body)) + return bool(_SESSION_IDENTITY_RE.search( + body if raw_body is None else raw_body)) def _is_zero_input_read_only(params, body: str) -> bool: @@ -3241,7 +3693,12 @@ def scan_file(path: str) -> int: # so IDOR is not structurally possible. See the Pattern 3 commentary # for why all three conditions are required. _params = _parameter_list(cleaned, sig_start) - if _is_session_scoped_no_reference(_params, body): + # ⚠️ `cleaned`, never `src`: comments and string literals are blanked, + # so a docblock naming `$this->userSession->getUser()` cannot satisfy + # condition 3. `#415` — prose is not the guard, and it is not the + # evidence either. Offsets are preserved, so the span is the same one. + if _is_session_scoped_no_reference(_params, body, + cleaned[body_start:body_end]): continue # ---- Pattern 3b: zero-input READ, no session identity needed ---- diff --git a/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py b/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py new file mode 100644 index 00000000..de3e097f --- /dev/null +++ b/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py @@ -0,0 +1,266 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: EUPL-1.2 +"""A CHECKER'S OWN PATTERN MUST MATCH THE SHAPES IT CLAIMS TO RECOGNISE. + +WHY THIS FILE EXISTS +-------------------- +Six defects in this package on 2026-08-16 were the same defect. Every one of +them was a pattern-matching instrument failing toward "no match", and every one +of them was invisible because a regex that matches nothing prints nothing, and +printing nothing is what a clean run looks like: + + 1. `\\b` does not match after `_` — a rename codemod missed every + `snake_case` occurrence and reported success. + 2. `\\b` does not match before the `I` in `ObjectServiceInterface` — gate-7's + `_OR_IMPORT_RE` stopped recognising OpenRegister delegation the moment a + repo adopted ADR-084. **decidesk went 5 -> 12 findings with no controller + edit.** + 3. A quoted-token rename could not see a bare identifier key. + 4. `[a-z-]` dropped the digit in `e2e-coverage`. + 5. gate-48's `MUTATING_METHOD` matched only a QUOTED literal, so + `const method = isNew ? 'POST' : 'PUT'` was invisible — **15 call sites + reported where 27 were unprotected.** + 6. gate-7's `_OR_FACADE_CALL_RE` required the facade to be the receiver of + the next `->`, so `$svc = $this->settingsService->getObjectService()` did + not count as reaching the facade. + +None of them was caught by a unit test, because the unit tests were written +from the same reading of the shape as the regex. What catches this class is a +registry that states, IN THE TEST, the spellings each pattern is responsible +for — including the ones nobody thought of when the pattern was written — and +asserts both directions. + +HOW TO USE IT +------------- +When you widen or narrow a checker's pattern, add the spelling you were +widening FOR to `MUST_MATCH`, and the shape you must not start matching to +`MUST_NOT_MATCH`. The second half is not optional: a pattern with no negative +control is a pattern that can be satisfied by `.*`. + +Run with:: + + python3 scripts/lib/test_checker_patterns_match_documented_shapes.py +""" +from __future__ import annotations + +import os +import sys +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import check_csrf_callers as csrf_callers # noqa: E402 +import check_no_admin_idor as idor # noqa: E402 + + +# --------------------------------------------------------------------------- +# The registry: (module, attribute, must-match, must-not-match) +# --------------------------------------------------------------------------- +# +# `attribute` is looked up BY NAME and the test fails if it is missing — a +# renamed or deleted pattern must break this file loudly rather than quietly +# stop being covered. (`.github` has already shipped a "12 passed / 0 failed" +# suite that compared over a set no longer containing the thing it was about.) +REGISTRY = [ + ( + idor, "_OR_IMPORT_RE", + # Every way a PHP file can name OpenRegister's ObjectService. + [ + "use OCA\\OpenRegister\\Service\\ObjectService;", + # ADR-084's published contract. `ObjectService\\b` cannot match + # this: `\\b` needs a non-word character and `I` is a word + # character. This is defect (2) above. + "use OCA\\OpenRegister\\Contract\\ObjectServiceInterface;", + # The container form — the MAJORITY shape in the fleet. + "$this->container->get('OCA\\OpenRegister\\Service\\ObjectService')", + "$c->get(id: 'OCA\\\\OpenRegister\\\\Service\\\\ObjectService')", + # Type position, no import at all — zaakafhandelapp's ZGW services. + "private \\OCA\\OpenRegister\\Service\\ObjectService $objectService;", + "private readonly \\OCA\\OpenRegister\\Contract\\ObjectServiceInterface $svc,", + "$c->get(\\OCA\\OpenRegister\\Contract\\ObjectServiceInterface::class)", + ], + [ + # A leaf app's OWN ObjectService is that app's storage and carries + # no OpenRegister authorisation. The `OCA\\OpenRegister\\` anchor + # is the whole safety of this pattern. + "use OCA\\ZaakAfhandelApp\\Service\\ObjectService;", + "use OCA\\Procest\\Service\\ObjectService;", + # Longer identifiers that merely START with the name. + "use OCA\\OpenRegister\\Service\\ObjectServiceHelper;", + "use OCA\\OpenRegister\\Contract\\ObjectServiceInterfaceFactory;", + "use OCA\\OpenRegister\\Service\\SearchService;", + ], + ), + ( + idor, "_OR_FACADE_CALL_RE", + [ + "return $this->objectService->find($id);", + "$objectService->searchObjects($query);", + # Accessor, chained. + "$this->getObjectService()->find($id);", + # Accessor, ASSIGNED then used — defect (6) above. + "$objectService = $this->settingsService->getObjectService();", + "$orService = $this->mapperService->getOpenRegisters();", + "return $this->getOpenRegisters();", + # The container resolution IS obtaining the facade. + "return $this->container->get('OCA\\OpenRegister\\Service\\ObjectService');", + ], + [ + # A NAME is not a call. These must keep needing the arrow. + "$this->objectService;", + "if ($objectService === null) {", + # A leaf app's own mapper is its own storage (Pattern 2's line). + "$this->invoiceMapper->findAll();", + # An unrelated getter. + "$this->getObjectStore()->read($id);", + ], + ), + ( + idor, "_IDENTITY_INTO_ARRAY_RE", + [ + "$options['participants'] = [$orgUuid];", + "$filters['owner'] = $userId;", + "$q['scope']['org'] = $orgUuid;", + ], + [ + # A whole-variable assignment is the OTHER rule's business. + "$options = $this->request->getParams();", + # A comparison is not an assignment. + "if ($options['owner'] == $userId) {", + ], + ), + ( + csrf_callers, "MUTATING_METHOD", + [ + "method: 'POST'", + 'method: "put"', + "method: `PATCH`", + "method: 'DELETE',", + ], + [ + "method: 'GET'", + # ⚠️ THESE ARE THE DEFECT (5) SHAPES, AND THEY BELONG HERE RATHER + # THAN IN MUST_MATCH: this regex is a LITERAL matcher and is not + # being asked to resolve a computed verb. What must handle them is + # `_fetch_is_mutating`, pinned by `ComputedVerbsAreMutating` below. + # Saying so in the registry is the point — a shape has to be + # SOMEBODY's responsibility, named. + "method,", + "method: isNew ? 'POST' : 'PUT'", + ], + ), +] + + +class PatternMatchesDocumentedShapes(unittest.TestCase): + """Every registered pattern matches what it claims and nothing it disclaims.""" + + def test_every_registered_attribute_still_exists(self): + """A renamed pattern must FAIL here, not silently stop being covered.""" + for module, name, _match, _no_match in REGISTRY: + with self.subTest(pattern=f"{module.__name__}.{name}"): + self.assertTrue( + hasattr(module, name), + f"{module.__name__}.{name} no longer exists — this " + f"registry is now covering nothing under that name.", + ) + + def test_documented_shapes_match(self): + for module, name, must_match, _no_match in REGISTRY: + pattern = getattr(module, name) + self.assertTrue(must_match, f"{name} has no documented shapes") + for shape in must_match: + with self.subTest(pattern=name, shape=shape): + self.assertIsNotNone( + pattern.search(shape), + f"{module.__name__}.{name} does NOT match a spelling " + f"it is responsible for: {shape!r}. A pattern that " + f"fails to match prints nothing, and nothing reads as " + f"clean.", + ) + + def test_disclaimed_shapes_do_not_match(self): + for module, name, _match, must_not_match in REGISTRY: + pattern = getattr(module, name) + self.assertTrue(must_not_match, + f"{name} has no negative control — a pattern with " + f"no negative control can be satisfied by '.*'") + for shape in must_not_match: + with self.subTest(pattern=name, shape=shape): + self.assertIsNone( + pattern.search(shape), + f"{module.__name__}.{name} matches a shape it " + f"disclaims: {shape!r}", + ) + + +class WordBoundaryAgainstIdentifierFragments(unittest.TestCase): + """The specific trap, stated as a property rather than as four examples. + + `\\b` between two word characters never matches. So any pattern that ends + an IDENTIFIER FRAGMENT with `\\b` is asserting that no longer identifier + starts with that fragment — which is a claim about a codebase, not about a + regex, and it has been wrong three times in this fleet. + """ + + def test_b_does_not_separate_ObjectService_from_Interface(self): + import re + narrow = re.compile(r"ObjectService\b") + self.assertIsNone( + narrow.search("ObjectServiceInterface"), + "If this ever passes, Python's \\b has changed and the whole " + "premise of this file is different.", + ) + self.assertIsNotNone(narrow.search("ObjectService;")) + + def test_the_shipped_pattern_does_separate_them(self): + self.assertIsNotNone( + idor._OR_IMPORT_RE.search( + "use OCA\\OpenRegister\\Contract\\ObjectServiceInterface;")) + self.assertIsNone( + idor._OR_IMPORT_RE.search( + "use OCA\\OpenRegister\\Contract\\ObjectServiceInterfaceFactory;")) + + +class ComputedVerbsAreMutating(unittest.TestCase): + """gate-48: a `method` value that cannot be PROVEN safe counts as mutating. + + The registry above records that `MUTATING_METHOD` is a literal matcher. + This is where the shapes it cannot see are made somebody's responsibility. + """ + + SAFE = "const method = 'GET'\nfetch('/x', { method })" + TERNARY_BINDING = "const method = isNew ? 'POST' : 'PUT'\nfetch('/x', { method })" + TERNARY_INLINE = "fetch('/x', { method: isNew ? 'POST' : 'PUT' })" + UNRESOLVABLE = "fetch('/x', { method: m })" + NO_METHOD_KEY = "fetch('/x')" + SAFE_LITERAL = "fetch('/x', { method: 'GET' })" + + def _verdict(self, text: str): + at = text.index("fetch(") + call = csrf_callers._call_text(text, text.index("(", at)) + return csrf_callers._fetch_is_mutating(call, text, at)[0] + + def test_a_ternary_binding_is_mutating(self): + self.assertIs(self._verdict(self.TERNARY_BINDING), True) + + def test_an_inline_ternary_is_mutating(self): + self.assertIs(self._verdict(self.TERNARY_INLINE), True) + + def test_an_unresolvable_value_is_mutating(self): + """Fail closed: an unreadable verb is not a pass.""" + self.assertIs(self._verdict(self.UNRESOLVABLE), True) + + def test_a_proven_safe_binding_is_not_mutating(self): + """The negative control — without it, every fetch() would report.""" + self.assertIs(self._verdict(self.SAFE), False) + + def test_a_literal_safe_verb_is_not_mutating(self): + self.assertIs(self._verdict(self.SAFE_LITERAL), False) + + def test_no_method_key_is_a_GET(self): + self.assertIsNone(self._verdict(self.NO_METHOD_KEY)) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/hydra-gates/scripts/run-hydra-gates.sh b/hydra-gates/scripts/run-hydra-gates.sh index 8b9ae42f..0dfea6de 100755 --- a/hydra-gates/scripts/run-hydra-gates.sh +++ b/hydra-gates/scripts/run-hydra-gates.sh @@ -7687,8 +7687,23 @@ elif [ "${HAVE_DELTA_BASE}" = "1" ]; then else set +e _csrf_err="${HYDRA_GATE_LOG_DIR}/hydra-gate-csrf-cochange.err" + # `--repo`/`--base` turn on the POST-IMAGE test: a removed annotation + # whose method no longer exists at HEAD is a DELETED endpoint, not a + # dropped protection, and a `-U0` diff cannot tell the two apart on its + # own. Measured on zaakafhandelapp#371 (five `DashboardController` + # methods deleted): 5 removals -> 0. The negative control is #380, which + # STRIPPED the annotation from ten surviving methods: 10 -> 10. + # Without these arguments the helper behaves exactly as before, so a + # repo it cannot read is never a reason to drop a security finding. + # ⚠️ THE DIFF IS THREE-DOT, SO ITS BASE SIDE IS THE MERGE BASE — not + # BASE_REF. Passing BASE_REF would address line numbers in the wrong + # image; the helper verifies the line content it was given and reports + # on a mismatch, so the error would be silent over-reporting rather + # than a false pass, but the right image is cheap to name. + _csrf_mb=$(git merge-base "${BASE_REF}" HEAD 2>/dev/null || true) + [ -n "${_csrf_mb}" ] || _csrf_mb="${BASE_REF}" _csrf_removed=$(git diff -U0 "${BASE_REF}...HEAD" -- 'lib/Controller/*.php' 2>/dev/null \ - | python3 "${_csrf_helper}" 2>"${_csrf_err}") + | python3 "${_csrf_helper}" --repo . --base "${_csrf_mb}" 2>"${_csrf_err}") _csrf_rc=$? if [ "${_csrf_rc}" -ne 0 ]; then _csrf_ran=0 From 1305352ccae10e886281ec13160b4af8194fd957 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Sun, 16 Aug 2026 21:23:03 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(gate-7):=20the=20accessor's=20NAME=20is?= =?UTF-8?q?=20not=20evidence=20=E2=80=94=20read=20its=20body?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../scripts/lib/check_no_admin_idor.py | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/hydra-gates/scripts/lib/check_no_admin_idor.py b/hydra-gates/scripts/lib/check_no_admin_idor.py index 2a851f40..3e8244ef 100644 --- a/hydra-gates/scripts/lib/check_no_admin_idor.py +++ b/hydra-gates/scripts/lib/check_no_admin_idor.py @@ -2365,12 +2365,28 @@ def _delegated_guard_methods(cleaned: str, src: str, guard_map: dict) -> set: # `->` answered a narrower question than the one the pattern is named for, and # answered it "no". # -# So the accessor alternative drops its trailing `->`, and a container -# resolution of OpenRegister's ObjectService is added. Note what is NOT -# relaxed: `->objectService` and `$objectService` still require the arrow, -# because those are NAMES and a name alone is not a call. `getObjectService()` -# and `container->get()` are calls that return the facade, so the -# call itself is the evidence. +# So a CONTAINER RESOLUTION of OpenRegister's ObjectService is added — that is +# the app naming the class it is obtaining, in code — and the reach of an +# ACCESSOR is left to the transitive pass to establish from the accessor's own +# BODY rather than from its name. +# +# 🔴 AND THE NAME MUST NOT BE ENOUGH, WHICH THIS CHANGE LEARNED THE HARD WAY. +# A first version added `->getObjectService()` (no trailing `->`) as an +# alternative here, and the package's own acceptance matrix caught it: +# `comment-silenced-guard/planted` — `.github#373`'s planted defect — is a +# SERVICE LOCATOR called `getObjectService()` whose body is +# +# if (!class_exists('\OCA\OpenRegister\Service\ObjectService')) { throw … } +# return $this->themes; // a LOCAL service +# +# It names OpenRegister and returns something else entirely. Matching the +# accessor by name cleared `ThemeController::show`, and the fixture went from +# naming its planted method to naming a different one. The rule that survives +# is the one the rest of this module already applies to collaborators: READ THE +# CALLEE, never infer it from what it is called. +# +# What is NOT relaxed either: `->objectService` and `$objectService` still +# require the arrow, because those are NAMES and a name alone is not a call. # # The safety is unchanged and it is upstream of this pattern: a clear also # requires the FILE to name `OCA\OpenRegister\…\ObjectService` in code @@ -2383,7 +2399,7 @@ def _delegated_guard_methods(cleaned: str, src: str, guard_map: dict) -> set: _OR_FACADE_CALL_RE = re.compile( r"->\s*objectService\s*->" r"|\$objectService\s*->" - r"|->\s*get(?:ObjectService|OpenRegisters?)\s*\(\s*\)" + r"|->\s*getObjectService\s*\(\s*\)\s*->" r"|" + _OR_CONTAINER_GET_RE.pattern ) From 287d20673e0738ec592d91870191296ca634215b Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Sun, 16 Aug 2026 21:24:26 +0200 Subject: [PATCH 3/3] =?UTF-8?q?test(gates):=20the=20registry=20caught=20my?= =?UTF-8?q?=20own=20inconsistency=20=E2=80=94=20record=20which=20layer=20o?= =?UTF-8?q?wns=20an=20accessor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...hecker_patterns_match_documented_shapes.py | 85 +++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py b/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py index de3e097f..5a3e421b 100644 --- a/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py +++ b/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py @@ -98,12 +98,10 @@ "$objectService->searchObjects($query);", # Accessor, chained. "$this->getObjectService()->find($id);", - # Accessor, ASSIGNED then used — defect (6) above. - "$objectService = $this->settingsService->getObjectService();", - "$orService = $this->mapperService->getOpenRegisters();", - "return $this->getOpenRegisters();", - # The container resolution IS obtaining the facade. + # The container resolution IS obtaining the facade — the app names + # the class it is getting, in code. Defect (6) above. "return $this->container->get('OCA\\OpenRegister\\Service\\ObjectService');", + "$svc = $c->get(id: 'OCA\\\\OpenRegister\\\\Service\\\\ObjectService');", ], [ # A NAME is not a call. These must keep needing the arrow. @@ -113,6 +111,18 @@ "$this->invoiceMapper->findAll();", # An unrelated getter. "$this->getObjectStore()->read($id);", + # ⚠️ AN ACCESSOR CALLED BY NAME IS DELIBERATELY NOT THIS PATTERN'S + # BUSINESS, and putting it here is the point. `.github#373`'s + # planted fixture is a `getObjectService()` whose body is + # `class_exists('\OCA\OpenRegister\…\ObjectService')` and which + # returns a LOCAL service — it names OpenRegister and delivers + # something else. Matching the name cleared the fixture's planted + # method. These shapes are `_or_delegating_methods_deep`'s + # responsibility, which READS the accessor's body; pinned by + # `AccessorReachIsEstablishedFromTheBody` below. + "$objectService = $this->settingsService->getObjectService();", + "$orService = $this->mapperService->getOpenRegisters();", + "return $this->getOpenRegisters();", ], ), ( @@ -222,6 +232,71 @@ def test_the_shipped_pattern_does_separate_them(self): "use OCA\\OpenRegister\\Contract\\ObjectServiceInterfaceFactory;")) +class AccessorReachIsEstablishedFromTheBody(unittest.TestCase): + """An accessor's NAME buys nothing; its BODY is the evidence. + + Two classes, identical accessor names, opposite verdicts. This is the arm + that would have caught the first version of the facade widening — it matched + `->getObjectService()` by name and cleared `.github#373`'s planted service + locator, which is called exactly that and returns a local service. + + ⚠️ Both use the ASSIGNMENT spelling (`$svc = $this->getObjectService();`), + which is the fixture's shape and the fleet's. The CHAINED spelling + `->getObjectService()->` is a pre-existing alternative in + `_OR_FACADE_CALL_RE` that has always matched by name; it is untouched here + and is deliberately not what this arm is about. + """ + + REAL = """container->get('OCA\\OpenRegister\\Service\\ObjectService'); + } + public function search(string $id): array { + $objects = $this->getObjectService(); + return $objects->find($id); + } +} +""" + LOCATOR = """themes; + } + public function search(string $id): array { + $objects = $this->getObjectService(); + return $objects->find($id); + } +} +""" + + def _or_methods(self, source: str) -> set: + import tempfile + # Written under a `lib/` segment so `_app_root_for` resolves. + root = tempfile.mkdtemp() + libdir = os.path.join(root, 'lib', 'Service') + os.makedirs(libdir) + path = os.path.join(libdir, 'X.php') + with open(path, 'w', encoding='utf-8') as handle: + handle.write(source) + idor._OR_DELEGATION_CACHE.clear() + idor._CLASS_INDEX_CACHE.clear() + idor._TYPE_RESOLVE_CACHE.clear() + return idor._or_delegating_methods_deep(path, idor._OR_DELEGATION_MAX_DEPTH) + + def test_an_accessor_that_resolves_the_facade_seeds_its_callers(self): + self.assertIn('search', self._or_methods(self.REAL)) + + def test_a_locator_that_merely_NAMES_openregister_does_not(self): + """The negative control, and the one the acceptance matrix pinned.""" + self.assertNotIn('search', self._or_methods(self.LOCATOR)) + + class ComputedVerbsAreMutating(unittest.TestCase): """gate-48: a `method` value that cannot be PROVEN safe counts as mutating.