Skip to content

Commit ca1c19f

Browse files
committed
fix: warn unconditionally when reflection drops a function
The warning was gated on `process.env.NODE_ENV !== 'production'`, copied from the client router's dev-warning idiom. That gate does not survive the dist build: `scripts/build-framework-dist.js` runs esbuild with `platform: 'browser'` and `minify: true`, which substitutes `process.env.NODE_ENV` with `"production"` and folds the check to a constant. The SSR half of the warning was therefore unreachable in every published build, which is how every installed app runs, and SSR is the half that matters most, since that is where the leaked source reached visitors. Dropping the gate also matches the sibling path this guard mirrors: the `.prop=${fn}` unserializable-value drop in render-server.js warns unconditionally. There is no volume to suppress either way, since the warning fires only on a genuine mistake and reflection runs per assignment rather than per frame. Adds test/bun/reflect-function-guard.mjs, which is what caught this. It imports through the bare `@webjsdev/core` specifier, so it resolves to dist/ and is the only layer that sees a bundler-folded guard. It also pins the cross-runtime claim the guard makes moot: Function.prototype .toString exposes different amounts on Node and Bun, so refusing to stringify is what makes the exposure runtime-independent.
1 parent f7a5666 commit ca1c19f

7 files changed

Lines changed: 151 additions & 9 deletions

File tree

‎.agents/skills/webjs/references/components.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ The bare form is shorthand: `count: Number` means `prop(Number)`. Use `prop()` t
5757

5858
For an array-typed prop pass `Array`, not `Object` (`array-prop-uses-array-type` flags the `Object` form). For anything the built-in converters cannot parse (Date, Map, Set) supply a `converter`.
5959

60-
**A `reflect: true` property holding a FUNCTION drops its attribute instead of writing one.** A function has no HTML attribute representation, and the serializations it would otherwise get are both useless and dangerous. `String(fn)` is the function's SOURCE, so a reflected `'use server'` action would ship its whole body, closure secrets included, to every visitor, and `JSON.stringify(fn)` is `undefined`, which lands in the attribute as the literal four-character string. So the reflection path treats a function like `null`, removes the attribute, and warns in dev naming the property, the tag, and the attribute. This holds on both sides, since SSR and the client-side setter run the same path, and it holds for every property name (the leak was never specific to one called `action`). The one exception is a property with a custom `converter.toAttribute`, which runs first and is left alone, because an author who writes one has taken responsibility for serializing whatever they are handed. If you need a function on a component, use a plain property or a signal and do not mark it `reflect`.
60+
**A `reflect: true` property holding a FUNCTION drops its attribute instead of writing one.** A function has no HTML attribute representation, and the serializations it would otherwise get are both useless and dangerous. `String(fn)` is the function's SOURCE, so a reflected `'use server'` action would ship its whole body, closure secrets included, to every visitor, and `JSON.stringify(fn)` is `undefined`, which lands in the attribute as the literal four-character string. So the reflection path treats a function like `null`, removes the attribute, and warns naming the property, the tag, and the attribute. This holds on both sides, since SSR and the client-side setter run the same path, and it holds for every property name (the leak was never specific to one called `action`). The one exception is a property with a custom `converter.toAttribute`, which runs first and is left alone, because an author who writes one has taken responsibility for serializing whatever they are handed. If you need a function on a component, use a plain property or a signal and do not mark it `reflect`.
6161

6262
**Never use a class-field declaration OR initializer** (`count = 0`, `student: Student = {...}`, `todos!: Todo[]`). Under `useDefineForClassFields` even a type-only `todos!: Todo[]` compiles to define an own property after `super()`, which clobbers the prototype's reactive accessor and silently breaks reactivity. Only declare props in the factory and read/write them off `this`. The `reactive-props-no-class-field` rule catches this.
6363

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ The bound, refused, and allowed shapes in full. Every "no" row is a binding that
111111
| `encoding="..."` as an ATTRIBUTE on a bound form | **no** | inert in HTML (`form.encoding` reads back `enctype`), so both renderers ignore it and still supply `enctype`. Only the `.encoding` PROPERTY aliases enctype, and that spelling IS refused, one row up |
112112
| `.formAction=` on a button or input | yes | same reason, that is where `formAction` reflects |
113113
| `.action=` on any other native tag | **no** | a plain expando (`<div .action=${fn}>`, `<button .action=${fn}>`), reflecting nothing, so nothing reaches the markup |
114-
| `.action=` on a custom element | **no** | an author-defined property, not a reflected IDL attribute, so a function is a legitimate value. One declared `reflect: true` reflects on a path outside these commit sites, which used to write `String(value)` and emit the source; it now removes the attribute and warns in dev instead |
114+
| `.action=` on a custom element | **no** | an author-defined property, not a reflected IDL attribute, so a function is a legitimate value. One declared `reflect: true` reflects on a path outside these commit sites, which used to write `String(value)` and emit the source; it now removes the attribute and warns instead |
115115
| `?action=` | yes | never leaked, but it is meaningless, so it is refused rather than silently emitting a bare `action=""` |
116116
| `@action=` unquoted | **no** | an event listener, and a function is exactly what one takes |
117117
| `@action="${fn}"` quoted | yes | quoting makes it an ordinary attribute again, so it leaks |

‎packages/core/src/component.js‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,16 +126,25 @@ function safeString(v) {
126126
* the leak this guard exists to prevent, and a server log is not always a
127127
* private place.
128128
*
129-
* Silent in production, matching the client router's dev-warning idiom. There
130-
* is nothing an end user can do about it, and the guard has already removed
131-
* the attribute either way.
129+
* UNCONDITIONAL, matching the `.prop=${fn}` unserializable-value drop in
130+
* `render-server.js`, which is the sibling path this guard mirrors. Two
131+
* reasons not to gate it on a dev flag. It fires only on a genuine mistake (a
132+
* function is never a meaningful attribute value), so there is no volume to
133+
* suppress, and reflection runs per assignment rather than per frame.
134+
*
135+
* More decisively, a `NODE_ENV` gate does not survive the dist build.
136+
* `scripts/build-framework-dist.js` runs esbuild with `platform: 'browser'`
137+
* and `minify: true`, which substitutes `process.env.NODE_ENV` with
138+
* `"production"` and folds the check to a constant. The SSR half of the
139+
* warning would then be unreachable in every published build, which is how
140+
* every installed app runs, and SSR is the half that matters most, since that
141+
* is where the leaked source reached visitors.
132142
*
133143
* @param {{ constructor: unknown, tagName?: string }} host
134144
* @param {string} propName
135145
* @param {string} attrName
136146
*/
137147
function warnFunctionReflection(host, propName, attrName) {
138-
if (typeof process !== 'undefined' && process.env && process.env.NODE_ENV === 'production') return;
139148
if (typeof console === 'undefined' || !console.warn) return;
140149
const tag = tagOf(/** @type any */ (host.constructor)) || host.tagName?.toLowerCase() || 'unknown';
141150
console.warn(
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
/**
2+
* Cross-runtime proof that a `reflect: true` property holding a function never
3+
* writes that function into its attribute, under WHICHEVER runtime executes
4+
* this file (#1169). Run under both:
5+
*
6+
* node test/bun/reflect-function-guard.mjs
7+
* bun test/bun/reflect-function-guard.mjs
8+
*
9+
* The reflect path is plain DOM-shim use rather than a serializer / listener /
10+
* stream surface, so a divergence in the GUARD itself is not what this is
11+
* watching for. What makes it worth proving on both runtimes is the thing the
12+
* guard removes: the severity of the leak was runtime-dependent.
13+
*
14+
* `String(fn)` is `Function.prototype.toString`, and what that returns is not
15+
* the same on both. On Node it is the source text as written, so a module-scope
16+
* `const` the body reads appears only as its identifier. Bun transpiles a
17+
* module before the engine sees it and can fold a module-scope string literal
18+
* straight into the body, so the same function can report the SECRET ITSELF
19+
* where Node reports the name it was read through. That is a transpiler's
20+
* internal choice rather than a documented boundary, so it is not a rule to
21+
* build a habit on, which is exactly why refusing to stringify is the fix
22+
* rather than sanitizing whatever came back.
23+
*
24+
* So this asserts the strong claim on both runtimes: neither the identifier
25+
* nor the folded value reaches the output, because nothing is stringified at
26+
* all. A plain assert script (not `*.test.mjs`, so the runner does not
27+
* double-run it), exiting non-zero on failure. Run from the repo root so the
28+
* bare `@webjsdev/core` specifier resolves to the workspace package.
29+
*/
30+
import assert from 'node:assert/strict';
31+
import { html, WebComponent, prop } from '@webjsdev/core';
32+
import { renderToString } from '@webjsdev/core/server';
33+
34+
const runtime = process.versions.bun ? `bun ${process.versions.bun}` : `node ${process.versions.node}`;
35+
36+
// A module-scope literal the action closes over. This is the binding Bun may
37+
// fold into the function body and Node will not, so both spellings are checked
38+
// below and neither may appear.
39+
const VENDOR_API_KEY = 'sk_live_REFLECT_FOLDED_MARKER';
40+
41+
async function leakyAction() {
42+
const header = `Bearer ${VENDOR_API_KEY}`;
43+
const inline = 'REFLECT_INLINE_MARKER';
44+
return header + inline;
45+
}
46+
47+
class ReflectProbe extends WebComponent({
48+
// The String branch, which is where the leak lived.
49+
label: prop(String, { reflect: true }),
50+
// Untyped, so the same fall-through is reached without a declared type.
51+
payload: prop({ reflect: true }),
52+
// JSON.stringify(fn) is `undefined`, which setAttribute wrote as that
53+
// literal four-character string. No source leak, same defect.
54+
config: prop(Object, { reflect: true }),
55+
// An ordinary value, to prove the guard did not break reflection.
56+
ok: prop(String, { reflect: true }),
57+
// An author-supplied converter runs first and is deliberately untouched.
58+
conv: prop(String, { reflect: true, converter: { toAttribute: (v) => `custom:${typeof v}` } }),
59+
}) {
60+
constructor() {
61+
super();
62+
this.label = leakyAction;
63+
this.payload = leakyAction;
64+
this.config = leakyAction;
65+
this.ok = 'plain-string';
66+
this.conv = leakyAction;
67+
}
68+
render() { return html`<span>x</span>`; }
69+
}
70+
ReflectProbe.register('bun-reflect-probe');
71+
72+
// The guard warns on every drop, and a passing run should not print a wall of
73+
// warnings that read as failures. Capture them and assert on the count instead.
74+
//
75+
// This capture is what caught the warning being dead code in the SHIPPED
76+
// bundle. The check used to be gated on `process.env.NODE_ENV`, which
77+
// `scripts/build-framework-dist.js` folds to a constant (esbuild substitutes
78+
// it under `platform: 'browser'` + `minify`), so the SSR half never warned in
79+
// any published build. Running this through the bare `@webjsdev/core`
80+
// specifier, which resolves to `dist/`, is the only layer that sees it.
81+
const warnings = [];
82+
const originalWarn = console.warn;
83+
console.warn = (...args) => warnings.push(args.join(' '));
84+
let out;
85+
try {
86+
out = await renderToString(html`<bun-reflect-probe></bun-reflect-probe>`);
87+
} finally {
88+
console.warn = originalWarn;
89+
}
90+
91+
// The claim the whole guard exists for, in both spellings the two runtimes
92+
// produce. The inline marker covers a literal written inside the body (which
93+
// both runtimes emit), the folded marker covers the module-scope binding Bun
94+
// may inline, and the identifier covers what Node emits in its place.
95+
for (const marker of ['REFLECT_INLINE_MARKER', 'REFLECT_FOLDED_MARKER', 'VENDOR_API_KEY']) {
96+
assert.ok(!out.includes(marker), `[${runtime}] the function source reached the output via ${marker}: ${out}`);
97+
}
98+
99+
// The attributes are absent, not merely emptied. An empty attribute would be a
100+
// different observable than the removal the guard promises.
101+
for (const attr of ['label=', 'payload=', 'config=']) {
102+
assert.ok(!out.includes(attr), `[${runtime}] ${attr} should be removed, not written: ${out}`);
103+
}
104+
assert.ok(!out.includes('undefined'), `[${runtime}] wrote a literal "undefined": ${out}`);
105+
106+
// Ordinary reflection is unchanged, and the author override still wins.
107+
assert.ok(out.includes('ok="plain-string"'), `[${runtime}] a normal value must still reflect: ${out}`);
108+
assert.ok(out.includes('conv="custom:function"'), `[${runtime}] converter.toAttribute must still win: ${out}`);
109+
110+
// One warning per dropped property, and none of them prints the value: the
111+
// warning path is the other place the source could escape, and a server log is
112+
// not always a private place.
113+
assert.equal(warnings.length, 3, `[${runtime}] expected one warning per dropped prop, got ${warnings.length}`);
114+
for (const message of warnings) {
115+
for (const marker of ['REFLECT_INLINE_MARKER', 'REFLECT_FOLDED_MARKER']) {
116+
assert.ok(!message.includes(marker), `[${runtime}] the warning leaked the source it refused to write: ${message}`);
117+
}
118+
}
119+
120+
console.log(`[${runtime}] reflect-function-guard: function props dropped, no source in output or warnings, string + converter intact ✓`);
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
/**
2+
* Run the cross-runtime reflect-function-guard proof under WHICHEVER runtime
3+
* executes the suite. Picked up by the root `node --test` runner (so `npm test`
4+
* exercises the Node path); CI also runs `bun test/bun/reflect-function-guard.mjs`
5+
* for the Bun path. The proof is a plain assert script
6+
* (`reflect-function-guard.mjs`, not `*.test.mjs`, so the runner does not
7+
* double-run it); importing it runs it and throws on any failure.
8+
*/
9+
import { test } from 'node:test';
10+
11+
test('a reflect:true prop never stringifies a function on this runtime', async () => {
12+
await import('./reflect-function-guard.mjs');
13+
});

‎website/app/docs/components/page.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ UserCard.register('user-card');</pre>
129129
<p>Property names are automatically converted between camelCase (JavaScript) and kebab-case (HTML). A property named <code>userName</code> observes the attribute <code>user-name</code>.</p>
130130
131131
<h3>Reflecting a function is refused</h3>
132-
<p>A <code>reflect: true</code> property holding a <strong>function</strong> removes its attribute instead of writing one, and warns in dev naming the property, the tag, and the attribute. A function has no HTML attribute representation, and the serializations it would otherwise get are both useless and dangerous. <code>String(fn)</code> is the function's source, so a reflected <code>'use server'</code> action would ship its whole body, closure secrets included, to every visitor, and <code>JSON.stringify(fn)</code> is <code>undefined</code>, which lands in an <code>Object</code>-typed or <code>Array</code>-typed attribute as that literal four-character string. Both sides behave the same, since SSR and the client-side setter run one reflection path, and it applies to every property name rather than only one called <code>action</code>.</p>
132+
<p>A <code>reflect: true</code> property holding a <strong>function</strong> removes its attribute instead of writing one, and warns naming the property, the tag, and the attribute. A function has no HTML attribute representation, and the serializations it would otherwise get are both useless and dangerous. <code>String(fn)</code> is the function's source, so a reflected <code>'use server'</code> action would ship its whole body, closure secrets included, to every visitor, and <code>JSON.stringify(fn)</code> is <code>undefined</code>, which lands in an <code>Object</code>-typed or <code>Array</code>-typed attribute as that literal four-character string. Both sides behave the same, since SSR and the client-side setter run one reflection path, and it applies to every property name rather than only one called <code>action</code>.</p>
133133
<p>A custom <code>converter.toAttribute</code> runs first and is left alone: an author who writes one has taken responsibility for serializing whatever they are handed. To keep a function on a component, use a plain property or a signal and do not mark it <code>reflect</code>. To bind a server action to a form, use <code>&lt;form action=\${importedAction}&gt;</code> (see <a href="/docs/server-actions">Server Actions</a>), which resolves the action's identity rather than stringifying it.</p>
134134
135135
<blockquote>If you are coming from React: properties in WebJs serve a similar role to props, but they are backed by real DOM attributes. You can inspect them in DevTools, set them from plain HTML, and they survive page serialization during SSR.</blockquote>

0 commit comments

Comments
 (0)