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..3e8244ef 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,63 @@ 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 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 +# (`_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"|" + _OR_CONTAINER_GET_RE.pattern ) # ``_rbac: false`` (or ``_rbac : FALSE``) anywhere in the call — the app has @@ -2157,12 +2590,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 +2637,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 +3709,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..5a3e421b --- /dev/null +++ b/hydra-gates/scripts/lib/test_checker_patterns_match_documented_shapes.py @@ -0,0 +1,341 @@ +#!/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);", + # 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. + "$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);", + # ⚠️ 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();", + ], + ), + ( + 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 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. + + 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