diff --git a/.changeset/8793-related-list-identity-fail-closed.md b/.changeset/8793-related-list-identity-fail-closed.md new file mode 100644 index 0000000000..df3685489d --- /dev/null +++ b/.changeset/8793-related-list-identity-fail-closed.md @@ -0,0 +1,36 @@ +--- +"@object-ui/plugin-detail": minor +--- + +fix(plugin-detail): `record:related_list` field security now fails closed on a column it cannot name + +The block filters `columns` against the field-security allow-list built from +`enforceFieldSecurity` / `redactFields`. Its else-branch KEPT any entry whose +identity it could not resolve. That branch was the bypass: the block resolves a +column through `columnIdentity`, which deliberately refuses the table library's +own `accessorKey` (objectui#3104 — TanStack's column key is not ObjectStack +metadata identity), while `RelatedList` renders a column as +`accessorKey || columnIdentity(c)`. So a column authored `{ accessorKey: 'salary' }` +was named by nobody in the filter, skipped both the FLS check and the redact +list, and then painted its real values through the table's own key. + +An entry the security fold cannot check is now excluded rather than kept. + +**Behaviour change, deliberately narrowing.** On a related list that switches +the filter on, a column whose identity resolves to none of `field` / `name` / +`fieldName` / `key` is no longer handed down — including one authored purely in +the `accessorKey` spelling, and including the case where the redacted or denied +field is some other column entirely. Lists that set neither key are untouched: +the fold does not run there and `columns` is still handed down by reference. +The protocol-declared spelling for this key is a field-name string +(`RecordRelatedListProps.columns`), which resolves and is unaffected. + +**What that moves on screen, measured by ablation on the current base.** Two of +the three legs now have a second gate below this one: `RelatedList.filterFLS` +refuses a declared field that field security denies, and since objectui#9090 +`RelatedList.filterRedacted` refuses a redacted one — both resolving the same +`accessorKey || columnIdentity` pair this fold refuses, so both already stopped +such a column from painting. The leg this repair still moves on its own is a key +the permission evaluator has no opinion about, one the child object never +declares: `checkField` default-allows it downstream, and the fold is the only +thing that can refuse it. diff --git a/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts b/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts index 63d90159fe..a4e5dabc6f 100644 --- a/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts +++ b/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts @@ -2375,7 +2375,7 @@ const MEMBER_PINS: Record = { }, 'record:related_list.columns': { file: 'packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.columnMembers.test.tsx', - pins: 'THE `page:header.actions` HOLE ON THIS KEY, asserted as the gap it is: the registration declares `of: \'string\'` and the block folds FIVE member spellings — a bare string, the spec-canonical `{ field }`, the legacy `{ name }` / `{ fieldName }`, and `{ key }`, a tail fallback that is this block\'s alone (`columnIdentity` REFUSES it, asserted next to it, which is the whole content of "tail"). Canonical-first is proven both ways on one mixed `{ field, name }` member, so a fold reading either key alone fails. Two rows carry the sharp edge: a member whose identity does not resolve is KEPT rather than dropped, so an entry the fold cannot name is an entry it cannot filter (`accessorKey` is the instance — excluded from `columnIdentity` on purpose, read by `RelatedList` as `accessorKey || columnIdentity`, so a redacted column authored that way is kept AND rendered: filed as objectui#8793, and this row reds when it lands), and a mixed set must come back SHORTER and in order, which is the non-vacuity a single-member array cannot give. Every positive carries its control in the same call (the member survives when a DIFFERENT field is redacted), and the instrument itself — `redactFields`, a renderer-only key on neither the spec nor `inputs` — is asserted to be undeclared so the file cannot be read as licensing it. The end-to-end half (an object member reaching the screen with VALUES) is objectui#5022\'s file, which drives `RelatedList` directly and never runs this fold (objectui#8071).', + pins: 'THE `page:header.actions` HOLE ON THIS KEY, asserted as the gap it is: the registration declares `of: \'string\'` and the block folds FIVE member spellings — a bare string, the spec-canonical `{ field }`, the legacy `{ name }` / `{ fieldName }`, and `{ key }`, a tail fallback that is this block\'s alone (`columnIdentity` REFUSES it, asserted next to it, which is the whole content of "tail"). Canonical-first is proven both ways on one mixed `{ field, name }` member, so a fold reading either key alone fails. Two rows carry the sharp edge: a member whose identity does not resolve is DROPPED — the fold fails closed (objectui#8793), because an entry it cannot name is an entry it cannot check, while `RelatedList` renders that entry anyway as `accessorKey || columnIdentity` (`accessorKey` is the instance, excluded from `columnIdentity` on purpose), so keeping it was a field-security bypass — ⚠️ two of the three legs of that bypass now have a SECOND gate downstream, `filterFLS` for a declared field that field security denies and, since objectui#9090, `filterRedacted` for a redacted one, both resolving the very `accessorKey || columnIdentity` pair this fold refuses, so what THIS fold alone still decides is a key the permission evaluator has no opinion about (one the child object never declares), measured by ablation on the merged base and pinned as THE RESIDUAL LEG in `RecordRelatedListRenderer.unresolvedIdentityFailClosed-8793.test.tsx`; the same row pins that the drop is by unresolvability rather than by matching the redacted name, and that an UNFILTERED list still hands the member down untouched — and a mixed set must come back SHORTER and in order, which is the non-vacuity a single-member array cannot give. Every positive carries its control in the same call (the member survives when a DIFFERENT field is redacted), and the instrument itself — `redactFields`, a renderer-only key on neither the spec nor `inputs` — is asserted to be undeclared so the file cannot be read as licensing it. The end-to-end half (an object member reaching the screen with VALUES) is objectui#5022\'s file, which drives `RelatedList` directly and never runs this fold (objectui#8071).', }, 'record:related_list.dataSource': { file: 'packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.elementDataSource.test.tsx', diff --git a/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.columnMembers.test.tsx b/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.columnMembers.test.tsx index 946ae10203..d414987600 100644 --- a/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.columnMembers.test.tsx +++ b/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.columnMembers.test.tsx @@ -29,6 +29,11 @@ * the canonical-first `field` / `name` / `fieldName` resolution shared with the * rest of the repo, and `key` is a tail fallback this block adds on top. * + * A member that resolves to NONE of them is excluded from the filtered set + * (objectui#8793): the fold fails closed, because an entry it cannot name is an + * entry it cannot check against the field-security allow-list, while + * `RelatedList` would render it anyway through the table library's own key. + * * The end-to-end half — an object column reaching the screen with VALUES rather * than a header over blank cells — is pinned next door in * `RelatedList.columnIdentityAccessor.test.tsx` (objectui#5022), which renders @@ -166,19 +171,38 @@ describe('record:related_list — the `columns` MEMBER shape the renderer reads expect(await columnsAfterFold([mixed], ['subject'])).toEqual([mixed]); }); - it('keeps a member whose identity it cannot resolve, rather than dropping it', async () => { - // `colName` returns null and the filter's else-branch keeps the entry - // (`return n ? allowed.has(n) : true`). Pinned because it is the member - // contract's sharp edge: an entry the fold cannot NAME is an entry the fold - // cannot filter, so whatever the entry means downstream is unfiltered. - // `accessorKey` is the concrete instance — the table LIBRARY's own key, - // excluded from `columnIdentity` on purpose (objectui#3104) and read by - // `RelatedList` as `c?.accessorKey || columnIdentity(c)`. So a column - // authored that way is kept by this fold AND rendered by the table: filed - // as objectui#8793. Pinned as the CURRENT behaviour it is, which means this - // row reds when that lands — deliberately, so the fix cannot be quiet. + it('DROPS a member whose identity it cannot resolve — the fold fails CLOSED (objectui#8793)', async () => { + // `colName` returns null and the filter's else-branch EXCLUDES the entry + // (`return n ? allowed.has(n) : false`). The member contract's sharp edge, + // now pointing the safe way: an entry the fold cannot NAME is an entry the + // fold cannot check, and `RelatedList` renders it anyway as + // `c?.accessorKey || columnIdentity(c)` — so keeping it was a field-security + // bypass, not a tolerance. `accessorKey` is the concrete instance: the table + // LIBRARY's own key, excluded from `columnIdentity` on purpose + // (objectui#3104). Until objectui#8793 this row pinned the opposite. + // + // ⚠️ Two of that bypass's three legs are now caught downstream as well and + // this row is the only place that still sees THIS one: `filterFLS` refuses a + // declared field field-security denies, and since objectui#9090 + // `filterRedacted` refuses a redacted one — both resolving the same + // `accessorKey || columnIdentity` pair. What the fold alone still decides is + // a key the permission evaluator has no opinion about, pinned over rendered + // cells as THE RESIDUAL LEG in + // `RecordRelatedListRenderer.unresolvedIdentityFailClosed-8793.test.tsx`. expect(columnIdentity({ accessorKey: 'status' })).toBeUndefined(); - expect(await columnsAfterFold([{ accessorKey: 'status' }], ['status'])).toEqual([ + expect(await columnsAfterFold([{ accessorKey: 'status' }], ['status'])).toEqual([]); + + // The drop is by UNRESOLVABILITY, not by matching the redacted name — this + // is the shape of the change's blast radius and it belongs in the pin, not + // only in the PR that made it. Redacting a DIFFERENT field drops it just + // the same, because the fold still cannot say what the column is. + h.captured = null; + expect(await columnsAfterFold([{ accessorKey: 'status' }], ['some_other_field'])).toEqual([]); + + // …and the bound on that radius: with nothing to filter, the fold does not + // run and the same member is handed down untouched. + h.captured = null; + expect(await columnsAfterFold([{ accessorKey: 'status' }])).toEqual([ { accessorKey: 'status' }, ]); }); diff --git a/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.unresolvedIdentityFailClosed-8793.test.tsx b/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.unresolvedIdentityFailClosed-8793.test.tsx new file mode 100644 index 0000000000..69c355c7b1 --- /dev/null +++ b/packages/plugin-detail/src/__tests__/RecordRelatedListRenderer.unresolvedIdentityFailClosed-8793.test.tsx @@ -0,0 +1,313 @@ +/** + * 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#8793 — `record:related_list`'s field-security fold must fail + * CLOSED on a column whose identity it cannot resolve. + * + * ## The defect, in one sentence + * + * The block filtered `columns` against the field-security allow-list with an + * else-branch that KEPT any entry it could not name. `columnIdentity` + * deliberately refuses the table library's own `accessorKey` (objectui#3104 — + * it is TanStack's column key, not ObjectStack metadata identity), so a column + * authored `{ accessorKey: 'salary' }` resolved to nothing, took that branch, + * skipped both `enforceFieldSecurity` and `redactFields`, and then rendered its + * real values through `RelatedList`'s own `accessorKey || columnIdentity` read. + * A security filter and a renderer disagreeing about what a column IS is the + * whole mechanism; the else-branch is where the disagreement pays out. + * + * ## Why these pins render the REAL table + * + * The member-level fold is pinned next door in + * `RecordRelatedListRenderer.columnMembers.test.tsx`, which MOCKS `RelatedList` + * and reads the column array the block hands down. That file cannot see the + * half that makes this a data-exposure bug rather than a filtering nit: the + * VALUE on screen. So these cases mount the block over the real `RelatedList` + * and the real `data-table`, and assert on rendered cells. + * + * ## The instrument, stated rather than smuggled + * + * `enforceFieldSecurity` / `redactFields` are renderer-only keys — on neither + * `@objectstack/spec`'s `RecordRelatedListProps` nor this block's registered + * `inputs` (asserted in the columnMembers file). They are used here because + * they are the ONLY switch that makes the block read a column member at all; + * their presence here is not evidence that they are an authoring surface. + */ + +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import * as React from 'react'; +import { RecordContextProvider } from '@object-ui/react'; +import { PermissionProvider } from '@object-ui/permissions'; +import type { ObjectPermissionConfig, RoleDefinition } from '@object-ui/types'; +import { RecordRelatedListRenderer } from '../renderers/record-related-list'; +import { RelatedList } from '../RelatedList'; + +/** + * Desktop, pinned rather than inherited (the reason spelled out in + * `RelatedList.columnIdentityAccessor.test.tsx`): under the 768 breakpoint a + * `type="table"` list renders a card gallery with no cells to read. + */ +beforeAll(() => { + Object.defineProperty(window, 'innerWidth', { configurable: true, value: 1280 }); +}); + +const fields = { + subject: { type: 'text', label: 'Subject' }, + status: { type: 'text', label: 'Status' }, + salary: { type: 'text', label: 'Salary' }, +}; + +const rows = [ + // `computed_total` is deliberately ABSENT from `fields` above: a key the child + // object never declares, which is the shape the residual leg below is about. + { id: 'c1', subject: 'Fix the pump', status: 'open', salary: '90000', computed_total: 'SECRET-42' }, + { id: 'c2', subject: 'Replace filter', status: 'closed', salary: '120000', computed_total: 'SECRET-43' }, +]; + +const makeDS = () => ({ + find: vi.fn(async () => rows), + getObjectSchema: vi.fn(async (name: string) => ({ name, fields })), +}); + +/** Mount the block over the REAL `RelatedList` with the given schema extras. */ +function renderBlock(extra: Record, wrap?: (node: React.ReactNode) => React.ReactElement) { + const node = ( + + + + ); + return render(wrap ? wrap(node) : node); +} + +/** Every rendered body cell's text, in DOM order. */ +const cellTexts = () => screen.getAllByRole('cell').map((c) => (c.textContent || '').trim()); + +/** Wait until the fetched rows have painted, then answer with the cell texts. */ +async function paintedCells(): Promise { + await waitFor(() => expect(screen.getAllByRole('cell').length).toBeGreaterThan(0)); + return cellTexts(); +} + +const roles: RoleDefinition[] = [{ name: 'restricted', label: 'Restricted' }]; + +/** `read` on `contact`, and `read` on every field EXCEPT the ones named. */ +function permsDenying(...deniedFields: string[]): ObjectPermissionConfig[] { + return [ + { + object: 'contact', + roles: { + restricted: { + actions: ['read'], + fieldPermissions: deniedFields.map((field) => ({ field, read: false })), + }, + }, + }, + ]; +} + +describe('objectui#8793 — an unresolvable column identity is EXCLUDED, not kept', () => { + it('THE REPORTED LEG — a redacted column authored in the table library key does not reach the screen', async () => { + // `salary` is redacted. Authored as `{ field: 'salary' }` it was always + // dropped; authored as the table's own `accessorKey` it used to sail + // through the fold and render its values. + renderBlock({ + columns: [ + { field: 'subject', label: 'Subject' }, + { accessorKey: 'salary', header: 'Salary' }, + ], + redactFields: ['salary'], + }); + + const cells = await paintedCells(); + + // THE LIVE CONTROL, in the same render: a column whose identity DOES + // resolve and IS allowed still renders its values. Without it, "the + // redacted value is gone" is equally satisfied by a fold that filtered + // everything out, and a reviewer could not tell the repair from a rout. + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'Replace filter'])); + + // The exposure itself: neither row's salary is on screen. + expect(cells).not.toContain('90000'); + expect(cells).not.toContain('120000'); + expect(screen.queryByText('90000')).not.toBeInTheDocument(); + }); + + it('THE BOUNDARY — three columns, three different reasons', async () => { + // allowed + resolvable -> renders + // denied + resolvable -> dropped by the allow-list (always was) + // denied + unresolvable-> dropped by the fail-closed branch (this fix) + renderBlock({ + columns: [ + { field: 'subject', label: 'Subject' }, + { field: 'status', label: 'Status' }, + { accessorKey: 'salary', header: 'Salary' }, + ], + redactFields: ['status', 'salary'], + }); + + const cells = await paintedCells(); + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'Replace filter'])); + expect(cells).not.toContain('open'); + expect(cells).not.toContain('closed'); + expect(cells).not.toContain('90000'); + }); + + it('THE FLS LEG — a field-security denial reaches the unresolvable column too', async () => { + // The same fold, driven through `enforceFieldSecurity` rather than + // `redactFields`. Pinned separately because the two legs are the same + // branch in the block but NOT the same story downstream — see the census + // case below. + renderBlock( + { + columns: [ + { field: 'subject', label: 'Subject' }, + { accessorKey: 'salary', header: 'Salary' }, + ], + enforceFieldSecurity: true, + }, + (node) => ( + + {node} + + ), + ); + + const cells = await paintedCells(); + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'Replace filter'])); + expect(cells).not.toContain('90000'); + }); + + it('COUNTER-PROBE — with neither key set the fold never runs and the same column still renders', async () => { + // The bound on the blast radius, measured rather than argued: this change + // moves nothing on a related list that does not switch the filter on, and + // no in-repo producer switches it on. An unresolvable column on an + // unfiltered list renders exactly as before. + renderBlock({ + columns: [ + { field: 'subject', label: 'Subject' }, + { accessorKey: 'salary', header: 'Salary' }, + ], + }); + + const cells = await paintedCells(); + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'Replace filter', '90000'])); + }); + + it('THE LIMIT OF THIS REPAIR — an EMPTIED set still falls through to auto-derived columns, and objectui#9090 filters THOSE too', async () => { + // The bound on this repair, pinned as the behaviour it is instead of living + // only in a pull-request body. + // + // When the fold removes EVERY authored member, `RelatedList` reads the empty + // array as "no columns were authored" and derives a set from the child + // object's schema instead. That derivation is a path the block's own list + // never reached, so when this row was first written the redacted field came + // back through it — filed as objectui#9053 and pinned here red-on-landing. + // + // objectui#9090 landed first and closed it: the block now hands `redactFields` + // DOWN as well, and `RelatedList` filters the derived set by the same + // `accessorKey || columnIdentity` key it renders through. The fall-through + // itself is unchanged — that is still the bound — but the redacted value no + // longer survives it, so this row now pins the pair. + renderBlock({ + columns: [{ accessorKey: 'salary', header: 'Salary' }], + redactFields: ['salary'], + }); + + const cells = await paintedCells(); + + // THE FALL-THROUGH, proven rather than assumed: `status` was never authored, + // so its values can only be on screen because the derivation ran. Without + // this control the assertion below is equally satisfied by a list that + // rendered no columns at all — a different outcome with the same shape. + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'open'])); + + // …and the redacted field does not come back with it (objectui#9090). + expect(cells).not.toContain('90000'); + expect(cells).not.toContain('120000'); + }); + + it('THE RESIDUAL LEG — FLS on a key the child object never declares, which nothing downstream refuses', async () => { + // The one leg where this repair still moves a rendered cell, and therefore + // the case this file needs in order to stay able to fail. + // + // objectui#9090 gave the REDACT leg a second gate downstream + // (`RelatedList.filterRedacted`, resolving `accessorKey || columnIdentity`), + // and `filterFLS` beside it has always been the second gate for a DECLARED + // field that field security denies — the CENSUS case below pins exactly + // that. Both of those resolve the entry the block could not, so neither can + // see this arm any more. + // + // What neither of them refuses is a key the permission evaluator has no + // opinion about: `computed_total` is not in the child object's `fields`, so + // `perms.checkField` default-ALLOWS it and the derived/authored column + // sails through downstream. The block's fold is the only thing standing + // between it and the screen, and the fold cannot name it — which is exactly + // when it must not pass. + renderBlock( + { + columns: [ + { field: 'subject', label: 'Subject' }, + { accessorKey: 'computed_total', header: 'Total' }, + ], + enforceFieldSecurity: true, + }, + (node) => ( + + {node} + + ), + ); + + const cells = await paintedCells(); + + // THE LIVE CONTROL, in the same render: the resolvable, allowed column + // still paints, so "the value is gone" cannot be satisfied by a fold that + // dropped everything. + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'Replace filter'])); + + // The unresolvable entry does not reach the screen. + expect(cells).not.toContain('SECRET-42'); + expect(cells).not.toContain('SECRET-43'); + }); + + it('CENSUS — `RelatedList` runs its OWN field-security filter, and that one reads `accessorKey`', async () => { + // Falsifies the brief's assumption that the block's fold is the only + // security gate on this path. `RelatedList.filterFLS` resolves a column as + // `accessorKey || columnIdentity(c)` and calls `perms.checkField`, so the + // FLS leg of the bypass was already caught HERE whenever a + // PermissionProvider was mounted and loaded. Driven directly — no block, + // no fold — so the assertion is about that filter alone. + // + // ⇒ what objectui#8793 actually moves for users is the REDACT leg, which + // has no second gate. The FLS leg is repaired one layer earlier than it + // was, at the layer that names the policy. + render( + + + , + ); + + const cells = await paintedCells(); + expect(cells).toEqual(expect.arrayContaining(['Fix the pump', 'Replace filter'])); + expect(cells).not.toContain('90000'); + }); +}); diff --git a/packages/plugin-detail/src/renderers/record-related-list.tsx b/packages/plugin-detail/src/renderers/record-related-list.tsx index 7ea2f6df88..bbbbea9a1c 100644 --- a/packages/plugin-detail/src/renderers/record-related-list.tsx +++ b/packages/plugin-detail/src/renderers/record-related-list.tsx @@ -189,7 +189,27 @@ const RecordRelatedListBody: React.FC = ({ ); filteredColumns = rawColumns.filter((c) => { const n = colName(c); - return n ? allowed.has(n) : true; + // Fail CLOSED on an entry this fold cannot NAME (objectui#8793). The + // else-branch used to KEEP such an entry, and that was the bypass: the + // block resolves identity through `colName`, which deliberately refuses + // the table library's own `accessorKey` (objectui#3104), while + // `RelatedList` renders a column as `accessorKey || columnIdentity(c)`. + // So a column authored `{ accessorKey: 'salary' }` was named by nobody + // here, skipped both `enforceFieldSecurity` and `redactFields`, and then + // painted its real values through the table's own key. An entry the + // security fold cannot check is an entry it must not pass. + // + // Since objectui#9090 that example has a second gate below it: the block + // now hands `redactFields` DOWN and `RelatedList` filters by the same + // `accessorKey || columnIdentity` pair, as `filterFLS` beside it always + // did for a declared field FLS denies. What this arm alone still decides + // is a key the permission evaluator has no opinion about — one the child + // object never declares, which `checkField` default-ALLOWS downstream. + // + // Scoped to the filtering path only: with neither key set this whole + // branch is skipped and `columns` is handed down by reference, so an + // ordinary related list renders exactly what it always did. + return n ? allowed.has(n) : false; }); }