Skip to content

Commit 7d86696

Browse files
committed
fix: refuse a function in form action=, it leaked server source
At SSR a `'use server'` import resolves to the REAL function (the RPC stub exists only in the browser), and `action=` is an ordinary escaped attribute hole, so the renderer stringified the function straight into the served HTML. A page doing what every Next-trained author writes, `<form action=${createTodo}>`, published the action's whole body, including any connection string or internal path its closure showed, to every visitor. Both hole shapes leaked identically (`action=${fn}` and `action="${fn}"`), as did a mixed value and `formaction=` on a submit button. Refuse it instead, on the server and on the client, so a client re-render cannot write the source into the live DOM either. The guard is name-based and deliberately narrow: only `action` and `formaction`, where a stringified function is never useful on any tag. Every other attribute keeps its existing behaviour, and a string-valued action is byte-identical to before. The thrown message names the fix without echoing the source.
1 parent ef9419e commit 7d86696

6 files changed

Lines changed: 211 additions & 2 deletions

File tree

.agents/skills/webjs/references/muscle-memory-gotchas.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,23 @@ const users = await getUsers();
4343

4444
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.
4545

46+
### A plain function in `<form action=${fn}>` is refused, not stringified
47+
48+
Next binds a Server Action with `<form action={createTodo}>`, and React serializes the binding into hidden fields. WebJs reads the same shape, but only for a real `'use server'` action. Any other function is a hard render error.
49+
50+
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. So 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}"`).
51+
52+
```ts
53+
// WRONG: a local handler. Throws at render; it would have leaked its body.
54+
const onSubmit = async (fd: FormData) => { /* ... */ };
55+
html`<form method="post" action=${onSubmit}>`;
56+
// RIGHT: a 'use server' action imported from a *.server.ts module.
57+
import { submitFeedback } from '#modules/feedback/actions/submit-feedback.server.ts';
58+
html`<form method="post" action=${submitFeedback}>`;
59+
```
60+
61+
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.
62+
4663
### `params` and `searchParams` are awaitable AND synchronously readable
4764

4865
Next 15/16 made `params` / `searchParams` Promises. WebJs supports BOTH, so either muscle memory is correct.

packages/core/src/form-action.js

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/**
2+
* Form-action binding shared bits (#1154, #1155).
3+
*
4+
* Both renderers (server + client) import from here so the guard cannot
5+
* drift between SSR and hydration.
6+
*/
7+
8+
/**
9+
* The hidden field that carries a form-bound action's identity
10+
* (`<hash>/<fn>`) from the SSR'd markup to the server dispatcher (#1155).
11+
* Reserved name; user form fields must not use it.
12+
*/
13+
export const ACTION_FIELD = '__webjs_action';
14+
15+
/**
16+
* Refuse to stringify a function interpolated into a form-action attribute
17+
* (#1154). At SSR a `'use server'` import is the REAL server function (the
18+
* RPC stub only exists in the browser), and a plain attribute hole commits
19+
* via `String(val)`, so without this guard the action's SOURCE, secrets
20+
* included, is serialized into the served HTML. The client renderer guards
21+
* the same commit so a client re-render cannot write the source into the
22+
* live DOM either.
23+
*
24+
* Name-based on purpose (`action`, plus `formaction`, the submit-button
25+
* override, which leaks identically): a function stringified under these
26+
* names is never useful on any tag, and other attributes keep today's
27+
* stringify behaviour so the claim stays narrow.
28+
*
29+
* @param {unknown} val the hole's resolved value
30+
* @param {string} attrName the attribute being committed (any case)
31+
* @param {string} [tag] lowercased owner tag, for the error message
32+
*/
33+
export function assertNotFunctionActionAttr(val, attrName, tag) {
34+
if (typeof val !== 'function') return;
35+
const name = String(attrName).toLowerCase();
36+
if (name !== 'action' && name !== 'formaction') return;
37+
throw new Error(
38+
`[webjs] a function was interpolated into ${name}= on <${tag || 'form'}>. `
39+
+ `Stringifying a function into HTML would leak its source (a 'use server' `
40+
+ `action's body included) to every visitor, so this is refused. `
41+
+ `Bind a 'use server' action exported from a *.server.{js,ts} module, `
42+
+ `or pass a URL string.`
43+
);
44+
}

packages/core/src/render-client.js

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { isTemplate, MARKER } from './html.js';
22
import { BINDING_PREFIXES, isBindingPrefix } from './binding-prefixes.js';
33
import { escapeAttr } from './escape.js';
4+
import { assertNotFunctionActionAttr } from './form-action.js';
45
import { isRepeat } from './repeat.js';
56
import { isUnsafeHTML, isLive, isKeyed, isGuard, isTemplateContent, isRef, isCache, isUntil, isAsyncAppend, isAsyncReplace, isWatch } from './directives.js';
67
import { Signal } from './signal.js';
@@ -671,7 +672,13 @@ function applyPart(part, value, _prev, allValues) {
671672
break;
672673
case 'attr': {
673674
if (value == null || value === false) part.el.removeAttribute(part.name);
674-
else part.el.setAttribute(part.name, String(value));
675+
else {
676+
// #1154: refuse to stringify a function into action=/formaction=
677+
// (mirrors the SSR guard, so a client re-render cannot write a
678+
// server action's source into the live DOM).
679+
assertNotFunctionActionAttr(value, part.name, part.el.localName);
680+
part.el.setAttribute(part.name, String(value));
681+
}
675682
break;
676683
}
677684
case 'prop':
@@ -692,7 +699,10 @@ function applyPart(part, value, _prev, allValues) {
692699
const mp = /** @type {{ statics: string[], group: number[] }} */ (/** @type any */ (part));
693700
let val = mp.statics[0];
694701
for (let j = 0; j < mp.group.length; j++) {
695-
val += String((allValues ? allValues[mp.group[j]] : value) ?? '');
702+
const piece = allValues ? allValues[mp.group[j]] : value;
703+
// #1154: same function guard for each piece of a mixed attribute.
704+
assertNotFunctionActionAttr(piece, part.name, part.el.localName);
705+
val += String(piece ?? '');
696706
val += mp.statics[j + 1] || '';
697707
}
698708
part.el.setAttribute(part.name, val);

packages/core/src/render-server.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { html, isTemplate } from './html.js';
22
import { BINDING_PREFIXES } from './binding-prefixes.js';
33
import { escapeText, escapeAttr } from './escape.js';
4+
import { assertNotFunctionActionAttr } from './form-action.js';
45
import { lookup, lookupModuleUrl, allTags } from './registry.js';
56
import { stylesToString, isCSS } from './css.js';
67
import { isRepeat } from './repeat.js';
@@ -323,11 +324,17 @@ async function renderTemplate(tr, ctx) {
323324
state = 'in-tag';
324325
attrName = '';
325326
} else {
327+
// #1154: never stringify a function into action=/formaction= (it
328+
// would serialize a server action's source into the served HTML).
329+
assertNotFunctionActionAttr(val, attrName, currentTag);
326330
out += `"${escapeAttr(String(val ?? ''))}"`;
327331
state = 'in-tag';
328332
attrName = '';
329333
}
330334
} else if (state === 'attr-quoted' || state === 'attr-unquoted') {
335+
// Same guard for a hole inside a quoted/unquoted value, the
336+
// `action="${fn}"` and mixed `action="/x/${fn}"` shapes (#1154).
337+
assertNotFunctionActionAttr(val, attrName, currentTag);
331338
out += escapeAttr(String(val ?? ''));
332339
}
333340
}
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// #1154, client half: the browser renderer refuses the same commit, so a
2+
// client re-render can never write a function's source into the live DOM
3+
// (`applyPart`'s 'attr' and 'attr-mixed' cases). Runs under linkedom.
4+
import { test, before } from 'node:test';
5+
import assert from 'node:assert/strict';
6+
import { parseHTML } from 'linkedom';
7+
8+
before(() => {
9+
const { window } = parseHTML('<!doctype html><html><body></body></html>');
10+
globalThis.document = window.document;
11+
globalThis.DocumentFragment = window.DocumentFragment;
12+
globalThis.Node = window.Node;
13+
globalThis.Element = window.Element;
14+
globalThis.Comment = window.Comment;
15+
globalThis.Text = window.Text;
16+
globalThis.NodeFilter = window.NodeFilter;
17+
globalThis.HTMLElement = window.HTMLElement;
18+
});
19+
20+
let html, render;
21+
before(async () => {
22+
({ html } = await import('../../src/html.js'));
23+
({ render } = await import('../../src/render-client.js'));
24+
});
25+
26+
async function fakeAction() { const S = 'CLIENT_SECRET'; return S; }
27+
28+
test('client render of action=${fn} throws, DOM never carries the source', () => {
29+
const host = document.createElement('div');
30+
assert.throws(
31+
() => render(html`<form method="post" action=${fakeAction}></form>`, host),
32+
/function was interpolated into action=/,
33+
);
34+
assert.ok(!host.innerHTML.includes('CLIENT_SECRET'));
35+
});
36+
37+
test('client render of mixed action="/x/${fn}" throws', () => {
38+
const host = document.createElement('div');
39+
assert.throws(
40+
() => render(html`<form action="/x/${fakeAction}"></form>`, host),
41+
/function was interpolated into action=/,
42+
);
43+
assert.ok(!host.innerHTML.includes('CLIENT_SECRET'));
44+
});
45+
46+
test('string action still renders on the client', () => {
47+
const host = document.createElement('div');
48+
render(html`<form method="post" action=${'/submit'}></form>`, host);
49+
const form = host.querySelector('form');
50+
assert.equal(form.getAttribute('action'), '/submit');
51+
});
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
// #1154: a function interpolated into a form-action attribute must throw,
2+
// never stringify. At SSR a `'use server'` import is the REAL function, so
3+
// `String(fn)` would serialize the action's source (secrets included) into
4+
// the served HTML. Covers every hole shape that used to leak (unquoted,
5+
// quoted, mixed, and `formaction` on a submit button), plus the byte-identical
6+
// passthrough for string-valued action attributes.
7+
import { test, before } from 'node:test';
8+
import assert from 'node:assert/strict';
9+
10+
let html, renderToString;
11+
before(async () => {
12+
({ html } = await import('../../src/html.js'));
13+
({ renderToString } = await import('../../src/render-server.js'));
14+
});
15+
16+
// The secret sentinel must never appear in any output, thrown or not.
17+
const SECRET = 'postgres://user:SECRET@host/db';
18+
async function leaky(input) { const conn = SECRET; return { success: true, conn, input }; }
19+
20+
test('unquoted action=${fn} throws instead of leaking source', async () => {
21+
await assert.rejects(
22+
() => renderToString(html`<form method="post" action=${leaky}></form>`, { ssr: true }),
23+
/function was interpolated into action=/,
24+
);
25+
});
26+
27+
test('quoted action="${fn}" throws identically', async () => {
28+
await assert.rejects(
29+
() => renderToString(html`<form method="post" action="${leaky}"></form>`, { ssr: true }),
30+
/function was interpolated into action=/,
31+
);
32+
});
33+
34+
test('mixed hole action="/x/${fn}" throws', async () => {
35+
await assert.rejects(
36+
() => renderToString(html`<form method="post" action="/x/${leaky}"></form>`, { ssr: true }),
37+
/function was interpolated into action=/,
38+
);
39+
});
40+
41+
test('formaction=${fn} on a submit button throws', async () => {
42+
await assert.rejects(
43+
() => renderToString(html`<form method="post"><button formaction=${leaky}>Go</button></form>`, { ssr: true }),
44+
/function was interpolated into formaction=/,
45+
);
46+
});
47+
48+
test('no thrown message ever carries the function source', async () => {
49+
for (const tpl of [
50+
html`<form action=${leaky}></form>`,
51+
html`<form action="${leaky}"></form>`,
52+
]) {
53+
let msg = '';
54+
try { await renderToString(tpl, { ssr: true }); } catch (e) { msg = String(e && e.message); }
55+
assert.ok(msg.length > 0, 'render must throw');
56+
assert.ok(!msg.includes('SECRET'), 'error message must not embed the source');
57+
}
58+
});
59+
60+
test('string-valued action renders byte-identically to before', async () => {
61+
assert.equal(
62+
await renderToString(html`<form method="post" action=${'/submit'}></form>`, { ssr: true }),
63+
'<form method="post" action="/submit"></form>',
64+
);
65+
assert.equal(
66+
await renderToString(html`<form method="post" action="${'/submit'}"></form>`, { ssr: true }),
67+
'<form method="post" action="/submit"></form>',
68+
);
69+
assert.equal(
70+
await renderToString(html`<form action="/x?a=${1}"></form>`, { ssr: true }),
71+
'<form action="/x?a=1"></form>',
72+
);
73+
});
74+
75+
test('functions in OTHER attributes keep the existing stringify behaviour', async () => {
76+
// Narrow claim: only action/formaction throw. Anything else is unchanged
77+
// (arguably also a bug, but out of #1154's scope by design).
78+
const out = await renderToString(html`<div title=${leaky}></div>`, { ssr: true });
79+
assert.ok(out.startsWith('<div title="'));
80+
});

0 commit comments

Comments
 (0)