fix(agent): serve only the columns of collections the caller may read - #1840
fix(agent): serve only the columns of collections the caller may read#1840PMerlet wants to merge 5 commits into
Conversation
3 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
A read was permission-checked on the root collection alone. Every column a projection, a filter or a sort reached through a relation path came back unchecked, so a role with `read` on `cards` and nothing on `holders` could ask for `holder:nationalId` — and get it. The filter is the sharper half: it never returns the column, but one row back or zero rows back answers a `starts_with` guess, which recovers the value character by character. Check the collection each path *ends* on. Collections crossed on the way confer and require nothing: reaching one through a relation is a join, not a read, so `account:organization:name` needs `read` on `organizations` alone. That also keeps a ManyToMany through-collection out of it, which contributes no returned column. What happens on a denial depends on who asked for the field: - named by the caller, through `fields[]` or `Forest-Projection` — refused, with every offending path in one message so a client drops them all and retries once; - never asked for — dropped from the projection. `ProjectionFactory.all` expands every column of every to-one relation when no `fields[]` is sent, so refusing would turn an ordinary listing into a 403. Filters, sorts, extended searches and a chart's group-by or aggregated field are always refused. They have no prunable equivalent — dropping a condition widens the result set, dropping a sort clause silently reorders it, and a grouped-by key is chart output. A leaderboard counting a relation is the one aggregation no path describes: its value traces back to no field, so the rule above sees nothing to check. What it exposes is the cardinality of the related collection, which is what `browse` governs on `/forest/<collection>/count` and on `/relationships/<name>/count` — so assert that, on the foreign collection rather than on the through-collection a ManyToMany aggregates. The check reads the caller's own query only. Scopes and segments are injected by the agent and may legitimately reference a collection the caller cannot read. Covers get, list, csv, list-related, csv-related, count, count-related and the chart routes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0099781 to
9ea876f
Compare
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (17) 🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
hercemer42
left a comment
hercemer42
left a comment
There was a problem hiding this comment.
Validator review — PR #1840
Spec (PRD-900): conforms on every route the ticket's What shipped section lists, and the three documented divergences (leaf-only checking, refuse-named / redact-default, no redaction announcement) are implemented as written — with one gap: the ticket's own read 3 oracle stays reachable through search, not filters, because the guard only inspects search when searchExtended is also set. See the must-fix on authorization.ts:112. The scope/segment exemption is implemented and tested as specified.
Claude Opus 5 (claude-opus-5): Should fix
Applies to: packages/agent/src/routes/modification/update.ts:40-46 (not in this diff)
PUT /forest/<collection>/:id re-lists the record with ProjectionFactory.all(this.collection) and serializes the result straight into the response, so a role with edit on cards and no read on holders gets holder.nationalId back from a no-op update. It is the same disclosure this PR closes on GET /forest/cards/:id, one HTTP verb away, and a role with edit almost always has it.
modification/delete.ts:43-45 and dissociate-delete-related.ts:119-122 accept filters on the same denied related paths — destructive, so a poor oracle, but the guard is absent there too. create.ts:41 serializes only caller-supplied PKs, so it does not leak.
Routing update.ts through redactProjection with explicit: false is a two-line change and closes the read-shaped half. The filter-shaped half on the modification routes is a follow-up worth naming in the ticket's Not in scope, since "serve only the columns of collections the caller may read" reads as a claim about the whole agent.
Claude Opus 5 (claude-opus-5): Should fix
Applies to: the PR as a whole
No ADR records the decisions this diff embeds, and agent-nodejs has no docs/adr directory at all — the org-wide corpus is four records, none touching permissions (control query verified, so this is an absence rather than a failed search). Three calls here are hard to reverse, surprising without context, and a real trade-off:
- explicitly named denied paths are refused while the implicit
ProjectionFactory.allexpansion is silently redacted — one class of request 403s, another is quietly narrowed; browsestands in forreadon aCountleaderboard;- rendering scopes, segments and a customized
replaceSearchare exempt.
PRD-900 carries the rationale and the comments at authorization.ts:55-60,88-92,148 carry part of it, but this is a cross-SDK contract — the ticket says agent-ruby should be built from it, and agent-python/php have no record either. Worth running /adr in this repo so the next implementer inherits the reasoning rather than the behaviour.
|
|
||
| if ( | ||
| QueryStringParser.parseSearch(collection, context) && | ||
| QueryStringParser.parseSearchExtended(context) |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Must fix — the filter oracle PRD-900 read 3 describes stays open through search, which this guard only inspects when searchExtended is also set.
GET /forest/cards/count?search=holder.nationalId:1850&timezone=UTC returns a count for a role with browse on cards and zero permissions on holders. No filters, no sort, no searchExtended — so assertCanReadQueryFields pushes no usages at all and passes. Extend the value one character at a time and you read holders.nationalId out of a collection the caller has no read on; AND in the same string narrows it to one holder. Same on /forest/cards, .csv, the related lists and the Value/Objective charts.
relation.childProperty:term is documented end-user syntax (/product/execute/browse § Advanced search syntax) and is deliberately independent of extended search:
search/collection.ts:84—extractSpecifiedFields(parsedQuery)runs unconditionally;options.extendedgates onlydefaultFieldsat:88.search/custom-parser/fields-query-walker.ts:9— the name is turned into a path (.→:).search/collection.ts:139-164—lenientGetSchemaresolves it fuzzily (normalizeName) throughManyToOne,OneToOneandOneToMany, at any depth.search/custom-parser/condition-tree-query-walker.ts:131-155— emits a leaf on that path alone;build-string-field-filter.ts:9-13prefersIContains, i.e. a substring oracle rather than equality.
Live on the standard SQL agent: datasource-sequelize/.../model-to-collection-schema-converter.ts:156 sets searchable: false, so SearchCollectionDecorator.refineFilter takes the "implement search ourselves" branch — the ANTLR path is the normal one, not a fallback.
test/security/related-read-permissions.test.ts:296 — "should accept a plain search, which never leaves the root collection" — pins the premise with search: 'martin', which contains no : and never reaches the property-matching branch.
The check has to reuse the decorator's own resolution (extractSpecifiedFields + a lenientGetSchema equivalent) and assert read on each resolved leaf regardless of searchExtended. A name-equality allowlist built from the top-level schema will not match, because that resolution is fuzzy and runs below the rename decorator.
There was a problem hiding this comment.
Confirmed and fixed — the hole was real, and I had dismissed it once on a bad check.
My first pass ran holder:nationalId:1850 through the ANTLR grammar, which rejects it, and I concluded there was nothing here. The documented form is the dot syntax, and FieldsQueryWalker normalises . to : before the fields are resolved — so the oracle was reachable exactly as you wrote it, with no filters, no sort and no searchExtended.
The guard no longer derives anything at the route. CollectionDecorator.getSearchedFields(search, extended) returns the paths a search will actually reach and the collection each one ends on; assertCanReadQueryFields turns those into read checks. a88dbfe, then 78944c5 which moved the resolution itself down into the search decorator.
Pinned by should refuse whatever the stack says the search will reach and should ask the stack with the extended flag the caller sent.
| QueryStringParser.parseSearchExtended(context) | ||
| ) { | ||
| for (const [name, field] of Object.entries(collection.schema.fields)) { | ||
| if (field.type === 'ManyToOne' || field.type === 'OneToOne') { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix — this sweep enumerates the wrong set, in both directions.
Under-inclusive, and it is a leak. It reads collection.schema.fields at the top of the decorator stack — above PublicationDataSourceDecorator and RenameFieldCollectionDecorator (decorators-stack.ts:71-72) — while SearchCollectionDecorator sits below both (:49) and enumerates this.childCollection (search/collection.ts:88,121-137). A customer who calls .removeField('holder') on cards while holders stays published hides the relation from this loop but not from the search decorator, so searchExtended=1 still emits holder:<col> conditions and no read check ever fires. It is also to-one only, while lenientGetSchema traverses OneToMany as well.
Over-inclusive, on a collection the PR deliberately exempts. refineFilter hands the string to this.replacer and never calls getFields when a replaceSearch is installed (search/collection.ts:56-61), so a customer whose custom search only ever touches panLast4 now loses extended search entirely for any role missing read on any to-one target. Fail-closed, so not a leak — but it contradicts the ticket's replaceSearch exemption in the strict direction, and the route cannot currently tell a customized collection from a default one (schema.searchable is true either way).
There was a problem hiding this comment.
Same root cause as the oracle above, and closed by the same change — the route stopped enumerating anything, so both directions disappear rather than getting patched.
- Under-inclusive.
SearchCollectionDecorator.getSearchedFieldsanswers againstthis.childCollection, which is what the search actually reads, so a field hidden by publication or renaming above it is still accounted for. Resolution goes throughlenientGetSchema, soOneToManyis traversed too. - Over-inclusive. It returns
nullwhen a replacer is installed, and the caller must readnullas "unknown", never as "none". The exemption argued in the Macroscope thread stands, but it is now expressed by the layer that owns the decision instead of being approximated one level up.
Four tests in datasource-customizer/test/decorators/search/collections.test.ts, plus should serve the request when the stack cannot say what a search reaches on the agent side.
| collectionNames: string[], | ||
| ): Promise<Map<string, boolean>> { | ||
| const toCheck = [...new Set(collectionNames)].filter(name => name !== rootCollectionName); | ||
| const allowed = await Promise.all(toCheck.map(name => this.canRead(context, name))); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix — on agents running instantCacheRefresh: false, every one of these denials clears the whole environment permission cache and refetches it from the Forest API.
canRead → PermissionServiceWithCache.canOnCollection → ActionPermissionService.can, which passes allowRefetch: !options.instantCacheRefresh (action-permission.ts:33); on a denial hasPermissionOrRefetch calls invalidateCache() → permissionsCache.clear() and refetches before answering (:49-57, :134-138). The default is safe — options-validator.ts:42 defaults instantCacheRefresh to true, so allowRefetch is false — but instantCacheRefresh: false is the documented opt-out for deployments that cannot hold the SSE connection, and it is also the only configuration where permissionsCacheDurationInSeconds is honoured (options-validator.ts:66).
Before this PR a denied can() was a rare event: someone attempting an action they lack. This PR makes denial the steady state — PRD-900 expects roles to be missing read on related collections until an admin sweeps — so one unreadable relation in a layout turns into a cache wipe plus a full permission refetch on every list, get, CSV and chart request, process-wide for all concurrent users. Only observable as Invalidating roles permissions cache.. at Debug.
A read lookup that does not refetch on deny, or memoising the permission map per request, removes it.
There was a problem hiding this comment.
Accurate, including the part that matters most: the mechanism predates this PR, but this PR is what turns denial from a rare event into the steady state, so the cost is fairly attributed here.
Deferred to PRD-1002 rather than fixed on this branch, deliberately:
- the fix belongs in
forestadmin-client—hasPermissionOrRefetchwiping the cache on every deny is wrong for every caller, not just this guard, and it ships on its own release cycle; - memoising per request here would only cover the routes this PR touches and would leave the same storm on
assertCanBrowse,assertCanEditand the rest; - it is a performance regression on an opt-out configuration, not a correctness or security one, so it should not gate the security fix.
Already in: getReadPermissions dedupes collection names within a call, so a request costs one lookup per distinct collection rather than one per path.
| } | ||
| } | ||
|
|
||
| /** The root is skipped: its own route already asserts `browse` on a listing, `read` on a get. */ |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Preferential — the justification does not hold for the chart route, which is one of the callers.
chart.ts:50-51 asserts only assertCanExecuteChart; nothing asserts browse or read on this.collection. So for charts the unconditional [rootCollectionName, true] entry rests on the chart-hash check alone, not on a collection-level assert. Charts were never collection-permission-gated, so this is not a regression — but the comment tells the next reader (and whoever ports this to agent-ruby) that an assert exists where it does not.
Compounding it slightly: assertCanReadAggregatedFields (chart.ts:300-316) passes this.collection.name as the root while resolving paths against aggregatedCollection. That is correct for both leaderboard shapes today, but two unrelated collections arriving as arguments to the same call is worth a word in the comment.
There was a problem hiding this comment.
Both halves fixed.
The comment no longer claims an assert that does not exist. It now reads: "The root is skipped: browse gates a listing, read a get, and the signed hash a chart." — naming the actual guarantee on each route, including that the chart's rests on the hash rather than on a collection permission.
For the second half I renamed the parameter instead of adding a comment: assertCanReadAggregatedFields(context, pathCollection, fields). The two collections reaching the same call are now distinguishable by name — pathCollection is what paths resolve against, this.collection.name is the permission root — so the leaderboard call site passing aggregatedCollection reads as intentional. 46659eb.
| // A count exposes the cardinality of the relation, which `/relationships/<name>/count` puts | ||
| // behind `browse`. | ||
| if (!aggregation.field) { | ||
| await this.services.authorization.assertCanBrowse(context, field.foreignCollection); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix — this breaks dashboard leaderboards for existing roles, and neither the sequencing note nor the front PR covers that path.
A Count leaderboard on holders counting cards, for a role without browse on cards, now returns 403 where it returned counts. The chart is still rendered — dashboard chart visibility follows the chart's own collection (/product/manage/dashboards § Access control: "Users only see the charts they have access to based on their role's collection permissions") — so the widget shows an error rather than disappearing.
ForestAdmin/forestadmin#9914 prunes record, list and export projections; it does not touch chart requests, so "do not merge before the front" does not protect this, and "Residual cost" names only related labels in list and detail views. The assertion itself is right and matches count-related.ts:20; what is missing is that dashboards are in the blast radius with no front-side change to absorb it. Worth naming in the PR body and the ticket before this ships.
There was a problem hiding this comment.
Agreed, and now named in both places you asked for.
PR body — a paragraph under "Residual cost": a Count leaderboard on holders counting cards now 403s for a role without browse on cards, the widget still renders and shows an error rather than disappearing, and ForestAdmin/forestadmin#9914 does not touch chart requests, so the front-first sequencing does not cover this path.
PRD-900 — the same paragraph under "Newly settled: a leaderboard that counts", so agent-ruby inherits it rather than rediscovering it.
The assertion itself stays as you read it. Worth flagging separately to whoever runs the sweep: for this path it is browse, not read.
| Authorizations.assertCanExecuteChart = jest.fn(); | ||
| Authorizations.invalidateScopeCache = jest.fn(); | ||
| Authorizations.canRead = jest.fn().mockResolvedValue(true); | ||
| Authorizations.assertCanReadQueryFields = jest.fn(); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Should fix — these stubs mean the three *-related guards and two of the four chart shapes are asserted by nothing.
assertCanReadQueryFields is stubbed to a no-op and redactProjection to an identity passthrough, so test/routes/access/list-related.test.ts, csv-related.test.ts and count-related.test.ts exercise the absent guard and stay green whether list-related.ts:21, csv-related.ts:26 and count-related.ts:23 call it or not. The new security suite imports only Chart, Count, Csv, Get and List — no related route — and its chart cases cover Pie group-by and Leaderboard Count only, leaving makeLineChart (chart.ts:156) and computeValue (chart.ts:287, reached from Value and Objective) unpinned.
Those three routes are also the only sites passing this.foreignCollection rather than this.collection, so passing the wrong one there reproduces the exact cross-collection read this PR closes, with a fully green suite.
skills/conventions/testing.md#Cover error and edge paths, not only the happy path
There was a problem hiding this comment.
Fixed in d90b26c — by covering the routes rather than by un-stubbing the factory, which would have rewritten a large number of unrelated route tests for no gain here.
related-read-permissions.test.ts now imports ListRelated, CsvRelated and CountRelated and drives them through a real AuthorizationService, so passing this.collection where this.foreignCollection is required now fails the suite — which is the substitution those three routes are uniquely exposed to.
Chart coverage extended to the two unpinned shapes, makeLineChart and computeValue (reached from Value and Objective), alongside the Pie group-by and Leaderboard Count cases already there.
| buildContext({ query: { filters: oracleFilter } }), | ||
| ); | ||
|
|
||
| expect(list).toHaveBeenCalled(); |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Violates conventions — bare called-ness on all four allow-side cases (:248, :264, :305, :317) proves the request was not refused, not that the clause survived.
These are the only guards on the permissive half of assertCanReadQueryFields. If a later change to the guard or to ContextFilterFactory dropped the holder:nationalId condition, the injected scope, or search/searchExtended from the filter handed to Collection.list, all four still pass — and silently narrowing a permitted query is the failure mode this PR chose 403 over. Assert the filter instead: the condition tree at :248, the injected scope leaf at :264, search: 'martin' at :305, search: 'martin', searchExtended: true at :317.
The not.toHaveBeenCalled() assertions at :128, :222, :236 are fine — a call that must not happen has no arguments to assert.
skills/conventions/testing.md#Assert behavior with args and outputs
There was a problem hiding this comment.
Fixed. The four allow-side cases now assert the clause survived, not that the request escaped refusal:
- filter:
list.mock.calls[0][1].conditionTreematches theholder:nationalIdleaf; - scope: the injected leaf is asserted in the tree handed to
list; - search:
{ search: 'martin', searchExtended: true }on the filter, andgetSearchedFieldsasserted to have been called with('martin', true).
Agreed on not.toHaveBeenCalled() for the refusal cases — a call that must not happen has no arguments to assert. Left as they were.
| buildContext({}, { 'forest-projection': 'id,account:organization:name' }), | ||
| ); | ||
|
|
||
| // The `account:` primary keys `withPks` re-adds are already on the row as `cards.accountId`. |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Preferential — the justification is ManyToOne-only, and the test is the place someone will read it as the rule.
For card.account as a ManyToOne it is exactly right: account:id is cards.accountId, already on the row. For a OneToOne intermediate the key lives on the foreign side, so withPks (projection/index.ts:56-64) re-adds a primary key that nothing the caller may read carries — and Serializer.buildRelationshipsConfiguration (serializer.ts:110-119) emits it as relationships.<relation>.data.id, telling the caller that a record exists in a collection they were denied, and which one.
Narrow, and it follows from the leaf-only rule the ticket chose deliberately — so the ask is just to scope the comment to the relation type it is true for, since agent-ruby will be built from this reasoning.
There was a problem hiding this comment.
Scoped, as asked. The comment now reads:
withPksre-addsaccount:id, which a ManyToOne already carries on the row ascards.accountId. A OneToOne intermediate would expose a key the row does not carry.
The reasoning agent-ruby inherits now states the relation type it holds for, and names the OneToOne case as something the leaf-only rule leaves open rather than letting it read as covered.
| }); | ||
| }); | ||
|
|
||
| describe('keepReadableProjection', () => { |
There was a problem hiding this comment.
Claude Opus 5 (claude-opus-5): Violates conventions — this block names a method that does not exist: every assertion inside it exercises AuthorizationService.redactProjection, and no keepReadableProjection symbol appears anywhere in packages/agent/src.
Anyone grepping for the suite that pins the redact-vs-refuse policy — the load-bearing decision in this PR — finds nothing. Rename to redactProjection.
skills/conventions/testing.md#Test name states the exact behavior it asserts
There was a problem hiding this comment.
Renamed to redactProjection in 46659eb. 26 tests in that file, all green.
…wn syntax `relation.column:term` is documented end-user search syntax, and it needs neither `filters` nor `searchExtended`: `FieldsQueryWalker` rewrites the dot to a colon, and the search decorator resolves the result across relations — to-many ones included. So `?search=holder.nationalId:1850` reached a column of a collection the caller had no `read` on, and the guard pushed no usage at all. That is PRD-900's read 3 oracle, still open through a second door. Resolve those paths with the decorator's own resolver instead of a second approximation of it. `lenientGetSchema` moves out of `SearchCollectionDecorator` into a module the decorator now calls, and `getSearchedFieldPaths(collection, search)` is exported from the customizer — a string in, resolved field paths out, so no ANTLR-generated type reaches the public API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`assertCanReadQueryFields` is stubbed to a no-op and `redactProjection` to an identity passthrough in the route test factory, so the existing suites stay green whether or not a route calls either. The three `*-related` routes are the only sites resolving against `foreignCollection` rather than the route's own collection, and nothing would have caught a swap between the two; the Line, Value and Objective chart shapes went through `assertCanReadAggregatedFields` unasserted as well. Cover all of them against a real `AuthorizationService`, and make the four allow-side cases assert the clause that survived — a filter condition, the injected scope leaf, `search` and `searchExtended` — rather than that the request was not refused. Bare called-ness passes just as well when a permitted query is silently narrowed, which is the failure mode this change chose 403 over. Also scope the `withPks` note to the relation type it holds for: a ManyToOne already carries the re-added key on the row, a OneToOne does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard enumerated the collections an extended search could touch from the schema it holds — the top of the decorator stack. The search decorator reads `childCollection`, below the publication and renaming layers, so the two sets disagreed in both directions. A relation hidden by `.removeField` while its target stayed published was absent from the guard's view and still searched, so `searchExtended=1` reached its columns with no check. And a collection using `replaceSearch` was refused on `searchExtended` although its handler never runs the default enumeration at all, which the deliberate exemption for replaced searches was supposed to spare. Move the question, not the decision: `getSearchedFields(search, extended)` walks down the stack from `CollectionDecorator` and the search decorator answers it from `childCollection`, returning `null` when a replacer makes the fields the customer's choice rather than the caller's. Enforcement stays at the route, which is the only place a caller's own query is still separable from the scope and the segment the agent injects into the same filter. Each half is now pinned where it lives: the decorator's answer in the customizer's suite, the agent's use of it — refuse what is named, forward the caller's flag, serve when the stack cannot say — in the security suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The describe block named a symbol that does not exist, so a grep for the suite pinning the redact-vs-refuse policy found nothing. Also renames the chart guard's path argument, which is not always the collection whose name is passed as the permission root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Why
A read is permission-checked on the root collection only. Every column a projection, a filter or a sort reaches through a relation path is served with no check on the collection it comes from.
A role with
readoncardsand nothing at all onholdersgets this in full:The header is not even required —
GET /forest/cardswith nofields[]returns the same columns, becauseProjectionFactory.allexpands every column of every to-one relation.The filter is the sharper half. It never returns the column, yet it answers one guess per request:
One row back or zero rows back is one digit. Ten iterations per character recovers a national id in full, from a collection with zero granted permissions, without the value ever appearing in a response.
fixes PRD-900
The rule
Check the collection each path ends on. Collections crossed on the way confer and require nothing — reaching one through a relation is a join, not a read — so
account:organization:nameneedsreadonorganizationsalone, and a ManyToMany through-collection stays out of it since it contributes no returned column.What a denial does depends on who asked for the field:
fields[],Forest-Projection)ProjectionFactory.alldefault)Filters, sorts, extended searches and a chart's group-by or aggregated field are always refused. None has a prunable equivalent: dropping a condition widens the result set, dropping a sort clause silently reorders it, and a grouped-by key is chart output.
The check reads the caller's own query only. Scopes and segments are injected by the agent and may legitimately reference a collection the caller cannot read — a test locks that down.
One case the rule does not describe
A leaderboard counting a relation has no aggregate field, so no path traces back to the collection being counted. What it exposes is that collection's cardinality, which is what
browsealready governs on/forest/<collection>/countand/relationships/<name>/count— so that is what gets asserted, on the foreign collection rather than on the through-collection a ManyToMany aggregates.Found by an adversarial review pass on the first revision of this branch, not by the original implementation.
Routes covered
get,list,csv,list-related,csv-related,count,count-related, and the chart routes.Sequencing — do not merge before the front
The front asks for these columns today. It stops in ForestAdmin/forestadmin#9914, which prunes projections by
canReadCollectionand renders a denied belongsTo as restricted. That PR is green but still open. Merging this first turns every list or detail view showing a belongsTo on a denied collection into a 403.Residual cost
Roles that display a related label today without
readon the target lose it until an admin grants it. One permission sweep per project, visible and diagnosable rather than silent.Dashboard leaderboards that count a relation break for existing roles. A
Countleaderboard onholderscountingcards, for a role withoutbrowseoncards, now returns 403 where it returned counts — and the widget still renders, since dashboard chart visibility follows the chart's own collection, so it shows an error rather than disappearing. ForestAdmin/forestadmin#9914 prunes record, list and export projections and does not touch chart requests, so the sequencing note below does not cover this path. It needs the same permission sweep as the rest, onbrowserather thanread.Any client requesting relation fields its role may not read — a customer script, an
agent-clientintegration, an MCP tool — starts getting a 403 where it got a 200. That is the fix working: only integrations running under a role that should never have had the data are affected, and the error names the field and the collection so they can be fixed rather than guessed at.Not in scope
browsestill stands in for a deniedreadwhen the front resolves a get-one through the list route — tracked in PRD-990.instantCacheRefresh: false, each denied check clears and refetches the whole permission cache; denials are routine on the redaction path, so that is one permission fetch per read — tracked in PRD-1002, to be fixed inforestadmin-clientrather than here.getSearchedFieldswalks down the stack and the search decorator answers fromchildCollection, so a field hidden by.removeFieldis still checked and a replaced search is no longer refused.readof its own is left open, as PRD-900 states.Tests
test/security/related-read-permissions.test.tsbuilds the ticket's schema and role, and each case fails against the previous implementation — verified by reverting the guard, not assumed.🤖 Generated with Claude Code
Note
Enforce read permissions on related collection fields across access routes
AuthorizationService.redactProjectionto drop unreadable related fields from implicit projections or return 403 when those fields are explicitly requestedAuthorizationService.assertCanReadQueryFieldsandassertCanReadUsagesto reject filters, sort, and extended search that traverse collections the caller cannot readFieldPathUtils.getLeafCollectionto resolve the owning collection of a field path by traversing to-one relationsparseProjectionFromHeaderOrQuerynow returns an explicitness flag so callers can choose redaction vs. rejection;CsvGenerator.filterHeaderprunes CSV labels to match the redacted projectionredactProjectionin authorization.ts and route handlers in routes/access/ handle theForbiddenErrorpath correctlyChanges since #1840 opened
AuthorizationServiceto validate read permissions on each field path explicitly referenced in relation.column searches and to assert extended-search traversal permissions only on to-one relations when both a search string andsearchExtendedflag are present [d90b26c]field-pathsmodule in@forestadmin/datasource-customizerexposinglenientGetSchemaandgetSearchedFieldPathsutilities for resolving field paths across to-one and to-many relations from search strings [d90b26c]SearchCollectionDecoratorto use the importedlenientGetSchemahelper from thefield-pathsmodule instead of maintaining a private implementation [d90b26c]getSearchedFieldPathsfrom the@forestadmin/datasource-customizerpublic API [d90b26c]AuthorizationService.assertCanOnCollectionto determine searched fields and collections by callinggetSearchedFieldson the collection decorator [78944c5]SearchedFieldtype andgetSearchedFieldsmethod to the decorator system, including implementation inSearchCollectionDecoratorand supporting utilities [78944c5]getSearchedFields-driven behavior [78944c5]ChartRoute.assertCanReadAggregatedFieldsmethod [46659eb]Macroscope summarized 9ea876f.