Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-grid): gate the server $select projection on field-level security - #7090
Merged
Conversation
…ecurity ObjectGrid's getSelectFields() built the projection from the authored columns/fields with no FLS gate, so after the render-half fix the hidden field was still being requested from the server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM
`vi.fn(async () => ...)` narrows the mock's arg tuple to `[]`, and every assertion reads `find.mock.calls.at(-1)?.[1].$select` — the second arg. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-warren
marked this pull request as ready for review
August 31, 2026 17:52
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6898
Field-level security on ObjectGrid's server
$selectprojection — the FETCH half of the gap #6799 closed on the RENDER half.The escalation gate, answered first (this is what the grade rests on)
Triage made one measurement the first mandatory step: does ObjectStack's own REST enforce FLS on the
$selectprojection? Answered by reading the enforcement path in theobjectstacksibling checkout, not by assuming the server catches it.Answer: ENFORCED — but on the RECORD, not on the projection. Branch 1 of the ruling. p2 stands; this PR is defence-in-depth.
Four independent readings, listed weakest to strongest:
plugins/plugin-security/src/security-plugin.tsstep 4 (post-next()) runsFieldMasker.maskResultson everyfind/findOne/insert/updateresult.field-masker.ts'smaskRecorddoesdelete result[field]for each non-readable field — the key is deleted, not nulled.predicate-guard.tssays so in terms, and explains why the projection is deliberately left unguarded:An executed end-to-end HTTP pin,
qa/dogfood/test/showcase-fls-read-mask-strip.dogfood.test.ts:GET /data/showcase_project/{id}?select=name,{denied}answers 200 with the denied key absent, and the same projection serves an admin the real value.That is precisely the ruling's first branch — "a server that enforces FLS refuses or omits it — no exposure". ObjectStack omits. So nothing here is load-bearing for ObjectStack, and the code comment at the gate says so, to stop a future reader concluding otherwise. It becomes load-bearing for any backend that does not strip — the same argument the #6723 / #6799 rulings accepted for the render half.
Mandatory question 2 — what else is concatenated into
$select, as an enumerationgetSelectFields()(re-derived at L1460, not the card's day-old L1402) composes from six sources, not the two the card named:schemaFields(thefieldsprop)schemaColumns(thecolumnsprop) viacolumnIdentityid, force-added byensureIdconditionalFormatting—condition,expression, and the native{ field }shaperowActionDefs/bulkActionDefs/ objectactions—visible,disabled, andrecordIdFielduserActions—visibleWhen/disabledWhenSource 5's
recordIdField(objectstack#8018) is the one the card did not name. All of 4–6 passisProjectableField, which also admits the undeclared platform columns (created_at,owner_id,organization_id, …).The inference-leak trap — measured, and it is NOT a new card
The ruling flagged that a field the principal cannot read used as a filter may be an inference leak even when the value never returns. Measured: already closed server-side, before this card existed.
plugin-security'sassertReadableQueryFields(anti filter-oracle, #2251) rejects with 403 PERMISSION_DENIED, naming the offending field, when the caller's ownwhere/orderBy/groupBy/having/aggregationsreference an unreadable field — and it deliberately runs against the caller's verbatim AST, before RLS injection, so injected policy filters referencingowner_idare not caught by it. Pinned live in the same dogfood file: filtering and sorting on the denied field each answer 403, while the entitled caller still can. No card filed — the suspected gap does not exist.The change
perms.checkField(object, field, 'read')now gates the projection, on both authored arms and on the predicate harvest.Two limits are deliberate, and both are pinned:
$selectit is what the host asked the server for, so it had to be re-derived. It lands in the same place for a reason aboutcheckFieldrather than about drawing:checkFieldanswersfalsefor a field no policy mentions, so judging an undeclared key is not a stricter reading of this rule, it is a different and wrong one — it would strip a host's derived or joined column out of its own query.idsurvives even a policy that denies it, structurally: every arm composesensureId(...)after the gate. Keeping the restoration in the composition rather than in a branch means it cannot drift out of one arm. This is the card's decision point 2 — a naive filter breaks navigation rather than closing a hole.Denied predicate operands are dropped too, and this costs nothing that was working: against ObjectStack the server already deletes that key from every row, so the operand never arrived and the CEL predicate was already faulting
No such keyand failing closed. Dropping it changes what we ask for, not what we got. Against a non-enforcing backend it turns "the button works, and the denied value sits in memory" into "the button hides" — the correct direction for a predicate gated on a field this principal may not read. Readable operands are untouched, so #3501 does not regress (pinned).One non-obvious dependency, and why it is load-bearing
The fetch effect now also depends on
perms.isLoaded./me/permissionsresolves asynchronously, so on the first renderisLoadedisfalseand the gate correctly defers. Without this dependency nothing would ever rebuild the projection after the policy answered, and the gate would be dead on the only fetch most grids make. Ablation B below is the isolated proof. The boolean rather thanpermsitself: it flips false→true exactly once, so this costs at most one refetch, where the context object's identity would refetch on every render.PermissionProviderreportstruesynchronously and the no-provider default staysfalseforever, so neither pays anything.Known limit, stated rather than papered over
Under
MePermissionsProviderthe first request still goes out ungated, in the window before/me/permissionsanswers; the projection is corrected on the refetch. Closing that window entirely would mean blocking the grid's fetch on permissions, which would hang every host with noPermissionProvider(isLoadedisfalsethere forever). Given the measured server behaviour the residual against ObjectStack is nil.Verification
All runs at final commit
91c67d2, through the shared verification lock. Exit codes captured before any pipe.vitest run packages/plugin-grid/pnpm --filter @object-ui/plugin-grid run type-check(tsc --noEmit && tsc -p tsconfig.test.json)eslint --no-inline-configon both changed filesThe suite was first run from the package directory and the repo's own invocation guard refused it (#3378 — a package-cwd run silently executes the console package's 22 files and reports green). Re-run from the repo root as the guard prescribes; the numbers above are from the correct invocation.
Lint narrowing, declared. Repo-wide
pnpm lintis CI's run. The narrowing to 2 files is a measurement, not a skip: the population comes from eslint's own config rather than my guess, the count (2) is read from--format json, and type-aware linting is not enabled ineslint.config.js(noparserOptions.project/projectService) — so this diff cannot move the verdict on any file it does not touch. Warning counts are byte-identical toorigin/mainrule-for-rule (213 → 213,react-hooks/exhaustive-depsunchanged at 10, so the new dependency satisfies the rule rather than suppressing it).Ablation — the gate can actually see the change
Both legs committed first, so the restore leg had a real reference. Each mutation was proven on disk by blob hash before the suite ran (an editor's exit code is not evidence — a zero-hit replace exits 0), and each restore proven by
git diff HEADempty and blob equality againstHEAD.Ablation A — strip the gate from both authored arms. HEAD blob
1de48053, mutated blob2c565d99.→ 4 of 10 red: PIN 1 (denied column still requested), PIN 2 (
fieldsarm), PIN 5 (legacy{ name }spelling), PIN 9.Ablation B — remove only the
perms.isLoadeddependency. HEAD blob1de48053, mutated blobbdac2a69.→ exactly 1 of 10 red: PIN 9, "the gate is not dead on the first fetch". The other nine stayed green — which is the point: with a dead gate, nine pins would have reported success.
Restore verified after both:
git hash-object=1de48053=HEAD, working tree clean.Scope
packages/plugin-grid/src/only —ObjectGrid.tsxplus its new test, and the changeset.generateColumns()is untouched: the maintainer's 2026-08-30 ruling deliberately scoped #6799 to that one function.The export path was checked and needs nothing: it derives its
fieldsfromgenerateColumns(), so it already inherits the #6799 render-half gate.Generated by Claude Code