Skip to content

fix: a function in an on* attribute leaks the action source, like action= did #1170

Description

@vivek7405

Problem

A function interpolated into an on* inline-handler attribute is stringified into the served HTML, exactly like the <form action=${fn}> leak that #1154 / #1167 closed. If that function is an imported 'use server' action, its whole body ships to every visitor.

Reproduced against packages/core/src on the #1167 branch:

async function deleteTodo(id) { const DB_SECRET = 'postgres://user:pw@host/db'; return { ok: true }; }
await renderToString(html`<button onclick=${deleteTodo}>x</button>`, { ssr: true });
<button onclick="async function deleteTodo(id) { const DB_SECRET = 'postgres://user:pw@host/db'; return { ok: true }; }">x</button>

onsubmit=${fn} and camelCase onClick=${fn} leak identically, verified for all three. The correct @click=${fn} form is unaffected (an event listener, dropped at SSR).

It also fails silently. A browser parses that attribute as an inline handler body containing only a function DECLARATION, so nothing happens on click and the author gets no signal that anything is wrong. There is no webjs check rule for a hole in an on* attribute and no dev warning.

Severity, stated honestly. This is a sibling of #1169 rather than a repeat of #1167, and it is lower severity than the action= leak. The counter-argument raised in review is correct and worth carrying: React's onClick={fn} takes a CLIENT handler, whose source already ships to the browser inside its component module, so nothing is disclosed in the ordinary case. The catastrophic case needs an author to bind an imported 'use server' action to an inline on* attribute, which is not a Next idiom the way <form action={serverAction}> is. Still a real source-disclosure path with nothing guarding it.

Design / approach

The guard added by #1167 lives in packages/core/src/form-action.js and is name-based on purpose: isFormActionAttr matches action / formaction only, so every other attribute keeps today's stringify behaviour. #1167 pins that scope boundary deliberately with a test (functions in OTHER attributes keep the existing stringify behaviour), so widening the predicate means updating that test rather than working around it.

Two directions, and they are not exclusive:

  1. Extend the refusal to on* names, in the same isFormActionAttr (rename it) so all four commit sites inherit it at once. Consistent with the existing guard and cheap. The error message should say to use @click=${fn}, since that is what the author meant.
  2. A webjs check rule, catching a function-valued hole in an on* attribute statically. Better authoring feedback than a render-time throw, but it cannot see a value that is only function-typed at runtime.

Option 1 is the one that actually closes the disclosure; option 2 is the nicer teaching surface. Note that unlike action=, an on* attribute with a STRING value is legitimate HTML that some authors do write, so the refusal must key on the value being a function, which is what carriesFunction already does.

Consider doing this together with #1169 so the three leak paths (attribute, reflected prop, inline handler) get one coherent answer rather than three differently-shaped ones.

Implementation notes (for the implementing agent)

Where to edit:

  • packages/core/src/form-action.js: isFormActionAttr() L87-91 is the name predicate, assertNotFunctionActionAttr() L35-39 the entry point, formActionError() L99-104 the message. carriesFunction() L65-72 already handles the array-wrapped case and a cyclic array, reuse it unchanged.
  • The four commit sites all route through that one assert, and must stay that way (they drifted apart three times during fix: refuse a function in form action=, it leaked server action source #1167):
    • packages/core/src/render-server.js L341 (quoted attr) and L349 (unquoted) in the buffered machine renderTemplate
    • packages/core/src/render-server.js L1928 / L1934, the same two branches in the streaming machine streamTemplate
    • packages/core/src/render-client.js applyPart L673 case 'attr', L684 case 'prop', L696 case 'bool', L713 case 'attr-mixed'
  • If adding a check rule: packages/server/src/check.js (rule list; there is currently no on* rule of any kind).

Landmines:

Invariants to respect:

  • AGENTS.md invariant 4 (event / property / boolean holes must be unquoted) is the reason the quoted form is refusable at all.
  • AGENTS.md invariant 1 (server-only code never reaches the browser) is the property being defended.

Tests + docs surfaces:

  • Unit, both SSR machines and the client: extend packages/core/test/rendering/form-action-attr-guard.test.js and form-action-attr-guard-client.test.js. The scope-boundary tests there (functions in OTHER attributes keep the existing stringify behaviour, and its streaming twin) assert the CURRENT narrow claim and will red by design; update them rather than deleting them, so the new boundary stays pinned.
  • Bun parity: test/bun/form-action-guard.mjs carries a refused table and an allowed carve-out table; add rows to both.
  • Browser: packages/core/test/rendering/browser/form-action-guard.test.js if the reflected-DOM path is involved.
  • Counterfactual: fix: refuse a function in form action=, it leaked server action source #1167 established that every guard call site must map to a test that reds when that site alone is reverted. Hold the new clauses to the same bar.
  • Docs: website/app/docs/troubleshooting/page.ts (the action= entry is the model), .agents/skills/webjs/references/muscle-memory-gotchas.md, and website/app/docs/components/page.ts, whose binding-prefix section already carries an action= carve-out note that would need widening.

Acceptance criteria

  • A function in onclick=, onsubmit=, and camelCase onClick= is refused on the buffered SSR machine, the streaming machine, and the client renderer
  • An unquoted @click=${fn} event listener still works, on every renderer
  • A string-valued on* attribute renders byte-identically to today
  • The thrown message never embeds the source it is withholding
  • A counterfactual proves each new call site's test reds when that site alone is reverted
  • Bun parity row added to test/bun/form-action-guard.mjs and green on Node and Bun
  • The fix: refuse a function in form action=, it leaked server action source #1167 scope-boundary tests are updated to describe the new boundary, not deleted
  • Docs updated on every surface listed above

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions