diff --git a/hydra-gates/scripts/lib/check_no_admin_idor.py b/hydra-gates/scripts/lib/check_no_admin_idor.py index cb20156..d8a750b 100644 --- a/hydra-gates/scripts/lib/check_no_admin_idor.py +++ b/hydra-gates/scripts/lib/check_no_admin_idor.py @@ -42,6 +42,44 @@ an explicit controller-level guard (or a Pattern-1 helper), so a real IDOR in a leaf app is never masked. See "Pattern 2 boundary" below. + Pattern 4 — delegated guard reached through a chain and/or a collaborator. + Patterns 1–3 all require the guard to be *one* call-hop away and on + ``$this``. Real controllers routinely centralise authorisation in an + injected responder/guard collaborator, and route several thin public + actions through private helpers before reaching it. Measured on decidesk + 2026-08-04, that shape produced 11 findings and **every one was guarded**: + 8 reached ``$this->responder->staffAction()`` → ``requireStaff()`` (which + checks ``currentUid() !== null`` AND ``isStaff()``), 3 reached + ``citizenAction()`` (401 for an anonymous caller), and ``validateProposal`` + took three hops (``validateProposal`` → ``approveProposal`` / + ``rejectProposal`` → ``applyProposalDecision`` → ``staffAction``). + + Pattern 4 therefore does two things, both evidence-based: + + a. *Cross-class resolution.* The controller's typed constructor-promoted + properties and property declarations give ``$prop -> ClassName``. The + class is resolved to a real file under the app's ``lib/`` tree (PSR-4 + basename, confirmed by an actual ``class `` declaration) and + **that file is parsed**. A call ``$this->prop->method(`` clears the + routed method only when ``method`` is demonstrably guard-bearing in + the collaborator's own source. Nothing is assumed from the name of + the property or of the class — an unresolvable class clears nothing. + + b. *Transitive closure.* A same-class method that reaches a guard (by + (a) or by a strict in-body signal) before its first data mutation is + itself guard-bearing, and the closure is iterated to a fixpoint, so an + arbitrarily long intra-class delegation chain is followed. + + Propagation deliberately uses a **stricter** signal than the one-hop + Pattern 1 (``_STRICT_GUARD_BODY_RE``): a bare ``throw`` or a 404 does not + seed a chain, because a method that throws ``NotFoundException`` is not an + authorisation guard and chaining would let that leak arbitrarily far. Only + an explicit deny (401/403, ``OCSForbiddenException``, ``isAdmin``, an + ``authorize*``/``require*``/``ensure*``/``assert*``/``guard*`` call, or an + anonymous-session rejection) starts or continues a chain. Every hop must + also occur before the caller's first data mutation, so a guard that runs + only after the write still fails the gate. + Exemptions (method skipped entirely): 1. ``__construct`` — not a routed action, the 20-line look-back window can accidentally catch it when a constructor follows an annotated method. @@ -76,6 +114,7 @@ """ from __future__ import annotations +import os import re import sys @@ -384,6 +423,235 @@ def _find_method_bodies(src: str): r"[A-Za-z0-9_]*\s*\(", ) +# --------------------------------------------------------------------------- +# Pattern 4 — delegation chains and collaborator-hosted guards +# --------------------------------------------------------------------------- + +# The signal that may START or CONTINUE a delegation chain. +# +# Deliberately STRICTER than _HELPER_GUARD_BODY_RE, which is kept as-is for the +# one-hop Pattern-1 clear. Two alternatives are dropped here on purpose: +# +# - a bare `throw`. A method that throws NotFoundException is not an +# authorisation guard; accepting it at one hop is already generous, and +# propagating it transitively would let "this function can fail" stand in +# for "this function checks who you are" arbitrarily far up the call chain. +# - `404` / STATUS_NOT_FOUND. Same reason: not-found is not access-denied. +# +# What remains is an explicit deny decision: a 401/403, a forbidden exception, +# an admin-membership test, a call to an authorize*/require*/ensure*/assert*/ +# guard* predicate, or the rejection of an anonymous session. +_STRICT_GUARD_BODY_RE = re.compile( + r"OCSForbiddenException" + r"|NotPermittedException" + r"|ForbiddenException" + r"|isAdmin\s*\(" + r"|isCurrentUserAdmin\s*\(" + r"|->\s*(?:authorize|authorise|require|ensure|assert|guard)[A-Z][A-Za-z0-9_]*\s*\(" + r"|Http::STATUS_(?:UNAUTHORIZED|FORBIDDEN)" + r"|(?:statusCode:\s*|,\s*)(?:401|403)\b" + r"|(?:getUser|getUID|currentUid|getCurrentUserId)\s*\(\s*\)\s*===\s*null" +) + +# Typed property declarations and constructor-promoted properties: +# private readonly ParticipationResponder $responder, +# protected ?FooGuard $guard; +# Captures (ClassName, propertyName). Scalar/builtin types are filtered out by +# _COLLABORATOR_SKIP_TYPES so `private string $key` never becomes a lookup. +_PROPERTY_DECL_RE = re.compile( + r"\b(?:private|protected|public)\s+(?:readonly\s+)?\??" + r"([A-Za-z_][A-Za-z0-9_]*(?:\\[A-Za-z_][A-Za-z0-9_]*)*)\s+" + r"\$([A-Za-z_][A-Za-z0-9_]*)" +) + +_COLLABORATOR_SKIP_TYPES = frozenset( + { + "array", "bool", "boolean", "callable", "float", "int", "integer", + "iterable", "mixed", "object", "string", "self", "static", "null", + "readonly", "false", "true", "void", + } +) + +_CLASS_DECL_TEMPLATE = r"\b(?:abstract\s+|final\s+|readonly\s+)*class\s+%s\b" + +# Per-repo index of `ClassName -> [path, ...]` under the app's lib/ tree, built +# lazily and cached. Bounded to lib/ so a scan never walks node_modules/vendor. +_CLASS_INDEX_CACHE: dict = {} +# Per-file cache of the strict guard-bearing method set of a collaborator class. +_COLLABORATOR_GUARD_CACHE: dict = {} + + +def _app_root_for(path: str): + """Return the app root for *path* — the parent of its ``lib/`` directory. + + Gate-7 is only ever handed ``lib/Controller/*.php``, so the root is the + directory containing the ``lib`` segment. Returns ``None`` when there is + no such segment (the caller then resolves no collaborators at all, which + fails closed: unresolved means unguarded means flagged). + """ + parts = os.path.abspath(path).split(os.sep) + for i in range(len(parts) - 1, -1, -1): + if parts[i] == "lib": + return os.sep.join(parts[:i]) or os.sep + return None + + +def _class_index(root: str) -> dict: + """Map ``ClassName -> [file, ...]`` for every PHP file under ``root/lib``. + + PSR-4 basename indexing: the fleet's apps all name the file after the + class. Candidates are *verified* by the caller against an actual ``class + `` declaration, so a basename collision cannot silently resolve to + the wrong file. + """ + cached = _CLASS_INDEX_CACHE.get(root) + if cached is not None: + return cached + index: dict = {} + lib_dir = os.path.join(root, "lib") + for dirpath, dirnames, filenames in os.walk(lib_dir): + dirnames[:] = [ + d for d in dirnames if d not in ("vendor", "node_modules", ".git") + ] + for fn in filenames: + if fn.endswith(".php"): + index.setdefault(fn[:-4], []).append(os.path.join(dirpath, fn)) + _CLASS_INDEX_CACHE[root] = index + return index + + +def _strict_guard_methods(cleaned: str, src: str) -> set: + """Names of methods in one class that reach a strict guard, transitively. + + Seeded with methods whose name is an authorisation predicate + (``_GUARD_HELPER_NAME_RE``) or whose body carries a strict deny signal + (``_STRICT_GUARD_BODY_RE``), then closed over same-class calls: a method + that invokes a known guard-bearing method *before its first data mutation* + becomes guard-bearing itself. Iterated to a fixpoint, so a chain of any + length is followed. + """ + spans = list(_all_method_spans(cleaned)) + known: set = set() + for name, body_start, body_end in spans: + if _GUARD_HELPER_NAME_RE.match(name): + known.add(name) + elif _STRICT_GUARD_BODY_RE.search(src[body_start:body_end]): + known.add(name) + changed = True + while changed: + changed = False + for name, body_start, body_end in spans: + if name in known: + continue + if _calls_guard_helper_before_mutation(src[body_start:body_end], known): + known.add(name) + changed = True + return known + + +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) + if cached is not None: + return cached + try: + with open(class_file, encoding="utf-8") as fh: + src = fh.read() + except OSError: + _COLLABORATOR_GUARD_CACHE[class_file] = set() + return set() + result = _strict_guard_methods(_strip_strings_and_comments(src), src) + _COLLABORATOR_GUARD_CACHE[class_file] = result + return result + + +def _collaborator_guard_map(cleaned: str, path: str) -> dict: + """Map ``propertyName -> {guard method names}`` for this class's collaborators. + + Reads the typed constructor-promoted properties and property declarations, + resolves each type to a real file under the app's ``lib/`` tree, confirms + the file actually declares that class, and parses it for guard-bearing + methods. A type that cannot be resolved contributes nothing — the routed + method then stays flagged, which is the fail-closed direction. + """ + root = _app_root_for(path) + if root is None: + return {} + index = _class_index(root) + out: 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 + guards: set = set() + decl_re = re.compile(_CLASS_DECL_TEMPLATE % re.escape(short)) + for candidate in index.get(short, []): + if os.path.abspath(candidate) == os.path.abspath(path): + continue + try: + with open(candidate, encoding="utf-8") as fh: + csrc = fh.read() + except OSError: + continue + if not decl_re.search(_strip_strings_and_comments(csrc)): + continue + guards |= _collaborator_guard_methods(candidate) + if guards: + out.setdefault(prop, set()).update(guards) + return out + + +def _calls_collaborator_guard_before_mutation(body: str, guard_map: dict) -> bool: + """True when *body* calls ``$this->->(`` before its first write. + + *guard_map* comes from :func:`_collaborator_guard_map`, so every method + named here was read out of the collaborator's own source — this is a + resolved delegation, not a naming convention. + """ + if not guard_map: + return False + mutation = _MUTATION_RE.search(body) + mutation_pos = mutation.start() if mutation is not None else None + for prop, methods in guard_map.items(): + for method in methods: + call_re = re.compile( + r"\$this\s*->\s*" + re.escape(prop) + r"\s*->\s*" + + re.escape(method) + r"\s*\(" + ) + for m in call_re.finditer(body): + if mutation_pos is None or m.start() < mutation_pos: + return True + return False + + +def _delegated_guard_methods(cleaned: str, src: str, guard_map: dict) -> set: + """Same-class methods that reach a guard through *any* resolved route. + + Seed = strict in-body guards (``_strict_guard_methods``) plus methods that + delegate straight to a resolved collaborator guard; then closed over + same-class calls to a fixpoint. This is what clears decidesk's + ``validateProposal`` → ``approveProposal`` → ``applyProposalDecision`` → + ``$this->responder->staffAction()`` three-hop chain. + """ + spans = list(_all_method_spans(cleaned)) + known = _strict_guard_methods(cleaned, src) + for name, body_start, body_end in spans: + if name in known: + continue + if _calls_collaborator_guard_before_mutation(src[body_start:body_end], guard_map): + known.add(name) + changed = True + while changed: + changed = False + for name, body_start, body_end in spans: + if name in known: + continue + if _calls_guard_helper_before_mutation(src[body_start:body_end], known): + known.add(name) + changed = True + return known + + # --------------------------------------------------------------------------- # Pattern 2 — OpenRegister data-layer RBAC delegation (ADR-022) # --------------------------------------------------------------------------- @@ -599,6 +867,12 @@ def scan_file(path: str) -> int: cleaned = _strip_strings_and_comments(src) is_or_repo = bool(_OR_NAMESPACE_RE.search(cleaned)) guard_helpers = _collect_guard_helpers(cleaned, src, is_or_repo) + # Pattern 4 context: resolve this class's typed collaborators to real files + # and read their guard-bearing methods out of their own source, then close + # the same-class delegation graph over that. Both are lazy/cached; a file + # with no @NoAdminRequired method never pays for them. + collaborator_guards = _collaborator_guard_map(cleaned, path) + delegated_guards = _delegated_guard_methods(cleaned, src, collaborator_guards) violations = 0 for name, head_start, sig_start, body_start, body_end, line_no in _find_method_bodies(src): @@ -657,6 +931,17 @@ def scan_file(path: str) -> int: if _calls_guard_helper_before_mutation(body, guard_helpers): continue + # ---- Pattern 4: resolved delegation chain / collaborator guard --- + # Either the routed method hands straight to a collaborator method + # that was READ and found guard-bearing in its own file + # ($this->responder->staffAction()), or it reaches one through a chain + # of same-class helpers. Every hop is required to occur before the + # first data mutation, and an unresolvable collaborator clears nothing. + if _calls_collaborator_guard_before_mutation(body, collaborator_guards): + continue + if _calls_guard_helper_before_mutation(body, delegated_guards): + continue + # ---- Pattern 2: OpenRegister data-layer RBAC delegation --------- # Inside the OpenRegister app, data access through ObjectService or a # *Mapper delegates per-object authz to OR's register RBAC + diff --git a/hydra-gates/scripts/lib/test_check_no_admin_idor.py b/hydra-gates/scripts/lib/test_check_no_admin_idor.py index cd09732..896fdc3 100644 --- a/hydra-gates/scripts/lib/test_check_no_admin_idor.py +++ b/hydra-gates/scripts/lib/test_check_no_admin_idor.py @@ -1107,5 +1107,376 @@ class C { self.assertIn("method=show", out[0]) +# --------------------------------------------------------------------------- +# Pattern 4 — delegation chains and collaborator-hosted guards +# --------------------------------------------------------------------------- + +def _scan_app(controller_src: str, collaborators: dict) -> list[str]: + """Scan a controller inside a throwaway app tree with real collaborators. + + Pattern 4 resolves a typed property to a *file* under the app's ``lib/`` + tree and reads that file, so these tests must lay out a real directory: + + /lib/Controller/TestController.php + /lib/Service/.php + + *collaborators* maps ``ClassName -> php source``. + """ + with tempfile.TemporaryDirectory() as root: + ctl_dir = Path(root) / "lib" / "Controller" + svc_dir = Path(root) / "lib" / "Service" + ctl_dir.mkdir(parents=True) + svc_dir.mkdir(parents=True) + for name, body in collaborators.items(): + (svc_dir / f"{name}.php").write_text(body, encoding="utf-8") + ctl = ctl_dir / "TestController.php" + ctl.write_text(controller_src, encoding="utf-8") + # Pattern 4 caches per-root and per-file; a temp dir is unique per test + # but clear anyway so a reused inode can never leak a stale answer. + cni._CLASS_INDEX_CACHE.clear() + cni._COLLABORATOR_GUARD_CACHE.clear() + buf = io.StringIO() + with redirect_stdout(buf): + cni.scan_file(str(ctl)) + cni._CLASS_INDEX_CACHE.clear() + cni._COLLABORATOR_GUARD_CACHE.clear() + return [ln for ln in buf.getvalue().splitlines() if ln.strip()] + + +# The decidesk responder, reduced to the shape that matters: staffAction() +# delegates to requireStaff() which denies with 401/403; citizenAction() +# denies an anonymous caller with 401; respond() is NOT a guard — it only maps +# a result or an exception onto a JSONResponse. +_RESPONDER = """\ +requireStaff() ?? $this->respond($operation, $key, $status)); + } + + public function citizenAction(callable $operation, ?string $key = null, int $status = 200) { + $uid = $this->staffGuard->currentUid(); + if ($uid === null) { + return new JSONResponse(['message' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED); + } + return $this->respond($operation, $key, $status); + } + + private function requireStaff() { + if ($this->staffGuard->currentUid() === null) { + return new JSONResponse(['message' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED); + } + if ($this->staffGuard->isStaff() === false) { + return new JSONResponse(['message' => 'Forbidden'], Http::STATUS_FORBIDDEN); + } + return null; + } + + private function respond(callable $operation, ?string $key, int $status) { + return new JSONResponse([$key => $operation()], $status); + } +} +""" + + +class CollaboratorGuardDelegationTest(unittest.TestCase): + """Pattern 4a — a guard reached through an injected collaborator. + + Regression cover for the decidesk measurement of 2026-08-04: gate-7 + reported 11 findings on ParticipationController / + ParticipationBudgetController and every one was guarded, because the + guard lives on ``$this->responder`` rather than in the method body. + """ + + def test_staffAction_delegation_is_recognised_as_guarded(self): + """$this->responder->staffAction() reaches requireStaff() -> not flagged.""" + src = """\ +responder->staffAction( + operation: fn (): array => $this->lifecycleService->transitionBudgetRound($budgetId, $status), + key: 'budgetRound' + ); + } +} +""" + self.assertEqual(_scan_app(src, {"ParticipationResponder": _RESPONDER}), []) + + def test_citizenAction_delegation_is_recognised_as_guarded(self): + """citizenAction() denies an anonymous caller with 401 -> not flagged.""" + src = """\ +responder->citizenAction( + operation: fn (string $uid): array => $this->budgetService->submitProposal($budgetId, $title, $uid), + key: 'proposal' + ); + } +} +""" + self.assertEqual(_scan_app(src, {"ParticipationResponder": _RESPONDER}), []) + + def test_three_hop_intra_class_chain_to_collaborator_guard(self): + """validateProposal -> approve/reject -> applyDecision -> staffAction(). + + The exact decidesk shape that needed three hops. Transitive closure + must follow it all the way to the collaborator guard. + """ + src = """\ +rejectProposal(proposalId: $proposalId); + } + return $this->approveProposal(proposalId: $proposalId); + } + + private function approveProposal(string $proposalId) { + return $this->applyProposalDecision(proposalId: $proposalId, approve: true); + } + + private function rejectProposal(string $proposalId) { + return $this->applyProposalDecision(proposalId: $proposalId, approve: false); + } + + private function applyProposalDecision(string $proposalId, bool $approve) { + return $this->responder->staffAction( + operation: fn (): array => $this->budgetService->validateProposal($proposalId, $approve), + key: 'proposal' + ); + } +} +""" + self.assertEqual(_scan_app(src, {"ParticipationResponder": _RESPONDER}), []) + + +class CollaboratorGuardStillCatchesRealIdorTest(unittest.TestCase): + """Pattern 4 must not become a blanket clear — the negative direction. + + Every test here is a shape the gate MUST still flag. Without these, the + Pattern 4 clear is only evidence about itself: a delegation-following + gate that follows delegation to *anything* has stopped gating. + """ + + def test_plain_unguarded_method_still_flagged(self): + """No responder, no helper, no guard, caller-supplied id -> flagged.""" + src = """\ +consultationService->deleteReaction($reactionId); + return new JSONResponse(['ok' => true]); + } +} +""" + out = _scan_app(src, {"ParticipationResponder": _RESPONDER}) + self.assertEqual(len(out), 1) + self.assertIn("method=deleteReaction", out[0]) + + def test_collaborator_method_that_is_not_a_guard_still_flagged(self): + """respond() EXISTS on the responder but performs no authorisation. + + The sharpest control: resolution must discriminate between methods of + the collaborator, not clear anything called on a property whose class + happens to contain a guard somewhere. + """ + src = """\ +responder->respond( + fn (): array => $this->consultationService->deleteReaction($reactionId), + 'reaction', + 200 + ); + } +} +""" + out = _scan_app(src, {"ParticipationResponder": _RESPONDER}) + self.assertEqual(len(out), 1) + self.assertIn("method=deleteReaction", out[0]) + + def test_unresolvable_collaborator_class_clears_nothing(self): + """A type with no file under lib/ must fail closed, not fail open.""" + src = """\ +responder->staffAction( + fn (): array => $this->consultationService->deleteReaction($reactionId) + ); + } +} +""" + out = _scan_app(src, {}) + self.assertEqual(len(out), 1) + self.assertIn("method=deleteReaction", out[0]) + + def test_intra_class_chain_ending_in_no_guard_still_flagged(self): + """A three-hop chain whose terminal method has no guard at all.""" + src = """\ +hopOne(reactionId: $reactionId); + } + + private function hopOne(string $reactionId) { + return $this->hopTwo(reactionId: $reactionId); + } + + private function hopTwo(string $reactionId) { + $this->consultationService->deleteReaction($reactionId); + return new JSONResponse(['ok' => true]); + } +} +""" + out = _scan_app(src, {"ParticipationResponder": _RESPONDER}) + self.assertEqual(len(out), 1) + self.assertIn("method=deleteReaction", out[0]) + + def test_collaborator_guard_after_the_write_still_flagged(self): + """The guard must run BEFORE the mutation or it protects nothing.""" + src = """\ +consultationService->deleteReaction($reactionId); + return $this->responder->staffAction(fn (): array => []); + } +} +""" + out = _scan_app(src, {"ParticipationResponder": _RESPONDER}) + self.assertEqual(len(out), 1) + self.assertIn("method=deleteReaction", out[0]) + + def test_bare_throw_does_not_seed_a_delegation_chain(self): + """A collaborator method that only throws NotFoundException is not a guard. + + `throw` is accepted by the one-hop Pattern-1 helper rule; propagation + deliberately requires a STRICTER signal, so "this can fail" never + becomes "this checks who you are" further up the chain. + """ + thrower = """\ +mapper->find($id); + } +} +""" + src = """\ +loader->load($thingId)); + } +} +""" + out = _scan_app(src, {"ThingLoader": thrower}) + self.assertEqual(len(out), 1) + self.assertIn("method=showThing", out[0]) + + if __name__ == "__main__": unittest.main() diff --git a/hydra-gates/scripts/run-hydra-gates.sh b/hydra-gates/scripts/run-hydra-gates.sh index 6e82910..e7d3bf8 100755 --- a/hydra-gates/scripts/run-hydra-gates.sh +++ b/hydra-gates/scripts/run-hydra-gates.sh @@ -581,6 +581,16 @@ while IFS= read -r f; do _oa_files+=("$f") done < <(_enum_tracked '\.php$' lib/Service lib/Controller) _oa_ran=1 +# An EMPTY scope is not a clean tree. With zero files the log stays empty, the +# count is 0, and the gate used to report PASS having inspected nothing — the +# same failure family as .github#147, where a missing helper made gate-7 report +# PASS over 11 real unguarded endpoints. SKIPPED is the honest verdict: it is +# excluded from _EMITTED_GATES, so the coverage summary lists this gate under +# "GATES THAT DID NOT RUN" instead of folding it into ALL GATES GREEN. +if [ "${#_oa_files[@]}" -eq 0 ]; then + _oa_ran=0 + _skip 6 "orphan-auth" "scope was empty — 0 lib/Service or lib/Controller PHP file(s) in this diff, so NOTHING was inspected; orphaned (defined-but-never-called) authorization methods are UNVERIFIED by this run." +fi if [ "${#_oa_files[@]}" -gt 0 ]; then _oa_lib_dir="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")/lib" 2>/dev/null && pwd)" if [ ! -f "${_oa_lib_dir}/check_orphan_auth.py" ]; then @@ -652,6 +662,13 @@ while IFS= read -r f; do _idor_files+=("$f") done < <(_enum_tracked '\.php$' lib/Controller) _idor_ran=1 +# See the gate-6 note above: an empty scope inspected nothing, so it cannot be +# a PASS. SKIPPED keeps it out of _EMITTED_GATES and therefore visible in the +# coverage summary rather than silently green. +if [ "${#_idor_files[@]}" -eq 0 ]; then + _idor_ran=0 + _skip 7 "no-admin-idor" "scope was empty — 0 lib/Controller PHP file(s) in this diff, so NOTHING was inspected; unguarded #[NoAdminRequired] endpoints (IDOR, OWASP A01:2021) are UNVERIFIED by this run." +fi if [ "${#_idor_files[@]}" -gt 0 ]; then _gate_lib_dir="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")/lib" 2>/dev/null && pwd)" if [ ! -f "${_gate_lib_dir}/check_no_admin_idor.py" ]; then