diff --git a/.changeset/6522-objects-page-lookup-keying.md b/.changeset/6522-objects-page-lookup-keying.md new file mode 100644 index 000000000..cde18d07a --- /dev/null +++ b/.changeset/6522-objects-page-lookup-keying.md @@ -0,0 +1,30 @@ +--- +'@object-ui/plugin-designer': patch +--- + +`MetadataObjectsPage` keys its object-name lookups as own entries, so deleting an object +named `constructor` (or `__proto__`) from the Object Manager actually deletes it +(objectui#6522). + +Both name lookups in the page were plain object literals filled by assignment, and the +consequential one was a READ. The delete scan asked `!nextByName[name]`, which for an +object named `constructor` answered out of `Object.prototype` with the `Object` function — +truthy — so the deletion read as "still present" and `client.reset('object', …)` never +fired. Not a refusal: the row disappeared from the manager, no error was shown, the save +reported success, and the object was still there after the next reload. Measured against +the installed `@objectstack/spec`, `ObjectSchema` pins object names to +`/^[a-z_][a-z0-9_]*$/` and accepts both `constructor` and `__proto__` — those two are +exactly the intersection with `Object.prototype`'s own names, so both are storable and +neither was deletable. + +The second lookup, one function over, failed on the WRITE instead: `byName[item.name] = +item` for an object named `__proto__` invoked the prototype setter rather than creating a +key, so the object never became an own property, never reached the Object Manager at all, +and left its payload on the lookup's prototype chain for later name lookups to answer out +of. Both are now `Map`s — neither container is ever serialised, only its values are, so a +`Map` fits where the sibling `MetadataFieldsPage` fields map (which IS the PUT body) needs +`Object.fromEntries`. + +Keying only. Nameless and duplicate entries behave exactly as before: this page writes +per-object, so the refusal semantics objectui#6489 added to the fields map are a separate +question and are deliberately not ported here. diff --git a/packages/plugin-designer/src/MetadataObjectsPage.lookupKeying.test.tsx b/packages/plugin-designer/src/MetadataObjectsPage.lookupKeying.test.tsx new file mode 100644 index 000000000..76b074a11 --- /dev/null +++ b/packages/plugin-designer/src/MetadataObjectsPage.lookupKeying.test.tsx @@ -0,0 +1,383 @@ +/** + * 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. + */ + +/** + * objectui#6522 — `MetadataObjectsPage` keys its name lookups so that an + * object whose name collides with `Object.prototype` is still reachable. + * + * Same construction defect the objectui#6489 / objectui#6240 family already + * ruled on, in a third page. This file is `MetadataObjectsPage`; the sibling + * `MetadataFieldsPage.fieldsMapKeying.test.tsx` holds the fields-map half. + * + * ## Two sites, one construction — and the consequential one is a READ + * + * SITE A — `handleObjectsChange`, the delete scan (the card's site). + * The lookup of the manager's NEW list was a plain object literal filled by + * assignment, and the delete scan asked `!nextByName[name]`. For an object + * named `constructor` that lookup answers out of `Object.prototype` and + * returns the `Object` function — truthy — so the object reads as "still + * present" and `client.reset('object', 'constructor')` never fires. The row + * disappears from the manager, no error is shown, the save reports success, + * and the object is still there after a reload. A silent no-op, not a + * refusal: the strongest evidence here is not the shape of the lookup but + * the object SURVIVING the round trip, which is what these tests assert. + * + * SITE B — `reload`, the raw-payload lookup keyed by object name. + * The same construction, one function over, failing on the write instead of + * the read: `byName[item.name] = item` for an object named `__proto__` + * invokes the prototype setter rather than creating a key. The entry never + * becomes an own property, so `Object.values(...)` never yields it and the + * object is invisible in the Object Manager — unlistable, uneditable, + * undeletable — while the server holds it happily. It also leaves that + * payload on the lookup's prototype chain, so later lookups for unrelated + * names (`label`, `icon`, `fields`, …) answer out of it. + * + * Both names are legal. Measured against the installed `@objectstack/spec`, + * and pinned below in `the instrument` so this file does not merely assert + * that the page handles a name the platform would have refused anyway. + * + * ## Why `Map` here and `Object.fromEntries` in the sibling + * + * The fields map in `MetadataFieldsPage` IS the serialised `fields` body of a + * PUT, so it must remain a plain object and the family fix there is + * `Object.fromEntries` + `Object.prototype.hasOwnProperty.call(...)` on every + * read. Neither lookup in THIS file is ever serialised: site A is built, read + * and discarded inside one callback, and site B holds raw payloads whose + * VALUES are spread into a PUT body while the container itself never reaches + * the wire. A `Map` has neither hazard structurally — a string key is just a + * key, there is no prototype to answer out of and no setter to trip — instead + * of requiring every future read in the file to remember a guard. + * + * ## ⛔ Fixture rule (objectui#6524) + * + * `{ __proto__: v }` in an object literal SETS THE PROTOTYPE (Annex B.3.1); + * it does not add a key. A fixture written that way has zero own keys and + * passes for the wrong reason. Every `__proto__` key in this file is spelled + * `['__proto__']`, a computed key, and the rule itself is pinned below. + * `JSON.parse` does define an own `__proto__` property, which is why the + * server payloads this page reads (parsed bytes) carry the name honestly. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, render, screen, waitFor } from '@testing-library/react'; +import { ObjectSchema } from '@objectstack/spec/data'; +import { MetadataClient } from '@object-ui/data-objectstack'; +import type { ObjectDefinition } from '@object-ui/types'; + +interface ServerItem { + name: string; + label: string; + pluralLabel?: string; + icon?: string; + isSystem?: boolean; + fields: Record; +} + +const item = (name: string, label: string): ServerItem => ({ + name, + label, + pluralLabel: `${label}s`, + isSystem: false, + fields: { name: { type: 'text', label: 'Name' } }, +}); + +interface RecordedManagerProps { + objects: ObjectDefinition[]; + onObjectsChange?: (objects: ObjectDefinition[]) => void; + showSystemObjects?: boolean; + readOnly?: boolean; +} + +let managerProps: RecordedManagerProps | null = null; + +vi.mock('./ObjectManager', () => ({ + ObjectManager: (props: RecordedManagerProps) => { + managerProps = props; + return null; + }, +})); + +import { MetadataObjectsPage } from './MetadataObjectsPage'; + +/** + * A server that actually holds state, so "the object survives the round trip" + * is observable rather than inferred. GET serves the current contents, DELETE + * removes, PUT upserts — the same three doors `MetadataClient` uses. + */ +let serverObjects: ServerItem[] = []; +let deletes: string[] = []; +let puts: Array<{ name: string; body: Record }> = []; + +function json(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +const nameFromUrl = (url: string): string => + decodeURIComponent(url.split('/api/v1/meta/object/')[1]?.split('?')[0] ?? ''); + +function realClient(): MetadataClient { + return new MetadataClient({ + baseUrl: 'http://localhost:3000', + fetch: (async (input: RequestInfo | URL, init?: RequestInit) => { + const method = (init?.method ?? 'GET').toUpperCase(); + const url = String(input); + if (method === 'DELETE') { + const name = nameFromUrl(url); + deletes.push(name); + serverObjects = serverObjects.filter((o) => o.name !== name); + return json({ success: true, reset: true }); + } + if (method === 'PUT') { + const name = nameFromUrl(url); + const body = JSON.parse(String(init?.body ?? '{}')) as Record; + puts.push({ name, body }); + serverObjects = [ + ...serverObjects.filter((o) => o.name !== name), + body as unknown as ServerItem, + ]; + return json({ success: true }); + } + return json({ items: serverObjects }); + }) as unknown as typeof fetch, + }); +} + +const shownError = (): string | null => + screen.queryByTestId('metadata-objects-page-error')?.textContent ?? null; + +/** Render against the current `serverObjects` and wait for the first paint. */ +async function renderPage(expectedRows: number) { + render(); + await waitFor(() => expect(managerProps).not.toBeNull()); + await waitFor(() => expect(managerProps!.objects).toHaveLength(expectedRows)); +} + +/** Remove one row through the manager, exactly as `ObjectManager` would. */ +async function deleteThroughManager(name: string) { + const next = managerProps!.objects.filter((o) => o.name !== name); + await act(async () => { + managerProps!.onObjectsChange!(next); + }); +} + +const namesOnServer = (): string[] => serverObjects.map((o) => o.name).sort(); +const namesInManager = (): string[] => managerProps!.objects.map((o) => o.name).sort(); + +beforeEach(() => { + serverObjects = []; + deletes = []; + puts = []; + managerProps = null; +}); + +afterEach(() => { + managerProps = null; +}); + +const parsesAsObjectName = (name: string): boolean => + ObjectSchema.safeParse({ + name, + label: 'C', + fields: { n: { type: 'text', label: 'N' } }, + }).success; + +describe('the instrument', () => { + it('the spec ACCEPTS both prototype-colliding names, so this is the page`s problem to solve', () => { + // Without this, every case below could be dismissed as "the platform would + // have refused that name anyway". It would not: measured against the + // installed `@objectstack/spec` 17.2.0. + expect(parsesAsObjectName('constructor')).toBe(true); + expect(parsesAsObjectName('__proto__')).toBe(true); + }); + + it('and those two are the WHOLE reachable set — the rest of the prototype is refused by name', () => { + // Why the cases below stop at two names rather than sweeping the whole + // prototype. `ObjectSchema` pins object names to /^[a-z_][a-z0-9_]*$/, and + // every other own name on `Object.prototype` carries a capital + // (`toString`, `hasOwnProperty`, `valueOf`, `__defineGetter__`, …), so it + // can never be stored in the first place. This is a measurement, not an + // assumption: if the spec ever loosens that pattern, or a future engine + // adds a lowercase member, this test reds and names what the page's + // lookups newly have to survive. + const reachable = Object.getOwnPropertyNames(Object.prototype) + .filter(parsesAsObjectName) + .sort(); + expect(reachable).toEqual(['__proto__', 'constructor']); + expect(parsesAsObjectName('toString')).toBe(false); + }); +}); + +describe('the mechanic — plain JavaScript, before any claim about the page', () => { + it('a plain-object lookup answers out of `Object.prototype`; a `Map` does not', () => { + // SITE A in one assertion. Nothing named `constructor` was ever put in. + const assigned: Record = {}; + assigned['account'] = { name: 'account' }; + expect(Boolean(assigned['constructor'])).toBe(true); + expect(Boolean(assigned['toString'])).toBe(true); + + const built = new Map([['account', { name: 'account' }]]); + expect(built.has('constructor')).toBe(false); + expect(built.has('toString')).toBe(false); + }); + + it('assigning `__proto__` sets the prototype instead of adding a key — and then answers for OTHER names', () => { + // SITE B in one assertion: the entry never becomes a key (so it is never + // yielded by `Object.values`), and it poisons unrelated lookups. + const assigned: Record = {}; + assigned['__proto__'] = { name: '__proto__', label: 'Proto' }; + expect(Object.keys(assigned)).toEqual([]); + expect(Object.values(assigned)).toEqual([]); + expect(assigned['label']).toBe('Proto'); + + const built = new Map([ + ['__proto__', { name: '__proto__', label: 'Proto' }], + ]); + expect([...built.keys()]).toEqual(['__proto__']); + expect(built.get('label')).toBeUndefined(); + }); + + it('is a fixture rule too: `{ __proto__: v }` in a LITERAL sets the prototype, it does not add a key', () => { + // objectui#6524. Why every `__proto__` key in this file is computed. A + // fixture written the plain way carries zero own keys and passes for the + // wrong reason — the assertion would be about `{}`. + const plain = { __proto__: { name: '__proto__', label: 'P' } } as Record; + expect(Object.keys(plain)).toEqual([]); + const computed = { ['__proto__']: { name: '__proto__', label: 'P' } } as Record; + expect(Object.keys(computed)).toEqual(['__proto__']); + // And why payloads parsed from the wire read honestly: `JSON.parse` + // defines an own property rather than invoking the setter. + expect(Object.keys(JSON.parse('{"__proto__":{"label":"P"}}'))).toEqual(['__proto__']); + }); +}); + +describe('objectui#6522 · SITE A — the delete scan issues the DELETE for a prototype-named object', () => { + it('C0 (control): an ordinary object is deleted and is gone after the reload', async () => { + // Green before and after this card. Its job is to prove the cases below + // are about the lookup keying and not about deletes having stopped. + serverObjects = [item('account', 'Account'), item('contact', 'Contact')]; + await renderPage(2); + + await deleteThroughManager('contact'); + + // Outcome first, mechanism second — the order every case in this group + // uses, so a regression fails on the user-visible fact rather than on the + // request log. + expect(shownError()).toBeNull(); + await waitFor(() => expect(namesInManager()).toEqual(['account'])); + expect(namesOnServer()).toEqual(['account']); + expect(deletes).toEqual(['contact']); + }); + + it('H1: an object named `constructor` is really deleted — it does NOT survive the reload', async () => { + // THE case this card exists for, and the realistic one: an ordinary + // lowercase identifier a business object could plausibly be called. + // + // Before the fix `!nextByName['constructor']` answered out of + // `Object.prototype` with the `Object` function — truthy — so `reset` was + // never called. The row vanished from the manager, NO error was shown, + // and the object came straight back on the next load. + serverObjects = [item('account', 'Account'), item('constructor', 'Constructor')]; + await renderPage(2); + + await deleteThroughManager('constructor'); + + // The silence first: whatever happened, the page reported success. + expect(shownError()).toBeNull(); + // THE silent-no-op assertion, and the one that reds without the fix: the + // object is still listed after the reload. It was never a refusal that + // stopped the delete — nothing was shown, the row just came back. + await waitFor(() => expect(namesInManager()).toEqual(['account'])); + expect(namesOnServer()).toEqual(['account']); + // Only then the mechanism that produced it. + expect(deletes).toEqual(['constructor']); + }); + + it('H2: an object named `__proto__` is really deleted — it does NOT survive the reload', async () => { + // Needs both sites correct: site B must key it as an own property for it + // to be listed at all, and site A must not read it back off the prototype. + serverObjects = [item('account', 'Account'), item('__proto__', 'Proto')]; + await renderPage(2); + + await deleteThroughManager('__proto__'); + + expect(shownError()).toBeNull(); + await waitFor(() => expect(namesInManager()).toEqual(['account'])); + expect(namesOnServer()).toEqual(['account']); + expect(deletes).toEqual(['__proto__']); + }); + + it('the deletes above are the only writes — no object is resurrected by a stray save', async () => { + // Falsification for the whole group: a page that PUT every row on every + // change would keep `namesOnServer()` correct for the wrong reason. + serverObjects = [item('account', 'Account'), item('constructor', 'Constructor')]; + await renderPage(2); + + await deleteThroughManager('constructor'); + + expect(puts).toEqual([]); + }); +}); + +describe('objectui#6522 · SITE B — the raw-payload lookup keys every server object as an own entry', () => { + it('an object named `__proto__` served by the server actually reaches the manager', async () => { + // The second construction in this file, failing on the WRITE. Built by + // assignment, `byName['__proto__'] = item` invoked the prototype setter, + // so `Object.values(...)` never yielded it: the object was invisible in + // the Object Manager while the server held it. + serverObjects = [item('account', 'Account'), item('__proto__', 'Proto')]; + await renderPage(2); + + expect(namesInManager()).toEqual(['__proto__', 'account']); + const proto = managerProps!.objects.find((o) => o.name === '__proto__')!; + expect(proto.label).toBe('Proto'); + // Derived like every other row — it is a first-class object, not a stub. + expect(proto.group).toBe('Custom Objects'); + expect(proto.fieldCount).toBe(1); + }); + + it('non-vacuity: the fixture really is what the server sent, own key and all', () => { + // If the `__proto__` fixture ever lost its name the case above would stay + // green while testing nothing. Parsed bytes, so the key is honest. + const [, proto] = JSON.parse( + JSON.stringify([item('account', 'Account'), item('__proto__', 'Proto')]), + ) as ServerItem[]; + expect(proto.name).toBe('__proto__'); + // …and the name really is one blind assignment drops: this is the exact + // construction the page used, run on the exact fixture. + const dropped: Record = {}; + dropped[proto.name] = proto; + expect(Object.keys(dropped)).toEqual([]); + }); + + it('an object named `constructor` round-trips an edit onto its OWN payload', async () => { + // The read half of site B: `prev[updated.name]` is what the save-back + // merges onto. Off a plain object literal a name that is not an own key + // answers with an inherited value instead of `undefined`, so the merge + // base and the redundant-save guard both consult the wrong thing. + serverObjects = [item('constructor', 'Constructor')]; + await renderPage(1); + + const next = managerProps!.objects.map((o) => ({ ...o, label: 'Renamed' })); + await act(async () => { + managerProps!.onObjectsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); + + expect(puts[0].name).toBe('constructor'); + expect(puts[0].body.label).toBe('Renamed'); + // The rest of the server document survived the merge — proof the base was + // the real payload rather than something off the prototype chain. + expect(puts[0].body.pluralLabel).toBe('Constructors'); + expect(puts[0].body.fields).toBeDefined(); + expect(deletes).toEqual([]); + expect(shownError()).toBeNull(); + }); +}); diff --git a/packages/plugin-designer/src/MetadataObjectsPage.tsx b/packages/plugin-designer/src/MetadataObjectsPage.tsx index 33e2fde1a..5cf52af3d 100644 --- a/packages/plugin-designer/src/MetadataObjectsPage.tsx +++ b/packages/plugin-designer/src/MetadataObjectsPage.tsx @@ -110,8 +110,23 @@ function toObjectDefinition(raw: ServerObjectSchema): ObjectDefinition { interface ServerObjectsState { loading: boolean; error: string | null; - /** Raw server payloads, indexed by object name (for save-back merging). */ - byName: Record; + /** + * Raw server payloads, indexed by object name (for save-back merging). + * + * A `Map`, not a plain object (objectui#6522). Object names are server data: + * `ObjectSchema` pins them to /^[a-z_][a-z0-9_]*$/, which accepts BOTH + * `constructor` and `__proto__`. Filled by assignment into an object + * literal, the `__proto__` entry invoked the prototype setter instead of + * creating a key — the object never became an own property, so it never + * reached the manager at all and left its payload on the lookup's prototype + * chain for every later name to answer out of. A `Map` key is just a key. + * + * Nothing serialises this container — only its VALUES are spread into a PUT + * body — so unlike the fields map in the sibling `MetadataFieldsPage` it has + * no reason to stay a plain object. Same ruling (objectui#6489 / + * objectui#6240), the shape that fits this lookup. + */ + byName: Map; } export function MetadataObjectsPage({ @@ -133,17 +148,17 @@ export function MetadataObjectsPage({ const [state, setState] = useState({ loading: true, error: null, - byName: {}, + byName: new Map(), }); const reload = useCallback(async () => { setState((s) => ({ ...s, loading: true, error: null })); try { const items = await client.list('object'); - const byName: Record = {}; + const byName = new Map(); for (const item of items) { if (item && typeof item === 'object' && typeof item.name === 'string') { - byName[item.name] = item; + byName.set(item.name, item); } } setState({ loading: false, error: null, byName }); @@ -151,7 +166,7 @@ export function MetadataObjectsPage({ setState({ loading: false, error: err instanceof Error ? err.message : String(err), - byName: {}, + byName: new Map(), }); } }, [client]); @@ -161,7 +176,7 @@ export function MetadataObjectsPage({ }, [reload]); const objects = useMemo( - () => Object.values(state.byName).map(toObjectDefinition), + () => [...state.byName.values()].map(toObjectDefinition), [state.byName], ); @@ -179,14 +194,21 @@ export function MetadataObjectsPage({ */ const handleObjectsChange = useCallback(async (next: ObjectDefinition[]) => { const prev = state.byName; - const nextByName: Record = {}; - for (const o of next) nextByName[o.name] = o; + // objectui#6522 — a `Map`, because the READ below is the consequential + // half. Built by assignment into an object literal, `!nextByName[name]` + // answered out of `Object.prototype`: for an object named `constructor` + // the lookup returned the `Object` function, the deletion read as "still + // present", and `client.reset` never fired. No error, no refusal — the row + // vanished from the manager, the save reported success, and the object was + // still there after the reload. `Map.has` consults nothing but the entries + // actually put in. Never serialised: built, read and discarded right here. + const nextByName = new Map(next.map((o) => [o.name, o])); const errors: string[] = []; // Deletions - for (const name of Object.keys(prev)) { - if (!nextByName[name]) { + for (const name of prev.keys()) { + if (!nextByName.has(name)) { try { await client.reset('object', name); } catch (err) { @@ -197,7 +219,13 @@ export function MetadataObjectsPage({ // Inserts + updates for (const updated of next) { - const base = prev[updated.name] ?? { name: updated.name }; + // One own-entry lookup feeding both the merge base and the + // redundant-save guard below (objectui#6522). Off a plain object literal + // a name that was never stored still answered — `prev['constructor']` + // handed back the `Object` function — so the merge spread the wrong base + // and the guard below compared against inherited `undefined`s. + const previous = prev.get(updated.name); + const base = previous ?? { name: updated.name }; const merged: ServerObjectSchema = { ...base, name: updated.name, @@ -217,11 +245,11 @@ export function MetadataObjectsPage({ delete merged.group; // Don't issue redundant saves if nothing visible changed. if ( - prev[updated.name] - && prev[updated.name].label === merged.label - && prev[updated.name].pluralLabel === merged.pluralLabel - && prev[updated.name].description === merged.description - && prev[updated.name].icon === merged.icon + previous + && previous.label === merged.label + && previous.pluralLabel === merged.pluralLabel + && previous.description === merged.description + && previous.icon === merged.icon ) { continue; } @@ -239,7 +267,7 @@ export function MetadataObjectsPage({ }, [client, reload, state.byName]); const handleSelectObject = useCallback((obj: ObjectDefinition) => { - const raw = state.byName[obj.name]; + const raw = state.byName.get(obj.name); if (raw && onSelectObject) onSelectObject(obj, raw); }, [onSelectObject, state.byName]);