gate-48: a net-zero MOVE of a docblock line counts as a removal (red on larpingapp development over a no-op), and the gate cannot see callers that already send a token

[rubenvdlinde]

Two defects, both measured on ConductionNL/larpingapp

1. A net-zero MOVE of a docblock line is counted as a removal

check_csrf_removal.py scans only ^- lines. It never asks whether an identical ^+ line exists in the same diff, so relocating a docblock line inside a file reads as deleting the annotation.

larpingapp #297 (4a3dfc3 → 085b36d) moved one comment line while refactoring SettingsController's docblocks. The diff:

--- a/lib/Controller/SettingsController.php
+++ b/lib/Controller/SettingsController.php
-     * @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).

Byte-identity check on the same diff:

REMOVED: '     * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206).'
  identical line also ADDED: True

Nothing about CSRF changed in that commit. Gate-48 nevertheless went red, and has kept quality / Hydra Gates red on development ever since — it was the sole finding in that job:

[hydra-gates] RESULT: 1 GATE(S) FAILED.
  [gate-48] csrf-cochange: FAIL — @NoCSRFRequired dropped without frontend CSRF co-change

This is the same family as the nldesign false positive the helper's own docstring documents (#191): there the token was in the wrong position, here it is in the wrong diff arithmetic. A removal paired with an identical addition is not a removal.

Suggested fix: subtract identical added lines before reporting, i.e. treat the multiset difference removals − additions as the finding set rather than removals.

2. The gate cannot see a caller that is already compliant

The check asks "did this diff add a CSRF signal under src/?" — never "do the callers send one?". So a PR that removes @NoCSRFRequired from an endpoint whose callers have always sent a token cannot pass, and the only exits are a waiver or leaving it red.

Reproduced on ConductionNL/larpingapp#298, which fixes a live CSRF hole: SettingsController::create() and reimport() carried

     * @NoCSRFRequired removed to close the CSRF-forgery surface (closes #206).

at docblock-tag position. Nextcloud's ControllerMethodReflector::reflect() parses annotations with

preg_match_all('/^\h+\*\h+@(?P<annotation>[A-Z]\w+)((?P<parameter>.*))?$/m', $docs, $matches);

so the sentence announcing the removal re-registered the annotation, SecurityMiddleware::isValidCSRF() returned early, and CSRF was never enforced on either state-mutating admin POST. The PR moves the token off tag position — a genuine removal, and the point of the change.

Every caller was verified to already send a token:

caller	token
src/store/modules/settings.js:70 → POST /api/settings	requesttoken: OC.requestToken
src/store/modules/settings.js:105 → POST /api/settings/reimport	requesttoken: OC.requestToken
src/views/settings/Settings.vue:320 → POST /api/settings	requesttoken: OC.requestToken
CnAdminSettingsShell.reimport() (via reimport-url)	@nextcloud/axios (injects it)

and OC.requestToken is kept current rather than snapshotted — core/src/OC/index.js subscribes to csrf-token-update and reassigns it, with getRequestToken() reading document.head.dataset.requesttoken live.

So the co-change gate-48 asks for does not exist to be made, because it was never missing. The gate is red on a PR that closes a real CSRF hole, and the cheapest way to clear it would be to make a cosmetic edit under src/ containing the word requesttoken — which is exactly the prose-satisfaction the helper's docstring warns against.

Suggested fix: when a removal is detected, look at the current state of the callers of the affected route (the app's own src/), and pass when every caller already carries a CSRF-bearing mechanism. Failing that, distinguish "no caller sends a token" (block) from "callers send tokens but were not touched here" (report, do not block) — the second is the case the gate currently cannot express.

Impact

Defect 1 is currently the only thing keeping Hydra Gates red on larpingapp development, over a commit that changed nothing about CSRF. Defect 2 blocks a security fix behind a gate whose premise the fix already satisfies.

Measured with gate package 651e5c5bb3ba8764903e5d6fc5bac5a208bd67fc, NODE_PATH set so ajv resolves to an absolute path, .err 0 bytes on every run.