diff --git a/.changeset/bridge-density-prototype-guard-4442.md b/.changeset/bridge-density-prototype-guard-4442.md new file mode 100644 index 000000000..ca6060e99 --- /dev/null +++ b/.changeset/bridge-density-prototype-guard-4442.md @@ -0,0 +1,24 @@ +--- +'@object-ui/react': patch +--- + +The spec bridge abstains on prototype-member `rowHeight` spellings instead of leaking a +function into `density`. + +`bridgeListView`'s `mapDensity` indexed a plain object literal with an unchecked key, so +the lookup reached `Object.prototype`. The parameter is typed `RowHeight`, but the +boundary a host's stored view definition actually crosses is `SpecBridge.transformListView`, +whose parameter is `any` — so `rowHeight: 'toString'` came back as `Object.prototype.toString`, +a **function**, out of a read whose return type is three strings or nothing. `bridgeListView` +then writes the key under `if (density)`, and a function is truthy, so the bad value was not +merely returned: it was stored on a `SchemaNode` whose renderer expects +`'compact' | 'comfortable' | 'spacious'`. Same for `constructor`, `valueOf`, +`hasOwnProperty`, `isPrototypeOf`, `propertyIsEnumerable` and `toLocaleString`. + +The lookup is now guarded with `Object.prototype.hasOwnProperty.call(...)` — the same guard +`@object-ui/core`'s `rowHeightToDensityMode` grew in objectui#4440, and the repo's existing +convention at eight other sites. Both `rowHeight` surfaces now abstain identically on every +off-spec **string** spelling, and objectui#4440's agreement pin covers the prototype-member +family instead of excluding it (objectui#4442). + +Runtime-only: no public type moved, and no spec-valid `rowHeight` changes its answer. diff --git a/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts b/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts index ca8c850d4..67cc8a393 100644 --- a/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts +++ b/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts @@ -57,32 +57,69 @@ describe('rowHeight → density: core and the spec bridge give one answer (#4440 }); describe('off-spec row heights — both abstain', () => { + // Two families of off-spec spelling, both pinned here. + // // `comfortable` / `spacious` / `small` / `large` are the four spellings // #4352 deleted from the bridge; `gargantuan` is a string in neither // vocabulary. Before #4440 core answered `'comfortable'` for every one of // them while the bridge answered nothing. - it.each(['comfortable', 'spacious', 'small', 'large', 'gargantuan'])( - 'neither surface invents a density for the off-spec rowHeight %s', - (rowHeight) => { - expect(rowHeightToDensityMode(rowHeight)).toBeUndefined(); - expect(bridgeDensityFor(rowHeight)).toBeUndefined(); - }, - ); + // + // The `Object.prototype` member names are the second family (#4442). Both + // lookup tables are plain object literals, so indexing one with an + // unguarded key reaches the prototype: `rowHeight: 'toString'` used to come + // back as `Object.prototype.toString` — a FUNCTION — from a read whose + // return type is three strings or nothing. Core closed that hole with + // `hasOwnProperty` in #4440, the bridge in #4442; these rows are what keeps + // either side from regrowing it. + it.each([ + 'comfortable', + 'spacious', + 'small', + 'large', + 'gargantuan', + 'toString', + 'constructor', + 'valueOf', + 'hasOwnProperty', + 'isPrototypeOf', + 'propertyIsEnumerable', + 'toLocaleString', + ])('neither surface invents a density for the off-spec rowHeight %s', (rowHeight) => { + expect(rowHeightToDensityMode(rowHeight)).toBeUndefined(); + expect(bridgeDensityFor(rowHeight)).toBeUndefined(); + }); - // NOT pinned here, deliberately: `Object.prototype` member names - // (`toString`, `constructor`, …). Core abstains for them since #4440, but - // the bridge still indexes its table with an unguarded key and hands back - // `Object.prototype.toString` — a FUNCTION — as the density. That is a - // different defect from the coercion this file is about, it lives in source - // outside #4440's surface, and it is filed as #4442. Extending the two - // `it.each` lists above with those keys is the assertion that fails until - // #4442 lands, and is the natural test half of its fix. + it('never writes a function into the SchemaNode density (#4442)', () => { + // The expression #4442 measured, read straight off the node instead of + // through the helper above. It is worth stating separately because the + // leak was not in the lookup alone: `bridgeListView` writes the key under + // `if (density)`, and a function is truthy — so the bad value was not + // merely returned, it was STORED on a SchemaNode whose renderer expects + // `'compact' | 'comfortable' | 'spacious'`. + const node = new SpecBridge().transformListView({ name: 'x', rowHeight: 'toString' }); + expect(node.density).toBeUndefined(); + expect(typeof node.density).not.toBe('function'); + }); it('agrees for every off-spec input without either side being read first', () => { // Same assertion phrased as the invariant itself: whatever the answer is, // it is ONE answer. A future edit that re-adds a fallback to either // surface breaks this even if it re-adds it to both differently. - for (const rowHeight of ['comfortable', 'spacious', 'small', 'large', 'gargantuan', '']) { + for (const rowHeight of [ + 'comfortable', + 'spacious', + 'small', + 'large', + 'gargantuan', + '', + 'toString', + 'constructor', + 'valueOf', + 'hasOwnProperty', + 'isPrototypeOf', + 'propertyIsEnumerable', + 'toLocaleString', + ]) { expect(rowHeightToDensityMode(rowHeight)).toBe(bridgeDensityFor(rowHeight)); } }); diff --git a/packages/react/src/spec-bridge/bridges/list-view.ts b/packages/react/src/spec-bridge/bridges/list-view.ts index ee4b859b4..e6571b5b2 100644 --- a/packages/react/src/spec-bridge/bridges/list-view.ts +++ b/packages/react/src/spec-bridge/bridges/list-view.ts @@ -70,10 +70,32 @@ const ROW_HEIGHT_TO_DENSITY: Record< extra_tall: 'spacious', }; +/** + * Runtime reader for {@link ROW_HEIGHT_TO_DENSITY} — the five spec row heights + * and **nothing else**. + * + * `hasOwnProperty`, not a bare index (objectui#4442): the table is a plain + * object literal, so indexing it with an unchecked key reaches + * `Object.prototype`. The parameter is typed `RowHeight`, but the boundary a + * host's stored view definition actually crosses is `SpecBridge.transformListView`, + * whose parameter is `any` — so `rowHeight: 'toString'` used to come back as + * `Object.prototype.toString`, a FUNCTION returned from a signature that + * promises three strings or nothing. `bridgeListView` then writes the key under + * `if (density)`, and a function is truthy, so it landed on the SchemaNode. + * + * This is the same guard `@object-ui/core`'s `rowHeightToDensityMode` grew in + * objectui#4440, and the repo's existing convention (`freeze-schema.ts:135`, + * `metadata-admin/predicate.ts:305`, six more). Both rowHeight surfaces now + * abstain identically on every off-spec spelling; the agreement is pinned by + * `__tests__/RowHeightDensityAgreement.test.ts`. + */ function mapDensity( rowHeight?: RowHeight, ): 'compact' | 'comfortable' | 'spacious' | undefined { if (!rowHeight) return undefined; + if (!Object.prototype.hasOwnProperty.call(ROW_HEIGHT_TO_DENSITY, rowHeight)) { + return undefined; + } return ROW_HEIGHT_TO_DENSITY[rowHeight]; }