fix: close the same form-action leak in the streaming renderer

vivek7405 · vivek7405 · commit cf72805a3f7a · 2026-07-28T16:52:51.000+05:30
The first pass guarded `renderTemplate` only. SSR has a SECOND, independent
state machine, `streamTemplate`, on the Suspense streaming path, and it had
no guard at all, so a streamed page still wrote a bound function's whole
source into the response while the buffered path already refused it.

Two renderers with one contract between them is what allowed the gap, so the
decision now lives in a single `commitFormActionAttr` helper both call, rather
than in duplicated logic at each site. The streaming path gets its own tests
instead of being assumed to inherit the buffered path's.

Also adds the identity plumbing the form binding needs: a server-installed
resolver (the same shape as the CSP nonce provider, so the browser bundle
never gets one and keeps refusing every function), the buffered hidden-field
emission, and the forced method/enctype that a no-JS submission depends on.
diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js
@@ -32,13 +32,124 @@ export const ACTION_FIELD = '__webjs_action';
  */
 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} */
+export function isFormActionAttr(attrName) {
   const name = String(attrName).toLowerCase();
-  if (name !== 'action' && name !== 'formaction') return;
-  throw new Error(
-    `[webjs] a function was interpolated into ${name}= 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. `
-    + `Bind a 'use server' action exported from a *.server.{js,ts} module, `
-    + `or pass a URL string.`
-  );
+  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'}>, and it is not a 'use server' action. Stringifying a `
+    + `function into HTML would leak its source (a server action's body included) `
+    + `to every visitor, so this is refused. Bind a 'use server' action exported `
+    + `from a *.server.{js,ts} module, or pass a URL string.`;
+}
+
+/**
+ * Resolves a form-bound action function to its wire identity, `<hash>/<fn>`
+ * (#1155). Server-only wiring, installed exactly like the CSP nonce provider:
+ * `@webjsdev/server` calls `setFormActionResolver` at load time, the browser
+ * bundle never does, so a client-side render keeps refusing every function.
+ *
+ * @type {((fn: Function) => string | null) | null}
+ */
+let _resolver = null;
+
+/**
+ * Internal: server-only wiring. Installed once by `@webjsdev/server`.
+ * @param {(fn: Function) => string | null} fn
+ */
+export function setFormActionResolver(fn) {
+  _resolver = fn;
+}
+
+/**
+ * Resolve a form-bound action to `<hash>/<fn>`, or throw the refusal above when
+ * it is not an identifiable `'use server'` action.
+ *
+ * Throwing on an unresolvable function is load-bearing for progressive
+ * enhancement, not defensive: emitting the form WITHOUT the identity field
+ * would render a form that posts to the page and dispatches nothing, a silent
+ * no-op with no error anywhere. Failing the render is the only outcome that
+ * cannot ship a dead form.
+ *
+ * @param {Function} val
+ * @param {string} attrName
+ * @param {string} [tag]
+ * @returns {string} the `<hash>/<fn>` identity
+ */
+export function resolveFormActionOrThrow(val, attrName, tag) {
+  const id = _resolver ? _resolver(val) : null;
+  if (!id) throw new Error(formActionError(attrName, tag));
+  return id;
+}
+
+/**
+ * The hidden field carrying the action identity, plus the attributes the
+ * framework forces rather than trusting the author to write.
+ *
+ * `method="post"`: a form with no method GETs, so the action would never run.
+ * `enctype="multipart/form-data"`: without it a no-JS file upload silently
+ * arrives as a filename string instead of the file.
+ *
+ * @param {string} id the `<hash>/<fn>` identity
+ * @param {(name: string) => boolean} has whether the form already set an attribute
+ * @returns {{ forced: string, field: string }}
+ */
+export function formActionMarkup(id, has) {
+  let forced = '';
+  if (!has('method')) forced += ' method="post"';
+  if (!has('enctype')) forced += ' enctype="multipart/form-data"';
+  const field = `<input type="hidden" name="${ACTION_FIELD}" value="${escapeAttrValue(id)}">`;
+  return { forced, field };
+}
+
+/** Minimal attribute escape; the identity is framework-generated, so this is belt and braces. */
+function escapeAttrValue(s) {
+  return String(s).replace(/&/g, '&amp;').replace(/"/g, '&quot;').replace(/</g, '&lt;');
+}
+
+/**
+ * The single decision point both SSR state machines route through when an
+ * attribute hole resolves to a function.
+ *
+ * There are two of them (`renderTemplate` and the Suspense-streaming
+ * `streamTemplate`), and they HAVE already diverged once: the guard was added
+ * to the buffered renderer alone, leaving the streamed path emitting a server
+ * action's whole body into the response. Anything that must hold for both
+ * belongs here rather than at either call site.
+ *
+ * Returns null when the value is not a function bound to an action attribute,
+ * meaning the caller proceeds with its normal stringify.
+ *
+ * @param {unknown} val the resolved hole value
+ * @param {string} attrName the attribute being committed
+ * @param {string} tag lowercased owner tag
+ * @param {Set<string>} seenAttrs attribute names already written on this tag
+ * @returns {{ attrValue: string, forced: string, field: string } | null}
+ */
+export function commitFormActionAttr(val, attrName, tag, seenAttrs) {
+  if (typeof val !== 'function') return null;
+  if (!isFormActionAttr(attrName)) {
+    // A function on any other attribute keeps the pre-existing stringify
+    // behaviour: widening the claim here would change unrelated rendering.
+    return null;
+  }
+  const id = resolveFormActionOrThrow(val, attrName, tag);
+  const { forced, field } = formActionMarkup(id, (n) => seenAttrs.has(n));
+  // The form posts to its OWN url; the identity rides the hidden field, which
+  // is what lets one action be bound from any page (#1155) while the failure
+  // re-render still targets the page the form was on.
+  return { attrValue: '""', forced, field };
 }
diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js
@@ -1,7 +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 { assertNotFunctionActionAttr, commitFormActionAttr } from './form-action.js';
 import { lookup, lookupModuleUrl, allTags } from './registry.js';
 import { stylesToString, isCSS } from './css.js';
 import { isRepeat } from './repeat.js';
@@ -152,6 +152,13 @@ async function renderTemplate(tr, ctx) {
   let commentDashes = 0;
   let currentTag = '';   // lowercased tag name currently being parsed
   let rawTail = '';      // rolling lowercased tail, tracks </script>/</style>
+  // Form-action binding (#1155). A `<form action=${action}>` commits its
+  // attribute mid-tag, but the identity field it needs is a CHILD, so the
+  // markup is buffered here and flushed the moment the open tag closes.
+  // `seenAttrs` tracks what the author already wrote on the current tag so a
+  // forced `method` / `enctype` never duplicates an explicit one.
+  let pendingFormField = '';
+  let seenAttrs = new Set();
 
   for (let i = 0; i < strings.length; i++) {
     const s = strings[i];
@@ -195,6 +202,9 @@ async function renderTemplate(tr, ctx) {
         case 'in-tag':
           out += c;
           if (c === '>') {
+            // The open tag just closed, so a buffered form-action identity
+            // field can now be emitted as the form's first child (#1155).
+            if (pendingFormField) { out += pendingFormField; pendingFormField = ''; }
             state = isRawtextTag(currentTag) ? 'rawtext' : 'text';
             if (state === 'rawtext') rawTail = '';
           } else if (!/\s/.test(c) && c !== '/') {
@@ -213,20 +223,34 @@ async function renderTemplate(tr, ctx) {
           }
           break;
         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; }
+          // Record every attribute the author wrote on this tag, so a forced
+          // method/enctype (#1155) never duplicates an explicit one.
+          if (c === '=') { state = 'after-eq'; seenAttrs.add(attrName.toLowerCase()); out += c; }
+          else if (/\s/.test(c)) { state = 'in-tag'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; out += c; }
+          else if (c === '>') {
+            state = 'text';
+            seenAttrs.add(attrName.toLowerCase());
+            attrName = '';
+            out += c;
+            if (pendingFormField) { out += pendingFormField; pendingFormField = ''; }
+          }
           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;
+            if (pendingFormField) { out += pendingFormField; pendingFormField = ''; }
+          }
           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;
+            if (pendingFormField) { out += pendingFormField; pendingFormField = ''; }
+          }
           else out += c;
           break;
         case 'attr-quoted':
@@ -324,16 +348,22 @@ 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 ?? ''))}"`;
+          // A function bound to action=/formaction= is either an identifiable
+          // server action (emit the identity field, #1155) or refused
+          // outright (#1154); it is never stringified.
+          const bound = commitFormActionAttr(val, attrName, currentTag, seenAttrs);
+          if (bound) {
+            out += bound.attrValue + bound.forced;
+            pendingFormField = bound.field;
+          } else {
+            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).
+        // A hole INSIDE a value (`action="${fn}"`, mixed `action="/x/${fn}"`)
+        // cannot carry an identity field, so a function here is always refused.
         assertNotFunctionActionAttr(val, attrName, currentTag);
         out += escapeAttr(String(val ?? ''));
       }
@@ -1775,6 +1805,9 @@ async function streamTemplate(tr, ctx, controller) {
   let rawTail = '';
   // Buffer used for attribute handling where we may need to backtrack.
   let buf = '';
+  // Form-action binding (#1155), mirroring renderTemplate.
+  let pendingFormField = '';
+  let seenAttrs = new Set();
 
   for (let i = 0; i < strings.length; i++) {
     const s = strings[i];
@@ -1788,8 +1821,8 @@ async function streamTemplate(tr, ctx, controller) {
         case 'tag-open':
           buf += c;
           if (c === '!') state = 'bang-1';
-          else if (c === '/') { state = 'tag-name'; currentTag = ''; }
-          else if (/[a-zA-Z]/.test(c)) { state = 'tag-name'; currentTag = c.toLowerCase(); }
+          else if (c === '/') { state = 'tag-name'; currentTag = ''; seenAttrs = new Set(); }
+          else if (/[a-zA-Z]/.test(c)) { state = 'tag-name'; currentTag = c.toLowerCase(); seenAttrs = new Set(); }
           else state = 'text';
           break;
         case 'bang-1':
@@ -1818,6 +1851,7 @@ async function streamTemplate(tr, ctx, controller) {
         case 'in-tag':
           buf += c;
           if (c === '>') {
+            if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; }
             state = isRawtextTag(currentTag) ? 'rawtext' : 'text';
             if (state === 'rawtext') rawTail = '';
           } else if (!/\s/.test(c) && c !== '/') {
@@ -1836,20 +1870,29 @@ async function streamTemplate(tr, ctx, controller) {
           }
           break;
         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; }
+          if (c === '=') { state = 'after-eq'; seenAttrs.add(attrName.toLowerCase()); buf += c; }
+          else if (/\s/.test(c)) { state = 'in-tag'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; buf += c; }
+          else if (c === '>') {
+            state = 'text'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; buf += c;
+            if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; }
+          }
           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;
+            if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; }
+          }
           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;
+            if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; }
+          }
           else buf += c;
           break;
         case 'attr-quoted':
@@ -1891,11 +1934,21 @@ async function streamTemplate(tr, ctx, controller) {
           state = 'in-tag';
           attrName = '';
         } else {
-          buf += `"${escapeAttr(String(val ?? ''))}"`;
+          // Same contract as the buffered renderer (they must not diverge; see
+          // commitFormActionAttr). Before this, a STREAMED page emitted a bound
+          // action's source verbatim while the buffered path already refused it.
+          const bound = commitFormActionAttr(val, attrName, currentTag, seenAttrs);
+          if (bound) {
+            buf += bound.attrValue + bound.forced;
+            pendingFormField = bound.field;
+          } else {
+            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/form-action-attr-guard.test.js b/packages/core/test/rendering/form-action-attr-guard.test.js
@@ -7,12 +7,18 @@
 import { test, before } from 'node:test';
 import assert from 'node:assert/strict';
 
-let html, renderToString;
+let html, renderToString, renderToStream;
 before(async () => {
   ({ html } = await import('../../src/html.js'));
-  ({ renderToString } = await import('../../src/render-server.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 }; }
@@ -72,6 +78,30 @@ test('string-valued action renders byte-identically to before', async () => {
   );
 });
 
+// The STREAMING renderer is a second, independent state machine. It was missed
+// by the first pass of this fix and kept emitting the action's whole body while
+// the buffered renderer already refused it, so it gets its own coverage rather
+// than being assumed to inherit the guard.
+test('the streaming renderer refuses the same function (ssr:false path)', async () => {
+  await assert.rejects(
+    () => drain(renderToStream(html`<form action=${leaky}></form>`, { ssr: false })),
+    /function was interpolated into action=/,
+  );
+});
+
+test('the streaming renderer never emits the source', async () => {
+  let out = '';
+  try {
+    out = await drain(renderToStream(html`<form action=${leaky}></form>`, { ssr: false }));
+  } catch { /* expected */ }
+  assert.ok(!out.includes('SECRET'), 'streamed output must not carry the function source');
+});
+
+test('streaming keeps string-valued actions working', async () => {
+  const out = await drain(renderToStream(html`<form action=${'/submit'}></form>`, { 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).
