fix(gate-50): a comment must neither consume the window nor satisfy it (#415) by rubenvdlinde · Pull Request #420 · ConductionNL/.github

rubenvdlinde · 2026-08-13T00:39:21Z

Closes #415.

gate-50's guard window was raw file text, so comment lines both consumed the budget and supplied the evidence. One cause, two failures pointing opposite ways.

arm	before	after
textbook guard, 12 comment lines above it	FAIL (false positive)	PASS
unguarded `api_token`, TODO naming the missing guard	PASS (false negative)	FAIL
clean guard, no comments	PASS	PASS
genuinely unguarded read	FAIL	FAIL

The false negative is the serious half. An unguarded read reported clean because the TODO above it read "we should throw new RuntimeException here when empty, and compare $tok === '' before use. Not done yet." — a comment stating the debt satisfied the gate that exists to collect it. That is the defect gate 19 was fixed for; nobody looked for it here.

The false positive is the one that erodes the gate. 8 of gate-50's 16 opencatalogi findings were real guards pushed out of range by the comment explaining them. The gate penalised the documented fix and passed the undocumented one — an author who deletes the comment goes green having changed nothing about the code's safety.

Fix

Strip comment text before building the window: comments occupy no budget and offer no evidence. Budget becomes eleven lines of code, not eleven lines of file. Line numbers are preserved (text blanked, lines kept) because findings report one.

String-safe rather than a naive split — 'https://x' must not truncate a line, #[PublicPage] is a PHP 8 attribute. Quote state resets per line, so an unterminated literal corrupts at most its own line.

Proof

New arms C14/C15/C16. With the runner change reverted, C14 and C15 both flip and C16 does not — so the two new arms detect this regression, and the third is the control against a stripper that eats code rather than comments. Suite 42 → 45 assertions, all green.

Half of every gate-50 count fleet-wide was suspect until this landed.

#415) gate-50's guard window was raw file text, so comment lines did both — and the two failures point in opposite directions from the one cause. FALSE POSITIVE. A textbook guard reported as an unsafe read because twelve lines of ordinary explanation sat between it and the call. 8 of gate-50's 16 opencatalogi findings were this. The gate penalised the DOCUMENTED fix and passed the undocumented one: deleting the comment goes green having changed nothing about the code's safety. FALSE NEGATIVE, and the more serious half. An unguarded read of an api_token reported CLEAN because the TODO above it read "we should throw new RuntimeException here when empty, and compare $tok === '' before use. Not done yet." A comment STATING THE DEBT satisfied the gate that exists to collect it. This is the defect gate 19 was fixed for; it was never looked for here. Both close by stripping comment TEXT before the window is built, so comments occupy no budget and offer no evidence. The budget is now eleven lines of CODE rather than eleven lines of file. Line numbering is preserved (text is blanked, lines are not removed) because findings report a line number. The stripper is string-safe rather than a naive split: 'https://x' must not truncate a line and #[PublicPage] is a PHP 8 attribute, not a comment. Quote state resets per line, so an unterminated literal corrupts at most its line. Acceptance: C14 (comment between read and guard -> PASS), C15 (TODO naming the missing guard -> FAIL), C16 (URL literal and attribute -> PASS). With the runner change reverted C14 and C15 both flip and C16 does not, so the two new arms are shown to detect this regression and the third is the control against a stripper that eats code. Suite 42 -> 45 assertions, all green.

rubenvdlinde · 2026-08-13T00:57:16Z

Correcting the impact claim in this PR body after measuring it.

I wrote "half of every gate-50 count fleet-wide is suspect until this landed." Measured, old checker vs new, over lib/**/*{Controller,Service}*.php:

repo (current `development`)	before	after	Δ
docudesk · procest · softwarecatalog · larpingapp	0	0	0
opencatalogi	4	4	0
opencatalogi @ `74e12c82` (the tree #415 was filed on)	16	12	−4

Two corrections:

gate-50's 10-line window counts comment lines — 8 of 16 findings in opencatalogi were guards pushed out of range by the comment explaining them #415's "8 of 16" is 4, not 8 — SetupController:299,300 and CatalogiService:371,372, all four genuine guards pushed out of the window by the comment explaining them.
The false-negative half has zero live instances across five repos. It is real and fixture-proven, but it is currently costing nothing. The value of this fix is prospective — it stops the class recurring — not a live cleanup, and I implied otherwise.

The rig was positive-controlled before I believed the zeros (five identical verdicts is the shape of a broken instrument): the same two checkers over the acceptance fixtures give B 0→1 and E 1→0. Layer: this is the checker over a file list I selected, not the gate through CI; it is corroborated by opencatalogi's 16 matching the figure CI reported on that tree.

The fix and its arms stand. Only the impact sentence was wrong.

@throws

…gate — 47 of 65 were (#415 class) (#425) * fix(gate-49): a comment must neither satisfy the gate nor manufacture a finding gate-50 was fixed for this in #415/#420. gate-49 has the same defect, one gate over, and nobody looked: it asked BOTH of its questions of the RAW method body. positive control an unhandled $this->objectService->deleteObject() -> FAIL — 1 method(s) (correct) false NEGATIVE the same method, plus a TODO reading "we should catch (DoesNotExistException $e) here ... Not done yet." -> PASS <- the defect false POSITIVE a method calling only $this->renderer->toArray(), plus a note "used to call $this->objectService->deleteObject ($id) directly. It no longer does." -> FAIL, names index() <- the defect The comment that switches the gate off is the comment that admits the debt. The comment that reddens it is the one explaining what was removed. Body questions now anchor on source_scope.php_mask (comments AND string contents blanked, offsets preserved), and the brace walk runs on the mask so a `{` in a string cannot hand a method a span it does not contain. The docblock is still read from the ORIGINAL text: @throws lives in a comment BY DESIGN, and masking it would trade one false negative for a fleet of false positives. NOT a fourth hand-rolled stripper. php_mask already exists in lib/source_scope.py and already knows `#[` opens a PHP 8 attribute and that `//` inside 'https://x' opens nothing. Import failure exits 3, so the gate says SKIPPED (wiring) rather than silently grading raw text again. Arms G1-G5 in test_gate_45_to_55_acceptance.sh. With the runner change reverted, G2 and G3 FLIP and G1/G4/G5 do not — so G2/G3 are the evidence and the other three are labelled controls. * fix(gate-25): a comment saying you still owe the test is not the test gate-25 is written as gate-19's API-layer companion and it inherited gate 19's ORIGINAL defect along with its shape. It had no suite of its own, so nothing had ever asked it to fail. positive control an untested #[NoAdminRequired] endpoint -> FAIL — 1 new public endpoint (correct) false NEGATIVE + a *Test.php whose only mention of the method is "// TODO: we still owe a contract test that calls $this->controller->destroy($id) ... Not written yet." -> PASS — 1 endpoint, all covered <- the defect false NEGATIVE + a .postman_collection.json with ZERO items whose description reads "NOTE: we do NOT yet cover /api/things — the DELETE endpoint is untested." -> PASS — 1 endpoint, all covered <- the defect Both greens were bought by a sentence admitting the debt, and an author who writes neither note gets the red — the gate rewarding silence. PHPUnit evidence is now source_scope.php_mask'd (comments and string contents blanked). Newman evidence is now built from the fields that DECLARE a request — raw/path/host/url/name/method — instead of the file's bytes, so a `description` is no longer a request. JSON has no comments; a description is a comment in JSON syntax, and the discriminator there is WHICH string, not whether it is one. An unparseable collection falls back to the raw bytes rather than to "not covered": converting a malformed fixture into findings would be this change inventing failures in repos it was never measured against. New suite test_check_contract_coverage.py, 8 arms. Reverted, three FLIP (TODO / description / string-literal) and five do not — the positive control and the four "the real artefact still counts" pairs, which are controls. tests/run-helper-suites.sh discovers it with no workflow edit. * fix(gate-7): a TODO naming the missing guard is not the guard The fleet's most expensive gate has gate-50's defect. `gsrc` — the text every guard lookup in scan_file runs against — was the RAW source with only authentication-only spans removed, so prose in a method body answered "is this endpoint guarded?". Measured through the runner, one fixture, ONE ADDED LINE: #[NoAdminRequired] index(int $id) { return $this->service->find($id); } -> FAIL — 1 method(s) with NoAdminRequired + no guard (correct) the same method with `// TODO: throw new OCSForbiddenException when the caller does not own $id.` -> PASS <- the defect This gate's known failure mode has always been false POSITIVES, which is exactly why its silences get believed — and it has already reported 0 findings across 18 apps while 167 real IDORs sat behind it. A false negative here is the most expensive kind in the package. `_strip_strings_and_comments` gains `keep_strings=True` and now also handles `#` comments — which it never did, so the repair would otherwise have been one alternative spelling away from being bypassed. `#[` is excluded, because `#[NoAdminRequired]` is the single token that makes this gate look at a method at all: blanking it would not widen the gate, it would switch it off. STRING LITERALS ARE KEPT, narrowly and deliberately. A guard's evidence is often an argument that IS a string, and blanking literals in a 2,800-line checker whose known failure mode is over-reporting would trade a measured false negative for an unmeasured wave of false positives on a gate the fleet already learned to distrust. That variant is reported, not smuggled in. 7 new arms. Reverted, three FLIP (`//`, `/* */`, `#`) and one errors on the new signature; the positive control and both anti-widening pairs pass either way and are labelled controls. All 145 existing tests still green, and test_gate7_verb_object_guards.sh still 10/10. fix(gate-48): a comment is not a CSRF token, on both paths Path A, the runner's diff-signal grep: this file's OWN note already named the hole — "the cheapest way to green would have been a cosmetic edit under src/ containing the word requesttoken: exactly the prose-satisfaction #191 warns against." It was written as a hypothetical. It was not one. a diff dropping #[NoCSRFRequired], no frontend change -> FAIL — @NoCSRFRequired dropped without frontend co-change the same diff plus ONE added line reading `// TODO: this call still needs a requesttoken header. Not done yet.` -> PASS <- the defect Worse than one missed finding: a non-zero count SHORT-CIRCUITS the caller-state check built as the mitigation for this exact shape, so one comment skips the guard and its backup. The count is now taken over the script scope of each changed file at HEAD, restricted to the lines the diff added. Path B, check_csrf_callers.py: every question was asked of the raw file, and this helper makes an AFFIRMATIVE claim the runner prints as a NOTE. So prose did not merely hide a finding, it manufactured a green with a sentence attached saying the code is safe. A commented-out `@nextcloud/axios` import granted whole-file amnesty to every axios.post in it. Both now anchor on source_scope.script_mask — comments blanked, string literals KEPT, because `'OCS-APIRequest': 'true'` is a header name that IS a string and `method: 'DELETE'` is how a mutating call is recognised at all. Arms H1/H2. Reverted, H1 flips PASS->FAIL; H2 (a real added header is still a signal) passes either way and is a control. ⚠️ Found while building the fixture: the gate's `src/**/*.vue` pathspec is a PLAIN git pathspec, where `*` matches `/` too — so it requires a SECOND slash and never matches `src/Del.vue`. Recorded in the suite, not fixed here. * test(gate-48): pin path B in check_csrf_callers' own suite 7 arms. Reverted against origin/main (NOT against HEAD — the first revert used `git checkout --`, which restores the COMMITTED file and therefore still contained the fix; all seven arms passed and looked like seven controls) TWO flip: the TODO sitting INSIDE the paren-balanced call span, and the commented-out @nextcloud/axios import that grants whole-file amnesty. The remaining five are labelled controls in their own docstrings, including the near-miss pair: the same sentence three characters outside the searched span never reached the question and passes either way. Saying so is the point — an arm that cannot fail is not proof that the gate cannot. * chore(gate-48): drop an unused import from the signal-count heredoc --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>

The window was eleven lines of CODE following the read, counted over the FILE, with no notion of where the method ends. A guard belonging to a DIFFERENT method therefore cleared an unguarded read in the method above it. Reproduced on c26f9a3, one file, one variable: readToken() unguarded + an ordinary guarded readRegister() below it PASS <- the defect delete readRegister() and nothing else FAIL — 1, api_token at :6 #420 changed the window's BUDGET (comments no longer spend it) but not its EXTENT, so that fix moved this defect closer rather than away: blanking comments means real code lines from the next method arrive sooner. The window is now clipped at the enclosing method's closing brace, via the same brace walk gate-49 runs over masked text — kept local to this checker so gate-50 gains no new import and no new wiring failure mode. _strip_php_comments grows a for_structure mode that also blanks string contents and heredoc bodies; that copy is only ever brace-walked, never searched for a guard, so no guard vocabulary changes meaning. FAILS LOOSE. A walk that never balances yields a span to EOF, i.e. exactly the pre-#429 window — a false negative at worst, never a finding at a line where no guard could be written. Findings now name the method the window was clipped to, because without it the reader cannot tell "unguarded" from "the guard is out of range". FAMILY J in test_gate_45_to_55_acceptance.sh. J1 is the evidence arm (PASS on origin/main, FAIL here). J2-J5 hold their verdict in BOTH arms and are labelled CONTROLs: J2 the issue's positive control, J3 a guard genuinely in the same method inside budget (anti-false-positive), J4 the same method one code line OUTSIDE budget (proves the clip did not shrink the budget), J5 a } inside a string literal and a heredoc body (load-bearing: walking the un-blanked text computes readToken() as ending on its second line). Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>

…ng-literal-as-evidence axis in 16 gates (#424) (#438) * fix(gates): harden markup_mask's comment delimiter, then consolidate seven private copies (#424) STEP 1 — HARDEN. source_scope treated `' }}` blanked the markup between them and gates 35/36 went green over live markup. Replaced the `` regex with html_comment_spans(), a scanner that knows a quoted attribute value and a Vue {{ }} interpolation are not comment scope, and that <script>/<style> bodies are raw text. Unterminated openers fall back to 'this was text' rather than swallowing the file — over-blanking is the failure mode. STEP 2 — CONSOLIDATE, only after step 1. Six a11y checkers (gates 37, 39, 40, 42, 43, 44), php_template_scope (gate 41) and check_orphaned_write_capability each carried their own copy of the same regex with the same hole. All eight now call source_scope.mask_html_comments. Also closes the string-literal-as-evidence axis in three PHP gates: gate-8 php_mask(blank_strings=True) — nothing it matches is ever a string, and blanking also repairs the brace walker. gate-59 two masks, one coordinate system: the CALL from a strings-blanked anchor, the config KEY from the string-preserving text. gate-64 same split; strip_comments() (the #184 state machine) now delegates to php_mask, removing a second PHP comment dialect. Every arm labelled EVIDENCE or CONTROL by measurement, not by assertion. * fix(gates): close the string-literal-as-evidence axis in gates 10, 11, 13, 18, 50, 53 (#424) gate-10 / gate-11 TWO MASKS, ONE COORDINATE SYSTEM. Both read evidence out of a literal — the attribute name in getAttribute('data-x'), the '/settings' path, the import specifier — so blanking string contents would delete the evidence. Instead the pattern runs on the string-preserving mask and source_scope.starts_in_code() asks the ANCHOR whether the match START is code. gate-11's route-object brace walk moves to the anchor too, so a '}' inside a literal stops truncating the object the anti-widening guard reads. gate-13 the runner's inline PYMI masked comments with a FOURTH private  and never masked literals, so 'use <NcDialog> …' in <script> reported an inline modal. Now scopes to vue_markup_mask — the SFC's rendered template — which is the question the rule actually asks. Not a weakening: an inline modal is by definition written in the template. gate-18 tokens were matched against json.dumps(rule), so a rule whose own description WARNED AGAINST the legacy dialect was reported as it, three times. Now walks the structure: three KEY tokens against keys, '@self.' against machine values, with per-locale/documentation fields excluded. Gate had NO helper suite; test_check_notification_dialect.py adds 14 arms. gate-50 #420 hand-rolled _strip_php_comments inside run-hydra-gates.sh, a fourth PHP comment dialect. Deleted; the block calls php_mask. Verified behaviour-preserving over 2481 fleet PHP files (one difference, and it is php_mask being right: a CSS /* inside a string is not a comment). gate-53 stripJsComments was two regexes with no string awareness, so the /* in glob: '/*.vue' opened a block comment that deleted later registry entries — and, when the swallowed span was brace-unbalanced, made parsed:false skip the whole cross-reference check silently. New scripts/lib/js_scope.js is the node port of source_scope.js_comment_mask, asserted BYTE-IDENTICAL to it over a corpus plus this package's own .js sources. * test(source_scope): arms for starts_in_code, the anchoring predicate (#424) --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>

rubenvdlinde merged commit c45d219 into main Aug 13, 2026
34 checks passed

rubenvdlinde deleted the fix/gate-50-comment-window branch August 13, 2026 00:49

Navigation Menu

Uh oh!

fix(gate-50): a comment must neither consume the window nor satisfy it (#415) - #420

fix(gate-50): a comment must neither consume the window nor satisfy it (#415)#420
rubenvdlinde merged 1 commit into
mainfrom
fix/gate-50-comment-window

rubenvdlinde commented Aug 13, 2026

Uh oh!

Uh oh!

rubenvdlinde commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rubenvdlinde commented Aug 13, 2026

Fix

Proof

Uh oh!

Uh oh!

rubenvdlinde commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants