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

hydra-gates/scripts/lib/check_csrf_callers.py

@@ -69,6 +69,117 @@
     r"""method\s*:\s*['"`](?P<verb>POST|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"""(?<![\w$.])method\s*:\s*(?P<val>[^,}\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<name>[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+)?(?<![\w$.])"""
+        + re.escape(name) + r"""\s*=\s*([^;\n]{1,200})""")
+    hits = [m for m in pattern.finditer(text)
+            if before is None or m.start() < before]
+    if before is None:
+        return [m.group(1) for m in hits]
+    return [hits[-1].group(1)] if hits else []
+
+
+def _method_value_is_mutating(value: str, text: str, depth: int = 0,
+                              before: int = None):
+    """Ternary verdict for one `method` value: True / False / None.
+
+    ``True``  — it is, or may be, a mutating verb.
+    ``False`` — proven to be a safe verb.
+    ``None``  — there is no `method` key at all (the caller decides).
+    """
+    value = value.strip()
+    if value == "":
+        return True
+    if SAFE_VERB_LITERAL.match(value):
+        return False
+    if MUTATING_VERB_ANYWHERE.search(value):
+        return True
+    ident = BARE_IDENTIFIER.match(value)
+    if ident is not None and depth < 2:
+        bindings = _binding_values(text, ident.group('name'), before)
+        if not bindings:
+            return True          # unresolvable binding — fail closed
+        return any(
+            _method_value_is_mutating(b, text, depth + 1, before) is not False
+            for b in bindings
+        )
+    # A call, a member expression, a template literal, a computed value: this
+    # helper cannot show it is safe, so it is not.
+    return True
+
+
+def _fetch_is_mutating(call: str, text: str, at: int = None):
+    """``(is_mutating, label)`` for one `fetch(...)` call expression.
+
+    *at* is the offset of the call inside *text*, used to resolve an identifier
+    to the binding that precedes it rather than to any binding in the file.
+    """
+    verdict = None
+    label = "method"
+    for m in METHOD_KEY_VALUE.finditer(call):
+        value = m.group('val')
+        if _method_value_is_mutating(value, text, before=at):
+            return True, value.strip()[:40]
+        verdict = False
+        label = value.strip()[:40]
+    if verdict is None and METHOD_SHORTHAND.search(call):
+        # `{ method }` — resolve the binding of that name.
+        if _method_value_is_mutating("method", text, before=at):
+            return True, "shorthand { method }"
+        return False, "shorthand { method }"
+    return (False, label) if verdict is False else (None, label)
 # axios.post( / axios.put( / this.$axios.delete( ...
 AXIOS_MUTATING = re.compile(
     r"""\baxios\s*\.\s*(?P<verb>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
 

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<path>\S+)')
+# `@@ -12,5 +12,0 @@` — the base-image start line of the hunk.
+HUNK_HEADER = re.compile(r'^@@\s+-(?P<start>\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 <name>` survives in that file at
+# HEAD, where `<name>` 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<name>[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 <name>(` 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