diff --git a/.agents/skills/webjs/references/muscle-memory-gotchas.md b/.agents/skills/webjs/references/muscle-memory-gotchas.md index bb1cf6f2e..2c0f22fb1 100644 --- a/.agents/skills/webjs/references/muscle-memory-gotchas.md +++ b/.agents/skills/webjs/references/muscle-memory-gotchas.md @@ -43,6 +43,25 @@ const users = await getUsers(); There is no React `cache()`, `use()`, or `unstable_cache`. Caching is the `cache()` query helper, `export const revalidate` on a page, or `export const cache` on a GET action. +### A function in `
` is refused, not stringified + +Next binds a Server Action with `` and React serializes the binding into hidden fields. WebJs does not read that shape. A function interpolated into `action=` is a hard render error. + +The reason is a source leak. During SSR a `.server.ts` import is the ACTUAL function (the RPC stub exists only in the browser), and `action=` is an ordinary attribute hole, so stringifying it would write the function's body, secrets included, into the HTML every visitor downloads. The renderer throws instead, on the server and on the client, for `action=` and `formaction=` alike, in every hole shape (`action=${fn}`, `action="${fn}"`, `action="/x/${fn}"`). + +```ts +// WRONG: throws at render; it would have leaked the action's body. +import { submitFeedback } from '#modules/feedback/actions/submit-feedback.server.ts'; +html``; +// RIGHT: omit action entirely to post to the page's own url, and handle the +// submission in that page's `action` export. (Omit it rather than writing +// action="": the HTML spec requires a non-empty URL when the attribute is +// present, so the empty string is a conformance error.) +html``; +``` + +A string stays a string: `action="/search"` and `action=${'/search'}` are unchanged. Other attributes keep their existing stringify behaviour; only `action` and `formaction` are claimed. + ### `params` and `searchParams` are awaitable AND synchronously readable Next 15/16 made `params` / `searchParams` Promises. WebJs supports BOTH, so either muscle memory is correct. diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js new file mode 100644 index 000000000..a2c19df2d --- /dev/null +++ b/packages/core/src/form-action.js @@ -0,0 +1,56 @@ +/** + * Form-action attribute guard (#1154). + * + * Both SSR state machines and the client renderer import from here so the + * rule cannot drift between them. It already drifted once: the guard was + * added to the buffered `renderTemplate` alone, leaving `streamTemplate` + * stringifying the function. That second machine is reached only through + * `renderToStream(v, { ssr: false })`, which no page render uses (the server + * renders every page, Suspense included, via `renderToString`), so it was a + * public-API hole rather than a live page leak. Worth closing on its own + * terms, and worth noting as the reason the rule lives in one place. + */ + +/** + * Refuse to stringify a function interpolated into a form-action attribute. + * + * At SSR a `'use server'` import is the REAL server function (the RPC stub + * only exists in the browser), and a plain attribute hole commits via + * `String(val)`, so without this guard the action's SOURCE, secrets included, + * is serialized into the served HTML. The client renderer guards the same + * commit so a client re-render cannot write the source into the live DOM + * either. + * + * Name-based on purpose (`action`, plus `formaction`, the submit-button + * override, which leaks identically): a function stringified under these + * names is never useful on any tag, and other attributes keep today's + * stringify behaviour so the claim stays narrow. + * + * @param {unknown} val the hole's resolved value + * @param {string} attrName the attribute being committed (any case) + * @param {string} [tag] lowercased owner tag, for the error message + */ +export function assertNotFunctionActionAttr(val, attrName, tag) { + if (typeof val !== 'function') return; + if (!isFormActionAttr(attrName)) return; + throw new Error(formActionError(attrName, tag)); +} + +/** @param {string} attrName @returns {boolean} */ +function isFormActionAttr(attrName) { + const name = String(attrName).toLowerCase(); + return name === 'action' || name === 'formaction'; +} + +/** + * The refusal message. Deliberately never echoes the function's source, which + * is the very thing being withheld. + * @param {string} attrName + * @param {string} [tag] + */ +function formActionError(attrName, tag) { + return `[webjs] a function was interpolated into ${String(attrName).toLowerCase()}= ` + + `on <${tag || 'form'}>. Stringifying a function into HTML would leak its ` + + `source (a 'use server' action's body included) to every visitor, so this ` + + `is refused. Pass a URL string instead.`; +} diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 7065d8a1c..17e0f2b6e 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1,6 +1,7 @@ import { isTemplate, MARKER } from './html.js'; import { BINDING_PREFIXES, isBindingPrefix } from './binding-prefixes.js'; import { escapeAttr } from './escape.js'; +import { assertNotFunctionActionAttr } from './form-action.js'; import { isRepeat } from './repeat.js'; import { isUnsafeHTML, isLive, isKeyed, isGuard, isTemplateContent, isRef, isCache, isUntil, isAsyncAppend, isAsyncReplace, isWatch } from './directives.js'; import { Signal } from './signal.js'; @@ -671,10 +672,25 @@ function applyPart(part, value, _prev, allValues) { break; case 'attr': { if (value == null || value === false) part.el.removeAttribute(part.name); - else part.el.setAttribute(part.name, String(value)); + else { + // #1154: refuse to stringify a function into action=/formaction= + // (mirrors the SSR guard, so a client re-render cannot write a + // server action's source into the live DOM). + assertNotFunctionActionAttr(value, part.name, part.el.localName); + part.el.setAttribute(part.name, String(value)); + } break; } case 'prop': + // `.action=${fn}` on a NATIVE element is a leak too, not just the + // attribute form: `action` is a reflected IDL attribute, so assigning a + // function stringifies it into the element's own `action` content + // attribute in a real browser. Guarded on native elements only; a custom + // element's `.action` is an ordinary author-defined property that never + // reflects, and passing a function to one is legitimate. + if (!part.el.localName.includes('-')) { + assertNotFunctionActionAttr(value, part.name, part.el.localName); + } /** @type any */ (part.el)[part.name] = value; break; case 'bool': @@ -692,7 +708,10 @@ function applyPart(part, value, _prev, allValues) { const mp = /** @type {{ statics: string[], group: number[] }} */ (/** @type any */ (part)); let val = mp.statics[0]; for (let j = 0; j < mp.group.length; j++) { - val += String((allValues ? allValues[mp.group[j]] : value) ?? ''); + const piece = allValues ? allValues[mp.group[j]] : value; + // #1154: same function guard for each piece of a mixed attribute. + assertNotFunctionActionAttr(piece, part.name, part.el.localName); + val += String(piece ?? ''); val += mp.statics[j + 1] || ''; } part.el.setAttribute(part.name, val); diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index d4c5e81b5..24bf7576b 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -1,6 +1,7 @@ import { html, isTemplate } from './html.js'; import { BINDING_PREFIXES } from './binding-prefixes.js'; import { escapeText, escapeAttr } from './escape.js'; +import { assertNotFunctionActionAttr } from './form-action.js'; import { lookup, lookupModuleUrl, allTags } from './registry.js'; import { stylesToString, isCSS } from './css.js'; import { isRepeat } from './repeat.js'; @@ -323,11 +324,17 @@ async function renderTemplate(tr, ctx) { state = 'in-tag'; attrName = ''; } else { + // #1154: never stringify a function into action=/formaction= (it + // would serialize a server action's source into the served HTML). + assertNotFunctionActionAttr(val, attrName, currentTag); out += `"${escapeAttr(String(val ?? ''))}"`; state = 'in-tag'; attrName = ''; } } else if (state === 'attr-quoted' || state === 'attr-unquoted') { + // Same guard for a hole inside a quoted/unquoted value, the + // `action="${fn}"` and mixed `action="/x/${fn}"` shapes (#1154). + assertNotFunctionActionAttr(val, attrName, currentTag); out += escapeAttr(String(val ?? '')); } } @@ -1884,11 +1891,19 @@ async function streamTemplate(tr, ctx, controller) { state = 'in-tag'; attrName = ''; } else { + // The SAME guard as the buffered renderer above. This is a second, + // independent state machine, so it does not inherit that one: before + // this it stringified the function while the buffered path already + // refused it. Reached only via `renderToStream(v, { ssr: false })`, + // which no page render uses, so this was a public-API hole rather + // than a live page leak. + assertNotFunctionActionAttr(val, attrName, currentTag); buf += `"${escapeAttr(String(val ?? ''))}"`; state = 'in-tag'; attrName = ''; } } else if (state === 'attr-quoted' || state === 'attr-unquoted') { + assertNotFunctionActionAttr(val, attrName, currentTag); buf += escapeAttr(String(val ?? '')); } } diff --git a/packages/core/test/rendering/browser/form-action-guard.test.js b/packages/core/test/rendering/browser/form-action-guard.test.js new file mode 100644 index 000000000..6918ef7ed --- /dev/null +++ b/packages/core/test/rendering/browser/form-action-guard.test.js @@ -0,0 +1,87 @@ +/** + * The form-action guard, in a REAL browser (#1154). + * + * The node-side coverage runs under linkedom, which does not implement IDL + * attribute reflection: there, `form.action = fn` sets a plain JS property and + * the `action` content attribute stays untouched, so the leak the property + * binding causes is INVISIBLE to that layer. In a real browser `action` is a + * reflected IDL attribute, so the assignment stringifies the function into the + * element's own markup, which is the actual leak. That difference is exactly + * why this file exists rather than trusting the linkedom tests. + * + * It also pins the real-DOM behaviour of the attribute path: a refused render + * must leave the previously-rendered value in place, never a half-written one. + */ + +import { html } from '../../../src/html.js'; +import { render } from '../../../src/render-client.js'; + +import { assert } from '../../../../../test/browser-assert.js'; + +// The sentinel lives inside the function body, so it appears in the output only +// if the function was stringified. +async function secretAction(formData) { + const CONNECTION = 'postgres://user:BROWSER_LEAK_MARKER@host/db'; + return CONNECTION; +} + +function mount() { + const host = document.createElement('div'); + document.body.appendChild(host); + return host; +} + +suite('form-action guard in a real browser', () => { + + test('action=${fn} throws and writes nothing to the document', () => { + const host = mount(); + let threw = null; + try { render(html`
`, host); } + catch (e) { threw = e; } + assert.ok(threw, 'render must throw'); + assert.ok(/function was interpolated into action=/.test(threw.message), threw.message); + assert.ok(!document.body.innerHTML.includes('BROWSER_LEAK_MARKER'), 'no source in the document'); + }); + + test('.action=${fn} property binding cannot reflect the source into the attribute', () => { + // THE case linkedom cannot see. Without the guard, `el.action = fn` here + // reflects the stringified function into the live `action` attribute. + const host = mount(); + let threw = null; + try { render(html`
`, host); } + catch (e) { threw = e; } + assert.ok(threw, 'render must throw'); + assert.ok(!document.body.innerHTML.includes('BROWSER_LEAK_MARKER'), 'no source in the document'); + const form = host.querySelector('form'); + if (form) { + assert.ok(!String(form.getAttribute('action') || '').includes('BROWSER_LEAK_MARKER'), 'no source in the action attribute'); + assert.ok(!String(form.action || '').includes('BROWSER_LEAK_MARKER'), 'no source on the action property'); + } + }); + + test('a refused re-render leaves the previously rendered action intact', () => { + const host = mount(); + const tpl = (a) => html`
`; + render(tpl('/submit'), host); + const before = host.querySelector('form').getAttribute('action'); + assert.equal(before, '/submit'); + + let threw = null; + try { render(tpl(secretAction), host); } catch (e) { threw = e; } + assert.ok(threw, 'the re-render must throw'); + assert.equal(host.querySelector('form').getAttribute('action'), '/submit', 'prior value survives'); + assert.ok(!document.body.innerHTML.includes('BROWSER_LEAK_MARKER'), 'no source in the document'); + }); + + test('a real form still submits to a string action', () => { + // The guard must not disturb the ordinary case: a string action has to + // survive into a working, submittable form. + const host = mount(); + render(html`
`, host); + const form = host.querySelector('form'); + assert.equal(form.getAttribute('action'), '/feedback'); + assert.equal(form.method, 'post'); + assert.equal(new FormData(form).get('msg'), 'hi'); + }); + +}); diff --git a/packages/core/test/rendering/form-action-attr-guard-client.test.js b/packages/core/test/rendering/form-action-attr-guard-client.test.js new file mode 100644 index 000000000..646b56ea4 --- /dev/null +++ b/packages/core/test/rendering/form-action-attr-guard-client.test.js @@ -0,0 +1,87 @@ +// #1154, client half: the browser renderer refuses the same commit, so a +// client re-render can never write a function's source into the live DOM +// (`applyPart`'s 'attr' and 'attr-mixed' cases). Runs under linkedom. +import { test, before } from 'node:test'; +import assert from 'node:assert/strict'; +import { parseHTML } from 'linkedom'; + +before(() => { + const { window } = parseHTML(''); + globalThis.document = window.document; + globalThis.DocumentFragment = window.DocumentFragment; + globalThis.Node = window.Node; + globalThis.Element = window.Element; + globalThis.Comment = window.Comment; + globalThis.Text = window.Text; + globalThis.NodeFilter = window.NodeFilter; + globalThis.HTMLElement = window.HTMLElement; +}); + +let html, render; +before(async () => { + ({ html } = await import('../../src/html.js')); + ({ render } = await import('../../src/render-client.js')); +}); + +async function fakeAction() { const S = 'CLIENT_SECRET'; return S; } + +// NOTE on what these DON'T assert: `createInstance` applies every part before +// it calls `container.replaceChildren(...)`, so a throwing part leaves `host` +// empty no matter what. An `assert.ok(!host.innerHTML.includes(SECRET))` here +// would pass unconditionally and prove nothing. The load-bearing assertion is +// the throw itself, plus the SECOND-render cases below, where the host DOES +// hold a live form and a leak would therefore be observable. +test('client render of action=${fn} throws', () => { + const host = document.createElement('div'); + assert.throws( + () => render(html`
`, host), + /function was interpolated into action=/, + ); +}); + +test('client render of mixed action="/x/${fn}" throws', () => { + const host = document.createElement('div'); + assert.throws( + () => render(html`
`, host), + /function was interpolated into action=/, + ); +}); + +// Re-render over an ALREADY-MOUNTED form. Here the host really does hold a live +// element, so if the guard let the value through, the source would be sitting +// in the DOM and this would catch it. +test('re-render swapping a string action for a function throws, live DOM stays clean', () => { + const host = document.createElement('div'); + const tpl = (a) => html`
`; + render(tpl('/ok'), host); + assert.equal(host.querySelector('form').getAttribute('action'), '/ok'); + assert.throws(() => render(tpl(fakeAction), host), /function was interpolated into action=/); + assert.ok(!host.innerHTML.includes('CLIENT_SECRET'), 'live DOM must not carry the source'); + assert.equal(host.querySelector('form').getAttribute('action'), '/ok', 'the prior value must survive'); +}); + +// `.action=${fn}` is a PROPERTY binding, a different commit site from the +// attribute one. `action` is a reflected IDL attribute, so assigning a function +// writes its source into the element's own `action` in a real browser. +test('client .action=${fn} property binding on a native form throws', () => { + const host = document.createElement('div'); + assert.throws( + () => render(html`
`, host), + /function was interpolated into action=/, + ); +}); + +test('a custom element keeps accepting a function on .action', () => { + // Not a reflected IDL attribute, just an author-defined property, so passing + // a function is legitimate and must not be refused. + const host = document.createElement('div'); + render(html``, host); + assert.equal(typeof host.querySelector('my-widget').action, 'function'); +}); + +test('string action still renders on the client', () => { + const host = document.createElement('div'); + render(html`
`, host); + const form = host.querySelector('form'); + assert.equal(form.getAttribute('action'), '/submit'); +}); diff --git a/packages/core/test/rendering/form-action-attr-guard.test.js b/packages/core/test/rendering/form-action-attr-guard.test.js new file mode 100644 index 000000000..220d0c888 --- /dev/null +++ b/packages/core/test/rendering/form-action-attr-guard.test.js @@ -0,0 +1,138 @@ +// #1154: a function interpolated into a form-action attribute must throw, +// never stringify. At SSR a `'use server'` import is the REAL function, so +// `String(fn)` would serialize the action's source (secrets included) into +// the served HTML. Covers every hole shape that used to leak (unquoted, +// quoted, mixed, and `formaction` on a submit button), plus the byte-identical +// passthrough for string-valued action attributes. +import { test, before } from 'node:test'; +import assert from 'node:assert/strict'; + +let html, renderToString, renderToStream; +before(async () => { + ({ html } = await import('../../src/html.js')); + ({ renderToString, renderToStream } = await import('../../src/render-server.js')); +}); + +async function drain(stream) { + let out = ''; + for await (const c of stream) out += typeof c === 'string' ? c : new TextDecoder().decode(c); + return out; +} + +// The secret sentinel must never appear in any output, thrown or not. +const SECRET = 'postgres://user:SECRET@host/db'; +async function leaky(input) { const conn = SECRET; return { success: true, conn, input }; } + +test('unquoted action=${fn} throws instead of leaking source', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into action=/, + ); +}); + +test('quoted action="${fn}" throws identically', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into action=/, + ); +}); + +test('mixed hole action="/x/${fn}" throws', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into action=/, + ); +}); + +test('formaction=${fn} on a submit button throws', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into formaction=/, + ); +}); + +test('no thrown message ever carries the function source', async () => { + for (const tpl of [ + html`
`, + html`
`, + ]) { + let msg = ''; + try { await renderToString(tpl, { ssr: true }); } catch (e) { msg = String(e && e.message); } + assert.ok(msg.length > 0, 'render must throw'); + assert.ok(!msg.includes('SECRET'), 'error message must not embed the source'); + } +}); + +test('string-valued action renders byte-identically to before', async () => { + assert.equal( + await renderToString(html`
`, { ssr: true }), + '
', + ); + assert.equal( + await renderToString(html`
`, { ssr: true }), + '
', + ); + assert.equal( + await renderToString(html`
`, { ssr: true }), + '
', + ); +}); + +// The STREAMING renderer is a second, independent state machine, missed by the +// first pass of this fix. It is reached only through +// `renderToStream(v, { ssr: false })`, which no page render uses (the server +// renders every page, Suspense included, via `renderToString`), so this was a +// public-API hole and not a live page leak. It gets its own coverage rather +// than being assumed to inherit the buffered renderer's guard. +test('the streaming renderer refuses the same function (ssr:false path)', async () => { + await assert.rejects( + () => drain(renderToStream(html`
`, { ssr: false })), + /function was interpolated into action=/, + ); +}); + +test('the streaming renderer never emits the source', async () => { + let out = ''; + try { + out = await drain(renderToStream(html`
`, { ssr: false })); + } catch { /* expected */ } + assert.ok(!out.includes('SECRET'), 'streamed output must not carry the function source'); +}); + +// The unquoted shape lands in the streaming machine's `after-eq` branch; the +// quoted and mixed shapes land in its SEPARATE `attr-quoted`/`attr-unquoted` +// branch. Without these two, that second branch could be deleted outright and +// the suite would stay green, which is the same guard-asymmetry this whole +// change exists to prevent, just moved into the test layer. +test('the streaming renderer refuses a quoted hole', async () => { + await assert.rejects( + () => drain(renderToStream(html`
`, { ssr: false })), + /function was interpolated into action=/, + ); +}); + +test('the streaming renderer refuses a mixed hole', async () => { + await assert.rejects( + () => drain(renderToStream(html`
`, { ssr: false })), + /function was interpolated into action=/, + ); +}); + +test('the streaming renderer refuses formaction', async () => { + await assert.rejects( + () => drain(renderToStream(html``, { ssr: false })), + /function was interpolated into formaction=/, + ); +}); + +test('streaming keeps string-valued actions working', async () => { + const out = await drain(renderToStream(html`
`, { ssr: false })); + assert.match(out, /action="\/submit"/); +}); + +test('functions in OTHER attributes keep the existing stringify behaviour', async () => { + // Narrow claim: only action/formaction throw. Anything else is unchanged + // (arguably also a bug, but out of #1154's scope by design). + const out = await renderToString(html`
`, { ssr: true }); + assert.ok(out.startsWith('
Getting Started to scaffold an app, Architecture for the execution model in depth, and Progressive Enhancement for the design posture that replaces the Client Component habit.

`; diff --git a/website/app/docs/troubleshooting/page.ts b/website/app/docs/troubleshooting/page.ts index 7c564cd20..583d3ff09 100644 --- a/website/app/docs/troubleshooting/page.ts +++ b/website/app/docs/troubleshooting/page.ts @@ -41,6 +41,11 @@ export default function Troubleshooting() {

Cause: WebJs instruments the native DOM mutation methods on every light-DOM host to keep the slot API live, and it installs those interceptors as own properties ON THE ELEMENT INSTANCE. An own instance property shadows a method defined on the class prototype, so the framework's interceptor wins and your same-named class method is never reached. The instrumented names are append, prepend, before, after, replaceWith, replaceChildren, remove, appendChild, insertBefore, removeChild, and replaceChild. TypeScript does not catch it because a shorter override (zero or one argument) is assignable to the native signature.

Fix: rename the member to a non-native name (appendRow(), removeItem()) and update its call sites. Only these native mutation members are affected; render() and the lifecycle hooks are meant to be overridden. The no-shadowed-native-member check rule catches this ahead of time (it flags only a real instance method, never a static member, a nested-object property, or a static shadow = true component, whose native shadow slots are not instrumented). See Components.

+

An error saying a function was interpolated into action=

+

Symptom: a page or component throws at render with a function was interpolated into action= on <form>, usually right after binding a server action to a form the way Next.js does it.

+

Cause: during SSR an imported 'use server' action is the REAL function (the RPC stub exists only in the browser), and action= is an ordinary attribute hole, so stringifying it would write the action's whole body, including anything its closure shows such as a connection string, into the HTML every visitor downloads. WebJs refuses rather than emitting it. The same applies to formaction= on a submit button, to every hole shape (action=\${fn}, action="\${fn}", action="/x/\${fn}"), and to the .action=\${fn} property form on a native form, where the reflected IDL attribute would carry the source into the DOM.

+

Fix: omit action so the form posts to the page's own URL, and handle the submission in that page's action export (write <form method="post"> rather than action="", which the HTML spec treats as a conformance error since the attribute must be a non-empty URL when present). A string action is unaffected. See Server Actions and Progressive Enhancement.

+

An error saying static properties is no longer supported

Symptom: a component throws at construction with static properties is no longer supported. Declare reactive properties via the factory instead.

Cause: the class body has a hand-written static properties = { ... } field. WebJs declares reactive properties only through the WebComponent({ ... }) base-class factory now, and the runtime throws on a direct static properties.