ReviewOS

stacks/stx

strict mode: validateClientScript matches prohibited APIs inside comments, blocking failOnViolation

#1911
Closed glennmichael123 opened this 10 days ago ·
10 days ago

validateClientScript (dist/script-validation.js) scans client script bodies as raw text, so a prohibited API named in a comment is reported as a violation.

Measured on a real app (analyticshq, 39 .stx files, stx 0.2.171) with strict: { enabled: true }:

violations reported11
pointing at a comment9
pointing at real code2

Examples

All three of these are comments, and all three are reported:

✗ localStorage is prohibited (script line 3)
    3 | // useLocalStorage rather than a raw read: localStorage is on stx's prohibited list,

✗ location.replace() is prohibited (script line 20)
   20 | // window.location.replace(url), so the emitted behaviour is byte-for-byte what it

✗ window.location is prohibited (script line 13)
   13 | // back here with Back. This used to be a native window.location.replace on the grounds

Note what those comments say. Every one documents that the file uses the stx primitive instead — they exist because someone did the migration and explained it. The rule fires hardest on the code that already complied.

One more form: a stx comment is also matched, so it is not only JS comments.

Why this blocks the documented path

stx-standards 12.1 prescribes failOnViolation: false until the list is clean, then flipping it so regressions throw at render. That flip is currently unavailable to any app that documents its own migrations: it would throw on 9 comment lines here and only 2 real calls.

Worse, the natural workaround is to delete the explanatory comments, which is the opposite of what anyone wants.

Fix

stx already has stripCommentsAndLiterals in strip-literals.ts — the sweep handoff specifically calls out keeping one implementation of it. Running the rule table over the stripped text should close this. A literal "document.cookie" inside a string would stop matching too, which is also correct.

Same false-positive class, same toolchain, filed separately:

  • #1905 — codemod matched rules inside comments (fixed for codemod)

That fix landed in codemod.ts only; script-validation.js has the same blindness and is the more consequential of the two, because this one is wired into every render and is meant to become a hard gate.

For reference the 2 genuine violations it found are real and worth having — document.createElement('a') for a CSV download, and a bare setTimeout that should be useTimeout(). The signal is good; it is buried 5:1 in noise.

10 days ago

Fixed in dff7e81357. All three comment forms in your report are clean now, and real use still fails.

Same fix as #1905 in the codemod, reached through a different scanner and using the same function: stripCommentsAndLiterals, which the server-to-client bridge and stx typecheck already share. is blanked too, since you were right that it is not only JS comments.

The framing that made this worth prioritising is yours: every one of those 9 comments documented that the file uses the stx primitive instead. They exist because someone did the migration and explained it, so the rule was loudest on the code that had already complied — and failOnViolation could not be switched on at all, which is the whole point of the setting.

An existing test caught a regression in my fix

Quoting the offending line from the blanked copy prints:

3 | document.querySelector(      )

with the string literal replaced by spaces. #1836 added that quote so the line is findable by search rather than by counting, and blanking it defeats exactly that. Line numbers now come from the blanked copy (line-for-line with the source); the quote comes from the source.

6 tests; reverting the fix fails 4.

Sign in to comment on this issue.