ReviewOS

stacks/stx

strict: `allowPatterns` is rule-global, so a justified violation forces you to disable the rule app-wide or never set `failOnViolation`

#2049
Closed glennmichael123 opened this 9 days ago ·
9 days ago

strict has no way to exempt one site, so an app with a justified exception can never reach failOnViolation: true

StrictModeConfig offers enabled, failOnViolation and allowPatterns. allowPatterns is the only escape hatch and it is rule-global:

// dist/script-validation.js, 0.2.396
const allowPatterns = strictConfig.allowPatterns ?? []
for (const { pattern, message, suggestion } of PROHIBITED_DOM_PATTERNS) {
  if (allowPatterns.some(allowed => /\w/.test(allowed) && message.includes(allowed)))
    continue
  // …
}

It is a substring match against the rule's message, decided once per rule for the whole app, with no reference to the file or the call site. There is nothing else: no stx-strict-ignore comment, no ignoreFiles, no excludeFiles, no per-block attribute. Grepped the whole of dist/ on 0.2.396 to be sure.

So the only ways to handle a violation that is correct are to turn the rule off everywhere, or to leave failOnViolation: false on forever.

The case this blocks

Six HQ apps use this guard. Measured with stx's own validator today: bughq 15 violating blocks, statushq 19, reportshq 10, analyticshq 7, loghq 0, commshq 0. Most are genuine debt and we are working through them.

Some are not debt. An auth form that must intercept its own submit before the runtime loads:

<form id="signin" method="post" action="/api/auth/login">

The endpoint parses JSON.parse(await request.text()), so that native POST is not a working fallback — it exists so password managers recognise the form. Under <script client> interception waits for hydration, and a submit in that window posts urlencoded data and signs in with empty fields. A plain <script> with document.getElementById is the correct implementation, and it trips getElementById forever.

Exempting it with allowPatterns: ['getElementById'] would disable that rule across every other page in the app, which is the rule we most want enforced. So the choice is: lose the rule everywhere, or never ratchet.

What would fix it

Any one of these, in preference order:

  1. A comment pragma, checked per block or per line, the way every other linter does it:

    // stx-strict-ignore-next-line getElementById -- pre-hydration submit guard
    var form = document.getElementById('signin')

    This is the one that documents why at the site, which is the thing a reviewer needs.

  2. File scoping in config, e.g. allowPatterns: [{ pattern: 'getElementById', files: ['resources/views/login.stx'] }], keeping today's plain-string form working.

  3. An ignoreFiles: string[], which is coarse but still better than global, and cheap to implement.

Why it matters beyond ergonomics

failOnViolation: true is the only state where this guard prevents a regression rather than reporting one. Four of our six apps sit at false with a comment saying "ratchet once the queue is empty". If a queue can contain entries that are correct and unfixable, it never empties, and the guard stays advisory indefinitely — which is close to the original problem, where an absent strict key dropped every finding silently.

A pragma also makes the deliberate cases self-documenting. Right now a reader cannot tell "not migrated yet" from "correct, and here is why", because both look identical in the output.

Found on 0.2.396.

9 days ago

Implemented option 1, the comment pragma, in ba4e53f.

// stx-strict-ignore-next-line getElementById -- pre-hydration submit guard
var form = document.getElementById('signin')

var form = document.getElementById('signin') // stx-strict-ignore
  • Bare suppresses every rule on the line; naming rules suppresses only those, matched against the rule's message exactly as allowPatterns is — so one name cannot mean two different things depending on where you wrote it.
  • Everything after -- is for the reader and ignored by the checker.
  • //, /* */ and all work.
  • Suppression is per line. A file can exempt one justified call and stay in breach three lines down. That is the property that makes it safe to adopt, and the one that lets an app actually ratchet.

Your framing is what shaped it. "A queue that can contain entries that are correct and unfixable never empties, and the guard stays advisory indefinitely" is the real argument here — ergonomics would not have justified a new syntax, but a guard that can never be armed is the same silent-failure shape strict mode was added to fix.

Two holes I found by writing the tests for them

Both are the same shape as ones allowPatterns already closed, which is worth recording since the next escape hatch will meet them again.

A directive naming only punctuation fell through to the bare case and disabled every rule on the line — // stx-strict-ignore-next-line ((. That is allowPatterns: ['('] disabling almost every rule by accident (#1792 P3) reappearing in a new place. It now suppresses nothing: a directive that cannot be read is not permission.

A directive inside a string literal was honoured, so a script that merely printed one switched the validator off beneath it:

var help = "// stx-strict-ignore-next-line"
var f = document.getElementById('x')          // was exempt. Now is not.

Directives are read from a comments-only view of the source — the mirror image of #1911's finding that a prohibited API named in a comment is not a use of it.

What I did not do

Options 2 and 3 — file-scoped patterns and ignoreFiles — are not implemented. Both are coarser than the thing your report says it actually wants, which is the justification at the site, and a file-level exemption has the same "cannot tell debt from a decision" problem as the global one, just smaller. Say if you still want one for a case the pragma cannot reach.

allowPatterns' own doc comment now says it is rule-global and points at the pragma, since that is where someone looks first.

21 tests; full suite 14810 pass.

Sign in to comment on this issue.