diff --git a/.changeset/spotty-pages-drop-leaks.md b/.changeset/spotty-pages-drop-leaks.md new file mode 100644 index 0000000000..4bd3fc7907 --- /dev/null +++ b/.changeset/spotty-pages-drop-leaks.md @@ -0,0 +1,23 @@ +--- +'@object-ui/components': patch +--- + +fix(components): the `page` wrapper now filters its DOM attributes through the shared whitelist instead of a hand-maintained list + +`PageRenderer` stripped PageSchema's descriptor keys with a hand-maintained +destructure list and spread the remainder onto its wrapper `
`. Whenever +that list fell behind the schema, an authored key was not dropped — it was +forwarded, and React stringifies unknown attributes in silence, so an authored +`actions: [{…}, {…}]` reached the DOM as `actions="[object Object],[object +Object]"`. + +The renderer now calls `toDomProps` from `@object-ui/core` — the same +whitelist every converged SDUI widget already uses (objectui#4425). Twenty-five +attributes measured leaking off the wrapper stop being emitted, including the +`context` bag the console injects into every page it renders. Everything the +wrapper legitimately carries is unchanged: `class`, `style`, `id`, `role`, +`tabindex`, `data-page-type`, `data-obj-id`, `data-obj-type`, and the open +`data-*` / `aria-*` families. + +This changes no schema's accept/reject behaviour — `BaseSchema.passthrough()` +is untouched — and adds no read point for any previously leaking key. diff --git a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx index 8615fb403a..7d493c8ff7 100644 --- a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx +++ b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx @@ -1056,21 +1056,21 @@ const COMPONENTS_LEAK_GROUPS: readonly LedgerGroup[] = [ 'not define one, so the authored identity key leaks too.', issue: 'objectui#5574', targets: [ - 'action:bar', 'ui:a', 'ui:abbr', 'ui:accordion', 'ui:address', 'ui:alert', 'ui:app', + 'action:bar', 'ui:a', 'ui:abbr', 'ui:accordion', 'ui:address', 'ui:alert', 'ui:article', 'ui:aside', 'ui:aspect-ratio', 'ui:avatar', 'ui:b', 'ui:badge', 'ui:blockquote', 'ui:br', 'ui:breadcrumb', 'ui:button-group', 'ui:card', 'ui:carousel', 'ui:cite', 'ui:collapsible', 'ui:command', 'ui:dd', 'ui:del', 'ui:div', 'ui:dl', 'ui:dt', 'ui:em', 'ui:empty', 'ui:figcaption', 'ui:figure', 'ui:footer', 'ui:h1', 'ui:h2', 'ui:h3', 'ui:h4', 'ui:h5', 'ui:h6', 'ui:header', - 'ui:home', 'ui:hr', 'ui:html', 'ui:i', 'ui:image', 'ui:img', 'ui:ins', 'ui:kbd', + 'ui:hr', 'ui:html', 'ui:i', 'ui:image', 'ui:img', 'ui:ins', 'ui:kbd', 'ui:label', 'ui:li', 'ui:list', 'ui:loading', 'ui:main', 'ui:mark', 'ui:menubar', - 'ui:nav', 'ui:navigation-menu', 'ui:ol', 'ui:p', 'ui:page', 'ui:pagination', 'ui:pre', - 'ui:progress', 'ui:q', 'ui:record', 'ui:resizable', 'ui:scroll-area', 'ui:section', + 'ui:nav', 'ui:navigation-menu', 'ui:ol', 'ui:p', 'ui:pagination', 'ui:pre', + 'ui:progress', 'ui:q', 'ui:resizable', 'ui:scroll-area', 'ui:section', 'ui:separator', 'ui:sidebar', 'ui:sidebar-content', 'ui:sidebar-footer', 'ui:sidebar-group', 'ui:sidebar-header', 'ui:sidebar-inset', 'ui:sidebar-menu', 'ui:sidebar-menu-item', 'ui:sidebar-provider', 'ui:skeleton', 'ui:small', 'ui:span', 'ui:strong', 'ui:sub', 'ui:sup', 'ui:table', 'ui:tabs', - 'ui:time', 'ui:toggle-group', 'ui:tree-view', 'ui:u', 'ui:ul', 'ui:utility', + 'ui:time', 'ui:toggle-group', 'ui:tree-view', 'ui:u', 'ui:ul', ], }, { diff --git a/packages/components/src/__tests__/page-dom-leak-whitelist-7933.test.tsx b/packages/components/src/__tests__/page-dom-leak-whitelist-7933.test.tsx new file mode 100644 index 0000000000..d5a5507d2c --- /dev/null +++ b/packages/components/src/__tests__/page-dom-leak-whitelist-7933.test.tsx @@ -0,0 +1,211 @@ +/** + * 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. + */ + +/** + * PIN: the `page` wrapper carries only what may legitimately be a DOM + * attribute, and an UNDECLARED authored key is DROPPED — never stringified + * onto the element (objectui#7933). + * + * ## The defect + * + * `PageRenderer` used to strip PageSchema's descriptor keys with a + * hand-maintained destructure list and spread the remainder onto its wrapper + * `
`, under a comment instructing the next reader to "keep this list + * aligned with PageSchema". The alignment is what failed. React forwards an + * unknown lowercase attribute in complete silence and stringifies object + * values, so a key the list did not name did not fail loudly — it landed: + * + * class="min-h-full w-full bg-background p-3 md:p-4 lg:p-6" + * data-page-type="record" + * data-obj-type="page" + * actions="[object Object],[object Object]" <- the defect + * + * `actions` is declared nowhere on `PageNodeSchema` (it survives parse only + * through `BaseSchema`'s `.passthrough()`) and `PageRenderer` has zero read + * points for it, so it was neither read nor dropped. Whether `page` should + * ever GROW an `actions` read point is a separate, open capability question + * (objectui#7926); this pin is orthogonal to it, because an authored key must + * end up either read or dropped under EITHER answer, and never as an illegal + * HTML attribute. + * + * ## Why this is a whitelist and not a longer list + * + * The set of keys an author may put on a node is unbounded; the set a widget + * may put on an element is declared. A deny-list bounded by enumeration cannot + * be finished — which is the objectui#4425 ruling, and why `toDomProps` lives + * in `@object-ui/core` as ONE executor for every converged SDUI surface. This + * renderer was one of the last faces still closing the leak with an + * enumeration of its own. + * + * ## What the two halves of this file prove, and why BOTH are needed + * + * - {@link LEGITIMATE_ATTRIBUTES} drives the POSITIVE half: nothing outside + * that declared set reaches the wrapper. Restoring the hand-maintained + * destructure list turns this red and prints the leaked attribute names. + * - The NEGATIVE half asserts the attributes the wrapper genuinely needs are + * still THERE. A whitelist is a dropping mechanism, so "the leak is gone" + * is half a measurement: a fix that also dropped `class`, `style` or + * `data-page-type` would satisfy the positive half completely while + * trading one bug for a worse one. `style` in particular is NOT on the + * element-agnostic whitelist — it survives only because the renderer + * forwards it by name — so nothing but this half is watching it. + */ + +import { describe, it, expect } from 'vitest'; +import { render } from '@testing-library/react'; +// Module scope, not a hook: registers PageRenderer under all five of its +// registry keys. A cold `await import()` inside a hook is billed to +// `hookTimeout` and races the assertions (AGENTS.md §测试纪律, objectui#3010). +import { ActionProvider, SchemaRenderer } from '@object-ui/react'; +import '../renderers'; + +/** The five registry keys `PageRenderer` is registered under. */ +const PAGE_REGISTRY_KEYS = ['page', 'app', 'home', 'utility', 'record'] as const; + +/** + * Everything the wrapper `
` may legitimately carry. Declared here rather + * than derived from the render, so this file states an EXPECTATION instead of + * echoing whatever the code happens to emit. + * + * `class` / `style` are the renderer's own; `data-page-type`, `data-obj-id` + * and `data-obj-type` are the debug/designer channel it forwards by name; the + * rest are `@object-ui/core`'s SDUI pass-through set (`id`, `role`, + * `tabindex`) plus the open `data-*` / `aria-*` families. + */ +const LEGITIMATE_ATTRIBUTES = new Set([ + 'class', + 'style', + 'id', + 'role', + 'tabindex', + 'data-page-type', + 'data-obj-id', + 'data-obj-type', +]); + +const isOpenFamily = (name: string) => name.startsWith('data-') || name.startsWith('aria-'); + +/** + * A page node authoring the reported instance verbatim, plus the other key + * shapes measured leaking on this tree — an object value, a scalar, a spec key + * with no read point, and the `context` bag `app-shell`'s `PageView` injects + * into EVERY page it renders (so the leak did not need an unusual author to + * appear in production). + */ +function canaryPage(registryKey: string) { + return { + type: registryKey, + pageType: registryKey === 'page' ? 'record' : registryKey, + label: 'Products', + regions: [ + { name: 'main', width: 'full', components: [{ type: 'element:text', properties: { text: 'body' } }] }, + ], + + /* ── undeclared / unread keys: every one of these must be DROPPED ────── */ + // The reported instance (objectui#7933 / objectui#7926). + actions: [ + { type: 'button', label: 'Add Product' }, + { type: 'button', label: 'Export' }, + ], + // Declared on `PageNodeSchema` but with no read point on this wrapper. + slots: { header: [] }, + // Host-injected, not authored: `PageView.tsx` adds this to every page. + context: { params: {}, refreshKey: 0 }, + // Scalars — these leak as readable-but-illegal attributes rather than + // `[object Object]`, which is the shape easiest to mistake for intentional. + name: 'product_page', + pageName: 'products', + // Internal synth metadata: the `_` prefix used to need its own filter. + _packageId: 'pkg_products', + + /* ── legitimate: every one of these must SURVIVE ─────────────────────── */ + // `data-obj-id` / `data-obj-type` are NOT authored here on purpose: + // `SchemaRenderer` derives them from the node's `id` and `type` + // (SchemaRenderer.tsx, `'data-obj-id': evaluatedSchema.id`), so authoring + // them would assert a value the pipeline overwrites. + id: 'page_products', + role: 'region', + tabIndex: -1, + style: { minHeight: '100vh' }, + 'data-testid': 'products-page', + 'aria-label': 'Products page', + } as unknown as Parameters[0]['schema']; +} + +function renderWrapper(registryKey: string): HTMLElement { + const { container } = render( + + + , + ); + const wrapper = container.querySelector('[data-page-type]'); + // A missing wrapper would make every assertion below vacuously true. + expect(wrapper, `PageRenderer rendered no [data-page-type] wrapper for type:'${registryKey}'`).not.toBeNull(); + return wrapper as HTMLElement; +} + +describe("PageRenderer — the wrapper's DOM attributes are a whitelist (objectui#7933)", () => { + it.each(PAGE_REGISTRY_KEYS)( + "type:'%s' — an undeclared authored key never reaches the wrapper as an attribute", + (registryKey) => { + const wrapper = renderWrapper(registryKey); + const illegitimateAttributes = [...wrapper.attributes] + .map((a) => a.name) + .filter((name) => !LEGITIMATE_ATTRIBUTES.has(name) && !isOpenFamily(name)) + .sort(); + + // Compared as a labelled object so a failure NAMES ITSELF: the report + // carries the pin, the exact element it read, and the leaked attribute + // names — not a bare "expected [] to equal [...]". + expect({ + pin: 'objectui#7933 — page wrapper DOM pass-through whitelist', + surface: `PageRenderer wrapper
for registry key '${registryKey}'`, + illegitimateAttributes, + }).toEqual({ + pin: 'objectui#7933 — page wrapper DOM pass-through whitelist', + surface: `PageRenderer wrapper
for registry key '${registryKey}'`, + illegitimateAttributes: [], + }); + + // The reported instance, asserted by name as well as by the set above — + // so the regression that reopened THIS card is legible on its own. + expect(wrapper.getAttribute('actions')).toBeNull(); + expect(wrapper.outerHTML).not.toContain('[object Object]'); + }, + ); + + it.each(PAGE_REGISTRY_KEYS)( + "type:'%s' — NEGATIVE CONTROL: the attributes the wrapper needs are still delivered", + (registryKey) => { + const wrapper = renderWrapper(registryKey); + + // The renderer's own styling channel and its debug/designer attributes. + expect(wrapper.getAttribute('class')).toContain('min-h-full'); + expect(wrapper.getAttribute('class')).toContain('bg-background'); + expect(wrapper.getAttribute('data-page-type')).toBe( + registryKey === 'page' ? 'record' : registryKey, + ); + // Derived by `SchemaRenderer` from the node's `id` / `type`, then + // forwarded by name here; they must survive the whitelist either way. + expect(wrapper.getAttribute('data-obj-id')).toBe('page_products'); + expect(wrapper.getAttribute('data-obj-type')).toBe(registryKey); + // Forwarded BY NAME, not by the whitelist — the one legitimate attribute + // a whitelist-only fix would silently drop. + expect(wrapper.style.minHeight).toBe('100vh'); + + // The SDUI pass-through set, which a blunter fix would have taken out. + expect(wrapper.getAttribute('id')).toBe('page_products'); + expect(wrapper.getAttribute('role')).toBe('region'); + expect(wrapper.getAttribute('tabindex')).toBe('-1'); + + // The two open families stay open. + expect(wrapper.getAttribute('data-testid')).toBe('products-page'); + expect(wrapper.getAttribute('aria-label')).toBe('Products page'); + }, + ); +}); diff --git a/packages/components/src/renderers/layout/page.tsx b/packages/components/src/renderers/layout/page.tsx index 331ca01ea1..be8b9d332c 100644 --- a/packages/components/src/renderers/layout/page.tsx +++ b/packages/components/src/renderers/layout/page.tsx @@ -15,7 +15,7 @@ import React, { useMemo } from 'react'; import type { BaseSchema, PageNodeSchema, PageNodeRegion, SchemaNode } from '@object-ui/types'; import { SchemaRenderer, toRenderableSchema, PageVariablesProvider, PageVariableActionBridge } from '@object-ui/react'; -import { ComponentRegistry } from '@object-ui/core'; +import { ComponentRegistry, toDomProps } from '@object-ui/core'; import { compile, manifestFromConfigs } from '@object-ui/sdui-parser'; import { ReactKindPage } from './react-page'; import { cn } from '../../lib/utils'; @@ -490,43 +490,35 @@ export const PageRenderer: React.FC<{ // (framework#1878 §3 naming-drift recheck). const pageTitle = schema.title ?? (schema as any).label; - // Extract designer-related props and strip schema-only metadata that - // would otherwise leak onto the wrapper
as invalid HTML attributes - // (e.g. `isDefault`, `assignedProfiles`, `_packageId`, `aria` object). - // We keep this list aligned with `PageSchema` in `@object-ui/types`. As a - // safety net we also drop any `_`-prefixed keys (internal metadata from - // the synth pipeline) before spreading the remainder onto the DOM. + // What may become an attribute on the wrapper
, and nothing else. + // + // This used to be a hand-maintained destructure list of every PageSchema + // descriptor, plus a `_`-prefix filter as a safety net, with the standing + // instruction to "keep this list aligned with PageSchema". Keeping it + // aligned is the part that failed: an authored key the list did not name was + // not dropped, it was SPREAD — and React forwards an unknown lowercase + // attribute in complete silence, stringifying object values. An authored + // `actions: [{…}, {…}]` reached the DOM as `actions="[object Object],[object + // Object]"` (objectui#7933), neither read by this renderer nor dropped. + // + // A deny-list bounded by enumeration cannot be finished, because the set of + // keys an author may put on a node is unbounded; a whitelist bounded by + // declaration can. That is the objectui#4425 ruling, and `toDomProps` is the + // one mechanism it promoted to `@object-ui/core` for exactly this — the same + // executor every converged SDUI widget already calls. This renderer was one + // of the last faces still closing the leak with an enumeration of its own. + // + // `style` stays forwarded BY NAME: it is a deliberate DOM pass-through that + // is not on the element-agnostic whitelist, which is how every other call + // site handles it (objectui#4435 — declare it and forward it, never reopen + // the spread). `data-obj-id` / `data-obj-type` are read by name here and + // also ride the open `data-*` family, so they arrive either way. const { 'data-obj-id': dataObjId, 'data-obj-type': dataObjType, style, - // PageSchema descriptors — UI metadata, not DOM attributes - pageType: _pageTypeProp, - schema: _schemaProp, - regions: _regionsProp, - template: _templateProp, - title: _titleProp, - icon: _iconProp, - description: _descriptionProp, - object: _objectProp, - variables: _variablesProp, - body: _bodyProp, - isDefault: _isDefaultProp, - assignedProfiles: _assignedProfilesProp, - aria: _ariaProp, - recordOverride: _recordOverrideProp, - permissions: _permissionsProp, - requiredPermissions: _requiredPermissionsProp, - enforceFieldSecurity: _enforceFLSProp, - redactFields: _redactFieldsProp, - children: _childrenProp, - ...rawPageProps } = props; - // Drop any `_`-prefixed keys (e.g. `_packageId`, `_synth`) — these are - // internal metadata that React would warn about if forwarded to the DOM. - const pageProps = Object.fromEntries( - Object.entries(rawPageProps).filter(([k]) => !k.startsWith('_')), - ); + const pageProps = toDomProps(props); // Select the layout variant based on template or page type const layoutElement = useMemo(() => {