From 429e35e38d55d43f947e91c9493a9d685682aba6 Mon Sep 17 00:00:00 2001 From: Vivek Date: Sat, 1 Aug 2026 02:17:04 +0530 Subject: [PATCH 01/38] feat(core): bind a server action into
The renderers now treat an unquoted action= hole on a as a binding rather than a value: they resolve the action's identity, omit the action attribute so the form posts to the page's own url, force method="post" and an enctype, and emit one hidden field carrying the identity. Every other function-in-an-action shape still refuses, so there is exactly one way to write this and no silently-broken near-miss. formaction= on a submit button stays refused: a per-submitter identity would have to ride the submitter's own name/value pair, which is also how a multi-button form tells its buttons apart. The forcing decision waits for the tag's `>` rather than firing at the hole, because an attribute written after the hole still counts and a hole-provided method=${m} cannot be read off the source template at all. The scan is a real tokenizer, since a regex reports a method attribute for a form whose data-note happens to contain the text. --- packages/core/index-browser.js | 4 + packages/core/index.js | 4 + packages/core/src/form-action.js | 355 ++++++++++++++++-- packages/core/src/render-client.js | 14 +- packages/core/src/render-server.js | 75 +++- .../rendering/form-action-attr-guard.test.js | 29 +- .../rendering/form-action-binding.test.js | 206 ++++++++++ 7 files changed, 638 insertions(+), 49 deletions(-) create mode 100644 packages/core/test/rendering/form-action-binding.test.js diff --git a/packages/core/index-browser.js b/packages/core/index-browser.js index 8ae6afe8e..ec8e45459 100644 --- a/packages/core/index-browser.js +++ b/packages/core/index-browser.js @@ -31,6 +31,10 @@ export { escapeText, escapeAttr } from './src/escape.js'; export { notFound, redirect, forbidden, unauthorized, isNotFound, isRedirect, isForbidden, isUnauthorized } from './src/nav.js'; export { cspNonce } from './src/csp-nonce.js'; export { asset } from './src/asset-url.js'; +// Form actions (#1155). The field name is shared with the server dispatcher; +// `setFormActionResolver` is server-only wiring and is stripped here, because +// a browser stub carries its own identity and needs no resolver. +export { FORM_ACTION_FIELD, FORM_ACTION_ID_KEY } from './src/form-action.js'; export { repeat, isRepeat } from './src/repeat.js'; export { Suspense, isSuspense } from './src/suspense.js'; export { connectWS } from './src/websocket-client.js'; diff --git a/packages/core/index.js b/packages/core/index.js index f95a66ffd..ed8e522d1 100644 --- a/packages/core/index.js +++ b/packages/core/index.js @@ -16,6 +16,10 @@ export { escapeText, escapeAttr } from './src/escape.js'; export { notFound, redirect, forbidden, unauthorized, isNotFound, isRedirect, isForbidden, isUnauthorized } from './src/nav.js'; export { cspNonce, setCspNonceProvider } from './src/csp-nonce.js'; export { asset, setAssetUrlProvider } from './src/asset-url.js'; +// Form actions (#1155). `FORM_ACTION_FIELD` is the hidden field both renderers +// emit and the server's form dispatcher reads; `setFormActionResolver` is the +// server-only wiring that maps a real action function back to its identity. +export { FORM_ACTION_FIELD, FORM_ACTION_ID_KEY, setFormActionResolver } from './src/form-action.js'; export { repeat, isRepeat } from './src/repeat.js'; export { Suspense, isSuspense } from './src/suspense.js'; export { connectWS } from './src/websocket-client.js'; diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js index 62d618d5d..e1c379001 100644 --- a/packages/core/src/form-action.js +++ b/packages/core/src/form-action.js @@ -1,44 +1,155 @@ import { isBindingPrefix } from './binding-prefixes.js'; +import { escapeAttr } from './escape.js'; /** - * Form-action attribute guard (#1154). + * Form actions: binding a `'use server'` action straight into a form (#1155), + * and the guard that refuses every OTHER way a function could reach the markup + * (#1154). + * + * ONE supported shape, and it is the shape a Next-trained author already + * writes: + * + * import { submitFeedback } from '#modules/feedback/actions/submit.server.ts'; + * html` ...
` + * + * The renderers do not stringify that function. They resolve its IDENTITY + * (`/`, the same naming the RPC endpoint uses), drop + * the `action` attribute so the form posts to the page's own url, force the + * attributes the submission needs (`method="post"`, `enctype`), and emit one + * hidden field carrying the identity. The server resolves that field back to + * the function and runs it. Nothing about the action's source reaches the + * browser, and the form works with JS off because it is an ordinary HTML + * submission. + * + * Every other function-in-an-action shape stays REFUSED, because it would + * stringify the function's body into the served HTML. At SSR a `'use server'` + * import is the REAL server function (the RPC stub only exists in the + * browser), so a plain `String(val)` commit serializes the action's body, + * secrets included, to every visitor. The refusal list is deliberately wider + * than the leak: a quoted `action="${fn}"`, `?action=${fn}`, an array holding + * a function, `formaction=` anywhere, and `action=` on a tag that is not a + * `
` all refuse, so there is exactly one way to write this and no + * silently-broken near-miss. + * + * `formaction=${fn}` on a submit button is refused rather than supported. It + * is legal HTML, and a per-submitter identity could ride the submitter's own + * `name`/`value` pair, but that pair is also how a multi-button form tells its + * buttons apart, so supporting it would either clobber an author's own + * `name=` or need a second parallel wire. Refusing states the boundary; a form + * per action is the shape to write instead. * * Both SSR state machines and the client renderer import from here so the - * rule cannot drift between them. Each renderer commits attribute, boolean and - * property holes on separate branches, so the rule has more call sites than it - * looks like from any one file, and they are easy to change one at a time. - * That is why the predicate lives here rather than at each site. - * - * Two entry points, and the difference between them is the point: - * `assertNotFunctionActionAttr` is name-based, because an ATTRIBUTE is - * stringified into the markup on whatever tag it appears. - * `assertNotFunctionReflectedActionProp` is name-and-tag-based, because a - * PROPERTY only reaches the markup where the DOM reflects it. Using the first - * on the property path refused `
`, which never leaked. + * rules cannot drift between them. Each renderer commits attribute, boolean + * and property holes on separate branches, so they have more call sites than + * it looks like from any one file and are easy to change one at a time. That + * is why the predicates live here rather than at each site. * * The second SSR machine (`streamTemplate`) is reached only through * `renderToStream(v, { ssr: false })`, which no page render uses: the server - * renders every page, Suspense included, via `renderToString`. Guarding it is - * about the public API surface rather than a live page leak, so do not infer - * from a bug there that pages were affected. + * renders every page, Suspense included, via `renderToString`. It is kept in + * lockstep for the public API surface rather than for a live page. */ /** - * Refuse to stringify a function interpolated into a form-action attribute. + * The hidden field carrying a bound action's identity. The server's form + * dispatcher reads it off the submitted `FormData`; nothing else is + * authoritative, so a form that lost this field is never dispatched by + * guesswork. + */ +export const FORM_ACTION_FIELD = '__webjs_action'; + +/** + * The property a generated client RPC stub carries its own identity on. + * Written by the generated stub source (`packages/server/src/actions.js`), read + * here. A plain string property rather than a symbol because the stub is + * generated source that only imports the framework's runtime helpers. + */ +export const FORM_ACTION_ID_KEY = '$$webjsAction'; + +/** @type {((fn: Function) => (string | null | Promise)) | null} */ +let _resolver = null; + +/** + * Internal: server-only wiring. `@webjsdev/server` installs the resolver that + * maps a REAL server-action function back to its `/` identity, which + * it knows from the module load hook that registered the function. The browser + * never calls this: a stub carries its identity on itself, so + * `formActionId` resolves synchronously there. + * + * @param {(fn: Function) => (string | null | Promise)} fn + * @returns {void} + */ +export function setFormActionResolver(fn) { + _resolver = fn; +} + +/** + * The identity of an action function, or null when it has none. * - * 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 - * through a template binding. It does NOT cover property REFLECTION: a prop - * declared `reflect: true` writes `String(value)` to its attribute from the - * setter, which is not a commit site and is not reachable from here. + * Synchronous, and that is a constraint rather than a preference: the client + * renderer commits attributes synchronously, so identity has to be readable + * without awaiting there. A stub carries its own, so the client path is a + * property read. On the server the resolver is consulted, and it may answer + * with a promise, which `resolveFormActionId` awaits and this does not. + * + * @param {unknown} fn + * @returns {string | null} + */ +export function formActionId(fn) { + if (typeof fn !== 'function') return null; + const own = /** @type any */ (fn)[FORM_ACTION_ID_KEY]; + return typeof own === 'string' && own ? own : null; +} + +/** + * The identity of an action function, consulting the server resolver when the + * function does not carry one. Used by the SSR renderers, which are async. + * + * @param {unknown} fn + * @returns {Promise} + */ +export async function resolveFormActionId(fn) { + const own = formActionId(fn); + if (own) return own; + if (typeof fn !== 'function' || !_resolver) return null; + try { + const id = await _resolver(/** @type {Function} */ (fn)); + return typeof id === 'string' && id ? id : null; + } catch { + return null; + } +} + +/** + * Is this hole the ONE supported form-action binding: an unquoted + * `action=${fn}` on a ``? + * + * Both halves matter. Unquoted, because quoting turns a binding hole back into + * a plain attribute the renderer stringifies, and there is no way to bind half + * an action into a url. On a ``, because that is the only element whose + * `action` submits anything; anywhere else the attribute is inert and binding + * a function to it means the author expected something that will never happen. + * + * @param {unknown} val the hole's resolved value + * @param {string} attrName the AUTHORED attribute name (no sigil, unquoted branch) + * @param {string} [tag] lowercased owner tag + * @returns {boolean} + */ +export function isBoundFormAction(val, attrName, tag) { + if (typeof val !== 'function') return false; + if (String(attrName).toLowerCase() !== 'action') return false; + return String(tag || '').toLowerCase() === 'form'; +} + +/** + * Refuse to stringify a function interpolated into a form-action attribute. * * 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. + * 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. The one supported shape + * (`isBoundFormAction`) is claimed by the caller BEFORE this runs, so + * everything reaching here is a shape with no meaning. * * @param {unknown} val the hole's resolved value * @param {string} attrName the attribute being committed (any case) @@ -73,6 +184,11 @@ export function assertNotFunctionActionAttr(val, attrName, tag) { * DOES write the source there, a prop declared `reflect: true`, which runs in * the setter and never reaches any commit site. * + * `` is refused rather than treated as the supported + * binding: the supported one is the plain `action=${fn}` attribute, and a + * `.prop` binding on a native element is dropped at SSR, so accepting it would + * mean a form that submits under JS and does nothing without it. + * * @param {unknown} val the hole's resolved value * @param {string} propName the property being assigned (any case) * @param {string} [tag] lowercased owner tag @@ -82,6 +198,184 @@ export function assertNotFunctionReflectedActionProp(val, propName, tag) { assertNotFunctionActionAttr(val, propName, tag); } +/** + * Refuse a bound action the renderer cannot identify. + * + * A function reaches here having passed `isBoundFormAction`, so the author + * meant it as a server action. If identity resolution came back empty it is + * not one: a local closure, a plain helper, or a `'use server'` module the + * server never registered. Emitting the form anyway would produce markup that + * looks right and silently posts nowhere, which is the one outcome a + * progressive-enhancement form must never have, so this throws instead. + * + * @param {string | null} id + * @param {string} [tag] + * @returns {string} the identity, narrowed + */ +export function assertIdentifiableAction(id, tag) { + if (id) return id; + throw new Error( + `[webjs] the function in action= on <${tag || 'form'}> is not a server action. ` + + `Only a function imported from a 'use server' *.server.{js,ts} module can be ` + + `bound to a form, because the identity the browser submits has to resolve ` + + `back to something the server can run. Move the handler into a server ` + + `action and import it, or pass a url string.`, + ); +} + +/** + * Rewrite a form's START TAG for a bound action and produce the hidden field + * that must follow it. + * + * `startTag` is the already-committed `` text, holes included, so + * this reads the attributes the browser will actually see rather than the ones + * the template literal spelled out. That matters for a hole-provided + * `method=${m}`, which no scan of the source template could resolve. + * + * Three edits, none of them optional: + * - `action` is gone already (the caller drops it at the hole), so the form + * posts to the page's own url. Omitted rather than emitted as `action=""`, + * which the HTML spec treats as a conformance error. + * - `method="post"` when absent. A GET form puts its fields in the query + * string and sends no body, so a bound action would never run. + * - `enctype="multipart/form-data"` when absent, so a file input works on + * the no-JS path. Text-only fields round-trip identically either way. + * + * An explicit `method="get"` throws rather than being silently upgraded: the + * author wrote two things that cannot both be true, and quietly picking one + * hides the mistake. + * + * @param {string} startTag the emitted start tag, ending in `>` + * @param {string} id the action identity + * @returns {{ tag: string, hidden: string }} + */ +export function bindFormActionStartTag(startTag, id) { + const attrs = parseStartTagAttrs(startTag); + const method = attrs.get('method'); + if (method != null && !/^post$/i.test(method.trim())) { + throw new Error( + `[webjs] cannot work: a bound ` + + `server action is submitted as a POST body, and a "${method}" form sends ` + + `no body. Drop the method attribute (WebJs emits method="post") or ` + + `remove the bound action.`, + ); + } + // The close is `>` or `/>`; a self-closing form is not valid HTML but the + // scanner can still hand one over, so keep whichever the author wrote. + const close = startTag.endsWith('/>') ? '/>' : '>'; + const head = startTag.slice(0, startTag.length - close.length); + let inject = ''; + if (!attrs.has('method')) inject += ' method="post"'; + if (!attrs.has('enctype')) inject += ' enctype="multipart/form-data"'; + return { tag: head + inject + close, hidden: formActionHiddenField(id) }; +} + +/** + * The hidden field carrying a bound action's identity into the submission. + * @param {string} id + * @returns {string} + */ +export function formActionHiddenField(id) { + return ``; +} + +/** + * Apply a bound action to a LIVE form element, producing the same three edits + * SSR makes. Runs when a shipping component re-renders a template holding + * ``: that render rebuilds the form from the template, so + * the SSR'd hidden field is gone and has to be put back. + * + * The identity is read synchronously off the stub, which is what the browser + * import of a `'use server'` module resolves to. A function with no identity + * throws for the same reason it does at SSR: a form that looks right and posts + * nowhere is the one outcome this feature cannot have. + * + * The field is inserted as the form's FIRST child, ahead of every child-part + * marker the template clone already contains, so a later child update cannot + * take it out again. Idempotent: a re-render finds the existing field and only + * refreshes its value. + * + * @param {HTMLFormElement} form + * @param {Function} fn + * @returns {void} + */ +export function bindFormActionElement(form, fn) { + const id = assertIdentifiableAction(formActionId(fn), form.localName); + form.removeAttribute('action'); + const method = form.getAttribute('method'); + if (method != null && !/^post$/i.test(method.trim())) { + throw new Error( + `[webjs] cannot work: a bound ` + + `server action is submitted as a POST body, and a "${method}" form sends ` + + `no body. Drop the method attribute (WebJs emits method="post") or ` + + `remove the bound action.`, + ); + } + if (method == null) form.setAttribute('method', 'post'); + if (!form.hasAttribute('enctype')) form.setAttribute('enctype', 'multipart/form-data'); + + let field = null; + for (const el of form.children) { + if (el.localName === 'input' && el.getAttribute('name') === FORM_ACTION_FIELD) { + field = el; + break; + } + } + if (!field) { + field = form.ownerDocument.createElement('input'); + field.setAttribute('type', 'hidden'); + field.setAttribute('name', FORM_ACTION_FIELD); + form.insertBefore(field, form.firstChild); + } + field.setAttribute('value', id); +} + +/** + * Parse the attributes of an emitted start tag into `name -> value`, where a + * valueless attribute maps to the empty string. + * + * A real tokenizer rather than a regex over the whole tag, because the tag is + * emitted HTML and an attribute VALUE can contain anything: `` has to report no `method` + * attribute, and a regex looking for `method=` reports one. Getting that wrong + * means silently skipping the forced `method="post"` and shipping a form that + * submits nothing. + * + * @param {string} startTag + * @returns {Map} + */ +export function parseStartTagAttrs(startTag) { + /** @type {Map} */ + const attrs = new Map(); + // Skip `<` and the tag name. + let i = 1; + while (i < startTag.length && !/[\s/>]/.test(startTag[i])) i++; + while (i < startTag.length) { + while (i < startTag.length && /[\s/]/.test(startTag[i])) i++; + if (i >= startTag.length || startTag[i] === '>') break; + let name = ''; + while (i < startTag.length && !/[\s/>=]/.test(startTag[i])) name += startTag[i++]; + while (i < startTag.length && /\s/.test(startTag[i])) i++; + if (startTag[i] !== '=') { + if (name) attrs.set(name.toLowerCase(), ''); + continue; + } + i++; // the `=` + while (i < startTag.length && /\s/.test(startTag[i])) i++; + let value = ''; + const q = startTag[i]; + if (q === '"' || q === "'") { + i++; + while (i < startTag.length && startTag[i] !== q) value += startTag[i++]; + i++; // closing quote + } else { + while (i < startTag.length && !/[\s>]/.test(startTag[i])) value += startTag[i++]; + } + if (name) attrs.set(name.toLowerCase(), value); + } + return attrs; +} + /** * Is `propName` an IDL attribute that `tag` reflects to a content attribute? * @param {string} propName @@ -151,7 +445,9 @@ function isFormActionAttr(attrName) { /** * The refusal message. Deliberately never echoes the function's source, which - * is the very thing being withheld. + * is the very thing being withheld. It names the one supported shape, because + * every refused shape here is a near-miss of it and the author's next question + * is what to write instead. * @param {string} attrName * @param {string} [tag] */ @@ -159,5 +455,6 @@ 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.`; + + `is refused. To run a server action from a form, write an unquoted ` + + `action=\${action} on the itself. Anywhere else, pass a URL string.`; } diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 1c62471ec..0817d970d 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1,7 +1,10 @@ import { isTemplate, MARKER } from './html.js'; import { BINDING_PREFIXES, isBindingPrefix } from './binding-prefixes.js'; import { escapeAttr } from './escape.js'; -import { assertNotFunctionActionAttr, assertNotFunctionReflectedActionProp } from './form-action.js'; +import { + assertNotFunctionActionAttr, assertNotFunctionReflectedActionProp, + bindFormActionElement, isBoundFormAction, +} 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'; @@ -672,7 +675,14 @@ function applyPart(part, value, _prev, allValues) { break; case 'attr': { if (value == null || value === false) part.el.removeAttribute(part.name); - else { + else if (isBoundFormAction(value, part.name, part.el.localName)) { + // #1155: the ONE supported form-action binding, applied to the live + // form exactly as SSR wrote it. A component that ships re-renders its + // whole template on hydration, so without this the SSR'd hidden field + // would be replaced by an `action` attribute holding a stringified + // function, and the form would post to a garbage url. + bindFormActionElement(/** @type any */ (part.el), 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). diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index e0e8bae1c..e54ec2023 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -1,7 +1,10 @@ import { html, isTemplate } from './html.js'; import { BINDING_PREFIXES } from './binding-prefixes.js'; import { escapeText, escapeAttr } from './escape.js'; -import { assertNotFunctionActionAttr, assertNotFunctionReflectedActionProp } from './form-action.js'; +import { + assertNotFunctionActionAttr, assertNotFunctionReflectedActionProp, + assertIdentifiableAction, bindFormActionStartTag, isBoundFormAction, resolveFormActionId, +} from './form-action.js'; import { lookup, lookupModuleUrl, allTags } from './registry.js'; import { stylesToString, isCSS } from './css.js'; import { isRepeat } from './repeat.js'; @@ -152,6 +155,22 @@ async function renderTemplate(tr, ctx) { let commentDashes = 0; let currentTag = ''; // lowercased tag name currently being parsed let rawTail = ''; // rolling lowercased tail, tracks / + let tagStart = -1; // index in `out` of the `<` opening the current tag + /** @type {string | null} */ + let pendingActionId = null; // identity of a bound form action, until the tag closes + + // A bound `action=${fn}` is committed at its hole, but the edits it implies + // (forcing `method` / `enctype`, and the hidden identity field) are only + // possible once the whole start tag is known: an attribute the author wrote + // AFTER the action hole still counts, and the hidden field belongs INSIDE + // the form, after the `>`. So the hole records the identity and this runs at + // the `>`, rewriting the start tag that was just emitted. + const closeBoundFormTag = () => { + if (pendingActionId == null) return; + const bound = bindFormActionStartTag(out.slice(tagStart), pendingActionId); + out = out.slice(0, tagStart) + bound.tag + bound.hidden; + pendingActionId = null; + }; for (let i = 0; i < strings.length; i++) { const s = strings[i]; @@ -160,7 +179,7 @@ async function renderTemplate(tr, ctx) { switch (state) { case 'text': out += c; - if (c === '<') state = 'tag-open'; + if (c === '<') { state = 'tag-open'; tagStart = out.length - 1; } break; case 'tag-open': out += c; @@ -187,6 +206,7 @@ async function renderTemplate(tr, ctx) { case 'tag-name': out += c; if (c === '>') { + closeBoundFormTag(); state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (/\s/.test(c)) state = 'in-tag'; @@ -195,6 +215,7 @@ async function renderTemplate(tr, ctx) { case 'in-tag': out += c; if (c === '>') { + closeBoundFormTag(); state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (!/\s/.test(c) && c !== '/') { @@ -215,18 +236,18 @@ async function renderTemplate(tr, ctx) { case 'attr-name': if (c === '=') { state = 'after-eq'; out += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { state = 'text'; attrName = ''; out += c; } + else if (c === '>') { state = 'text'; attrName = ''; out += c; closeBoundFormTag(); } else { attrName += c; out += c; } break; case 'after-eq': if (c === '"' || c === "'") { state = 'attr-quoted'; attrQuote = c; out += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { state = 'text'; attrName = ''; out += c; } + else if (c === '>') { state = 'text'; attrName = ''; out += c; closeBoundFormTag(); } else { state = 'attr-unquoted'; out += c; } break; case 'attr-unquoted': if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { state = 'text'; attrName = ''; out += c; } + else if (c === '>') { state = 'text'; attrName = ''; out += c; closeBoundFormTag(); } else out += c; break; case 'attr-quoted': @@ -338,6 +359,19 @@ async function renderTemplate(tr, ctx) { if (val) out += `${name}=""`; state = 'in-tag'; attrName = ''; + } else if (isBoundFormAction(val, attrName, currentTag)) { + // #1155: the ONE supported form-action binding. Drop the `action=` + // attribute entirely so the form posts to the page's own url (an + // omitted attribute, not `action=""`, which the spec calls a + // conformance error), and remember the identity so the `>` can force + // the submission attributes and emit the hidden field. + pendingActionId = assertIdentifiableAction(await resolveFormActionId(val), currentTag); + // Trailing whitespace goes with the attribute: every injected + // attribute carries its own leading space, so keeping the old one + // would double it in the emitted tag. + out = out.slice(0, attrStart).replace(/\s+$/, ''); + 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). @@ -1790,6 +1824,19 @@ async function streamTemplate(tr, ctx, controller) { let rawTail = ''; // Buffer used for attribute handling where we may need to backtrack. let buf = ''; + let tagStart = -1; + /** @type {string | null} */ + let pendingActionId = null; + + // See the buffered machine for why this runs at the `>` rather than at the + // hole. `tagStart` indexes into `buf`, which is safe because `buf` is only + // flushed on a `text`-state hole and a start tag contains none. + const closeBoundFormTag = () => { + if (pendingActionId == null) return; + const bound = bindFormActionStartTag(buf.slice(tagStart), pendingActionId); + buf = buf.slice(0, tagStart) + bound.tag + bound.hidden; + pendingActionId = null; + }; for (let i = 0; i < strings.length; i++) { const s = strings[i]; @@ -1798,7 +1845,7 @@ async function streamTemplate(tr, ctx, controller) { switch (state) { case 'text': buf += c; - if (c === '<') state = 'tag-open'; + if (c === '<') { state = 'tag-open'; tagStart = buf.length - 1; } break; case 'tag-open': buf += c; @@ -1825,6 +1872,7 @@ async function streamTemplate(tr, ctx, controller) { case 'tag-name': buf += c; if (c === '>') { + closeBoundFormTag(); state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (/\s/.test(c)) state = 'in-tag'; @@ -1833,6 +1881,7 @@ async function streamTemplate(tr, ctx, controller) { case 'in-tag': buf += c; if (c === '>') { + closeBoundFormTag(); state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (!/\s/.test(c) && c !== '/') { @@ -1853,18 +1902,18 @@ async function streamTemplate(tr, ctx, controller) { case 'attr-name': if (c === '=') { state = 'after-eq'; buf += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { state = 'text'; attrName = ''; buf += c; } + else if (c === '>') { state = 'text'; attrName = ''; buf += c; closeBoundFormTag(); } else { attrName += c; buf += c; } break; case 'after-eq': if (c === '"' || c === "'") { state = 'attr-quoted'; attrQuote = c; buf += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { state = 'text'; attrName = ''; buf += c; } + else if (c === '>') { state = 'text'; attrName = ''; buf += c; closeBoundFormTag(); } else { state = 'attr-unquoted'; buf += c; } break; case 'attr-unquoted': if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { state = 'text'; attrName = ''; buf += c; } + else if (c === '>') { state = 'text'; attrName = ''; buf += c; closeBoundFormTag(); } else buf += c; break; case 'attr-quoted': @@ -1923,6 +1972,14 @@ async function streamTemplate(tr, ctx, controller) { if (val) buf += `${name}=""`; state = 'in-tag'; attrName = ''; + } else if (isBoundFormAction(val, attrName, currentTag)) { + // The SAME binding as the buffered renderer (#1155), in the second + // machine, so `renderToStream(v, { ssr: false })` emits an identical + // form rather than refusing one the page renderer accepts. + pendingActionId = assertIdentifiableAction(await resolveFormActionId(val), currentTag); + buf = buf.slice(0, attrStart).replace(/\s+$/, ''); + state = 'in-tag'; + attrName = ''; } else { // The SAME guard as the buffered renderer above. This is a second, // independent state machine, so it inherits nothing from that one; diff --git a/packages/core/test/rendering/form-action-attr-guard.test.js b/packages/core/test/rendering/form-action-attr-guard.test.js index 0c5a97008..f4bfa3e54 100644 --- a/packages/core/test/rendering/form-action-attr-guard.test.js +++ b/packages/core/test/rendering/form-action-attr-guard.test.js @@ -1,9 +1,18 @@ -// #1154: a function interpolated into a form-action attribute must throw, -// never stringify. At SSR a `'use server'` import is the REAL function, so +// #1154: a function interpolated into a form-action attribute must 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. +// the served HTML. Covers every hole shape that used to leak (quoted, mixed, +// and `formaction` on a submit button), plus the byte-identical passthrough +// for string-valued action attributes. +// +// #1155 later made ONE of those shapes meaningful rather than merely refused: +// an unquoted `action=${fn}` on a `` binds the action. It still cannot +// stringify anything, so the security claim is unchanged, but the refusal it +// hits is now the identity one ("is not a server action") for a function the +// server never registered, which is every function in this file. The binding +// itself has its own suite in `form-action-binding.test.js`; here the point is +// that the source never escapes on any path. +const NOT_AN_ACTION = /is not a server action/; import { test, before } from 'node:test'; import assert from 'node:assert/strict'; @@ -124,7 +133,7 @@ async function leaky(input) { test('unquoted action=${fn} throws instead of leaking source', async () => { await assert.rejects( () => renderToString(html``, { ssr: true }), - /function was interpolated into action=/, + NOT_AN_ACTION, ); }); @@ -167,9 +176,11 @@ test('camelCase formAction=${fn} throws (React spells it this way)', async () => }); test('upper-case ACTION=${fn} throws', async () => { + // Attribute names are case-insensitive in HTML, so this is the binding shape + // spelled loudly and it hits the identity refusal, not the stringify one. await assert.rejects( () => renderToString(html`
`, { ssr: true }), - /function was interpolated into action=/, + NOT_AN_ACTION, ); }); @@ -223,7 +234,7 @@ test('string-valued action renders byte-identically to before', async () => { 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=/, + NOT_AN_ACTION, ); }); @@ -243,7 +254,7 @@ test('the streaming renderer never emits the source, not even before it refuses' const sink = { text: '' }; await assert.rejects( () => drainInto(renderToStream(html`
`, { ssr: false }), sink), - /function was interpolated into action=/, + NOT_AN_ACTION, ); assert.ok(!sink.text.includes('SECRET'), `received bytes must not carry the source, got: ${sink.text}`); assert.ok(!sink.text.includes('async function'), 'no function source of any kind reached the client'); diff --git a/packages/core/test/rendering/form-action-binding.test.js b/packages/core/test/rendering/form-action-binding.test.js new file mode 100644 index 000000000..74fe7423e --- /dev/null +++ b/packages/core/test/rendering/form-action-binding.test.js @@ -0,0 +1,206 @@ +// #1155: `
` is the ONE way to submit a form to +// a server action. Both SSR machines resolve the action's identity, drop the +// `action` attribute so the form posts to the page's own url, force the +// attributes the submission needs, and emit the hidden identity field the +// server dispatches on. +// +// What these tests pin, in order of how much it would cost to get wrong: +// - the function's source never appears, on any path (it is still #1154's +// claim; binding changed how the value is USED, not whether it stringifies) +// - the hidden field lands INSIDE the form, because a field emitted after +// the close tag is not submitted and the form would silently post nothing +// - `method="post"` is forced from the attributes the browser will actually +// see, not from the ones the template literal spelled out +import { test, before } from 'node:test'; +import assert from 'node:assert/strict'; + +let html, renderToString, renderToStream, setFormActionResolver, FORM_ACTION_FIELD, FORM_ACTION_ID_KEY; +before(async () => { + ({ html } = await import('../../src/html.js')); + ({ renderToString, renderToStream } = await import('../../src/render-server.js')); + ({ setFormActionResolver, FORM_ACTION_FIELD, FORM_ACTION_ID_KEY } = await import('../../src/form-action.js')); +}); + +// The marker is INLINE in the body, not read from an outer const: `String(fn)` +// reproduces source, so a body reading an outer binding stringifies to the +// IDENTIFIER and a /SECRET/ assertion against it is a tautology. +async function submitFeedback(formData) { + const conn = 'postgres://user:SECRET_MARKER@host/db'; + return { success: true, conn, got: formData }; +} + +const ID = 'a1b2c3d4e5/submitFeedback'; + +/** Install a resolver that identifies only `submitFeedback`. */ +function withResolver() { + setFormActionResolver((fn) => (fn === submitFeedback ? ID : null)); +} + +async function drain(stream) { + let out = ''; + for await (const c of stream) out += typeof c === 'string' ? c : new TextDecoder().decode(c); + return out; +} + +test('a bound action emits a hidden identity field and no action attribute', async () => { + withResolver(); + const out = await renderToString( + html`
`, { ssr: true }); + assert.doesNotMatch(out, /SECRET/, 'the action source must never reach the markup'); + assert.doesNotMatch(out, /\saction=/, 'the attribute is omitted, so the form posts to its own url'); + assert.match(out, new RegExp(``)); +}); + +test('the hidden field is inside the form, not after it', async () => { + // A field emitted after `` is not part of the submission, so the + // server would see no identity and the form would do nothing. Asserting the + // field merely EXISTS somewhere in the output cannot tell those apart. + withResolver(); + const out = await renderToString(html`
body
`, { ssr: true }); + const field = out.indexOf(FORM_ACTION_FIELD); + const close = out.indexOf(''); + assert.ok(field > 0 && close > 0, 'both markers present'); + assert.ok(field < close, `hidden field must precede , got field@${field} close@${close}`); +}); + +test('method and enctype are forced when the author omits them', async () => { + withResolver(); + const out = await renderToString(html`
`, { ssr: true }); + assert.match(out, /method="post"/); + assert.match(out, /enctype="multipart\/form-data"/); +}); + +test('an author-written method and enctype are left alone', async () => { + withResolver(); + const out = await renderToString( + html`
`, + { ssr: true }); + assert.match(out, /method="POST"/); + assert.match(out, /enctype="application\/x-www-form-urlencoded"/); + assert.doesNotMatch(out, /multipart/, 'the author enctype wins, nothing is appended'); +}); + +test('an attribute written AFTER the action hole still counts as present', async () => { + // The forcing decision cannot be made at the hole, because the author may + // spell `method` later in the same tag. Made there, this would emit two + // `method` attributes and the browser would keep the first (`post`), so the + // form would work and the bug would sit invisible until someone wrote + // `method="get"` after the hole. + withResolver(); + const out = await renderToString( + html`
`, { ssr: true }); + assert.equal(out.match(/enctype=/g).length, 1, 'exactly one enctype attribute'); + assert.match(out, /enctype="text\/plain"/); +}); + +test('a method= inside an unrelated attribute VALUE does not count as present', async () => { + // The attribute scan is a tokenizer, not a regex over the tag text. A regex + // reports a `method` attribute here, skips the forced `method="post"`, and + // ships a GET form that submits its fields in the query string and never + // runs the action. + withResolver(); + const out = await renderToString( + html`
`, { ssr: true }); + assert.match(out, /method="post"/, 'the real method attribute is still forced'); +}); + +test('an explicit method="get" is refused rather than silently upgraded', async () => { + withResolver(); + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /cannot work/, + ); +}); + +test('a hole-provided method is judged on its resolved value', async () => { + // `method=${m}` cannot be read off the source template at all, which is the + // second reason the decision waits for the `>`. + withResolver(); + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /cannot work/, + ); + const ok = await renderToString( + html`
`, { ssr: true }); + assert.match(ok, /method="post"/); + assert.equal(ok.match(/method=/g).length, 1); +}); + +test('an unidentifiable function is refused, never rendered as an inert form', async () => { + withResolver(); + await assert.rejects( + () => renderToString(html`
{}}>
`, { ssr: true }), + /is not a server action/, + ); +}); + +test('a function carrying its own identity resolves with no resolver', async () => { + // This is the browser stub's path: the generated stub stamps its identity on + // itself, so it needs no server resolver. Pinning it on the server renderer + // keeps the two identity sources from drifting. + setFormActionResolver(() => null); + const stub = async () => {}; + Object.defineProperty(stub, FORM_ACTION_ID_KEY, { value: 'ffff000011/stubbed' }); + const out = await renderToString(html`
`, { ssr: true }); + assert.match(out, /value="ffff000011\/stubbed"/); +}); + +test('the identity is attribute-escaped', async () => { + setFormActionResolver(() => 'a">/x'); + const out = await renderToString(html`
`, { ssr: true }); + assert.doesNotMatch(out, /