Skip to content

fix(gate-12): the element ended at the arrow of :reduce="(o) => o.id" — 18 of 18 findings false on scholiq - #219

Closed
rubenvdlinde wants to merge 1 commit into
mainfrom
fix/gate-12-nc-select-tag-extraction
Closed

fix(gate-12): the element ended at the arrow of :reduce="(o) => o.id" — 18 of 18 findings false on scholiq#219
rubenvdlinde wants to merge 1 commit into
mainfrom
fix/gate-12-nc-select-tag-extraction

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

fix(gate-12): the element ended at the arrow of :reduce="(o) => o.id"

Gate-12 extracted <NcSelect> elements with

grep -oE '<NcSelect[^>]*>'

[^>]* stops at the FIRST > character in the flattened source. In an
NcSelect that is almost never the end of the tag, because the idiomatic
vue-select usage puts an arrow function in an attribute value:

<NcSelect
    v-model="selectedRoundId"
    :options="roundOptions"
    :reduce="(o) => o.id"
    :input-label="t('scholiq', 'Round')"
    :aria-label-combobox="t('scholiq', 'Round')" />
              ^ extraction ends here

Everything after :reduce is invisible, so the two props the gate looks
for are cut off and the element is reported as unnamed.

MEASURED, scholiq 2026-08-08 full-tree: 18 findings, 18 of them false.
Every flagged element already carried :input-label AND
:aria-label-combobox, in every case written after :reduce. A
quote-aware re-scan of the same nine files found 18 NcSelect elements and
18 with a label prop.

The gate was anti-correlated with its own subject in those files: adding
the label prop could not clear it, and deleting :reduce could. Same
shape as gate-9's [^)]* bug (#198) — a character class used as a
delimiter over content that legitimately contains the delimiter.

FIX

Extraction moves to scripts/lib/check_nc_select_labels.py, using the same
quote-aware tag pattern check_form_labels.py (gate-40) already uses, so
the two a11y gates now agree on where an element stops. Wired with the
missing-helper _skip pattern from #147: an absent helper reports
SKIPPED (wiring) naming how many components went uninspected, never PASS.

The accepted prop set is UNCHANGED — input-label / inputLabel /
aria-label-combobox / ariaLabelCombobox, bare or bound. This changes
where an element ENDS, not what counts as a name, so before/after numbers
stay comparable. aria-label is still not accepted; a manual <label for=…> next to an NcSelect is still a finding.

Markup inside an HTML comment is no longer scanned — a commented-out
NcSelect renders nothing and can carry no accessible name.

EVIDENCE

  • 17 new unit tests in test_check_nc_select_labels.py, discovered
    automatically by tests/run-helper-suites.sh. Every relaxation is
    paired with the true positive it must not swallow:
    :reduce + label -> clean, the same element with the label deleted ->
    1 finding; two selects where only one is named -> exactly 1 finding
    (the greedy-match trap a naive fix falls into); an unclosed <NcSelect
    followed by a <div> -> 1 finding that does not name the div.
  • Whole package suite: 28 passed, 0 failed, 2 pre-existing quarantines.
  • Against scholiq's tree, unchanged app code: gate-12 goes 18 -> PASS.
  • Can-fail on the wiring: renaming the helper away makes the run print
    SKIPPED (wiring) — … 72 component(s) were in scope and NONE had their NcSelect elements inspected, not PASS.

Gate-12 extracted `<NcSelect>` elements with

    grep -oE '<NcSelect[^>]*>'

`[^>]*` stops at the FIRST `>` character in the flattened source. In an
NcSelect that is almost never the end of the tag, because the idiomatic
vue-select usage puts an arrow function in an attribute value:

    <NcSelect
        v-model="selectedRoundId"
        :options="roundOptions"
        :reduce="(o) => o.id"
        :input-label="t('scholiq', 'Round')"
        :aria-label-combobox="t('scholiq', 'Round')" />
                  ^ extraction ends here

Everything after `:reduce` is invisible, so the two props the gate looks
for are cut off and the element is reported as unnamed.

MEASURED, scholiq 2026-08-08 full-tree: **18 findings, 18 of them false.**
Every flagged element already carried `:input-label` AND
`:aria-label-combobox`, in every case written after `:reduce`. A
quote-aware re-scan of the same nine files found 18 NcSelect elements and
18 with a label prop.

The gate was anti-correlated with its own subject in those files: adding
the label prop could not clear it, and deleting `:reduce` could. Same
shape as gate-9's `[^)]*` bug (#198) — a character class used as a
delimiter over content that legitimately contains the delimiter.

FIX
---
Extraction moves to `scripts/lib/check_nc_select_labels.py`, using the same
quote-aware tag pattern `check_form_labels.py` (gate-40) already uses, so
the two a11y gates now agree on where an element stops. Wired with the
missing-helper `_skip` pattern from #147: an absent helper reports
SKIPPED (wiring) naming how many components went uninspected, never PASS.

The accepted prop set is UNCHANGED — `input-label` / `inputLabel` /
`aria-label-combobox` / `ariaLabelCombobox`, bare or bound. This changes
where an element ENDS, not what counts as a name, so before/after numbers
stay comparable. `aria-label` is still not accepted; a manual `<label
for=…>` next to an NcSelect is still a finding.

Markup inside an HTML comment is no longer scanned — a commented-out
NcSelect renders nothing and can carry no accessible name.

EVIDENCE
--------
- 17 new unit tests in `test_check_nc_select_labels.py`, discovered
  automatically by `tests/run-helper-suites.sh`. Every relaxation is
  paired with the true positive it must not swallow:
  `:reduce` + label -> clean, the same element with the label deleted ->
  1 finding; two selects where only one is named -> exactly 1 finding
  (the greedy-match trap a naive fix falls into); an unclosed `<NcSelect`
  followed by a `<div>` -> 1 finding that does not name the div.
- Whole package suite: 28 passed, 0 failed, 2 pre-existing quarantines.
- Against scholiq's tree, unchanged app code: gate-12 goes 18 -> PASS.
- Can-fail on the wiring: renaming the helper away makes the run print
  `SKIPPED (wiring) — … 72 component(s) were in scope and NONE had their
  NcSelect elements inspected`, not PASS.
rubenvdlinde added a commit that referenced this pull request Aug 8, 2026
…224, #226, #230, #235, #236, #266) (#269)

* fix(gates): nine checkers matched prose, not code — one shared scope, nine gates

Every gate below decided a question about CODE by grepping the raw bytes of a
file. Prose is made of the same bytes, so each one failed in BOTH directions
at once — the shape first written down in #184: "a checker that greps a STRING
LITERAL misses every constant and matches every comment."

  #191  gate-48  a REMOVED COMMENT naming `#[NoCSRFRequired]` read as a removed
                 attribute. nldesign red for one rewritten docblock sentence.
  #196  gate-5   a docblock saying `#[NoAdminRequired]` is deliberately NOT
                 used SATISFIED the auth gate. A false NEGATIVE on a security
                 gate, and a pass leaves no log.
  #220  gate-31  an `<img>` in a JSDoc comment in <script> (launchpad).
  #235  gate-31  the same, 3 of 3 findings on openbuild.
  #224  gate-34  false RED on a comment AND false GREEN on window['confirm']().
  #226  gate-3   a run() delegating to one helper read as a stub, and the gate
                 was closable by an inert `$unused = 1;`.
  #230  gate-58  a comment WARNING AGAINST networkidle counted as a use of it.
  #236  gate-12  `<NcSelect[^>]*>` truncated at the `>` of `option =>`.
  #236  gate-32  a comment describing the `<div @click>` an element replaced
                 scored as that `<div @click>`.
  #266  gate-41  a PHP comment mentioning `<html>` made a mount point a page
                 root.

ONE SCOPE, NOT NINE
-------------------
scripts/lib/source_scope.py generalises the two precedents that already got
this right — #184's PHP stripper (which knows `#` opens a comment but `#[`
opens an attribute) and #249's gate-19 tokeniser (blank once, PRESERVE
OFFSETS, keep string delimiters). Every mask returns a same-length string, so
a gate can report a line number computed on the mask and read a suppression
marker out of the ORIGINAL at that line — which matters because every
suppression marker in this package lives in a comment.

Gate-19 keeps its own copy of the JS tokeniser; a drift test asserts the two
byte-identical over a corpus and over this package's own .js sources, and
asserts the keyword sets equal — the corpus alone SURVIVED deleting "await"
from one set, so the corpus alone was not enough.

#196 SHIPS WITH A DECLARATION, NOT JUST A TIGHTENING
-----------------------------------------------------
Admin-only is expressed in Nextcloud by the ABSENCE of an attribute, and
absence is the only thing gate-5 reports. Closing the false negative alone
would have converted it into a PERMANENT false positive on correct code, with
no legitimate way to satisfy the gate. So `@auth admin-only <reason>` joins
the `@spec exclude` family. Making bare absence sufficient was considered and
rejected: it would empty the gate completely.

MEASURED, NOT ASSUMED
---------------------
- 3 fixtures from #226's table, the 4 arms from #224, the nldesign line from
  #191 and the larpingapp line from #230, all verbatim.
- Every relaxation is paired with the true positive it must not swallow, and
  every wiring is covered both ways: a MISSING helper and a CRASHING helper
  must report SKIPPED, never PASS (#147, #245, #249). gate-5 additionally runs
  a positive control on the mask itself, because a mask that silently returns
  its input is invisible to `[ -f helper ]` and puts the gate straight back
  into the false negative.
- A nested `<template #default>` slot regression was caught by measurement
  before landing: a lazy `(.*?)` ended the SFC template at the first slot
  close and deleted a real finding at openconnector EditMapping.vue:376.
  Boundaries are found by depth now, and there is a test.

Closes #191, #196, #220, #224, #226, #230, #235, #266
Refs #236 (parts 1 and 2; part 3 was already fixed by #247)
Supersedes #219, whose gate-12 helper is carried here with its 17 tests.

* fix(gate-34,gate-48): a guard is not a second dialog, and an FQCN attribute is one

Both found by MEASURING the fix rather than by reading the issues.

gate-34 — 7 defects reported as 14 findings
------------------------------------------
The first cut accepted any `window.confirm` REFERENCE, called or not, so on
openbuild every native dialog was reported twice:

    const ok = typeof window !== 'undefined' && window.confirm     <- guard
        ? window.confirm(t('openbuild', 'Delete this automation?')) <- call

A feature-detection guard is a truthiness test, not a second native dialog,
and inflating a security-adjacent count is its own false report (#254: a count
is not a defect count). A reference now counts only when it is an ALIAS — a
binding whose call site is elsewhere and therefore invisible:

    const c = window.confirm        counts
    const { confirm } = window      counts
    x && window.confirm ? … : …     does not

openbuild: 7 before, 7 after, same seven lines.

The anchor also lost a character it should never have had. Written
`=\s*window\s*[.\[]` it CONSUMED the `window` that follows, and `finditer`
returns non-overlapping matches — so `const r = window.confirm('x')` matched
only the alias rule, failed it because a `(` follows, and reported NOTHING. A
real call dropped by an anchor one character too greedy. It is a lookahead
now, and there is a test.

gate-48 — the old regex could not see a fully-qualified attribute
-----------------------------------------------------------------
Running #191's arm 2 end-to-end through the runner reported PASS on a genuine
removal of

    -    #[\OCP\AppFramework\Http\Attribute\NoCSRFRequired]

because the pre-fix pattern alternated on the literal `#[NoCSRFRequired]`.
A false NEGATIVE hiding behind the false positive #191 reported — the same
both-ways failure as every other gate in this change. The new bracket-bounded
rule matches it.

Refs #191, #224

* fix(source_scope): `</script bar>` ends a script, and the mask must know it

CodeQL raised py/bad-tag-filter (HIGH) against this branch, and it is right.

    r'<script(\s[^>]*)?>(.*?)</script\s*>'

does not match `</script bar>` or `</script\t\n foo>`, both of which an HTML
parser treats as the end of the element. When the close is spelled that way
the block regex fails to match AT ALL, the script body is never
comment-masked, and a JSDoc `<img>` inside it is scanned as markup — #235
reintroduced by the mask written to fix it. `</style …>` had the same hole.

⚠️ THE FIRST TEST FOR THIS SURVIVED THE MUTANT. It exercised
`vue_markup_mask`, which keeps `<template>` spans and never goes through
`_SCRIPT_BLOCK` at all, so reverting the regex changed nothing and the suite
stayed green. The assertion now runs through `html_markup_mask` and
`script_mask`, the two functions that actually use it, and the reverted regex
kills both. A mutation test that does not kill is not evidence — it is a
second thing to check.

Refs #235
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Superseded by #269, which landed on main as cdfbd7a.

This branch no longer merged cleanly (run-hydra-gates.sh moved under it), so rather than redo the work, check_nc_select_labels.py and all 17 of its tests were carried into #269 verbatim and rebased onto current main. Two things were added on top:

Measured post-merge on openconnector, attribution-isolated between main@32fa23e and main@cdfbd7a, from the runner's stdout: gate-12 13 → 10, with all three removals verified by hand as elements carrying input-label= written after :selectable="(option) => …".

Closing as superseded, not rejected — the analysis in this PR was correct and is quoted in the merged one. The branch is left in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant