diff --git a/.changeset/7216-listview-speculative-fls.md b/.changeset/7216-listview-speculative-fls.md new file mode 100644 index 000000000..29ba6a188 --- /dev/null +++ b/.changeset/7216-listview-speculative-fls.md @@ -0,0 +1,45 @@ +--- +'@object-ui/plugin-list': patch +--- + +FLS-gate the speculative half of `ListView`'s `$select` projection (objectui#7216). + +`ListView` builds `$select` from two populations and, until now, asked them different +questions. The user-declared `columns` were passed through `perms.checkField(...)` by +objectui#6898. Everything the builder adds ON TOP — the kanban / gantt / timeline / +calendar / gallery field bindings, the timeline's auto-added `status` / `priority` +badge fields, and the operands harvested from row-action and conditional-formatting +predicates (objectui#3501) — went through `addSpeculative`, which intersected them +against the object's declared fields and then added them unconditionally. + +The two gates it needed answer unrelated questions and neither substitutes for the +other. The known-field gate keeps an **unknown** key out, because some backends answer +an unknown `$select` key with an empty result set rather than ignoring it. The FLS gate +keeps a **known but denied** key out, because sending it leaks the value at the server +boundary even though the UI hides it. A field can be perfectly well-declared and still +denied, and that was the case this path did not handle: a kanban grouped by a denied +field, or a gantt bound to a denied date, still named it in the request. + +**Reproduced before it was fixed.** Eight of fourteen new pins fail on the unmodified +tree, each reaching the denied field only through a view binding — a denied *column* has +been dropped since objectui#6898, so a pin naming one would have proved nothing. + +The gate goes inside `addSpeculative` rather than at the five call sites, so every route +into the speculative union is covered at once; gating call sites one at a time is how +the asymmetry arose. It runs **after** the known-field intersection, the ordering +objectui#7179 established: `checkField` answers false for an undeclared key, so asking +it first would drop derived and computed bindings — and would be the reason they were +dropped. The platform record columns (`created_at`, `owner_id`, the audit FKs) are +carved out for the reason they already are elsewhere in this builder: every object +carries them and none declares them, so no field policy mentions them and an FLS answer +about them is always false. Without the carve-out a calendar bound to `created_at` would +go blank for everybody. + +**Nothing a permitted view did stops working.** A permitted binding is still projected, +an unanswered permission policy filters nothing, and the projection is rebuilt when the +policy answers — pinned, because a gate that only runs before `/me/permissions` resolves +is a dead gate that passes almost every test written for it. + +objectui#7179's `addGroupingField` wrapper is removed: its predicate was identical to +the one now inside `addSpeculative`, and two spellings of one gate is the shape that +lets them drift. That path keeps its behaviour and gains the FLS pin it never had. diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 02f6e7df2..0f575c8e8 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -1731,9 +1731,44 @@ export const ListView = React.forwardRef(({ s.add('id'); s.add('created_at'); s.add('updated_at'); return s; })(); + // [objectui#7216] TWO gates, asked in this order and of this + // population — they answer unrelated questions and neither + // substitutes for the other: + // + // 1. KNOWN-FIELD gate (above) — keeps an UNKNOWN key out, because + // some backends answer an unknown `$select` key with an EMPTY + // result set rather than ignoring it. + // 2. FLS gate (here) — keeps a KNOWN BUT DENIED key out, because + // sending it leaks the value at the server boundary even though + // the UI hides it (objectui#6898). + // + // A field can be perfectly well-declared and still denied; that is + // the case this helper did not handle. The user-declared `cols` are + // FLS-filtered a few lines up, so every OTHER route into `$select` + // was gated and this one — the auto-included view bindings, the + // predicate refs, the grouping fields — was not. + // + // ⚠️ ORDER IS LOAD-BEARING (objectui#7179's shape): intersect against + // the declared fields FIRST, ask `checkField` only about the + // survivors. `checkField` answers **false for an undeclared key**, so + // asking it first would drop derived and computed bindings that are + // not real object fields — and would be the REASON they were dropped, + // which is a different and much worse failure than the known-field + // gate declining them. + // + // ⚠️ The platform columns are carved out for the same reason they are + // carved out of `addPredicateField`: every object CARRIES them and + // none DECLARES them, so no field policy mentions them and + // `checkField` answers false for every one. Without the carve-out a + // calendar bound to `created_at` would go blank for everybody. const addSpeculative = (f: unknown) => { if (typeof f !== 'string' || !f) return; - if (!knownObjectFields || knownObjectFields.has(f)) required.add(f); + if (knownObjectFields && !knownObjectFields.has(f)) return; + if (perms?.isLoaded && schema.objectName + && knownObjectFields?.has(f) + && !PLATFORM_RECORD_COLUMNS.has(f) + && !perms.checkField(schema.objectName, f, 'read')) return; + required.add(f); }; // Predicate refs take the same guard, widened by the platform columns // every object carries but no object DECLARES (`owner_id` and the @@ -1821,19 +1856,18 @@ export const ListView = React.forwardRef(({ // // FLS-gated on top (objectui#6898): the grid half of this same fix // takes `passesProjectionGate`, and a grouping field can name a - // denied field exactly as a column can. Safe to ask `checkField` - // here precisely BECAUSE `addSpeculative` ran first — everything - // reaching this point is a field the object declares, so the - // "`checkField` answers false for an undeclared key" trap that makes - // this gate wrong on derived columns cannot fire. - const addGroupingField = (f: string) => { - if (perms?.isLoaded && schema.objectName - && knownObjectFields?.has(f) - && !PLATFORM_RECORD_COLUMNS.has(f) - && !perms.checkField(schema.objectName, f, 'read')) return; - addSpeculative(f); - }; - for (const f of collectGroupingFieldRefs(groupingConfig)) addGroupingField(f); + // denied field exactly as a column can. + // + // [objectui#7216] That FLS check used to live in an `addGroupingField` + // wrapper right here. It now lives INSIDE `addSpeculative`, with a + // byte-identical predicate, because every OTHER caller of that helper + // needed the same gate and gating them one call site at a time is how + // this asymmetry arose in the first place. The wrapper is gone rather + // than left as a duplicate: two spellings of one gate is the shape + // that lets them drift. `ListView.speculativeFls-7216.test.tsx` PIN 9 + // pins this path through the moved gate — plugin-list had no FLS pin + // for grouping before it. + for (const f of collectGroupingFieldRefs(groupingConfig)) addSpeculative(f); for (const f of collectPredicateFieldRefs(listViewPredicates({ conditionalFormatting: schema.conditionalFormatting as unknown[] | undefined, diff --git a/packages/plugin-list/src/__tests__/ListView.speculativeFls-7216.test.tsx b/packages/plugin-list/src/__tests__/ListView.speculativeFls-7216.test.tsx new file mode 100644 index 000000000..99e1b6322 --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.speculativeFls-7216.test.tsx @@ -0,0 +1,382 @@ +/** + * 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#7216 — field-level security on the SPECULATIVE half of `$select`. + * + * `ListView`'s projection builder asks two different questions and, before this + * card, asked them of two different populations: + * + * | field source | known-field gate | FLS gate | + * |-----------------------------------|:----------------:|:--------:| + * | `schema.columns` | — | yes | + * | grouping fields (objectui#7179) | yes | yes | + * | view bindings via `addSpeculative`| yes | NO | + * + * The two gates exist for unrelated failures and neither substitutes for the + * other. The known-field gate keeps an UNKNOWN key out, because some backends + * (the cloud multi-tenant runtime) answer an unknown `$select` key with an + * EMPTY result set rather than ignoring it. The FLS gate keeps a KNOWN BUT + * DENIED key out, because sending it leaks the value at the server boundary + * even though the UI hides it (objectui#6898). A field can be perfectly + * well-declared and still denied; that is the case this path did not handle. + * + * ## Why these pins drive the bindings and not the columns + * + * A denied COLUMN has been dropped since objectui#6898 by + * `rawCols.filter(c => perms.checkField(...))` a few lines above. So a pin that + * named the denied field in `columns` would pass on the UNFIXED tree and prove + * nothing. Every pin below therefore reaches the denied field ONLY through a + * view binding, with the columns list holding permitted fields — that is what + * discriminates the new gate from the old one. PIN 12 states the discriminator + * as an assertion rather than leaving it as a convention. + * + * ## Ordering + * + * The gate goes INSIDE `addSpeculative`, AFTER the known-field intersection it + * already performs — the ordering objectui#7179 established for the grouping + * fields. `checkField` answers **false for an undeclared key**, so asking it + * first would drop derived and computed columns that are not real object + * fields. PIN 10 pins that ordering; PIN 11 pins the platform-column carve-out + * that comes with it (`created_at` is in `knownObjectFields` but is not a + * field any object DECLARES, so a policy never mentions it and `checkField` + * answers false for it — a calendar bound to `created_at` must not go blank). + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, waitFor, cleanup } from '@testing-library/react'; + +/** + * The stub is swapped by IDENTITY when the policy answers, because that is what + * production does: `usePermissions()` returns one identity per context value + * and "a new context value still produces a new identity, on purpose". The + * fetch effect carries `perms` in its dependency list, so identity is the + * signal that rebuilds the projection (PIN 14). + */ +const { holder, makePerms } = vi.hoisted(() => { + const makePerms = (isLoaded: boolean, readable: string[]) => ({ + isLoaded, + checkField: (_object: string, field: string, action: string) => + action === 'read' ? readable.includes(field) : true, + check: () => ({ allowed: true }), + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [], + userId: null, + systemPermissions: undefined, + hasCapabilities: () => true, + can: () => true, + cannot: () => false, + }); + return { holder: { current: makePerms(true, []) }, makePerms }; +}); + +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, usePermissions: () => holder.current as any }; +}); + +import { ListView } from '../ListView'; +import { SchemaRendererProvider } from '@object-ui/react'; + +const OBJECT = 'duly_task'; + +/** + * `computed_score` is deliberately NOT declared — it is the derived column the + * ordering trap would take down. Everything else is declared, so every pin + * below is about a KNOWN field, which is the whole point of the card. + */ +const objectDef = { + name: OBJECT, + label: 'Task', + fields: { + id: { name: 'id', type: 'text' }, + subject: { name: 'subject', type: 'text', label: 'Subject' }, + region: { name: 'region', type: 'text', label: 'Region' }, + status: { name: 'status', type: 'select', label: 'Status' }, + priority: { name: 'priority', type: 'select', label: 'Priority' }, + industry: { name: 'industry', type: 'text', label: 'Industry' }, + salary: { name: 'salary', type: 'currency', label: 'Salary' }, + start_date: { name: 'start_date', type: 'datetime', label: 'Start' }, + end_date: { name: 'end_date', type: 'datetime', label: 'End' }, + due_date: { name: 'due_date', type: 'datetime', label: 'Due' }, + cover_image: { name: 'cover_image', type: 'text', label: 'Cover' }, + }, +}; + +/** Everything the principal may read. Nothing else is readable. */ +const READABLE = ['id', 'subject', 'region', 'end_date']; + +const makeDataSource = () => + ({ + find: vi.fn(async () => ({ data: [], total: 0 })), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => objectDef), + }) as any; + +/** Mount a list view and return the params it actually asked the server for. */ +async function paramsFor(schemaExtra: Record) { + const dataSource = makeDataSource(); + const schema: any = { + type: 'list-view', + objectName: OBJECT, + viewType: 'grid', + ...schemaExtra, + }; + render( + + + , + ); + await waitFor(() => expect(dataSource.find).toHaveBeenCalled()); + const call = dataSource.find.mock.calls.at(-1)?.[1] ?? {}; + return { + select: (call.$select ?? []) as string[], + expand: (call.$expand ?? []) as string[], + }; +} + +const selectFor = async (schemaExtra: Record) => + (await paramsFor(schemaExtra)).select; + +/** The columns every pin declares: permitted, so `cols` is never empty. */ +const COLUMNS = ['subject', 'region']; + +beforeEach(() => { + vi.clearAllMocks(); + holder.current = makePerms(true, READABLE); +}); +afterEach(() => cleanup()); + +describe('ListView — speculative view bindings are FLS-gated (objectui#7216)', () => { + // ── PIN 1: kanban `groupByField` ──────────────────────────────────────── + it('does not project a DENIED kanban grouping field', async () => { + const select = await selectFor({ + columns: COLUMNS, + kanban: { groupByField: 'industry' }, + }); + expect( + select, + 'the kanban binding is intersected against the declared fields and then added ' + + 'unconditionally — `industry` is declared, denied, and reached `$select` anyway', + ).not.toContain('industry'); + }); + + // ── PIN 2: the live control for PIN 1 ─────────────────────────────────── + it('still projects a PERMITTED kanban grouping field', async () => { + const select = await selectFor({ + columns: COLUMNS, + kanban: { groupByField: 'region' }, + }); + expect( + select, + 'a gate that dropped this too would be indistinguishable from deleting the binding arm', + ).toContain('region'); + }); + + // ── PIN 3: gantt date bindings, denied and permitted in one mounting ───── + it('drops a denied gantt `startDateField` and keeps the permitted `endDateField`', async () => { + const select = await selectFor({ + columns: COLUMNS, + gantt: { startDateField: 'start_date', endDateField: 'end_date' }, + }); + expect(select).not.toContain('start_date'); + expect( + select, + 'both bindings ride the same helper, so the pin is only meaningful if the permitted ' + + 'one survives the same call', + ).toContain('end_date'); + }); + + // ── PIN 4: timeline `dateField` ───────────────────────────────────────── + it('does not project a DENIED timeline date binding', async () => { + const select = await selectFor({ + columns: COLUMNS, + timeline: { dateField: 'due_date', metaFields: ['region'] }, + }); + expect(select).not.toContain('due_date'); + expect(select).toContain('region'); + }); + + // ── PIN 5: calendar bindings ──────────────────────────────────────────── + it('does not project a DENIED calendar date binding', async () => { + const select = await selectFor({ + columns: COLUMNS, + calendar: { startDateField: 'due_date', colorField: 'region' }, + }); + expect(select).not.toContain('due_date'); + expect(select).toContain('region'); + }); + + // ── PIN 6: gallery bindings ───────────────────────────────────────────── + it('does not project a DENIED gallery cover binding', async () => { + const select = await selectFor({ + columns: COLUMNS, + gallery: { coverField: 'cover_image', titleField: 'subject' }, + }); + expect(select).not.toContain('cover_image'); + expect(select).toContain('subject'); + }); + + // ── PIN 7: the `options.` spelling is the same path ─────────────── + it('gates the nested `options.kanban` spelling too', async () => { + const select = await selectFor({ + columns: COLUMNS, + options: { kanban: { groupByField: 'industry', columns: ['salary'] } }, + }); + expect(select).not.toContain('industry'); + expect( + select, + 'kanban card `columns` are collected by the same helper as the grouping binding', + ).not.toContain('salary'); + }); + + // ── PIN 8: the timeline's AUTO-ADDED status / priority badge fields ───── + // + // These two are not authored anywhere — the builder adds them itself when a + // timeline is configured with no explicit `metaFields`. Nothing upstream can + // have filtered them, so this caller is the one with no other line of + // defence at all. + it('does not project the auto-added `status` / `priority` badges when denied', async () => { + const select = await selectFor({ + columns: COLUMNS, + timeline: { titleField: 'subject' }, + }); + expect(select).not.toContain('status'); + expect(select).not.toContain('priority'); + }); + + // ── PIN 9: `grouping.fields[]` keeps the guard objectui#7179 gave it ──── + // + // The gate moved from the `addGroupingField` wrapper into `addSpeculative`; + // that wrapper's predicate was byte-identical to the one now inside the + // helper. This pin is what makes the collapse checkable — plugin-list had no + // FLS pin for the grouping path before this file. + it('still gates a denied `grouping.fields[]` entry after the gate moved', async () => { + const select = await selectFor({ + columns: COLUMNS, + grouping: { fields: [{ field: 'industry', order: 'asc', collapsed: false }] }, + }); + expect(select).not.toContain('industry'); + expect(select).toContain('subject'); + }); + + // ── PIN 10: THE ORDERING LIMIT ────────────────────────────────────────── + it('leaves an UNDECLARED binding to the known-field gate and keeps the rest', async () => { + const select = await selectFor({ + columns: COLUMNS, + kanban: { groupByField: 'computed_score', columns: ['region'] }, + }); + expect( + select, + 'the known-field gate drops it because some backends answer an unknown `$select` key ' + + 'with an empty result set', + ).not.toContain('computed_score'); + expect( + select, + '`checkField` answers false for a key no policy mentions; a gate asked BEFORE the ' + + 'known-field intersection would judge the derived binding and, worse, would be the ' + + 'reason it was dropped', + ).toContain('region'); + }); + + // ── PIN 11: the platform-column carve-out ─────────────────────────────── + // + // `created_at` is in `knownObjectFields` (the builder adds it) but no object + // DECLARES it, so no field policy mentions it and `checkField` answers false. + // Gating it would blank every calendar bound to a platform timestamp. + it('keeps a platform record column bound by a view even though no policy grants it', async () => { + const select = await selectFor({ + columns: COLUMNS, + calendar: { startDateField: 'created_at' }, + }); + expect( + select, + 'the platform columns are provisioned on every object and published in none, so an ' + + 'FLS answer about them is always false — the carve-out is objectui#7179\'s and is ' + + 'load-bearing, not symmetry', + ).toContain('created_at'); + }); + + // ── PIN 12: THE DISCRIMINATOR ─────────────────────────────────────────── + // + // Proves the pins above measure the NEW gate and not objectui#6898's older + // column gate: the same denied field is dropped from `columns` on the + // unfixed tree, and was NOT dropped from a binding. + it('drops a denied COLUMN through the pre-existing gate (control for the new one)', async () => { + const select = await selectFor({ columns: [...COLUMNS, 'industry'] }); + expect( + select, + 'objectui#6898 already filters `schema.columns`; if this ever fails the pins above ' + + 'stop discriminating the two gates', + ).not.toContain('industry'); + }); + + // ── PIN 13: deferral — an unanswered policy filters nothing ───────────── + it('filters NOTHING while `/me/permissions` has not answered', async () => { + holder.current = makePerms(false, []); + const select = await selectFor({ + columns: COLUMNS, + kanban: { groupByField: 'industry' }, + }); + expect( + select, + 'never filter on an unanswered policy — the no-provider default is `isLoaded: false` ' + + 'forever, and a kanban with no PermissionProvider must keep grouping', + ).toContain('industry'); + }); + + // ── PIN 14: 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 stops + // depending on `perms`, nothing rebuilds the projection and this gate never + // runs on the only fetch most lists make. 9 of 10 pins passed on exactly + // such a dead gate in objectui#6898; this is the one that would not. + it('re-projects once the policy answers — the gate is not dead on the first fetch', async () => { + holder.current = makePerms(false, []); + const ds = makeDataSource(); + const schema: any = { + type: 'list-view', + objectName: OBJECT, + viewType: 'grid', + columns: COLUMNS, + kanban: { groupByField: 'industry' }, + }; + const { rerender } = render( + + + , + ); + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + expect( + (ds.find.mock.calls.at(-1)?.[1]?.$select ?? []) as string[], + 'baseline: while unanswered the denied binding is still requested', + ).toContain('industry'); + + // The policy answers — a NEW context value, hence a new identity. + holder.current = makePerms(true, READABLE); + rerender( + + + , + ); + await 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` is missing ' + + 'from the fetch effect deps and this gate is dead in practice', + ).not.toContain('industry'); + }); + }); +});