fix: refuse a function in form action=, it leaked server action source

.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 `<form action=${fn}>` is refused, not stringified
+
+Next binds a Server Action with `<form action={createTodo}>` 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`<form method="post" action=${submitFeedback}>`;
+// 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`<form method="post">`;
+```
+
+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.

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.`;
+}

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);

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 ?? ''));
       }
     }

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`<form method="post" action=${secretAction}></form>`, 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`<form .action=${secretAction}></form>`, 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`<form method="post" action=${a}></form>`;
+    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`<form method="post" action=${'/feedback'}><input name="msg" value="hi"></form>`, 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');
+  });
+
+});

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('<!doctype html><html><body></body></html>');
+  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`<form method="post" action=${fakeAction}></form>`, host),
+    /function was interpolated into action=/,
+  );
+});
+
+test('client render of mixed action="/x/${fn}" throws', () => {
+  const host = document.createElement('div');
+  assert.throws(
+    () => render(html`<form action="/x/${fakeAction}"></form>`, 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`<form method="post" action=${a}></form>`;
+  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`<form .action=${fakeAction}></form>`, 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`<my-widget .action=${fakeAction}></my-widget>`, 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`<form method="post" action=${'/submit'}></form>`, host);
+  const form = host.querySelector('form');
+  assert.equal(form.getAttribute('action'), '/submit');
+});