diff --git a/.changeset/6898-objectgrid-select-fls.md b/.changeset/6898-objectgrid-select-fls.md new file mode 100644 index 0000000000..1ad21568fd --- /dev/null +++ b/.changeset/6898-objectgrid-select-fls.md @@ -0,0 +1,35 @@ +--- +'@object-ui/plugin-grid': patch +--- + +ObjectGrid: field-level security on the server `$select` projection +(objectui#6898) — the FETCH half of the gap objectui#6799 closed on the RENDER +half. + +`getSelectFields()` built the projection from the authored `columns` / `fields` +with no FLS gate, so after objectui#6799 hid the column the field name was still +being ASKED for. `perms.checkField(object, field, 'read')` now gates the +projection, on both authored arms and on the predicate-operand harvest. + +Measured, because the grade depended on it: ObjectStack's own server enforces +FLS on the RECORD, not on the projection — `plugin-security`'s read middleware +deletes an unreadable key from every returned row, and its `predicate-guard` +says in terms that the projection is deliberately unguarded because the masker +strips the value anyway (pinned over real HTTP by objectstack's +`showcase-fls-read-mask-strip.dogfood.test.ts`, where `?select=name,` +answers 200 with the key absent). So against ObjectStack this is +defence-in-depth; it becomes load-bearing for any backend that does not strip. + +Two limits are deliberate and pinned: + +- Only keys the object DECLARES are judged. `checkField` answers `false` for a + field no policy mentions, so judging an undeclared key would strip a host's + derived or joined column out of its own query. +- `id` survives even a policy that denies it, structurally — `ensureId` composes + after the gate — so row navigation cannot break. Readable predicate operands + are untouched, so objectui#3501 does not regress. + +The fetch effect now also depends on `perms.isLoaded`: `/me/permissions` +resolves asynchronously, so without it nothing would rebuild the projection +after the policy answered and the gate would never run on the only fetch most +grids make. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index e263b832dc..1de48053a1 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -1469,6 +1469,75 @@ export const ObjectGrid: React.FC = ({ const names = list.map((f: any) => columnIdentity(f)); return names.includes('id') ? list : ['id', ...list]; }; + // [objectui#6898] FIELD-LEVEL SECURITY ON THE PROJECTION — the FETCH + // half of the gap objectui#6799 closed on the RENDER half. + // + // objectui#6799 made `generateColumns()` drop a column naming a + // declared field the principal cannot read. That is what reaches the + // SCREEN. This is what goes on the WIRE: without this gate the same + // field name is still handed to the server in `$select`, so a + // backend that does not enforce FLS on the projection would return + // the value into `data` with no column on screen to reveal it. + // + // ⭐ MEASURED, not assumed (the escalation gate this card was graded + // on). ObjectStack's own server DOES enforce it — but on the RECORD, + // not on the projection. `plugin-security`'s read middleware runs + // `FieldMasker.maskResults`, whose `maskRecord` DELETES an unreadable + // key from every returned row, and `predicate-guard.ts` says in terms + // that the projection is deliberately NOT guarded because "selecting + // a hidden field is harmless because FieldMasker strips it from the + // result". Pinned end-to-end over real HTTP by objectstack's + // `showcase-fls-read-mask-strip.dogfood.test.ts`: an explicit + // `?select=name,` answers 200 with the denied key ABSENT. + // So against ObjectStack this gate is defence-in-depth (p2), exactly + // as triage graded it — it is NOT load-bearing for that backend, and + // this comment is what stops a future reader from concluding it is. + // It becomes load-bearing for any other backend, which is the same + // argument the objectui#6723 / objectui#6799 rulings accepted: the + // invariant must not rest on every future backend having enforced it. + // + // ⭐ THE DECLARED-KEY LIMIT IS THE SAME ONE, AND THE CARD IS RIGHT + // THAT THE REASONING DOES NOT TRANSFER AUTOMATICALLY — it has to be + // re-derived here, and it lands in the same place. On the render path + // an undeclared key is a legitimate derived / host-joined column. In + // a `$select` an undeclared key is what the host asked the SERVER + // for, so the question is genuinely different. It resolves the same + // way for a reason that is about `checkField`, not about drawing: + // `checkField` answers FALSE for a field the policy has never heard + // of, so judging an undeclared key is not a stricter reading of this + // rule — it is a different, wrong one, and it would strip a host's + // derived or joined column out of its own query. Undeclared ⇒ not + // this gate's business, on both halves. + // + // ⛔ NAVIGATION IS PRESERVED STRUCTURALLY, NOT BY A SPECIAL CASE: + // every call below composes `ensureId(...)` AFTER this gate, so `id` + // is re-added even in the pathological case where a policy marks it + // unreadable. A gate that filtered `id` out last would break row + // click / navigation for everyone — the naive-filter failure the + // card names by name. Keeping the restoration in the composition + // rather than in a branch here means it cannot drift out of one arm. + // + // Keyed on `objectName` (the object actually being FETCHED — + // `dataConfig.object` when a data block names one) rather than the + // render half's `schema.objectName`: the projection is judged against + // whatever object the server is about to read. + const passesProjectionGate = (entry: unknown): boolean => { + // Not loaded ⇒ nothing to ask yet; never filter on an unanswered + // policy. Same deferral as the render half — and the fetch effect + // re-runs on `perms.isLoaded` so the projection is rebuilt the + // moment the answer arrives (without that dep this gate would be + // dead on the first, and usually only, fetch). + if (!perms?.isLoaded || !objectName) return true; + const fieldName = columnIdentity(entry); + // No readable identity ⇒ nothing to ask the policy about. + if (!fieldName) return true; + // Undeclared ⇒ host-joined / derived / platform column ⇒ see above. + // `hasOwnProperty` rather than a truthiness read so an inherited + // name (`constructor`, `toString`) cannot be mistaken for a + // declared field and dropped out of the query. + if (!Object.prototype.hasOwnProperty.call(resolvedSchema?.fields ?? {}, fieldName)) return true; + return perms.checkField(objectName, fieldName, 'read'); + }; // Fields the view's PREDICATES read but no column shows // (objectui#3501). Without them the projection asks the // server for everything except the field a row action is gated on, @@ -1529,6 +1598,18 @@ export const ObjectGrid: React.FC = ({ // pinned by `__tests__/gridNonAuthorKeys.test.tsx`. userActions: (resolvedSchema as any)?.userActions, })).filter((f) => isProjectableField(f, declared as Record)) + // [objectui#6898] A predicate operand the principal cannot read + // is dropped from the projection too. This costs nothing that + // was working: against ObjectStack the server already DELETES + // that key from every row (measured above), so the operand was + // never arriving and the CEL predicate was already faulting + // `No such key` and failing CLOSED. Dropping it from `$select` + // changes what we ASK for, not what we got. Against a + // non-enforcing backend it converts "the button works, and the + // denied value sits in memory" into "the button hides" — which + // is the correct direction for a predicate gated on a field + // this principal may not read. + .filter((f) => passesProjectionGate(f)) : []; const withPredicates = (list: any[]): any[] => { if (predicateFields.length === 0) return list; @@ -1536,9 +1617,12 @@ export const ObjectGrid: React.FC = ({ const extra = predicateFields.filter((f) => !names.has(f)); return extra.length > 0 ? [...list, ...extra] : list; }; - if (schemaFields) return withPredicates(ensureId(schemaFields as any[])); + if (schemaFields) { + return withPredicates(ensureId((schemaFields as any[]).filter(passesProjectionGate))); + } if (schemaColumns && Array.isArray(schemaColumns)) { const fields = schemaColumns + .filter(passesProjectionGate) .map((c: any) => columnIdentity(c)) .filter((v): v is string => !!v); return withPredicates(ensureId(fields)); @@ -1662,7 +1746,16 @@ export const ObjectGrid: React.FC = ({ return () => { cancelled = true; }; - }, [objectName, schemaFields, schemaColumns, schemaFilter, schemaSort, headerSort, searchTerm, schemaPagination, schemaPageSize, serverPage, serverPageSize, dataSource, hasInlineData, dataConfig, refreshKey]); + // `perms.isLoaded` (objectui#6898): the projection is FLS-gated above, and + // `/me/permissions` resolves asynchronously — on the first render it is still + // `false`, so the gate defers and the first request goes out ungated. Without + // this dep nothing would ever rebuild it and the gate would be dead on the + // only fetch most grids make. The boolean, not `perms` itself: it flips + // false -> true exactly once, so this costs at most one refetch, where the + // context object's identity would re-fetch the grid on every render. + // `PermissionProvider` reports `true` synchronously and the no-provider + // default stays `false` forever, so neither of those pays anything. + }, [objectName, schemaFields, schemaColumns, schemaFilter, schemaSort, headerSort, searchTerm, schemaPagination, schemaPageSize, serverPage, serverPageSize, dataSource, hasInlineData, dataConfig, refreshKey, perms.isLoaded]); // The same reset, for the path the loader above never runs on (objectui#4501 // clause 2). "All N matching are selected" is a claim about ONE query, so it diff --git a/packages/plugin-grid/src/__tests__/projectionFls-6898.test.tsx b/packages/plugin-grid/src/__tests__/projectionFls-6898.test.tsx new file mode 100644 index 0000000000..0fce8a52b7 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/projectionFls-6898.test.tsx @@ -0,0 +1,296 @@ +/** + * 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#6898 — field-level security on the `$select` PROJECTION. + * + * ## The two halves + * + * objectui#6799 closed the RENDER half: `generateColumns()` drops a column + * naming a declared field the principal cannot read. This is the FETCH half — + * what `getSelectFields()` hands the server. Before this change the field name + * was still in `$select`, so the column was hidden while the value was still + * being ASKED for. + * + * ## What the escalation gate measured (why this file pins a p2, not a p1) + * + * ObjectStack's own server enforces FLS on the RECORD, not on the projection: + * `plugin-security`'s read middleware runs `FieldMasker.maskResults`, whose + * `maskRecord` DELETES an unreadable key from every returned row, and + * `predicate-guard.ts` states outright that the projection is deliberately not + * guarded because "selecting a hidden field is harmless because FieldMasker + * strips it from the result". Pinned end-to-end over real HTTP in objectstack's + * `showcase-fls-read-mask-strip.dogfood.test.ts`: `?select=name,` + * answers 200 with the denied key ABSENT. + * + * So against ObjectStack this gate is defence-in-depth and nothing here is + * load-bearing for that backend. It is load-bearing for any backend that does + * not strip — the same argument objectui#6723 / objectui#6799 accepted for the + * render half. + * + * ## The limit is the point (PIN 4 / PIN 5) + * + * Only keys the object DECLARES are judged. `checkField` answers `false` for a + * field no policy mentions, so judging an undeclared key would strip a host's + * derived or joined column out of its own query — a different, wrong rule. And + * the judged key is read through `columnIdentity`, never off a bare string, so + * the legacy `{ name }` spelling cannot walk a denied field through (PIN 5). + * + * ## `id` and the predicate operands (the card's decision point 2) + * + * `id` is force-added for row navigation and predicate operands are added + * though no column shows them, so a NAIVE filter breaks navigation or an action + * rather than closing a hole. PIN 3 pins that `id` survives even when the + * policy denies it — structurally, because `ensureId` composes AFTER the gate. + * + * ## Why the stub `checkField` is an ALLOWLIST + * + * Inherited from `authoredColumnsFls-6799.test.tsx` for the same reason: + * `PermissionProvider` answers `true` for a field no policy mentions, so under + * it the limit would be untestable and PIN 4 would be green in both worlds for + * the wrong reason. The stub models a server that ENUMERATES readable fields. + * + * ## ABLATION — see the PR body for the recorded run. + * + * This file imports `../ObjectGrid` relatively and the root vitest config + * aliases `@object-ui/*` to each package's `src`, so no build step stands + * between the edit and the run: the ablation reads source directly. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; +import React from 'react'; + +/** Stable stub identity — `ObjectGrid` carries `perms` in memo dep arrays. */ +const { permsStub, state } = vi.hoisted(() => { + const state: { isLoaded: boolean; readable: string[] } = { + isLoaded: true, + readable: [], + }; + return { + state, + permsStub: { + get isLoaded() { return state.isLoaded; }, + checkField: (_object: string, field: string, action: string) => + action === 'read' ? state.readable.includes(field) : true, + check: () => ({ allowed: true }), + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [], + userId: null, + systemPermissions: undefined, + hasCapabilities: () => true, + can: () => true, + cannot: () => false, + }, + }; +}); + +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, usePermissions: () => permsStub as any }; +}); + +import { ObjectGrid } from '../ObjectGrid'; +import { ActionProvider } from '@object-ui/react'; + +const OBJECT = 'opportunity'; + +/** + * `salary` is DECLARED and denied — the field under test. `computed_score` is + * deliberately NOT declared: it is the derived / host-joined key the limit + * protects. `stage` is declared and readable, and is the predicate operand. + */ +const OBJECT_FIELDS = { + name: { type: 'text', label: 'Name' }, + salary: { type: 'number', label: 'Salary' }, + stage: { type: 'select', label: 'Stage' }, +}; + +const makeDataSource = (schemaExtra: Record = {}) => ({ + // `vi.fn().mockResolvedValue(...)` rather than `vi.fn(async () => ...)`: the + // inline implementation narrows the mock's ARG tuple to `[]`, and every + // assertion here reads `find.mock.calls.at(-1)?.[1].$select` — the second arg. + find: vi.fn().mockResolvedValue({ data: [], total: 0 }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => ({ + name: OBJECT, + fields: OBJECT_FIELDS, + ...schemaExtra, + })), +}); + +/** Render a grid and return the `$select` it actually asked the server for. */ +const selectFor = async ( + schemaExtra: Record, + objectSchemaExtra: Record = {}, +): Promise => { + const ds = makeDataSource(objectSchemaExtra); + const schema: any = { type: 'object-grid', objectName: OBJECT, ...schemaExtra }; + render( + + + , + ); + await vi.waitFor(() => expect(ds.find).toHaveBeenCalled()); + return (ds.find.mock.calls.at(-1)?.[1]?.$select ?? []) as string[]; +}; + +beforeEach(() => { + vi.clearAllMocks(); + state.isLoaded = true; + state.readable = []; +}); +afterEach(() => cleanup()); + +describe('ObjectGrid — `$select` is FLS-gated (objectui#6898)', () => { + // ── PIN 1: the defect itself ──────────────────────────────────────────── + it('does NOT ask the server for a declared column the principal cannot read', async () => { + state.readable = ['name', 'stage', 'id']; + const select = await selectFor({ columns: ['name', 'salary'] }); + expect( + select, + 'the denied declared field is still being REQUESTED — objectui#6799 hid the column, ' + + 'this is the fetch half and the value would still arrive from a non-enforcing backend', + ).not.toContain('salary'); + }); + + it('still asks for the readable columns — the gate narrows, it never empties', async () => { + state.readable = ['name', 'stage', 'id']; + const select = await selectFor({ columns: ['name', 'salary'] }); + expect(select).toContain('name'); + }); + + // ── PIN 2: the `fields` arm, not just `columns` ───────────────────────── + it('gates the `fields` arm too, not only the `columns` arm', async () => { + state.readable = ['name', 'id']; + const select = await selectFor({ fields: ['name', 'salary'] }); + expect(select).not.toContain('salary'); + expect(select).toContain('name'); + }); + + // ── PIN 3: navigation survives (the card's decision point 2) ──────────── + it('keeps `id` even when the policy denies it — row navigation must not break', async () => { + // `id` deliberately absent from `readable`: the pathological case. + state.readable = ['name']; + const select = await selectFor({ columns: ['id', 'name'] }); + expect( + select, + 'without `id` the record key is undefined and row click / the primary-field ' + + 'link silently no-op — a naive filter breaks navigation rather than closing a hole', + ).toContain('id'); + }); + + // ── PIN 4: THE LIMIT — undeclared keys are not this gate's business ───── + it('leaves an UNDECLARED (derived / host-joined) key in the projection', async () => { + state.readable = ['name', 'id']; + const select = await selectFor({ columns: ['name', 'computed_score'] }); + expect( + select, + '`checkField` answers false for a field no policy mentions, so judging an undeclared ' + + 'key is a different, wrong rule — it would strip a host derived column from its own query', + ).toContain('computed_score'); + }); + + // ── PIN 5: the judged key is read through `columnIdentity` ────────────── + it('judges the legacy `{ name }` spelling — a bare-string read would wave it through', async () => { + state.readable = ['name', 'id']; + const select = await selectFor({ columns: [{ name: 'salary' }, { field: 'name' }] }); + expect(select).not.toContain('salary'); + expect(select).toContain('name'); + }); + + // ── PIN 6/7: the predicate harvest is gated, readable operands survive ── + it('drops a denied predicate operand from the projection', async () => { + state.readable = ['name', 'id']; + const select = await selectFor({ + columns: ['name'], + rowActionDefs: [{ + name: 'raise', label: 'Raise', type: 'api', locations: ['list_item'], + target: '/x', visible: 'record.salary > 0', + }], + }); + expect( + select, + 'against ObjectStack the server already DELETES this key from every row, so the ' + + 'operand never arrived and CEL already failed closed — dropping it changes what ' + + 'we ASK for, not what we got', + ).not.toContain('salary'); + }); + + it('keeps a READABLE predicate operand — objectui#3501 must not regress', async () => { + state.readable = ['name', 'stage', 'id']; + const select = await selectFor({ + columns: ['name'], + rowActionDefs: [{ + name: 'advance', label: 'Advance', type: 'api', locations: ['list_item'], + target: '/x', visible: 'record.stage == "open"', + }], + }); + expect( + select, + 'the operand of a predicate this principal CAN read must still be projected, or CEL ' + + 'faults `No such key`, fails closed, and the row action vanishes for everyone', + ).toContain('stage'); + }); + + // ── PIN 8: deferral — an unanswered policy filters nothing ────────────── + it('filters NOTHING while `/me/permissions` has not answered', async () => { + state.isLoaded = false; + state.readable = []; + const select = await selectFor({ columns: ['name', 'salary'] }); + expect( + select, + 'never filter on an unanswered policy — the no-provider default is `isLoaded: false` ' + + 'forever, and a grid with no PermissionProvider must keep working', + ).toEqual(expect.arrayContaining(['name', 'salary'])); + }); + + // ── PIN 9: the gate is not DEAD — it must survive the async answer ────── + // + // `/me/permissions` resolves asynchronously, so on the first render + // `isLoaded` is false and the gate defers. If the fetch effect does not + // depend on `perms.isLoaded`, nothing ever rebuilds the projection and the + // gate never runs on the only fetch most grids make. This is the pin that + // fails if that dependency is dropped, and no other case here would notice. + it('re-projects once the policy answers — the gate is not dead on the first fetch', async () => { + state.isLoaded = false; + state.readable = ['name', 'id']; + const ds = makeDataSource(); + const schema: any = { type: 'object-grid', objectName: OBJECT, columns: ['name', 'salary'] }; + const { rerender } = render( + + + , + ); + await vi.waitFor(() => expect(ds.find).toHaveBeenCalled()); + expect( + (ds.find.mock.calls.at(-1)?.[1]?.$select ?? []) as string[], + 'baseline: while unanswered the denied field is still requested', + ).toContain('salary'); + + // The policy answers. + state.isLoaded = true; + rerender( + + + , + ); + await vi.waitFor(() => { + const latest = (ds.find.mock.calls.at(-1)?.[1]?.$select ?? []) as string[]; + expect( + latest, + 'the projection was never rebuilt after the policy answered — `perms.isLoaded` is ' + + 'missing from the fetch effect deps and this gate is dead in practice', + ).not.toContain('salary'); + }); + }); +});