feat: resolve form-submitter boundness in webjs check and make the residual loud - #1314
feat: resolve form-submitter boundness in webjs check and make the residual loud#1314vivek7405 wants to merge 20 commits into
Conversation
A `<button formaction=${fn}>` needs its enclosing form bound, because
`method="post"` and the enctype are supplied on the form's start tag and
a per-button action cannot retrofit them. Neither renderer can always
tell: SSR reads one template at a time and a component renders its own
template in a separate pass with no view of the host page, so a
submitter in a component is a cannot-tell, and cannot-tell has to bind
(refusing there would drop an isolated component from a page that still
returned 200).
What ships then is silent. The unbound form has no `method`, so the
browser submits it as a GET, the reserved identity rides the query
string, the action never runs, and the page re-renders with a 200. No
throw, no log, no 405.
`webjs check` has neither renderer's limit: it reads every template in
the app at once. The new `submitter-needs-bound-form` rule resolves the
enclosing form across module boundaries and transitively through
intermediate components, and stays silent on everything indefinite (a
tag with mixed call sites, no call site, a submitter in a bare `html`
helper, a multi-component file, a reference cycle), so it cannot
false-positive on the shapes the fallback exists to protect.
`matchClosingBrace` needed a fix to get there. It incremented the brace
depth at a `${` hole and then never decremented it, so depth could never
return to zero and a class body holding a template hole was unmatchable.
Every existing caller passes a masked source where holes are already
blanked, which is why the bug was invisible until one passed raw source.
A submitter bound inside a component whose host form is unbound is the one form-binding mistake that reaches production, and until now it was invisible everywhere. Measured rather than reasoned: the dominant case is not a 405. An unbound form has no `method`, so the browser submits it as a GET, the reserved identity rides the query string, and the page re-renders with a 200. An unbound `<form method="post">` actually WORKS, because the submitter's own name/value pair carries the identity into the body. Only an unparseable enctype reaches the 405. Two layers, both detect-only. In dev the client logs once at submit time. The client cannot answer boundness at reconcile time, but by submit time both the form and the body are in hand. It logs and never throws: the listener is delegated at the document, so a throw would escape uncaught AND abort before preventDefault, making dev behave differently from production. In production both server-visible fingerprints reach the onError hook with a code to group on. A page GET carrying `__webjs_action` in the query string is `WEBJS_FORM_SUBMITTED_AS_GET`; nothing else in the framework ever puts that reserved field in a url. A form body carrying no identity is `WEBJS_FORM_ACTION_MISSING`. The response never changes, because answering a GET differently on a query parameter would hand any visitor a way to turn any page into an error. Reports carry the field NAMES, which are template constants that say WHICH form posted nowhere, and never the values, which are user data. Both are deduplicated per process on method plus pathname with a 256-entry cap, since either is reachable by an unauthenticated request and an uncapped report would be a free amplifier into a paid APM sink. The other two 405s are left alone: a non-form POST is answered before the body is read and is the cheapest thing on the file to flood, and the GET-declared-action refusal already has its own check rule.
No in-repo app exercised the fallback. `/feedback/triage` keeps the form
and the submitter in ONE template, so the renderer resolves boundness in
a single scan and the cannot-tell path is never taken.
`/feedback/triage-split` is the other half: the form is bound in the page
and the submitter is bound one module over, in a component that renders
in its own pass. That is the shape SSR cannot judge and therefore binds
on faith, and the e2e submits it with JavaScript off.
It doubles as the counterfactual for the whole change. Make cannot-tell
refuse and the component renders empty (an SSR component error is
isolated), so the button leaves the DOM and the e2e goes red on a page
that still returns 200.
Also pins the streamed twin of the two KNOWABLE refusals. The streamed
state machine is a second implementation of the same scan, so a shape
both machines express has to be asserted through both entry points. A
cannot-tell cannot arise there at all: the component pass that seeds
'unknown' lives in injectDSD, which both entry points share and
`{ ssr: false }` never reaches.
|
Design rationale: why the check rule refuses to guess, and what it gives up The rule could have been much louder and I deliberately made it quiet. The failure it catches is real but rare; a false positive would land on a per-row button in a list or a button inside a component, which are the two most ordinary shapes there are. So every ambiguity resolves to silence. Concretely it goes quiet on: a tag rendered inside a bound form somewhere and an unbound one elsewhere, a tag with no call site anywhere in the app, a submitter in a bare The bare-helper carve-out is the one worth explaining. A fragment returned by a helper is rendered inside the CALLER's scan and inherits the caller's form scope, so a helper that looks form-less at rest can be perfectly bound at runtime. Attribution therefore requires the submitter to sit inside the single WebComponent class body of a file that registers exactly one tag. Anything looser and the rule starts guessing about a scope it cannot see. The recursion is deliberate rather than a one-level lookup. A page binding The cost of all this is that silence from the rule is not proof a form is bound, which is why the docs say so in as many words rather than overselling it. |
|
Correction: the production symptom is a silent 200, not the 405 I expected Worth recording because it changed the design. I went in expecting the unbound-host-form case to surface as the bind-nothing 405 in Running the shape through the real renderer, an unbound Two more outcomes fall out of that. An unbound So telemetry hung on the 405 alone would have reported close to nothing on the dominant case. That is why the primary signal here is the query-string GET instead: nothing in the framework ever puts that reserved field in a url, so it is an unambiguous fingerprint of a bound submitter submitted through an unbound form. It also settled the response question. The GET keeps returning 200 and rendering. Answering it differently because of a query parameter would hand any visitor a way to turn any page into an error, and this layer is diagnostics, not enforcement. |
|
Found on the way: matchClosingBrace could never close a brace containing a template hole This was not in the plan and I want it visible rather than buried in the diff.
The reason nobody hit it is that every existing caller passes a MASKED source, where Fixed with a small context stack: a |
|
Decision: the dev guard logs and never throws, and the browser test dispatches instead of clicking Two small calls worth writing down. The guard sits in The browser test dispatches a synthetic |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through the whole diff against the invariants. The shape of the change is right and the conservatism in the check rule is the correct instinct, but the central classification is wrong in a way that matters: the rule treats "unbound" as the defect when the real defect is "cannot deliver the identity". Those come apart exactly on <form method="post">, which is the case the PR body itself calls out as working. As written the rule fires on working code and tells you it submitted as a GET, which sends you hunting a symptom that is not happening. The docs inherit the same over-broad claim in four places while progressive-enhancement states it correctly, so the doc set currently contradicts itself.
The other one I would not ship is the telemetry dedupe. Keyed on the request pathname it is exhaustible: a dynamic route gives unbounded distinct keys, so a few hundred crafted requests fill the cap and silence both diagnostics for the process, and because the two signals share one key space a flood of either kills the other. The cap was argued only in the amplification direction; the silencing direction is the more likely one to bite.
Two smaller things: this is the first caller to hand raw source to matchClosingBrace, which does not lex regex literals, so a component with a brace in a regex loses its class body and the rule quietly stops covering that file. And the per-package AGENTS.md module maps for js-scan.js, form-dispatch.js, and router-client.js are all stale.
The matchClosingBrace fix itself is good and the never-decremented hole depth was a real latent bug.
The rule treated "unbound" as the defect, but the defect is "cannot
deliver the identity", and the two come apart on exactly the shape the
PR body already called out as working. An unbound `<form method="post">`
delivers: a submitter's identity rides its own name/value pair, so it
reaches the body and the dispatcher runs the action. Flagging it was a
false positive on working code, told through a message describing a GET
and a query string that never happen.
The scanner now reports, per unbound form, whether it would still carry
the identity (`method="post"` plus a parseable enctype), with null when a
hole makes the answer dynamic. The cross-module verdict fires only when
EVERY call site cannot deliver. The same-scan case still fires whatever
its method, because the renderer refuses that shape outright, and its
message now says so instead of describing the silent path.
Three more from the same read:
The dedupe key is the matched ROUTE, not the request pathname. Keyed on
the pathname a dynamic route yields unbounded distinct keys, so a few
hundred crafted urls filled the 256-entry cap and permanently silenced
both diagnostics, which is worse than the amplification the cap exists
to stop. The code is part of the key too, so one signal cannot silence
the other, and a slot is no longer spent when there is no sink and no dev
logger to receive the report. That last one needed `hasOnError` threaded
into the request context, because `reportError` is always a function and
no-ops internally, so it can never say nobody is listening.
The class body is located in the MASK and sliced out of the raw source at
the same offsets, so the brace matcher is never asked to lex raw source.
It does not handle regex literals, and a component carrying
`static re = /[{]/` produced zero class bodies, silently dropping the
cross-module half of the rule for that file.
A file that opens a `<form>` of its own can no longer attribute a
scope-none tag use to its component. A fragment built into a local and
spliced into that form inherits the splice point's scope, which is the
same reasoning the submitter half already applied to a bare helper.
Also brings the per-package module maps in packages/server/AGENTS.md and
packages/core/AGENTS.md up to date, and corrects the four doc surfaces
that had copied the over-broad "an unbound form submits as a GET" claim.
|
Regression evidence for the matchClosingBrace change, since it is shared code
Differential, old implementation against new, over all 1171 tracked source files:
Suite-level, |
Both are false positives in a rule whose whole design property is that it
never produces one.
`enctype` is an enumerated attribute whose MISSING and INVALID value
defaults are both `application/x-www-form-urlencoded`, so
`enctype="nonsense"` submits a perfectly parseable body and the identity
arrives. Testing it against an allowlist of the two parseable types
inverted that and called a working form broken. Only `text/plain`, the
third valid keyword, is a real loss, so the check is now a denylist of
one. The renderer refuses the wider set, but there it is a loud throw on
an attribute the author wrote deliberately, not a silent verdict about
someone else's form.
A template in a START-TAG hole is an attribute or property VALUE, so it
is handed to the receiving element and rendered in THAT component's own
pass. Scoring it by lexical nesting meant a submitter template passed as
`.tpl=${html`...`}` inside a form was recorded conclusively unbound,
while the renderer sees a cannot-tell there and binds. A hole in CHILD
position is rendered inline by the same scan and still inherits, which is
the pair that keeps this from being a blanket opt-out.
Three more from the same read:
The `opensForm` guard now covers the SUBMITTER half too, which the rule's
own description already claimed it did, and it tests the whole-file scan
rather than the class body. A module-scope helper can open a form the
class body never sees, and splicing into that is the same hole.
The same-scan message no longer asserts the page throws at render, which
the scan cannot guarantee for every shape it scores as unbound.
The enctype constant now has the drift guard its comment promised: the
test imports the renderer's own `PARSEABLE_ENCTYPES` and pins the
relationship, instead of hardcoding the same strings and claiming they
are kept in sync.
Doc surfaces: the dedupe key changed to code plus method plus route in
the previous commit and three surfaces still documented the pathname key,
which is the exact exhaustion that commit fixed. Four more still carried
the over-broad "an unbound form submits as a GET" claim, and three
silence lists were missing the two silences that landed with the
deliverability split.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Traced the previous fix commit and its blast radius. The deliverability split is the right model and the dedupe rekey holds up, but the commit introduced one new false positive and left another standing, which matters more than usual here because "never false-positive" is the property the rule is sold on.
The new one is the enctype allowlist. enctype defaults to urlencoded for a missing value AND an invalid one, so enctype="nonsense" submits a parseable body and works; only text/plain actually loses it. The standing one is a template passed as a component property inside a form: lexically nested, but rendered in the receiving component's own pass, so the renderer treats it as cannot-tell and binds while the scan called it conclusively unbound.
Two places where the code and its own documentation disagree. The rule description says a cannot-tell submitter is attributed only when its file opens no form of its own, and the submitter half never got that guard; the tag half's guard reads the class body while its comment says the file, so a module-scope helper opening a form slips through. Both are the same splice hole the guard was added to close.
The doc drift is the part I would not let ride: three surfaces still document the dedupe key the previous commit changed, and they describe the pathname keying whose exhaustibility was the reason for changing it. Four more still carry the unqualified GET claim that was corrected elsewhere in the same PR, so the doc set contradicts itself.
The previous commit marked a template in a start-tag hole `'none'`, which
silenced the false positive on a page and recreated it in a component
file. `'none'` is not a neutral "no enclosing form": it is specifically
the cannot-tell that the cross-module half ATTRIBUTES to the file's own
registered tag and then resolves against that tag's call sites. So a
component handing a submitter to `<my-thing .tpl=${...}>` had its button
resolved through the component's own call sites, when `<my-thing>` is
what decides where that button lands.
The two states are now distinct. `'none'` stays the attributable
cannot-tell; `'handed'` means some other element received this template,
so nothing in this file can speak for it. Every consumer already tested
for `'none'` exactly, so `'handed'` falls through to unknowable with no
other change, and there is a comment on `resolveTagScope` recording that
those exact tests are what make it work.
The same root cause reached the tag half, where a hyphenated tag inside a
property hole was recorded as this component's call site.
Both cases now have tests, and both go red with the fix neutered. The
previous test covered only the page variant, which is exactly why it
passed while the component variant was unguarded.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Scoped to the previous fix commit and traced what it can reach. The enctype half is right, the opensForm widening is strictly more conservative, and the doc surfaces are consistent now (no remaining pathname dedupe key, no remaining unqualified GET claim anywhere in the tree).
The start-tag-hole half is not right, and it fails in a way that is easy to miss because the test written for it passes. Marking a handed-off template 'none' reads as neutral but is not: 'none' is specifically the cannot-tell that the cross-module half attributes to the file's own registered tag and resolves against that tag's call sites. On a page there is no registered tag so the fix appears to work, and in a component file the same shape is resolved through the wrong component. The test only covers the page variant, so it never exercised the path the fix was for.
That makes it a live false positive on the branch, in a rule whose stated invariant in three places is that it can never produce one.
"A hole in a start tag is handed to the receiving element" was stated as
a universal and is not one. `<webjs-suspense .fallback=${html`...`}>` is
rendered INLINE, in the enclosing scan and with the enclosing form scope:
a TemplateResult is not serializer-safe and a hydration-time property is
too late for a streaming placeholder, so render-server renders it right
there and carries the HTML as `data-webjs-fallback`.
So a submitter in a fallback IS judged against the enclosing form, and
the renderer really does throw for an unbound one. Marking it handed off
made the same-scan half blind to a real render-time throw, which is the
one thing that half exists to pre-warn about.
The carve-out is one tag and one property name, matched case-sensitively
because a property name is a JS identifier rather than an attribute. It
is pinned by a DIFFERENTIAL test rather than an assertion about the
scanner alone, since the whole `'handed'` state is a claim about the
renderer: the test asserts the scanner's verdict AND that `renderToString`
throws for the fallback and does not for an ordinary property. An
assertion about the scanner by itself could have stayed green through
exactly this bug.
Also corrects the two places still documenting the removed universal (the
`scanHtmlFormScopes` contract and the packages/server module map), and
adds the handed-off silence to the four lists that enumerate them,
including the one `webjs check --rules` prints.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Scoped to the 'none' to 'handed' split. The split itself is right and no false positive is reachable from it: 'handed' is tested for nowhere, every consumer uses an exact comparison, and the state can only turn a verdict into unknowable. The two new tests do go red with the fix neutered.
What is wrong is the premise it was justified with. "A hole in a start tag is a value the receiving element places" is stated as a universal, and <webjs-suspense .fallback=${…}> is the exception: the renderer renders it inline with the enclosing form scope, because a TemplateResult is not serializer-safe and a hydration-time property is too late for a streaming placeholder. I checked this against the branch's own renderer rather than reading the code, and it throws for a submitter in a fallback inside an unbound form while the scanner now stays silent. The direction is silence rather than a false positive, so it is not dangerous, but it blinds the same-scan half to the exact case that half is for.
The rest is the documentation catching up: two places still state the inheritance rule the commit removed, and four silence lists (including the one the CLI prints) do not mention the silence it added.
The negative half asserted that rendering an ordinary `.prop` template succeeded, and that proves nothing: it succeeds for ANY scanner verdict. It also succeeds for a reason the surrounding comment got wrong. SSR does not hand that template to the receiving element at all. A template carrying a function cannot serialize, so `render-server.js` DROPS the whole property binding with a warning and emits nothing for it, and the submitter is never rendered or judged there. The hand-off that justifies `'handed'` happens in the browser, at hydration, which is still exactly what this scan cannot see. So the assertion is now the mechanism: the button is absent from the SSR output AND the drop warning fired. The previous version silenced that warning, which was the one observable saying what really happened. The suspense carve-out also changed the TAG half, where it can create violations rather than only silence them, and nothing covered it. A tag in a fallback is a real call site (rendered inline, inside the enclosing form) while one in an ordinary property hole is not, and both directions are now asserted at the scanner and through `webjs check`. Corrects the contract in `js-scan.js` and the packages/server module map to state that mechanism rather than the tidier version, and retitles the scanner test, which still carried the removed universal.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Scoped to the suspense carve-out. The carve-out is behaviourally right: the fallback is the only start-tag hole the renderer renders inline, the tag-name casing matches on both sides, and a before/after differential of the scanner over every tracked source file shows zero verdict changes in-repo.
What is wrong is the evidence around it. The differential's negative half asserts that rendering an ordinary property template succeeded, which is true for any scanner verdict, so it cannot fail for the reason the commit claims it protects against. It succeeds because a function-carrying template cannot serialize and SSR drops the binding entirely, so the submitter is never rendered or judged. That also makes the surrounding prose wrong: the hand-off is not SSR passing the template to the receiving element, it is the browser doing so at hydration. Same claim in two doc surfaces, and the test's own title still carries the universal the previous commit removed.
The gap I would not ship is the tag half. The carve-out flips a hyphenated tag inside a fallback from unknowable to a real call site, which can CREATE violations rather than only silence them, and nothing asserts it in either direction. That is the same tag-half blind spot the immediately preceding commit fixed, reappearing on the other side of the same mechanism.
The two use a denylist and an allowlist for the same attribute, which reads like an inconsistency waiting to be unified. It is not: they ask different questions. This rule asks whether the identity ARRIVES, and an invalid enctype falls back to application/x-www-form-urlencoded, so it does. The renderer asks whether the form does what the author wrote, and there the invalid value is the dangerous one: a typo'd `multipart/form-dat` falls back to urlencoded and silently drops every FILE from the submission, so a hard render error is the only signal the author gets. Unifying them would either teach the renderer to accept a typo that loses uploads, or make this rule report a working form as broken. The test asserts both sides so that change breaks something carrying its own reason.
"A TemplateResult is not serializer-safe" is false, and it had spread to
five places. Measured against the renderer: `.tpl=${html`<b>x</b>`}`
emits `data-webjs-prop-tpl="{…}"` with no warning, so a plain
TemplateResult rides the normal property path perfectly well. Only one
carrying a FUNCTION fails to serialize, which is a different case.
The real reason `<webjs-suspense .fallback>` is rendered inline is
timing: a custom-element property applies at hydration, far too late for
a placeholder that has to be in the first flushed bytes. That reason
stands on its own and is what the comments now say. Two of the five were
pre-existing (`render-server.js` where the claim originated, and the
packages/core module map); three arrived with this PR. Comment-only, no
behaviour change.
Also corrects the three places that outlived the mechanism they
described. The comment AT the hand-off site still said a start-tag
template is "rendered in THAT component's own pass", which is the
sentence this PR already corrected in the JSDoc 180 lines above and in
the packages/server module map, left in the one place a reader of
`handOff` actually looks. Its worked example described a cannot-tell that
never happens, since nothing renders that button at SSR at all.
And the three prose silence lists claimed the start-tag-hole silence with
no exception, while the rule demonstrably fires on a submitter inside a
`<webjs-suspense .fallback>`. Each now names that exception.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fifth and final delta round, scoped to the test-strengthening commit. No behaviour defect in it, and the two new assertions do go red when the carve-out is neutered, which is what the previous round asked for.
Everything found is prose that outlived the mechanism it described. The commit corrected the hand-off explanation in the JSDoc and the module map and left the copy at the code site, which is the one a reader of handOff actually opens, still describing SSR passing the template to the receiving element. The same sentence's worked example describes a cannot-tell that does not occur.
The one worth calling out beyond this PR: "a TemplateResult is not serializer-safe" is simply false, and it had propagated to five places, two of them pre-existing including the renderer comment where it started. A plain TemplateResult serializes and rides data-webjs-prop-*; only one carrying a function fails. The inline fallback is justified by timing alone, which is the half of the sentence that was always true.
Last, three prose silence lists say a start-tag hole is never judged, and the rule fires on a submitter in a suspense fallback. Each now names the exception.
|
Status: review cycle stopped at its round cap, PR deliberately left in draft Recording where this stands so the next reader does not have to reconstruct it. Six review rounds ran: one over the whole diff, then five delta rounds each scoped to the previous round's fix commit. Every round found something, all of it fixed, nothing rejected and nothing deferred. The chain hit the five-delta cap, so it stops here rather than running a sixth, and the last round's fixes ( What the rounds actually found is worth separating, because the raw count reads worse than the trajectory:
The code has been behaviourally stable for two rounds. What the last two found was my own evidence and prose: a differential whose negative half could not fail, a comment justifying the right verdict with the wrong mechanism, and a serializer claim that was simply false and had propagated to five places (two of them pre-existing in core, including the renderer comment where it started). The pattern behind most of it, since it is the useful thing to carry forward: on a rule this inference-heavy, a test that asserts my scanner's own output tends to agree with my mistake. Four separate findings were tests of mine that passed while the thing they claimed to protect was broken. The tests that survived scrutiny are the ones that assert against the REAL renderer and go red when the fix is neutered, which is now how the load-bearing ones are written. Regression evidence for the shared code is in a separate comment above and was re-run after every fix: rule outcomes repo-wide identical to CI has not completed a run on this branch: each push superseded the previous run before it finished. That needs to go green before this is mergeable, independent of the review state. |
Two leftovers from the same sweep. The rule description `webjs check --rules` prints is the fourth silence list, and the one a user reads straight out of the tool (it is also what the MCP check surface serves). It was the only one of the four still claiming a start-tag hole is never judged, while two tests assert the rule fires on a submitter and on a tag inside a `<webjs-suspense .fallback>`. And the clause added to AGENTS.md was spliced INTO the parenthetical rather than after it, so the trailing "a reference cycle" stopped reading as a silence-list item and started reading as something the rule judges. That is the opposite of the behaviour, which a test pins. Closing the list before stating the exception is what the other two surfaces already did.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Sixth round, scoped to the serializer-claim correction. I re-checked its central claims against the renderer rather than taking them on trust: a plain TemplateResult really does emit data-webjs-prop-tpl with no warning, one carrying a function really is dropped, and the isInlineStartTagHole carve-out matches the renderer on the spacing and casing variants I threw at it. So the correction is right.
Two leftovers from the sweep it was doing. The rule description that webjs check --rules prints is the fourth silence list, and the previous commit updated the three prose copies and missed it, so the one a user reads out of the tool is now the only one that does not name the exception two tests assert.
The other is worse than a miss. The clause added to AGENTS.md landed inside the parenthetical instead of after it, so the trailing "a reference cycle" now reads as something the rule judges rather than a silence, which is the reverse of the behaviour and of what the cycle test asserts. The two sibling surfaces avoided it by closing the list first.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Seventh round, scoped to the silence-list fix. Nothing to report: the rule text now names the suspense exception, the AGENTS.md list closes before the exception so the reference-cycle item reads as a silence again, and the two sibling surfaces already had it right.
Clean.
The rule reported a false positive on ordinary code, which is the one
thing its own description promises it never does.
The scan is lexical and treated EVERY `formaction=${…}` hole on a
button as an action binding. The renderer's test is narrower:
`isBoundFormAction` binds only when the value is a FUNCTION, so a plain
progressive-enhancement form whose buttons post to different `route.ts`
endpoints (`formaction=${'/api/items/' + id + '/archive'}`) is an
ordinary url attribute that renders and ships. The rule flagged it, and
told the author the page throws at render, which it does not.
The scanner now carries the hole's expression and the rule requires a
bare identifier imported from a `.server.{js,ts}` module, which is the
discriminator `form-action-not-a-get-action` already applies one rule
over. The counterfactual is a test: narrowing must not silence the real
binding, and it does not.
Two more from the same pass.
The dev client guard tested the enctype against the renderer's
ALLOWLIST, so it false-alarmed on `enctype="nonsense"`. That is the exact
inversion the scanner documents as forbidden and avoids, so the two
halves of one feature disagreed on the same input. It is a one-keyword
denylist now, and its message no longer claims a 405 for the JS path,
where the router posts FormData and the action runs.
And the bind-nothing 405 cannot report the unparseable-enctype variant:
that request arrives as `content-type: text/plain`, so it is answered by
the non-form 405 before its body is read. That early return is
deliberate, since buffering an attacker-chosen body to classify it is
what it exists to avoid, so the fix is to stop claiming the coverage.
Corrected in the comment and on the four doc surfaces that repeated it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff, and it earned its place: three real problems that six delta rounds all missed, because each of them was scoped to a fix and none of them re-read the rule against ordinary app code.
The one that matters is a false positive. The scan is lexical and treated every formaction=${…} hole as an action binding, while the renderer binds only a function. So a plain progressive-enhancement form whose buttons post to different route.ts endpoints was reported as broken, and told the author the page throws at render, which it does not. That is precisely what the rule's registered description promises never happens, and the "zero hits repo-wide" evidence did not catch it because this repo happens to contain no such form.
The other two are the same feature disagreeing with itself. The dev client guard tested the enctype against the renderer's allowlist rather than the one-keyword denylist the scanner uses and documents, so the check rule and the runtime guard gave opposite answers for enctype="nonsense". And the bind-nothing 405 cannot report the unparseable-enctype variant at all, because that request is answered by the non-form 405 before its body is read, so the comment and four doc surfaces claimed coverage that does not exist.
The previous narrowing was not narrow enough. "A bare identifier
imported from a `.server` module" still admits a url CONSTANT, and a
server module exports plenty of those, so `formaction=${ARCHIVE_URL}`
kept firing with the same wrong diagnosis the fix was supposed to
remove.
The rule now resolves the import to its target file and requires the
name to be exported as something PROVABLY callable: a function
declaration, or a const bound to an arrow or function expression. The
bias is deliberately the opposite of the sibling `'use server'` rules.
They tolerate a false negative, so anything ambiguous counts as
possibly-callable; here an ambiguous export firing is a false positive on
working code, so a factory (`export const go = cache(fn)`) stays silent
too.
That buys a new silence class, and it is real: a namespace import, a
default import, a barrel re-export, and any non-identifier expression
(`acts.publishDraft`, `this.act`) are bindings the rule now MISSES rather
than misreports. Silence is the safe direction, and all four are now
named in the rule's own description and in the three prose silence lists.
The fixtures needed real action modules and a `package.json` imports map
to prove any of this, since an unresolvable import is correctly silent
and a fixture without them would have passed no matter what the rule did.
Three more from the same pass. The client guard's denylist change shipped
with no test, so there is one now covering all three enctypes, plus a
drift guard pinning the client copy against the renderer's allowlist,
because there are three hardcoded copies of that keyword and the halves
disagreeing is exactly what went wrong. The guard's JSDoc still said the
action never runs, which its own new message contradicts for the JS path.
And the two module maps documented the pre-fix behaviour.
The previous commit's message said it had done this and it had not. The resolver narrowing added a real silence class (a url string or constant, a factory-produced export, a namespace or default import, a barrel re-export, a non-identifier expression), and only the internal JSDoc described it. The registered rule description matters most here, since that is what `webjs check --rules` and the MCP check surface print, and it is the copy a user reads when a shape they expected to be flagged is not.
The callable test accepted a bare `(` after the `=`, and a paren proves a
parenthesized EXPRESSION rather than a function. So the url constant came
straight back in its ordinary spellings: `= ('/api/x')`, and the
env-fallback `= (process.env.X || '/api/x')` that is how anyone actually
writes one in a `.server.ts`. That is the same false positive, on the
same shape, for the third time.
It is a small scanner now instead of a regex: skip an optional `async`,
then accept a `function` keyword, or a parenthesized parameter list that
REACHES a `=>`, or a single bare parameter that reaches one. The
committed test only covered the unparenthesized url, which is why the
gap survived.
The mirror error went with it. A single-parameter arrow (`export const
publishDraft = async fd => 1`) and a clause export of a local function
(`function f(){}; export { f }`) are provably callable and were being
dropped, so real bindings went silent. Both fire now, and a clause export
of a CONST still does not.
The whole table is one test, since two rounds of defects here were each a
single spelling: four url shapes and a factory stay silent, six function
shapes fire.
Also removes a stale JSDoc block the previous commit left orphaned onto
`const byAbs`, still describing the superseded discriminator, and
corrects the function's own doc, which claimed it needs no `appDir` while
its signature takes one and its body resolves imports with it.
The previous commit swapped a regex for a regex-plus-scanner hybrid and
the seam leaked. The lazy annotation group stopped at the FIRST `=`,
which for an arrow-TYPED const is the `=` inside the annotation's own
`=>`, so the scanner resumed on `>` and every branch missed. The old
single regex could backtrack past it; the hybrid cannot.
That silenced `export const publishDraft: (fd: FormData) =>
Promise<void> = async (fd) => …`, which is the spelling this repo's
derive-the-type rule pushes authors toward, so the shape most likely to
appear in a real app was the one the rule stopped seeing. The table
committed one commit earlier had thirteen rows and not one annotated
declaration, which is exactly why it did not catch this.
The annotation is walked with bracket depth now, stepping over `=>`, so
the assignment is found wherever it really is. Four annotated rows join
the table, including the annotated NON-callable, which must stay silent.
The `(?!from)` guard on the export clause never rejected anything: `\s*`
backtracks to zero width and the lookahead then reads the whitespace, so
`export { x } from './y'` matched. The common barrel answered correctly
by accident, having no local to find, but a module that re-exports a name
AND declares a same-named local read as exporting that local, which is
not the barrel silence the four doc lists promise. It tests the text
after the clause now, with a case covering exactly that collision.
Three shapes the annotation walker got wrong, all of them ordinary.
A `;` ended the declaration at ANY depth, but `;` is TypeScript's
canonical member separator, so `: ActionFn<{ id: string; title: string }>`
carries one inside its own brackets. The bail only makes sense at depth
zero.
The operator guard rejected an `=` preceded by `<` or `>`, to avoid
reading a comparison as the assignment. In annotation position those are
brackets the same loop has already consumed as brackets, so
`: Promise<void>= async () => 1` classified one character two
contradictory ways and went silent.
And the arrow branch demanded `=>` immediately after the parameter list,
which drops every arrow carrying a RETURN type. That is at least as
common as annotating the const, and the rows added last commit annotate
only the const, which is why the table did not catch it. The return type
is walked with bracket depth now, so a nested
`: Promise<ActionResult<Draft>>` is fine.
Nine rows join the table, including the three non-callables that must
stay silent (an annotated string, an indexed object type, and the
parenthesized env fallback), since every one of these fixes moves the
line between callable and not, and that line is where every defect in
this helper has been.
Five rounds each found one more spelling this resolver got wrong, one at a time. This sweeps the space instead: generics, a destructured parameter, a rest parameter, a multi-line parameter list, a named function expression, `export let`, a clause export of a local const, and an `as` alias all bind; a template-literal url, a number, and an expression result do not. All of them already behave correctly. The point is that the next refactor of the annotation walker has to keep them that way, since the line between callable and not is where every defect in this helper has been.
The previous commit fixed the depth-guarded separator bail on the declaration side and reproduced the un-guarded version on the return-type side, in the same hunk that advertised walking with bracket depth. So an arrow whose return type contains a comma or a `;` went silent, and `Promise<Record<string, string>>` is the fieldErrors shape the ActionResult envelope pushes authors toward. Any two-argument generic return type hit it. Same one-line guard as the declaration walk. The sweep added alongside it did not catch this, and the reason is worth recording: it swept SPELLINGS (generics, destructuring, rest params, multi-line, aliases) but every row happened to avoid a separator inside a return type, so the sweep sat entirely on the passing side of the boundary it claimed to pin. The rows added here put both separators in both annotation positions, and they go red when the guard is neutered, which the previous rows did not.
… rest
Two more grammar gaps, both silent misses.
The return-type walk broke on `=>` only at depth zero and otherwise fell
through, so a nested arrow's `>` was eaten by the closer branch and the
depth skewed: `: { a: () => void, b: string } =>` and the tuple form both
resolved as non-callable. It steps over a nested arrow whole now, which
is what the declaration walk already did.
And a generic arrow never reached the parameter-list branch, because that
branch required `(` immediately, so `async <T>(fd: T) => …` was dropped.
The optional type-parameter list is walked ahead of it.
The more useful change is how this was verified. Seven rounds have found
one more spelling each, and the sweeps I wrote kept sitting on the
passing side of the boundary they claimed to pin, so I mutation-tested
the resolver instead of guessing: break each guard, require the suite to
go red. Six of seven mutations are killed. Rows were added until the
generic-arrow branch was killed too, and the three the last round showed
were insensitive are replaced (one of them was a verbatim duplicate of a
row already in the sibling table).
The seventh mutation survives and stays that way on purpose: the `==` /
`!=` rejection is unreachable in valid TypeScript, so no test can pin it.
It is documented as defensive, since the input is a mask of arbitrary
source that may not be valid at all.
|
CI status: one green run, eight commits behind head, and why there is no newer one Recording this so nobody reads the green check on this PR as covering what is on the branch now. The only CI run this branch has is on I am deliberately not forcing a run. The workflow triggers only on What the green run does cover, since it is not nothing: the check rule, both telemetry signals, the dev client guard, the e2e route, and the first round of review fixes were all present at that commit. What it does NOT cover is 14 commits, and specifically the Locally those are covered: 3991 node tests with 6 failures that all reproduce with this branch's source reverted, the full browser suite green on all three engines, Bun parity green on both runtimes, the form-action e2e block green, and the check output repo-wide byte-identical to So: not mergeable yet, and the blocker is external. |
1 participant
Closes #1307
A
<button formaction=${fn}>submitter whose enclosing<form>cannot carry its identity posts nowhere, and today that reaches production silently whenever the two live in different modules.The residual, measured
The renderer's boundness inference is four-state and the
'unknown'branch binds anyway. That fallback is correct and stays: refusing on cannot-tell would reject a per-row button in a list and a button inside a component, and an SSR refusal is isolated per component, so production would return 200 with the button silently gone.What the residual actually does is narrower and quieter than "an unbound form is broken". Boundness and deliverability are two different questions and only both together are a defect. An unbound
<form method="post">WORKS: a submitter's identity rides its ownname/valuepair, which the browser submits for the pressed button alone, so it reaches the body and the dispatcher runs the action. What breaks is a form that sends nothing readable, and there the failure is a silent 200: with nomethod(ormethod="get") the browser submits a GET, the reserved identity rides the QUERY STRING, the action never runs, and the page simply re-renders. No throw, no log, no 405. Onlyenctype="text/plain"reaches the 405, becauseenctypefalls back to urlencoded for a missing AND an invalid value.That measurement is what the design is built on: telemetry hung on the 405 alone would report almost nothing, so the primary production signal is the query-string GET.
Three layers
webjs check: a newsubmitter-needs-bound-formrule. It has neither renderer's limit, reading every template in the app at once, so it resolves the enclosing form across module boundaries and transitively through intermediate components. It fires only when EVERY call site puts the tag in a form that cannot deliver.preventDefaultand make dev behave differently from production.WEBJS_FORM_SUBMITTED_AS_GETandWEBJS_FORM_ACTION_MISSINGreach the existingonErrorhook. Detect-only, so no status changes; field NAMES ride and values never do; deduplicated per process on the code, the method, and the matched ROUTE, so crafted urls on a dynamic route cannot exhaust the cap and silence the diagnostics.No file implementing the render-time refusal matrix changes. The cannot-tell fallback keeps binding,
assertSubmitterFormIsBoundkeeps its signature, and neither SSR state machine is edited.The rule is conservative by construction and is silent on: a tag with mixed call sites, a tag whose unbound host form still delivers, a form whose
method/enctypecomes from a hole, a tag with no call site, a submitter in a barehtmlhelper, a multi-component file, a file that opens a form of its own, a template passed as a component property, and a reference cycle. Silence is therefore not proof a form is bound, and the docs say so rather than overselling it.One fix outside the plan
matchClosingBraceincremented the brace depth at a${hole and never decremented it, so depth could never return to zero and a class body holding a template hole was unmatchable. Every existing caller passes a masked source where holes are already blanked, which is why it was invisible. Fixed with a context stack, with a regression test.Regression evidence (shared code)
matchClosingBraceis the one thing here that pre-existing code depends on (five check rules and the elision analyser reach it throughextractWebComponentClassBodies), so it was measured rather than reasoned about:extractWebComponentClassBodieson masked input, both mask modes: 4 diffs of 1185 bodies. Two are the bug-fix direction; two are phantom bodies read out ofclass X extends WebComponentwritten inside a quoted STRING in a test fixture, where neither answer is meaningful.check.jsreport the identical 61 violations, 0 added and 0 removed, so the phantom-body difference changes no verdict.examples/blog,website, and a freshly scaffolded app.Test plan
packages/server/test/check/submitter-needs-bound-form.test.js, 21 cases: the counterfactual plus every non-firing case abovepackages/server/test/scanner/html-form-scopes.test.js, 16 cases: the lexical half, the delivery matrix, thematchClosingBraceregression, and a drift guard against the renderer's ownPARSEABLE_ENCTYPESpackages/server/test/routing/form-dispatch.test.js: both codes, the unchanged 200 and 405, the dedupe, a 400-request dynamic-route flood, and no report for a non-form POSTpackages/core/test/rendering/form-action-binding.test.js: streamed twins of both knowable refusals; the existing cannot-tell tests pass untouchedtest/bun/form-action-dispatch.mjsextended with both codes and the dedupe, green on node 26.1.0 and bun 1.3.14/feedback/triage-split, the cannot-tell shape rendering correctly and submitting with JavaScript off (14/14 in the form-actions block)npm test: 3978 tests, 6 failures, all reproducing identically with this branch's source reverted (3 elision differential, 2 Bun listener, 1 blog HTTP integration)/, the touched docs pages,/ui, and/ui/button, with no broken modulepreload; scaffold generates, boots, and checks cleanDocs
AGENTS.mdinvariant 12,.agents/skills/webjs/SKILL.md, and the skill'smuscle-memory-gotchas.md,data-and-actions.md,built-ins.mdpackages/server/AGENTS.mdandpackages/core/AGENTS.mdmodule mapswebsite/app/docs/:server-actions,progressive-enhancement,troubleshooting(a new symptom entry, since this one had none),deploymentpackages/cli/templates/gallery/: the caveat beside the existing submitter note. The scaffold's copy of the skill is the repo-root one, copied at generate time, so there is no second file to edit.