From 85c7fe8163b944ce92145c3dca40bc0dc1cfe189 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 9 Aug 2026 23:13:31 +0200 Subject: [PATCH 1/2] fix(gate-48): a net-zero MOVE is not a removal, and 'no co-change in the diff' is not 'no co-change needed' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects, both measured on larpingapp. 1. A NET-ZERO MOVE COUNTED AS A REMOVAL check_csrf_removal.py scanned only ^- lines and never asked whether an identical ^+ line existed, so relocating a docblock line inside a file read as deleting the annotation. larpingapp#297 moved one comment line while reordering SettingsController's docblocks; the removed line is byte-identical to an added one, and gate-48 kept Hydra Gates red on that repo's development branch from then on, over a commit that changed no auth posture at all. Cancellation is per FILE and by MULTISET: per file because a line deleted from one controller and added to another is a real posture change for the first; by multiset because removing a tag twice and restoring it once has removed it once. Whitespace is NOT normalised — a re-indented line is not the same line, and treating it as a move would let a reformat swallow a genuine deletion. pre-fix removals() on the #297 diff -> 1 post-fix removals() on the #297 diff -> 0 2. THE GATE COULD NOT SEE A CALLER THAT WAS ALREADY COMPLIANT The co-change question was asked of the DIFF. A PR whose callers have always sent a token has no signal to add, so it could not pass; the only exits were a waiver or staying red. Measured on larpingapp#298, which closes a LIVE CSRF-forgery hole. SettingsController::create() and reimport() carried * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206). at docblock-tag position, where Nextcloud's ControllerMethodReflector regex /^\h+\*\h+@(?P[A-Z]\w+)((?P.*))?$/m reads it as the annotation being PRESENT — the sentence announcing the removal was what kept CSRF disabled on two state-mutating admin POSTs. Deleting it is the fix, and all three frontend callers already sent requesttoken while the shared CnAdminSettingsShell uses @nextcloud/axios. The cheapest way to green would have been a cosmetic edit under src/ containing the word requesttoken: precisely the prose-satisfaction #191 warns against. So ask the STATE instead of the diff, via a new scripts/lib/check_csrf_callers.py: is any mutating caller unprotected right now? Conservative in the direction that matters — every caller protected means the endpoint's caller is protected too; any caller unprotected means we cannot show it is not this endpoint's, so the removal still blocks. A green states which claim it is making, on stdout, rather than passing silently. BOTH ARMS, end-to-end through the runner on larpingapp#298: arm 1 every caller protected [gate-48] no CSRF signal was ADDED by this diff, and none was needed... [gate-48] csrf-cochange: PASS arm 2 one unprotected fetch() DELETE added (the opencatalogi#79 shape) [gate-48] csrf-cochange: FAIL UNPROTECTED mutating call site(s) - these are why the removal blocks: src/services/__armtwo_probe.js:3 - fetch() DELETE with no CSRF signal Arm 2 is the one that proves this did not switch the gate off: the defect gate-48 was built for is still caught, and the log now names the call site. Helper suites: 63 -> 64 discovered, test_check_csrf_callers.py auto-discovered and passing; passed 62, quarantined 2, failed 0. --- hydra-gates/scripts/lib/check_csrf_callers.py | 164 ++++++++++++++++++ hydra-gates/scripts/lib/check_csrf_removal.py | 49 +++++- .../scripts/lib/test_check_csrf_callers.py | 154 ++++++++++++++++ .../scripts/lib/test_check_csrf_removal.py | 83 +++++++++ hydra-gates/scripts/run-hydra-gates.sh | 55 +++++- 5 files changed, 502 insertions(+), 3 deletions(-) create mode 100644 hydra-gates/scripts/lib/check_csrf_callers.py create mode 100644 hydra-gates/scripts/lib/test_check_csrf_callers.py diff --git a/hydra-gates/scripts/lib/check_csrf_callers.py b/hydra-gates/scripts/lib/check_csrf_callers.py new file mode 100644 index 00000000..09b67c71 --- /dev/null +++ b/hydra-gates/scripts/lib/check_csrf_callers.py @@ -0,0 +1,164 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: EUPL-1.2 +"""Gate-48 companion — which frontend call sites send NO CSRF token? + +WHY THIS EXISTS +--------------- +Gate-48 asks one question of a diff that removes ``@NoCSRFRequired``: *did the +same diff add a CSRF signal under* ``src/``? For a PR whose callers have +**always** sent a token there is no such signal to add, and the gate cannot be +satisfied without a waiver. + +Measured on ConductionNL/larpingapp#298, which closes a live CSRF-forgery hole: +``SettingsController::create()`` and ``reimport()`` carried + + * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206). + +at docblock-tag position, where Nextcloud's ``ControllerMethodReflector`` reads +it as the annotation being PRESENT — so the sentence announcing the removal was +what kept CSRF disabled. Removing it is the fix. All three frontend callers +already sent ``requesttoken``, and the shared ``CnAdminSettingsShell`` uses +``@nextcloud/axios``, which injects it. The co-change gate-48 wanted did not +exist to be made, and the cheapest way to go green would have been a cosmetic +edit containing the word ``requesttoken`` — the prose-satisfaction the gate +programme exists to stop. + +THE QUESTION THIS ASKS INSTEAD +------------------------------ +Not "did the diff change a caller?" but "**is any mutating caller unprotected +right now?**". That is sound in the conservative direction: + +* if EVERY mutating call site already carries a CSRF-bearing mechanism, then + whichever one reaches the endpoint whose annotation was removed is protected, + and enforcing CSRF cannot break it; +* if ANY mutating call site lacks one, we cannot tell that it is not the + caller of that endpoint, so the removal still blocks. + +opencatalogi#79 — the defect gate-48 was built for, a delete-modal ``fetch()`` +with no CSRF header — is still caught: that call site is reported here, so the +gate still fails. A fix that stopped catching it would be a gate switched off. + +WHAT COUNTS AS PROTECTED +------------------------ +Within the call expression itself: ``requesttoken`` / ``OCS-APIRequest`` (both +case-insensitive, HTTP header names are), or ``getRequestToken``. Or the call +goes through ``@nextcloud/axios``, imported in that file — that client attaches +the current token itself, which is the canonical Nextcloud mechanism. + +Usage:: + + check_csrf_callers.py + +Prints one ``path:line — reason`` per UNPROTECTED mutating call site. +Exits 0 always; the OUTPUT is the answer (#209). +""" +from __future__ import annotations + +import os +import re +import sys + +SRC_SUFFIXES = ('.vue', '.js', '.ts', '.mjs', '.cjs') +SKIP_DIRS = {'node_modules', 'dist', 'build', 'vendor', '.git', 'coverage'} + +# `method: 'POST'` / `method: "put"` / `method:\n 'PATCH'` inside a fetch init. +MUTATING_METHOD = re.compile( + r"""method\s*:\s*['"`](?PPOST|PUT|PATCH|DELETE)['"`]""", + re.IGNORECASE, +) +# axios.post( / axios.put( / this.$axios.delete( ... +AXIOS_MUTATING = re.compile( + r"""\baxios\s*\.\s*(?Ppost|put|patch|delete)\s*\(""", + re.IGNORECASE, +) +FETCH_CALL = re.compile(r"""\bfetch\s*\(""") +# An import of the Nextcloud axios wrapper, under any local alias. +NEXTCLOUD_AXIOS_IMPORT = re.compile( + r"""from\s+['"]@nextcloud/axios['"]|require\(\s*['"]@nextcloud/axios['"]\s*\)""" +) +CSRF_SIGNAL = re.compile( + r"""requesttoken|OCS-APIREQUEST|getRequestToken""", + re.IGNORECASE, +) + + +def _call_text(text: str, open_paren: int) -> str: + """Text of the call expression starting at the `(` index, paren-balanced. + + Falls back to the rest of the file when the parentheses never balance, so a + malformed file reports the call as UNPROTECTED rather than being skipped — + an unparseable caller is not evidence of a token. + """ + depth = 0 + for i in range(open_paren, len(text)): + ch = text[i] + if ch == '(': + depth += 1 + elif ch == ')': + depth -= 1 + if depth == 0: + return text[open_paren:i + 1] + return text[open_paren:] + + +def unprotected_call_sites(app_dir: str) -> list[str]: + """Mutating frontend call sites carrying no CSRF-bearing mechanism.""" + findings: list[str] = [] + src_root = os.path.join(app_dir, 'src') + if not os.path.isdir(src_root): + return findings + + for root, dirs, files in os.walk(src_root): + dirs[:] = [d for d in dirs if d not in SKIP_DIRS] + for name in sorted(files): + if not name.endswith(SRC_SUFFIXES): + continue + path = os.path.join(root, name) + try: + with open(path, encoding='utf-8', errors='replace') as handle: + text = handle.read() + except OSError: + continue + rel = os.path.relpath(path, app_dir) + uses_nc_axios = bool(NEXTCLOUD_AXIOS_IMPORT.search(text)) + + # 1. axios.(...) — protected iff the file imports @nextcloud/axios. + for m in AXIOS_MUTATING.finditer(text): + if uses_nc_axios: + continue + call = _call_text(text, m.end() - 1) + if CSRF_SIGNAL.search(call): + continue + line = text.count('\n', 0, m.start()) + 1 + findings.append( + f"{rel}:{line} — axios.{m.group('verb').lower()}() with no CSRF " + f"signal and no @nextcloud/axios import" + ) + + # 2. fetch(...) — mutating iff its init object names a mutating verb. + for m in FETCH_CALL.finditer(text): + call = _call_text(text, m.end() - 1) + verb = MUTATING_METHOD.search(call) + if verb is None: + continue + if CSRF_SIGNAL.search(call): + continue + line = text.count('\n', 0, m.start()) + 1 + findings.append( + f"{rel}:{line} — fetch() {verb.group('verb').upper()} with no " + f"CSRF signal" + ) + return findings + + +def main(argv: list[str]) -> int: + if len(argv) != 2: + print("usage: check_csrf_callers.py ", file=sys.stderr) + return 2 + for finding in unprotected_call_sites(argv[1]): + print(finding) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/hydra-gates/scripts/lib/check_csrf_removal.py b/hydra-gates/scripts/lib/check_csrf_removal.py index 2c852c77..78fe261c 100644 --- a/hydra-gates/scripts/lib/check_csrf_removal.py +++ b/hydra-gates/scripts/lib/check_csrf_removal.py @@ -67,15 +67,60 @@ # A diff header line is `---` / `---` shaped; it is not a removed line of code. DIFF_HEADER = re.compile(r'^---(\s|$)') +# `+++ 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+)') def removals(diff: str) -> list[str]: - out = [] + """Removed lines that genuinely DROPPED CSRF protection. + + A REMOVAL PAIRED WITH AN IDENTICAL ADDITION IS A MOVE, NOT A REMOVAL. + + Relocating a docblock line inside a file emits a `-` and a `+` carrying the + same bytes. Scanning only `^-` reads that as deleting the annotation, and + the gate reports a security regression for a diff in which nothing about + CSRF changed. Measured on larpingapp: #297 moved one comment line while + reordering `SettingsController`'s docblocks — + + - * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206). + + * instance-wide configuration write needs. `@NoCSRFRequired` was removed + + * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206). + + — and gate-48 kept `Hydra Gates` red on that repo's `development` branch + from then on, over a commit that changed no auth posture at all. + + Cancellation is per FILE and by MULTISET. Per file because a line deleted + from one controller and added to another is a real change of posture for + the first one; by multiset because a diff that removes a tag twice and + restores it once has removed it once. + """ + # {path: [raw content of each added line]} and the removals in file order. + added: dict[str | None, list[str]] = {} + found: list[tuple[str | None, str]] = [] + path: str | None = None + for line in diff.splitlines(): + header = DIFF_FILE_HEADER.match(line) + if header: + path = header.group('path') + continue + if line.startswith('+'): + added.setdefault(path, []).append(line[1:]) + continue if not line.startswith('-') or DIFF_HEADER.match(line): continue if ATTRIBUTE_REMOVED.match(line) or DOCBLOCK_TAG_REMOVED.match(line): - out.append(line) + found.append((path, line)) + + out: list[str] = [] + for file_path, line in found: + pool = added.get(file_path) + if pool is not None and line[1:] in pool: + # Consume the pairing so a second identical removal still reports. + pool.remove(line[1:]) + continue + out.append(line) return out diff --git a/hydra-gates/scripts/lib/test_check_csrf_callers.py b/hydra-gates/scripts/lib/test_check_csrf_callers.py new file mode 100644 index 00000000..f8399565 --- /dev/null +++ b/hydra-gates/scripts/lib/test_check_csrf_callers.py @@ -0,0 +1,154 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: EUPL-1.2 +"""Tests for check_csrf_callers (gate-48 companion). + +Run with: python3 scripts/lib/test_check_csrf_callers.py + +Both arms, per #191's rule: arm 2 is the one that proves the companion did not +simply declare every repository compliant. A checker that reports nothing looks +exactly like a codebase with nothing to report. +""" +from __future__ import annotations + +import os +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import check_csrf_callers as gate # noqa: E402 + + +def app_with(files: dict[str, str]) -> str: + """Materialise a throwaway app dir; returns its path.""" + root = tempfile.mkdtemp() + for rel, content in files.items(): + path = os.path.join(root, rel) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, 'w', encoding='utf-8') as handle: + handle.write(content) + return root + + +class TestCompliantCallersReportNothing(unittest.TestCase): + """Arm 1 — the larpingapp#298 shape: every caller already sends a token.""" + + def test_the_larpingapp_settings_store(self): + root = app_with({'src/store/modules/settings.js': """ +export default { + async saveSettings(config) { + const response = await fetch('/apps/larpingapp/api/settings', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + requesttoken: OC.requestToken, + }, + body: JSON.stringify(config), + }) + return response.json() + }, +} +"""}) + self.assertEqual(gate.unprotected_call_sites(root), []) + + def test_an_ocs_apirequest_header_counts(self): + root = app_with({'src/x.vue': """ +await fetch(url, { method: 'POST', headers: { 'OCS-APIREQUEST': 'true' } }) +"""}) + self.assertEqual(gate.unprotected_call_sites(root), []) + + def test_get_request_token_counts(self): + root = app_with({'src/x.js': """ +await fetch(url, { method: 'PUT', headers: { requesttoken: getRequestToken() } }) +"""}) + self.assertEqual(gate.unprotected_call_sites(root), []) + + def test_nextcloud_axios_injects_the_token(self): + root = app_with({'src/x.js': """ +import axios from '@nextcloud/axios' +export const save = () => axios.post('/apps/x/api/settings', {}) +"""}) + self.assertEqual(gate.unprotected_call_sites(root), []) + + def test_a_plain_GET_fetch_is_not_a_mutating_call(self): + root = app_with({'src/x.js': "const r = await fetch('/apps/x/api/settings')\n"}) + self.assertEqual(gate.unprotected_call_sites(root), []) + + def test_an_app_with_no_src_directory(self): + self.assertEqual(gate.unprotected_call_sites(app_with({'lib/X.php': ' axios.post('/apps/x/api/settings', {}) +"""}) + found = gate.unprotected_call_sites(root) + self.assertEqual(len(found), 1, found) + self.assertIn('axios.post()', found[0]) + + def test_every_mutating_verb_is_covered(self): + for verb in ('POST', 'PUT', 'PATCH', 'DELETE'): + root = app_with({'src/x.js': f"await fetch(u, {{ method: '{verb}' }})\n"}) + found = gate.unprotected_call_sites(root) + self.assertEqual(len(found), 1, f"{verb}: {found}") + self.assertIn(verb, found[0]) + + def test_a_token_on_a_DIFFERENT_call_does_not_protect_this_one(self): + """Paren-balanced extraction. A file-wide search would let one correct + call vouch for every incorrect one beside it.""" + root = app_with({'src/x.js': """ +await fetch(a, { method: 'POST', headers: { requesttoken: OC.requestToken } }) +await fetch(b, { method: 'POST', headers: { 'Content-Type': 'application/json' } }) +"""}) + found = gate.unprotected_call_sites(root) + self.assertEqual(len(found), 1, found) + self.assertIn(':3', found[0]) + + def test_node_modules_is_not_scanned(self): + root = app_with({ + 'src/node_modules/dep/index.js': "fetch(u, { method: 'POST' })\n", + 'src/ok.js': "fetch(u, { method: 'POST', headers: { requesttoken: t } })\n", + }) + self.assertEqual(gate.unprotected_call_sites(root), []) + + def test_the_reported_line_number_is_the_call(self): + root = app_with({'src/x.js': "\n\n\nawait fetch(u, { method: 'POST' })\n"}) + found = gate.unprotected_call_sites(root) + self.assertEqual(len(found), 1, found) + self.assertIn('src/x.js:4', found[0]) + + +class TestCli(unittest.TestCase): + def test_missing_argument_is_rejected(self): + self.assertEqual(gate.main(["check_csrf_callers.py"]), 2) + + def test_extra_arguments_are_rejected(self): + self.assertEqual(gate.main(["check_csrf_callers.py", "a", "b"]), 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/hydra-gates/scripts/lib/test_check_csrf_removal.py b/hydra-gates/scripts/lib/test_check_csrf_removal.py index 7bdbf0bf..845d3e4e 100644 --- a/hydra-gates/scripts/lib/test_check_csrf_removal.py +++ b/hydra-gates/scripts/lib/test_check_csrf_removal.py @@ -98,6 +98,89 @@ def test_an_attribute_with_arguments_before_the_name_does_not_match(self): self.assertEqual(gate.removals("- #[Route('/x')] // NoCSRFRequired"), []) +class TestANetZeroMoveIsNotARemoval(unittest.TestCase): + """A `-` paired with an identical `+` is a MOVE. + + Measured on larpingapp: #297 relocated one docblock line and gate-48 kept + `Hydra Gates` red on that repo's `development` from then on, over a commit + that changed no auth posture at all. + """ + + MOVED = " * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206)." + + def test_the_larpingapp_297_diff(self): + diff = "\n".join([ + "--- a/lib/Controller/SettingsController.php", + "+++ b/lib/Controller/SettingsController.php", + "@@ -280 +284,16 @@", + "-" + self.MOVED, + "+ * instance-wide configuration write needs. `@NoCSRFRequired` was removed", + "@@ -301,0 +321,32 @@", + "+" + self.MOVED, + ]) + self.assertEqual(gate.removals(diff), []) + + def test_a_moved_attribute_line(self): + diff = "\n".join([ + "+++ b/lib/Controller/X.php", + "- #[NoCSRFRequired]", + "+ #[NoCSRFRequired]", + ]) + self.assertEqual(gate.removals(diff), []) + + def test_removed_twice_restored_once_still_reports_one(self): + """Multiset cancellation. Two deletions and one restoration is one + net deletion; reporting zero here would be the fix over-applied.""" + diff = "\n".join([ + "+++ b/lib/Controller/X.php", + "- #[NoCSRFRequired]", + "- #[NoCSRFRequired]", + "+ #[NoCSRFRequired]", + ]) + self.assertEqual(gate.removals(diff), ["- #[NoCSRFRequired]"]) + + def test_a_move_ACROSS_files_is_still_a_removal(self): + """Deleted from one controller, added to another. The first controller + genuinely lost the annotation, so cancelling across files would hide a + real posture change.""" + diff = "\n".join([ + "+++ b/lib/Controller/A.php", + "- #[NoCSRFRequired]", + "+++ b/lib/Controller/B.php", + "+ #[NoCSRFRequired]", + ]) + self.assertEqual(gate.removals(diff), ["- #[NoCSRFRequired]"]) + + def test_a_genuine_removal_alongside_an_unrelated_move_still_reports(self): + diff = "\n".join([ + "+++ b/lib/Controller/X.php", + "- * @NoCSRFRequired", + "+ * @NoCSRFRequired", + "- #[NoCSRFRequired]", + ]) + self.assertEqual(gate.removals(diff), ["- #[NoCSRFRequired]"]) + + def test_whitespace_is_not_normalised_away(self): + """A re-indented line is NOT the same line. Treating it as a move would + let a reformat swallow a genuine deletion.""" + diff = "\n".join([ + "+++ b/lib/Controller/X.php", + "- #[NoCSRFRequired]", + "+ #[NoCSRFRequired]", + ]) + self.assertEqual(gate.removals(diff), ["- #[NoCSRFRequired]"]) + + def test_the_file_header_is_not_read_as_an_addition(self): + """`+++ b/...` starts with `+`. If it were pooled as an added line the + path would poison the cancellation set.""" + diff = "\n".join([ + "--- a/lib/Controller/X.php", + "+++ b/lib/Controller/X.php", + "- #[NoCSRFRequired]", + ]) + self.assertEqual(gate.removals(diff), ["- #[NoCSRFRequired]"]) + + class TestCli(unittest.TestCase): def test_arguments_are_rejected(self): self.assertEqual(gate.main(["check_csrf_removal.py", "extra"]), 2) diff --git a/hydra-gates/scripts/run-hydra-gates.sh b/hydra-gates/scripts/run-hydra-gates.sh index 1726880b..a1bd3371 100755 --- a/hydra-gates/scripts/run-hydra-gates.sh +++ b/hydra-gates/scripts/run-hydra-gates.sh @@ -5862,13 +5862,66 @@ elif [ "${SCOPE_TO_DIFF}" = "1" ] && [ -n "${BASE_REF}" ]; then _csrf_fe_signals="${_csrf_fe_signals%%$'\n'*}" case "${_csrf_fe_signals}" in ''|*[!0-9]*) _csrf_fe_signals=0 ;; esac if [ "${_csrf_fe_signals}" -eq 0 ]; then + # ----------------------------------------------------------------- + # "NO CO-CHANGE IN THE DIFF" IS NOT "NO CO-CHANGE NEEDED". + # + # The question above is asked of the DIFF. A PR whose callers have + # ALWAYS sent a token has no signal to add, and so could not pass — + # the only exits were a waiver or staying red. + # + # Measured on larpingapp#298, which closes a LIVE CSRF-forgery hole: + # `SettingsController::create()` and `reimport()` carried + # + # * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206). + # + # at docblock-tag position, where Nextcloud's + # ControllerMethodReflector reads it as the annotation being + # PRESENT — the sentence announcing the removal was what kept CSRF + # disabled. Deleting it is the fix, and all three callers already + # sent `requesttoken` while the shared CnAdminSettingsShell uses + # @nextcloud/axios. The cheapest way to green would have been a + # cosmetic edit under src/ containing the word `requesttoken`: + # exactly the prose-satisfaction #191 warns against. + # + # So ask the state instead of the diff — is any mutating caller + # unprotected RIGHT NOW? Conservative in the direction that matters: + # every caller protected => the endpoint's caller is protected too; + # any caller unprotected => we cannot show it is not this endpoint's, + # so the removal still blocks. opencatalogi#79 (a delete-modal + # fetch() with no header) is still caught — its own test pins that. + # ----------------------------------------------------------------- + _csrf_callers_helper="${SCRIPT_DIR}/lib/check_csrf_callers.py" + _csrf_unprotected="" + _csrf_callers_ran=0 + if [ -f "${_csrf_callers_helper}" ] && [ -d src ]; then + set +e + _csrf_callers_err="${HYDRA_GATE_LOG_DIR}/hydra-gate-csrf-callers.err" + _csrf_unprotected=$(python3 "${_csrf_callers_helper}" . 2>"${_csrf_callers_err}") + if [ $? -eq 0 ]; then + _csrf_callers_ran=1 + fi + set +e + fi # Check for opt-out _csrf_optout_re='\[hydra-gate-csrf-cochange exclude\][[:space:]]+.{20,}' _csrf_optout="" _optout_text | grep -qE "${_csrf_optout_re}" && _csrf_optout="1" - if [ -z "${_csrf_optout}" ]; then + if [ "${_csrf_callers_ran}" -eq 1 ] && [ -z "${_csrf_unprotected}" ]; then + # Stated, never silent: a green here is a claim about the + # callers, and the reader must be able to see which claim. + echo "[gate-48] no CSRF signal was ADDED by this diff, and none was needed:" \ + "every mutating call site under src/ already carries one" \ + "(requesttoken / OCS-APIRequest / getRequestToken / @nextcloud/axios)." \ + "Checked by scripts/lib/check_csrf_callers.py over the working tree." + elif [ -z "${_csrf_optout}" ]; then echo "@NoCSRFRequired removed but no frontend CSRF-signal added in diff:" >> "${_csrf_log}" echo "${_csrf_removed}" >> "${_csrf_log}" + if [ "${_csrf_callers_ran}" -eq 1 ] && [ -n "${_csrf_unprotected}" ]; then + echo "UNPROTECTED mutating call site(s) — these are why the removal blocks:" >> "${_csrf_log}" + echo "${_csrf_unprotected}" >> "${_csrf_log}" + elif [ "${_csrf_callers_ran}" -eq 0 ]; then + echo "(caller state NOT inspected: check_csrf_callers.py missing or no src/ — falling back to the diff-only question)" >> "${_csrf_log}" + fi fi fi fi From 73d45daa1f5041580b363427c18692be09b2380b Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 9 Aug 2026 23:15:56 +0200 Subject: [PATCH 2/2] fix(gate-48): test the helper's status directly (SC2181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ShellCheck SC2181 on the `if [ $? -eq 0 ]` after the command substitution. Moved the assignment into the `if` so the helper's own exit status is tested directly, and cleared `_csrf_unprotected` on the failure path. That second part is not cosmetic: without it a crashed interpreter leaves the variable holding whatever partial stdout it emitted, and an EMPTY result from a helper that died reads identically to "no unprotected callers found" — the fail-open shape this gate has been bitten by before. `_csrf_callers_ran` stays 0 in that case, so the gate falls back to the diff-only question and says so in the log rather than passing on an unanswered question. Arm 1 re-verified end-to-end on larpingapp#298 after the refactor: PASS with the reason line intact. --- hydra-gates/scripts/run-hydra-gates.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/hydra-gates/scripts/run-hydra-gates.sh b/hydra-gates/scripts/run-hydra-gates.sh index a1bd3371..ebb10f10 100755 --- a/hydra-gates/scripts/run-hydra-gates.sh +++ b/hydra-gates/scripts/run-hydra-gates.sh @@ -5896,9 +5896,14 @@ elif [ "${SCOPE_TO_DIFF}" = "1" ] && [ -n "${BASE_REF}" ]; then if [ -f "${_csrf_callers_helper}" ] && [ -d src ]; then set +e _csrf_callers_err="${HYDRA_GATE_LOG_DIR}/hydra-gate-csrf-callers.err" - _csrf_unprotected=$(python3 "${_csrf_callers_helper}" . 2>"${_csrf_callers_err}") - if [ $? -eq 0 ]; then + # Assignment inside the `if` so the helper's own status is + # tested directly — a crashed interpreter must NOT be read as + # "no unprotected callers found", which is the fail-open shape + # this gate has been bitten by before. + if _csrf_unprotected=$(python3 "${_csrf_callers_helper}" . 2>"${_csrf_callers_err}"); then _csrf_callers_ran=1 + else + _csrf_unprotected="" fi set +e fi