Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .changeset/spotty-pages-drop-leaks.md
Original file line number Diff line number Diff line change
@@ -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 `<div>`. 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.
10 changes: 5 additions & 5 deletions packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
],
},
{
Expand Down
Original file line number Diff line number Diff line change
@@ -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
* `<div>`, 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 `<div>` 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<typeof SchemaRenderer>[0]['schema'];
}

function renderWrapper(registryKey: string): HTMLElement {
const { container } = render(
<ActionProvider>
<SchemaRenderer schema={canaryPage(registryKey)} />
</ActionProvider>,
);
const wrapper = container.querySelector<HTMLElement>('[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 <div data-page-type> for registry key '${registryKey}'`,
illegitimateAttributes,
}).toEqual({
pin: 'objectui#7933 — page wrapper DOM pass-through whitelist',
surface: `PageRenderer wrapper <div data-page-type> 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');
},
);
});
58 changes: 25 additions & 33 deletions packages/components/src/renderers/layout/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 <div> 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 <div>, 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(() => {
Expand Down
Loading