diff --git a/.changeset/field-widget-dom-prop-whitelist.md b/.changeset/field-widget-dom-prop-whitelist.md new file mode 100644 index 000000000..1fe444442 --- /dev/null +++ b/.changeset/field-widget-dom-prop-whitelist.md @@ -0,0 +1,90 @@ +--- +"@object-ui/fields": minor +--- + +Field widgets no longer spread renderer-only props — or arbitrary keys from a +field config — onto the DOM element they render (objectui#3291). + +**Behaviour change:** an unknown key written on a field configuration (or on an +SDUI `field:*` node) stops becoming an HTML attribute on the rendered control. +Nothing reads those attributes, but they were serialized into the DOM, into +snapshots, and into anything scraping rendered markup. + +## What was happening + +Widgets forwarded their leftover props with a bare spread, so whatever a host +handed them became an attribute. Measured on a real form with a real widget: + +``` + +``` + +`zzcanaryobj="[object Object]"` is an ordinary extra key on the field config +being `String()`-ed onto an attribute. React 19 does not warn about any of it: +an all-lowercase unknown attribute is passed through in complete silence, which +is why this went unnoticed. + +Eleven widgets carried a line that looked like it prevented exactly this: + +```ts +const { inputType, ...domProps } = props as any; // "Filter out non-DOM props" +``` + +`inputType` is already stripped by the form renderer before a widget sees it, +so that line filtered nothing — the comment actively misled. It is gone. + +## What changed + +- New `toDomProps(props)`, exported from `@object-ui/fields`. It keeps only + what may legitimately become a DOM attribute and drops the rest. +- The 14 field widgets that spread onto a host element now go through it: + `text`, `textarea`, `number`, `boolean`, `date`, `datetime`, `time`, `email`, + `phone`, `url`, `password`, `currency`, `percent`, and `select`. + +**It is a whitelist, not a list of keys to drop.** The largest leak source is +not any named renderer prop — it is the open tail of author-supplied keys. The +form renderer destructures a fixed set of known keys and forwards the rest +verbatim, and `SchemaRenderer` is wider still: it spreads the whole authored +node as props with no strip layer at all, so on that path a widget's own spread +is the only line of defence. A blacklist of today's renderer-only props would +pass every canary above and would not stop the next authored key either. + +The forwarded set is the one `FieldWidgetComponentProps` already **declares**: +`id`, `name`, `autoFocus`, `tabIndex`, `onBlur`, `onFocus`, `onClick`, +`className`, `disabled`, plus `aria-*` and the `data-*` family. Until now that +was a type-level claim a widget could violate at runtime just by spreading; +`toDomProps` is its executable form. + +Two compile-time assertions tie the helper to the declaration, and it is worth +being exact about which drift each one prevents: + +- the contract's DOM pass-through block is now a named type + (`FieldWidgetDomProps`), and **both directions are compiler-bound**: + forwarding a key the contract does not declare fails to compile, and + declaring a DOM key the helper does not forward fails to compile too. The + second direction guards *declared but not delivered* — a key that + type-checks, reads as supported, and silently never reaches the element. The + leak test structurally cannot see that class of bug: it looks for attributes + that arrive, not for ones that go missing. +- `className` and `disabled` are bound in the **forward direction only**. They + are DOM-legal and are forwarded, but they live in the controlled-input block + because widgets also interpret them, so they are deliberately outside + `FieldWidgetDomProps`. + +An HTML global attribute the contract does not declare (`role`, say) is no +longer forwarded. It only ever arrived through the open spread. If a field node +should be able to author one, declare it on `FieldWidgetComponentProps` and add +it to the whitelist — the fix belongs at the contract, not in a wider spread. + +## Regression gate + +A new contract test renders **every** registered field widget through **both** +hosts — the real form renderer and `SchemaRenderer` — and fails on any +attribute HTML does not define for that element. It walks real DOM attributes +rather than listening for React warnings (React 19 is silent for the exact case +that leaked), asserts a validation error genuinely rendered before scanning the +error variant, and calibrates its own judge against two fixtures: standard +markup that must produce zero findings, and planted fake attributes that must +all be found. A new widget type is covered automatically — the sweep is derived +from the widget registry, so adding one without covering it fails the test. diff --git a/packages/fields/src/__tests__/widget-dom-leak-e2e.test.tsx b/packages/fields/src/__tests__/widget-dom-leak-e2e.test.tsx new file mode 100644 index 000000000..1d898dd77 --- /dev/null +++ b/packages/fields/src/__tests__/widget-dom-leak-e2e.test.tsx @@ -0,0 +1,598 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * GATE: no field widget may leak a non-DOM prop onto a DOM element + * (objectui#3291). + * + * Every registered field widget is rendered through BOTH hosts, and every + * attribute of every element it produced is checked against what HTML actually + * defines. An attribute nobody can explain is a leak. + * + * ## What this catches that a unit test cannot + * + * The leak was never in one widget. It was structural: a widget spread + * `{...props}` onto its control, and the hosts hand a widget more than the DOM + * can take. Measured on `origin/main`, a real form, a real widget: + * + * ``` + * + * ``` + * + * `zzcanaryobj="[object Object]"` is an ordinary key an author wrote on the + * field config. That is why the fix is a WHITELIST (`toDomProps`) and why this + * test plants canaries rather than checking a list of known-bad names: a + * blacklist of today's renderer props would pass all three canaries above. + * + * ## Four things that silently defeat a test like this + * + * 1. **React warnings prove nothing.** React 19 passes an all-lowercase + * unknown attribute through in COMPLETE silence. In the audit sweep the + * only warning came from a camelCase canary — which was written to the DOM + * anyway. So this walks real DOM attributes and never listens for console + * output. + * 2. **Both hosts, or the result is a false pass.** The form renderer strips a + * known set before forwarding; `SchemaRenderer` spreads the whole authored + * node with NO strip layer, so it leaks strictly more (`label` is only + * visible there). On that path the widget's own spread is the only defence. + * 3. **An error variant that produces no error tests nothing.** `required` + + * a `false` boolean does NOT fail here — this repo made `required` a + * presence check, so `false` is a value (cloud#972). The audit's first pass + * under-reported the `error` leak by one widget for exactly this reason. + * Every error variant therefore ASSERTS the message rendered before it + * scans; a variant that silently produces no error fails the test. + * 4. **This repo runs happy-dom, not jsdom** (`vitest.config.mts`), whose IDL + * coverage has real gaps — `select[size]`, `option[label]`, `textarea[wrap]` + * and `col[span]` are all standard HTML that happy-dom does not reflect. + * See {@link isKnownAttribute} for how the judge is built, and + * {@link HAPPY_DOM_IDL_GAPS} for each measured exception and its reason. + * Every one of those four was found BY the calibration fixture below, not + * by guesswork. + * + * ## The judge proves itself + * + * Two fixtures run before the sweep: standard markup that must yield ZERO + * findings, and markup with planted fake attributes that must ALL be found. + * When a happy-dom upgrade changes IDL coverage, those fail loudly instead of + * the sweep going quietly blind. + * + * ## Deliberate coverage boundary + * + * Popovers are NOT opened. Radix needs pointer-capture APIs happy-dom does not + * implement. Every widget's props spread lands on its inline control (the + * trigger for a picker), which always renders — so the spread site IS covered, + * but content that exists only inside an open dropdown is NOT scanned by this + * test. + * + * Widgets are registered from STATIC imports wrapped in `withFieldCarrier`, + * never via `registerAllFields()`, which wraps every loader in `React.lazy` — + * an unbounded module load inside a bounded `waitFor` is this repo's known + * flake generator (AGENTS.md 测试纪律 / objectui#3010). + */ + +import type { ComponentType } from 'react'; +import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ComponentRegistry } from '@object-ui/core'; +// Module scope: pulls in the form renderer's registration side effect. +import '@object-ui/components'; +import { SchemaRenderer } from '@object-ui/react'; + +import { withFieldCarrier } from '../withFieldCarrier'; +import { FORM_FIELD_TYPES } from '../index'; + +import { TextField } from '../widgets/TextField'; +import { TextAreaField } from '../widgets/TextAreaField'; +import { NumberField } from '../widgets/NumberField'; +import { BooleanField } from '../widgets/BooleanField'; +import { SelectField } from '../widgets/SelectField'; +import { DateField } from '../widgets/DateField'; +import { DateTimeField } from '../widgets/DateTimeField'; +import { TimeField } from '../widgets/TimeField'; +import { EmailField } from '../widgets/EmailField'; +import { PhoneField } from '../widgets/PhoneField'; +import { UrlField } from '../widgets/UrlField'; +import { MultiSelectField } from '../widgets/MultiSelectField'; +import { RadioField } from '../widgets/RadioField'; +import { CheckboxesField } from '../widgets/CheckboxesField'; +import { TagsField } from '../widgets/TagsField'; +import { CurrencyField } from '../widgets/CurrencyField'; +import { PercentField } from '../widgets/PercentField'; +import { PasswordField } from '../widgets/PasswordField'; +import { RichTextField } from '../widgets/RichTextField'; +import { LookupField } from '../widgets/LookupField'; +import { FileField } from '../widgets/FileField'; +import { ImageField } from '../widgets/ImageField'; +import { LocationField } from '../widgets/LocationField'; +import { FormulaField } from '../widgets/FormulaField'; +import { SummaryField } from '../widgets/SummaryField'; +import { AutoNumberField } from '../widgets/AutoNumberField'; +import { UserField } from '../widgets/UserField'; +import { ObjectField } from '../widgets/ObjectField'; +import { VectorField } from '../widgets/VectorField'; +import { GridField } from '../widgets/GridField'; +import { ColorField } from '../widgets/ColorField'; +import { SliderField } from '../widgets/SliderField'; +import { RatingField } from '../widgets/RatingField'; +import { CodeField } from '../widgets/CodeField'; +import { AvatarField } from '../widgets/AvatarField'; +import { AddressField } from '../widgets/AddressField'; +import { GeolocationField } from '../widgets/GeolocationField'; +import { SignatureField } from '../widgets/SignatureField'; +import { QRCodeField } from '../widgets/QRCodeField'; +import { ObjectRefField } from '../widgets/ObjectRefField'; +import { FilterConditionField } from '../widgets/FilterConditionField'; +import { RecipientPickerField } from '../widgets/RecipientPickerField'; + +/* ════════════════════════════════════════════════════════════════════════════ + * The judge: is this attribute one HTML actually defines? + * ══════════════════════════════════════════════════════════════════════════ */ + +/** Open families. `data-*` is the one open family the widget contract declares. */ +const OPEN_PREFIXES = [ + 'data-', + 'aria-', + // `cmdk-root` / `cmdk-input` / `cmdk-list` … are marks the cmdk library puts + // on ITS OWN DOM. Not prop pass-through, and present on every cmdk-based + // picker (`object-ref`, `lookup`, …) the moment it renders. + 'cmdk-', +]; + +/** + * Global HTML attributes. Most are also IDL properties and would be caught by + * the reflection check below; they are listed because a missing IDL for a + * genuinely global attribute would otherwise read as a leak. + */ +const GLOBAL_HTML_ATTRIBUTES = new Set([ + 'id', 'class', 'style', 'title', 'lang', 'dir', 'hidden', 'tabindex', 'role', + 'slot', 'part', 'exportparts', 'itemid', 'itemprop', 'itemref', 'itemscope', + 'itemtype', 'translate', 'draggable', 'spellcheck', 'autocapitalize', + 'autocorrect', 'contenteditable', 'enterkeyhint', 'inputmode', 'accesskey', + 'nonce', 'is', 'popover', 'inert', 'autofocus', +]); + +/** + * Attributes whose IDL property is spelled differently enough that the + * case-insensitive reflection match below cannot find them. + */ +const ATTRIBUTE_TO_IDL_ALIAS: Record = { + 'class': 'className', + 'for': 'htmlFor', + 'accept-charset': 'acceptCharset', + 'http-equiv': 'httpEquiv', +}; + +/** + * MEASURED gaps in happy-dom's IDL, kept deliberately tiny — each entry is an + * attribute HTML defines that happy-dom's element does not reflect as a + * property, so reflection alone would report it as a leak. + */ +const HAPPY_DOM_IDL_GAPS: Record> = { + // `HTMLSelectElement.size` is standard; happy-dom does not define it. + select: new Set(['size']), + // `HTMLOptionElement.label` is standard; happy-dom does not define it. + option: new Set(['label']), + // `HTMLTextAreaElement.wrap` is standard; happy-dom does not define it. + textarea: new Set(['wrap']), + // `HTMLTableColElement.span` is standard; happy-dom does not define it. + col: new Set(['span']), + colgroup: new Set(['span']), +}; + +/** + * SVG needs its own list: the reflection trick does NOT hold for SVG under + * happy-dom (`SVGElement` reflects almost nothing), and lucide icons put a + * fixed set of presentation attributes on every icon they render. + */ +const SVG_ATTRIBUTES = new Set([ + 'xmlns', 'xmlns:xlink', 'version', 'viewbox', 'preserveaspectratio', + 'width', 'height', 'x', 'y', 'x1', 'y1', 'x2', 'y2', 'cx', 'cy', 'r', 'rx', + 'ry', 'd', 'points', 'transform', 'fill', 'fill-rule', 'fill-opacity', + 'stroke', 'stroke-width', 'stroke-linecap', 'stroke-linejoin', + 'stroke-dasharray', 'stroke-dashoffset', 'stroke-opacity', 'opacity', + 'clip-path', 'clip-rule', 'mask', 'offset', 'stop-color', 'stop-opacity', + 'gradientunits', 'gradienttransform', 'patternunits', 'text-anchor', + 'dominant-baseline', 'font-size', 'font-family', 'font-weight', 'vector-effect', + 'shape-rendering', 'focusable', 'overflow', 'color', +]); + +/** Lowercased IDL property names on a tag's prototype chain, cached per tag. */ +const idlCache = new Map>(); + +function idlPropertiesFor(tagName: string): Set { + const tag = tagName.toLowerCase(); + const cached = idlCache.get(tag); + if (cached) return cached; + + const names = new Set(); + const element = document.createElement(tag); + for (const own of Object.getOwnPropertyNames(element)) names.add(own.toLowerCase()); + for ( + let proto = Object.getPrototypeOf(element); + proto && proto !== Object.prototype; + proto = Object.getPrototypeOf(proto) + ) { + for (const name of Object.getOwnPropertyNames(proto)) names.add(name.toLowerCase()); + } + idlCache.set(tag, names); + return names; +} + +/** + * The rule: an attribute is legitimate when HTML/SVG defines it for that + * element, or when it belongs to an open family. + * + * The reflection check ("does the element's prototype chain carry a property + * with this name, case-insensitively?") is what makes this maintainable — it + * covers `readonly→readOnly`, `maxlength→maxLength`, `colspan→colSpan` and + * every other per-tag attribute automatically, instead of a hand-kept table + * per element type that would rot. + */ +function isKnownAttribute(element: Element, attribute: string): boolean { + const name = attribute.toLowerCase(); + + if (OPEN_PREFIXES.some((prefix) => name.startsWith(prefix))) return true; + + // Inline event handlers (`onclick`) reflect as IDL properties on every + // element; React never emits them as attributes, so reaching one means a + // handler-shaped prop was stringified onto the DOM. Treat as a leak. + if (name.startsWith('on')) return false; + + const tag = element.tagName.toLowerCase(); + + if (element.namespaceURI === 'http://www.w3.org/2000/svg') { + return SVG_ATTRIBUTES.has(name) || GLOBAL_HTML_ATTRIBUTES.has(name); + } + + if (GLOBAL_HTML_ATTRIBUTES.has(name)) return true; + if (HAPPY_DOM_IDL_GAPS[tag]?.has(name)) return true; + + const idl = idlPropertiesFor(tag); + const alias = ATTRIBUTE_TO_IDL_ALIAS[name]; + if (alias && idl.has(alias.toLowerCase())) return true; + return idl.has(name); +} + +interface Leak { + tag: string; + attribute: string; + value: string; + outerHTML: string; +} + +/** Every unexplained attribute on `root` and its descendants. */ +function findLeaks(root: Element): Leak[] { + const leaks: Leak[] = []; + const elements: Element[] = [root, ...Array.from(root.querySelectorAll('*'))]; + for (const element of elements) { + for (const attribute of Array.from(element.attributes)) { + if (isKnownAttribute(element, attribute.name)) continue; + leaks.push({ + tag: element.tagName.toLowerCase(), + attribute: attribute.name, + value: attribute.value, + outerHTML: element.outerHTML.slice(0, 400), + }); + } + } + return leaks; +} + +/** + * Renders the finding as the assertion's "actual" value, so a failure names + * the widget, the element, the attribute, its value and the markup. A 46-widget + * sweep failing as `expected [] to equal [ …47 items ]` is unusable. + */ +function leakReport(widgetType: string, variant: string, leaks: Leak[]): string { + if (leaks.length === 0) return ''; + const lines = leaks.map( + (leak) => + ` <${leak.tag}> leaked ${leak.attribute}="${leak.value}"\n` + + ` in: ${leak.outerHTML}`, + ); + return ( + `field:${widgetType} [${variant}] leaked ${leaks.length} non-DOM ` + + `attribute(s):\n${lines.join('\n')}` + ); +} + +/* ════════════════════════════════════════════════════════════════════════════ + * The judge proves itself, BEFORE it is trusted on 46 widgets + * ══════════════════════════════════════════════════════════════════════════ */ + +/** + * Ordinary, correct markup. Every attribute here is one HTML defines, several + * chosen precisely because their IDL name differs from the attribute + * (`readonly`/`maxlength`/`colspan`/`class`/`for`), plus the two happy-dom IDL + * gaps (`select[size]`, `option[label]`) and a cmdk mark. + */ +const CLEAN_FIXTURE = ` + +`; + +/** Every planted attribute here MUST be reported. */ +const PLANTED_LEAKS: ReadonlyArray = [ + // The renderer-only props that reached the DOM in the audit. + ['schema', '[object Object]'], + ['error', 'Title is required'], + ['emptyhint', 'Select country first'], + ['datasource', '[object Object]'], + ['dependentvalues', '[object Object]'], + ['dependson', 'country'], + ['inputtype', 'text'], + ['compact', 'true'], + ['onselectrecord', 'function'], + // The SDUI-only extra. + ['label', 'Title'], + // The open tail: arbitrary keys an author wrote on the field config. + ['zzcanary', 'CANARY-STR'], + ['zzcanaryobj', '[object Object]'], + ['zzcanarynum', '42'], + ['zzcanarycamel', 'CANARY-CAMEL'], + ['reference_to', 'contacts'], +]; + +describe('the leak judge is calibrated (objectui#3291)', () => { + it('reports NOTHING on standard markup — no false positives', () => { + const host = document.createElement('div'); + host.innerHTML = CLEAN_FIXTURE; + document.body.appendChild(host); + try { + const leaks = findLeaks(host); + expect( + leaks.map((l) => `<${l.tag}> ${l.attribute}="${l.value}"`).join('\n'), + ).toBe(''); + } finally { + host.remove(); + } + }); + + it('reports EVERY planted fake attribute — no false negatives', () => { + const host = document.createElement('div'); + const planted = PLANTED_LEAKS.map(([name, value]) => `${name}="${value}"`).join(' '); + // On an , so nothing can be excused by a permissive container. + host.innerHTML = ``; + document.body.appendChild(host); + try { + const found = new Set(findLeaks(host).map((leak) => leak.attribute)); + const missed = PLANTED_LEAKS.map(([name]) => name).filter((name) => !found.has(name)); + expect(missed).toEqual([]); + expect(found.size).toBe(PLANTED_LEAKS.length); + } finally { + host.remove(); + } + }); +}); + +/* ════════════════════════════════════════════════════════════════════════════ + * Every registered field widget, both hosts + * ══════════════════════════════════════════════════════════════════════════ */ + +/** + * Static components for the widget map's keys. Kept as one object so the + * parity assertion below can prove it covers the whole registry: a NEW field + * type added to `fieldWidgetMap` without a line here fails loudly rather than + * quietly going unscanned. + */ +const WIDGETS: Record> = { + text: TextField, + textarea: TextAreaField, + number: NumberField, + boolean: BooleanField, + select: SelectField, + date: DateField, + datetime: DateTimeField, + time: TimeField, + email: EmailField, + phone: PhoneField, + url: UrlField, + multiselect: MultiSelectField, + radio: RadioField, + checkboxes: CheckboxesField, + tags: TagsField, + currency: CurrencyField, + percent: PercentField, + password: PasswordField, + markdown: RichTextField, + html: RichTextField, + richtext: RichTextField, + lookup: LookupField, + master_detail: LookupField, + file: FileField, + image: ImageField, + location: LocationField, + formula: FormulaField, + summary: SummaryField, + auto_number: AutoNumberField, + user: UserField, + owner: UserField, + object: ObjectField, + vector: VectorField, + grid: GridField, + color: ColorField, + slider: SliderField, + rating: RatingField, + code: CodeField, + avatar: AvatarField, + address: AddressField, + geolocation: GeolocationField, + signature: SignatureField, + qrcode: QRCodeField, + 'object-ref': ObjectRefField, + 'filter-condition': FilterConditionField, + 'recipient-picker': RecipientPickerField, +}; + +/** Option widgets render an "unfillable" placeholder unless offered a list. */ +const OPTION_TYPES = new Set(['select', 'multiselect', 'radio', 'checkboxes', 'tags']); +const OPTIONS = [ + { label: 'Alpha', value: 'alpha' }, + { label: 'Beta', value: 'beta' }, +]; + +/** + * The value each variant starts from: `null` is MISSING for the required check + * this repo implements (presence, not truthiness — `false` and `0` are values, + * cloud#972), so one value drives a real validation failure for every type, + * including `boolean`. Trap 3 above is why that matters, and why every error + * variant asserts the message before it scans. + */ +const MISSING_VALUE = null; + +/** Author-written extras — the open tail that a blacklist cannot close. */ +const AUTHORED_EXTRAS = { + zzcanary: 'CANARY-STR', + zzcanaryobj: { nested: true }, + zzcanarynum: 42, + zzcanaryCamel: 'CANARY-CAMEL', + reference_to: 'contacts', +}; + +function fieldConfig(type: string, extras: Record = {}) { + return { + name: 'f', + label: 'F', + type: `field:${type}`, + ...(OPTION_TYPES.has(type) ? { options: OPTIONS } : {}), + ...extras, + }; +} + +function renderForm(field: Record, required: boolean) { + const Form = ComponentRegistry.get('form')!; + return render( +
{}, + }} + />, + ); +} + +/** + * The form row. Waiting on `[data-field]` (emitted by `FormItem` for EVERY + * field) rather than widget-specific copy keeps the wait condition identical + * for all 46 widgets and independent of what any one of them renders. + */ +async function formRow(): Promise { + return waitFor(() => { + const row = document.querySelector('[data-field="f"]'); + if (!row) throw new Error('field row never rendered'); + return row; + }); +} + +beforeAll(() => { + for (const [type, Widget] of Object.entries(WIDGETS)) { + // `withFieldCarrier` is the real registration seam (objectui#3233) — going + // around it would test a widget in a shape no host ever produces. + ComponentRegistry.register(type, withFieldCarrier(Widget) as any, { + namespace: 'field', + skipFallback: true, + }); + } +}, 60000); + +beforeEach(() => { + if (!(Element.prototype as any).scrollIntoView) { + (Element.prototype as any).scrollIntoView = () => {}; + } +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('no field widget leaks non-DOM props to the DOM (objectui#3291)', () => { + it('covers every field type the form can render', () => { + // The gate's reach. Without this, adding a widget adds an unscanned widget. + expect(Object.keys(WIDGETS).sort()).toEqual([...FORM_FIELD_TYPES].sort()); + }); + + const types = Object.keys(WIDGETS); + + it.each(types)('field:%s — form path, plain field', async (type) => { + renderForm(fieldConfig(type), false); + const row = await formRow(); + expect(leakReport(type, 'form/plain', findLeaks(row))).toBe(''); + }); + + it.each(types)('field:%s — form path, author-written extra keys', async (type) => { + // The largest measured leak source: the form renderer destructures a fixed set + // of known keys and forwards the rest verbatim, so anything else an author + // put on the field config arrives at the widget as a prop. + renderForm(fieldConfig(type, AUTHORED_EXTRAS), false); + const row = await formRow(); + expect(leakReport(type, 'form/authored-extras', findLeaks(row))).toBe(''); + }); + + it.each(types)('field:%s — form path, after a real validation failure', async (type) => { + renderForm(fieldConfig(type), true); + const row = await formRow(); + + fireEvent.click(screen.getByRole('button', { name: /create/i })); + + // Trap 3: scan only once the error is PROVEN to exist. A variant that + // silently produced none would otherwise pass while testing nothing. + await waitFor(() => { + expect(document.querySelector('[data-field="f"]')?.textContent ?? '').toContain( + 'is required', + ); + }); + + expect(leakReport(type, 'form/validation-error', findLeaks(await formRow()))).toBe(''); + }); + + it.each(types)('field:%s — SDUI path, plain node', async (type) => { + // Strictly wider than the form path: `SchemaRenderer` spreads the whole + // authored node as props with no strip layer, so the widget's own spread + // is the only defence here. + const { container } = render( + , + ); + await waitFor(() => expect(container.firstElementChild).toBeTruthy()); + expect(leakReport(type, 'sdui/plain', findLeaks(container))).toBe(''); + }); + + it.each(types)('field:%s — SDUI path, author-written extra keys', async (type) => { + const { container } = render( + , + ); + await waitFor(() => expect(container.firstElementChild).toBeTruthy()); + expect(leakReport(type, 'sdui/authored-extras', findLeaks(container))).toBe(''); + }); +}); diff --git a/packages/fields/src/index.tsx b/packages/fields/src/index.tsx index 650e449ee..1bada455d 100644 --- a/packages/fields/src/index.tsx +++ b/packages/fields/src/index.tsx @@ -2461,5 +2461,13 @@ export * from './widgets/TagsField'; // read of its own. export { withFieldCarrier } from './withFieldCarrier'; +// The whitelist that decides what a widget's `...props` spread may put on a +// DOM element (objectui#3291) — the runtime executor of the "DOM pass-through" +// block of `FieldWidgetComponentProps`. Exported for the same reason as +// `withFieldCarrier` above: a widget authored outside this repo needs to reach +// it, or it re-grows the bare spread this closed. +export { toDomProps } from './widgets/toDomProps'; +export type { DomProps } from './widgets/toDomProps'; + // Initialize registry registerAllFields(); diff --git a/packages/fields/src/widgets/BooleanField.tsx b/packages/fields/src/widgets/BooleanField.tsx index bf2ee60ef..a9b41665f 100644 --- a/packages/fields/src/widgets/BooleanField.tsx +++ b/packages/fields/src/widgets/BooleanField.tsx @@ -1,6 +1,7 @@ import React, { useId } from 'react'; import { Switch, Checkbox, Label } from '@object-ui/components'; import { FieldWidgetComponentProps } from './types'; +import { toDomProps } from './toDomProps'; /** * BooleanField - Toggle input supporting switch and checkbox variants @@ -19,8 +20,7 @@ export function BooleanField({ value, onChange, field, readonly, ...props }: Fie return {value ? 'Yes' : 'No'}; } - // Filter out non-DOM props - const { inputType, ...domProps } = props as any; + const domProps = toDomProps(props); if (widget === 'checkbox') { return ( diff --git a/packages/fields/src/widgets/CurrencyField.tsx b/packages/fields/src/widgets/CurrencyField.tsx index 2c42730e3..9ca299fbe 100644 --- a/packages/fields/src/widgets/CurrencyField.tsx +++ b/packages/fields/src/widgets/CurrencyField.tsx @@ -1,6 +1,7 @@ import React from 'react'; import { Input, EmptyValue } from '@object-ui/components'; import { FieldWidgetComponentProps } from './types'; +import { toDomProps } from './toDomProps'; import { useLocalization } from '@object-ui/i18n'; import { resolveFieldCurrency } from '../currency'; @@ -70,7 +71,7 @@ export function CurrencyField({ value, onChange, field, readonly, error, classNa )} { diff --git a/packages/fields/src/widgets/DateField.tsx b/packages/fields/src/widgets/DateField.tsx index 063261381..384071204 100644 --- a/packages/fields/src/widgets/DateField.tsx +++ b/packages/fields/src/widgets/DateField.tsx @@ -1,6 +1,7 @@ import React from 'react'; import { Input, EmptyValue } from '@object-ui/components'; import { FieldWidgetComponentProps } from './types'; +import { toDomProps } from './toDomProps'; import { openNativePicker } from './openNativePicker'; /** @@ -12,8 +13,7 @@ export function DateField({ value, onChange, field, readonly, ...props }: FieldW return value ? {new Date(value).toLocaleDateString()} : ; } - // Filter out non-DOM props - const { inputType, ...domProps } = props as any; + const domProps = toDomProps(props); return ( ••••••••; } - // Filter out non-DOM props - const { inputType, ...domProps } = props as any; + const domProps = toDomProps(props); return (
diff --git a/packages/fields/src/widgets/PercentField.tsx b/packages/fields/src/widgets/PercentField.tsx index 10e0d7bee..0b7b9b041 100644 --- a/packages/fields/src/widgets/PercentField.tsx +++ b/packages/fields/src/widgets/PercentField.tsx @@ -1,6 +1,7 @@ import React from 'react'; import { Input, Slider, EmptyValue } from '@object-ui/components'; import { FieldWidgetComponentProps } from './types'; +import { toDomProps } from './toDomProps'; /** * PercentField - Percentage input with configurable decimal precision @@ -64,7 +65,7 @@ export function PercentField({ value, onChange, field, readonly, error, classNam
) { const config = props.field as SelectFieldMetadata | undefined; if ((config as any)?.multiple) { + // NOT `toDomProps` — this is a widget-to-widget delegation, not a DOM + // spread. `MultiSelectField` implements the same contract and needs the + // whole of it (`value`, `onChange`, `field`, `dataSource`, …); narrowing + // here to the DOM whitelist would hand it an empty widget. return ; } return )} />; @@ -110,7 +115,13 @@ function SingleSelectField({ return ( `. Whatever a host handed the widget therefore became a DOM + * attribute. Measured on `origin/main`, a real form + a real widget: + * + * ``` + * + * ``` + * + * The `[object Object]` this issue is named for is an authored field-config + * key being `String()`-ed onto an attribute. React 19 does not warn: an + * all-lowercase unknown attribute is passed through in complete silence, which + * is why this survived so long. + * + * ## Why a whitelist, and not a list of keys to drop + * + * The biggest leak source is not any NAMED renderer prop — it is the open tail + * of author-supplied keys. The form renderer destructures a fixed set of known + * keys and forwards `...fieldProps` verbatim + * (`components/src/renderers/form/form.tsx`), and `SchemaRenderer` is wider + * still: it spreads the whole authored node as props and has no strip layer at + * all, so on the SDUI path a widget's own spread is the ONLY line of defence. + * + * A blacklist enumerating today's renderer-only keys (`error`, `emptyHint`, + * `dataSource`, `dependentValues`, `dependsOn`, `options`, `inputType`, …) + * would pass every one of the canaries above and would not stop the next + * authored key either. Only "keep the known-safe set, drop the rest" closes + * it, which is what this function does. + * + * ## Declared = enforced + * + * {@link FieldWidgetComponentProps} already DECLARES the closed set of keys a + * widget may receive, and {@link FieldWidgetDomProps} names the subset that may + * legitimately reach a DOM element (objectui#3221). Until now that was a + * type-level claim a widget could violate at runtime simply by spreading. This + * function is that declaration's executable form, and TWO compile-time + * assertions below bind them in both directions — forwarding an undeclared key + * and declaring an unforwarded DOM key are each a compile error. Neither + * direction can drift silently. + * + * ## Deliberately NOT forwarded + * + * `role` and other HTML global attributes are absent because the contract does + * not declare them. They reached the DOM before this change only through the + * open spread. If a field node should be able to author one, DECLARE it on + * `FieldWidgetComponentProps` first and add it here — do not reopen the spread + * (AGENTS.md #0.1: fix the contract, never widen the consumer). + */ +const DOM_PASS_THROUGH_KEYS = [ + /* ── The contract's own "DOM pass-through" block, verbatim ─────────────── */ + 'id', + 'name', + 'autoFocus', + 'tabIndex', + 'onBlur', + 'onFocus', + 'onClick', + + /* ── Two more declared keys that ARE valid DOM attributes ──────────────── */ + // + // Both are declared on `FieldWidgetComponentProps` under the + // controlled-input contract rather than the DOM block, because widgets also + // INTERPRET them (`className` gets composed with the widget's own classes; + // `disabled` is OR-ed with `readonly`). They are still `class` and + // `disabled` on the element, and every widget here already forwards them. + // + // Withholding them would make this helper a silent styling- and + // interactivity-dropper: `` — the shape this + // change teaches every future widget author to write — would quietly lose + // the host's className and disabled state. That is a NEW silent failure of + // exactly the kind the whitelist exists to prevent, so they are forwarded. + 'className', + 'disabled', +] as const; + +type DomPassThroughKey = (typeof DOM_PASS_THROUGH_KEYS)[number]; + +/** + * Compile-time link to the declaration, direction 1 of 2: every key forwarded + * at runtime must exist on {@link FieldWidgetComponentProps}. Deleting a key + * from the contract without deleting it here is a compile error. + * + * Catches: helper forwards something the contract no longer declares. + */ +type EveryForwardedKeyIsDeclared = + DomPassThroughKey extends keyof FieldWidgetComponentProps ? true : never; +const _everyForwardedKeyIsDeclared: EveryForwardedKeyIsDeclared = true; +void _everyForwardedKeyIsDeclared; + +/** + * Direction 2 of 2: every key of {@link FieldWidgetDomProps} — the contract's + * DOM pass-through block — must be one this helper actually forwards. Adding a + * DOM key to the contract without adding it to `DOM_PASS_THROUGH_KEYS` is a + * compile error. + * + * Catches: DECLARED BUT NOT DELIVERED — a key that type-checks, reads as + * supported, and silently never reaches the element. That is the failure this + * repo treats as first-class (objectui#3290's `aria-required` that never + * reached a control; objectui#3222's validation slot nobody produced), and it + * is the one direction the leak test structurally CANNOT see: that test looks + * for attributes that arrive, not for ones that go missing. + * + * `className` / `disabled` are not part of `FieldWidgetDomProps` (they are + * controlled-input keys this helper also forwards — see above), so they are + * bound by direction 1 only. That is deliberate, not an oversight. + */ +type EveryDeclaredDomKeyIsForwarded = + keyof FieldWidgetDomProps extends DomPassThroughKey ? true : never; +const _everyDeclaredDomKeyIsForwarded: EveryDeclaredDomKeyIsForwarded = true; +void _everyDeclaredDomKeyIsForwarded; + +const DOM_PASS_THROUGH: ReadonlySet = new Set(DOM_PASS_THROUGH_KEYS); + +/** + * `aria-*` is declared on the contract as React's `AriaAttributes`; `data-*` + * is declared as an open template-literal family (open in HTML too, and the + * only open family the contract has). Both are matched by prefix so the helper + * needs no per-attribute list. + */ +function isOpenDomFamily(key: string): boolean { + return key.startsWith('data-') || key.startsWith('aria-'); +} + +/** The subset of `P` this helper forwards, with each key's declared type. */ +export type DomProps

= Pick> & { + [K in Extract]: P[K]; +}; + +/** + * Keep only what may legitimately become a DOM attribute, and drop everything + * else — renderer plumbing, and any extra key an author put on the field + * config or SDUI node. + * + * Replaces the bare `{...props}` spread in every field widget that renders a + * host element: + * + * ```tsx + * // before — forwards whatever it was handed + * const { inputType, ...domProps } = props as any; + * return ; + * + * // after + * return ; + * ``` + * + * Semantic props a widget INTERPRETS (`value`, `onChange`, `field`, + * `readonly`, `error`, …) are read from `props` as before; this function only + * governs what gets spread. + */ +export function toDomProps

(props: P): DomProps

{ + const domProps: Record = {}; + for (const key of Object.keys(props)) { + if (DOM_PASS_THROUGH.has(key) || isOpenDomFamily(key)) { + domProps[key] = (props as Record)[key]; + } + } + return domProps as DomProps

; +} diff --git a/packages/fields/src/widgets/types.ts b/packages/fields/src/widgets/types.ts index 753694c35..cdfea8c77 100644 --- a/packages/fields/src/widgets/types.ts +++ b/packages/fields/src/widgets/types.ts @@ -1,6 +1,49 @@ import type { AriaAttributes, FocusEventHandler, MouseEventHandler } from 'react'; import type { DependsOnInput, FieldMetadata } from '@object-ui/types'; +/** + * DOM pass-through: what a widget's `...props` spread may legitimately put on + * the element it renders. + * + * A NAMED type rather than an inline block of {@link FieldWidgetComponentProps} + * so the compiler can bind it to its runtime executor in BOTH directions + * (objectui#3291). `toDomProps` asserts: + * + * - every key it forwards is declared on the widget contract — deleting one + * here without deleting it there is a compile error; + * - every key of THIS type is one it forwards — adding one here without + * adding it to `DOM_PASS_THROUGH_KEYS` is a compile error too. + * + * The second direction is the one that matters for the failure mode this repo + * treats as first-class: DECLARED BUT NOT DELIVERED (objectui#3290's + * `aria-required` that never reached a control, objectui#3222's validation slot + * nobody produced). Without it, a key added here would type-check, read as + * supported, and silently never reach the DOM — and the leak test cannot see + * that class of bug, because it looks for attributes that ARRIVE, not for ones + * that go missing. + * + * `className` and `disabled` are deliberately NOT here. They are DOM-legal and + * `toDomProps` does forward them, but they are declared on the controlled-input + * block of {@link FieldWidgetComponentProps} because widgets also INTERPRET + * them (className is composed with the widget's own classes; disabled is OR-ed + * with readonly). They are therefore bound in the forward direction only. + * + * `AriaAttributes` and the `data-${string}` family are matched by prefix at + * runtime rather than key-by-key, so they are intersected in separately. + * + * Adding a key here is a contract change: say who produces it and who reads it. + */ +export type FieldWidgetDomProps = { + id?: string; + /** react-hook-form's field name, spread in by the form renderer. */ + name?: string; + autoFocus?: boolean; + tabIndex?: number; + onBlur?: FocusEventHandler; + onFocus?: FocusEventHandler; + onClick?: MouseEventHandler; +}; + /** * Props every field widget in this package receives at RUNTIME. * @@ -168,17 +211,13 @@ export type FieldWidgetComponentProps = { */ onCreateNew?: (searchQuery: string) => void; - /* ── DOM pass-through: what `...props` may legitimately reach an input ──── */ - - id?: string; - /** react-hook-form's field name, spread in by the form renderer. */ - name?: string; - autoFocus?: boolean; - tabIndex?: number; - onBlur?: FocusEventHandler; - onFocus?: FocusEventHandler; - onClick?: MouseEventHandler; -} & AriaAttributes & { + /* ── DOM pass-through ───────────────────────────────────────────────────── */ + // + // Lives in the named {@link FieldWidgetDomProps} above, because that is what + // lets the compiler bind the declaration to `toDomProps` — its runtime + // executor — in BOTH directions (objectui#3291). Add a DOM key THERE, not + // here, and the compiler will make you add it to the whitelist too. +} & FieldWidgetDomProps & AriaAttributes & { /** * Arbitrary `data-*` attributes (test ids, analytics hooks). Open by design, * but a template-literal key — `keyof` stays finite, so this does NOT