fix(components): page wrapper filters DOM attributes through the shared whitelist - #7958
Merged
Merged
Conversation
…ed whitelist (objectui#7933) `PageRenderer` stripped 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 the list aligned with PageSchema. The alignment is what failed: React forwards an unknown lowercase attribute in silence and stringifies object values, so an authored key the list did not name was not dropped but emitted — `actions="[object Object],[object Object]"`. Replace the enumeration with `toDomProps` from `@object-ui/core`, the one executor objectui#4425 promoted for exactly this. `style` stays forwarded by name (it is not on the element-agnostic whitelist), as does the `data-obj-id` / `data-obj-type` designer channel. Measured on all five registry keys PageRenderer is registered under (page/app/home/utility/record): 25 illegitimate attributes before, 0 after, with all 7 legitimate attributes unchanged and none added. No schema accept/reject behaviour changes and no read point is added for any previously leaking key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
…losed (objectui#7933) The leak ledger in widget-dom-leak-sweep is a two-way ratchet: a new leaked attribute fails it, and so does FIXING one, until the row is deleted in the same change. The previous commit routed PageRenderer's wrapper through `toDomProps`, so `found` went empty for the five registry keys the registry resolves to that renderer (`ui:page` and bare `page` are the same registration), while the ledger still recorded 14 attributes each. Delete exactly those five names -- 'ui:app', 'ui:home', 'ui:page', 'ui:record', 'ui:utility' -- from the BARE_SPREAD group's `targets`. The list goes 91 -> 86 entries; nothing else in the file moves, and all five stay in COMPONENTS_PLAIN_TYPES, so they are still SWEPT, just no longer ledgered. Gate reading after the deletion: 202/202 passed (was 5 failed | 197 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7933
PageRendererclosed its DOM leak with a hand-maintained destructure list andspread the remainder onto its wrapper
div, under a comment telling the nextreader to keep that list aligned with PageSchema. The alignment is what failed:
React forwards an unknown lowercase attribute in silence and stringifies object
values, so a key the list did not name was not dropped — it was emitted.
Clause-2
no. No schema's accept/reject behaviour changes (BaseSchema.passthrough()is untouched), no public surface widens, and no gate's scan population moves —
the five names this PR expires from the leak ledger stay in
COMPONENTS_PLAIN_TYPES, so they are still SWEPT, just no longer recorded asleaking. What moves is a behaviour population, which the survey below
measures.
The mechanism, 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 — the objectui#4425 ruling — and
toDomPropsis the one executor itpromoted to
@object-ui/corefor exactly this.page.tsxwas one of the lastfaces still closing the leak with an enumeration of its own. This PR makes it
call the shared whitelist; it invents nothing.
stylestays forwarded BY NAME (it is not on the element-agnostic whitelist,so nothing else would deliver it), as does the
data-obj-id/data-obj-typedesigner channel.
Survey — the behaviour population this moves
Corpus: every statically-readable authored node in the repo whose
typeis oneof the five registry keys
PageRendereris registered under, from.json,fenced
jsonin.md/.mdx, and object literals in.ts/.tsx.type: 'page'with no enclosing object literal (TS type positions), 6 whose keys would not extract, 6 nodes built with a spreadtype-name collisions excludedtype: 'record'FIELD definition), counted separately, never silently droppedA static key list is not a leak measurement, so every key was then driven
through the real
SchemaRendererpath and the wrapper's attributes read off therendered element, on all five registry keys:
pageapphomeutilityrecordDropped:
actionsalloworgoverrideanchorsbadgebarekeycanonicalkeycontextcreatedefaultscreatederivecreatefieldscreateschemacreateseeddatadomainhrefinterfaceconfigitemlabellabelkeylistcolumnsnamepagenamepropertiesslotssomerendererprop.Kept, unchanged:
classidaria-labeldata-obj-iddata-obj-typedata-page-typedata-canary.Two findings the corpus scan alone would have missed:
contextis host-injected.app-shell'sPageView.tsxadds acontextbag to every page it renders, so
context="[object Object]"was on thewrapper of every page the console showed — the leak never needed an unusual
author to reach production.
slotsandlabelare declared spec keys that the hand list did notname, so a spec-conformant page leaked them.
Safety gate — is any leaked attribute consumed downstream as a DOM
attribute? No. Searched for CSS attribute selectors (
[actions],[actions=,…),
getAttribute('KEY'), e2e locators, and snapshot pins over all 25 names.Every hit was JS array-destructuring or a computed key (
[canonicalKey]:), anunrelated
a[href]focus selector, or ASCII art. No snapshot pins the wrapper.Nothing reads these as attributes, so the replacement is safe.
The nail
packages/components/src/__tests__/page-dom-leak-whitelist-7933.test.tsx— 10cases, 2 per registry key.
Positive control. Restoring the hand-maintained destructure list turns the
pin red and it names itself, rather than reporting a generic assertion failure:
Restored by STATE, not by exit code: the worktree blob returned to
be8b9d332c1696554caf82e7bb95f6b69eb226b9, identical to theHEADblob, withgit diff HEADat 0 bytes andgit status --shortempty. The mutation wasconfirmed on disk first (
rawPageProps0 to 2 occurrences,toDomProps(props)1 to 0) so a no-op edit could not have been read as a passing control.
Negative control. Five more cases assert the attributes the wrapper genuinely
needs are still delivered —
class,data-page-type,data-obj-id,data-obj-type,style,id,role,tabindex, and the opendata-*/aria-*families. A whitelist is a dropping mechanism, so "the leak is gone" ishalf a measurement;
stylein particular survives only because it is forwardedby name, and nothing but this half is watching it. Writing it caught a real
fact:
data-obj-id/data-obj-typeare DERIVED bySchemaRendererfrom thenode's
idandtype, not authored, so the pin asserts the values the pipelineactually produces.
The coupled ledger expiry (second commit)
packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsxcarries atwo-way ratchet: a new leaked attribute fails it, and so does FIXING one, until
the row is deleted in the same change — "Rows leave this ledger by being
DELETED in the change that fixes them." With only the first commit the gate
read
Tests 5 failed | 197 passed (202),found: []againstledgered: [14 attributes]onui:appui:homeui:pageui:recordui:utility. The registry resolvesui:pageand barepageto the sameregistration, which is how the sweep reaches
PageRendererat all.Causality was proven by ablation rather than assumed: with the pre-fix mechanism
restored the same gate is 202/202 green, so those five were this change's doing
and not a pre-existing red.
The second commit deletes exactly those five names from the
targetslist ofthe
BARE_SPREADgroup inCOMPONENTS_LEAK_GROUPS. The list goes 91 to 86entries — five fewer, no more and no less. Nothing else in that file moves: no
structural change, no docstring edit, no new group, no other target touched. All
five remain in
COMPONENTS_PLAIN_TYPES, so they are still swept on every run,now expected to be clean.
Verification
All Vitest runs from the repo root (
RUN v4.1.10 /home/user/objectui-issue-7933),never a package cwd. Re-run after the final commit
bdf78fc4c, at whichgit diff HEADis 0 bytes.widget-dom-leak-sweepafter the expirybdf78fc4cvitest run packages/components/@object-ui/components type-checkerror TS;--listFilesprovespage.tsxin both programs, the pin in the test program@object-ui/app-shell type-checkerror TS;--listFilesproves the sweep file is in the test program@object-ui/components lint@object-ui/app-shell lintcheck:control-bytescheck:vi-mock-specifierscheck:vi-mock-inheritcheck:handler-key-readscheck:action-forward-paritycheck:element-data-source-declarationcheck:side-effects-arraycheck:unreferenced-sourcescheck:self-importcheck:phantom-depscheck:entry-guardcheck:sdui-registration-pinscheck:eager-closureapps/console/dist/eager-closure.json; needs a console build CI doesnode scripts/check-changeset-presence.mjsre-run after the app-shell fileentered the diff (not carried over), verbatim:
node scripts/check-changeset-no-major.mjs:No changeset declares a major bump.https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3