fix(agent): serve only the columns of collections the caller may read - #365
fix(agent): serve only the columns of collections the caller may read#365PMerlet wants to merge 4 commits into
Conversation
A read was permission-checked on the root collection only, so every column a projection, filter or sort reached through a relation was served with no check on the collection it came from — and a starts_with filter answered one guess per request without returning a column at all. The collection a path ends on is now checked. A field the caller named is refused with every offending path in one message; the default expansion is dropped from the projection instead, since refusing there would turn an ordinary listing into a 403. A polymorphic relation resolves to several collections and carries no discriminant, so every target must be readable: one denied is enough to deny the path. The resolver lives in the toolkit so the search layer and the routes cannot disagree about what a path reaches. A Count leaderboard names no path back to the collection it counts, so browse is asserted on it directly. fixes PRD-900
read_permissions returned only the root collection, so every related name was missing from the map and read as denied. can? allows everything when no permission system is configured, and this side of the check has to agree: without the fix, any projection through a relation was redacted and any named field 403ed on those deployments. Three more routes serialize a record back with a projection of ours: store, and both sites in update_field. They are redacted like update, through one helper the four of them now share. The search guard no longer parses a search on a collection that has none — parse_search raises there, which turned a parameter the chart routes ignore into a 400. Pins the toolkit path resolver and the search layer's answer, neither of which had a test.
Eleven blocks restated a signature, a method name or a doc sitting on the declaration they were calling. The rationale that cannot be read off the code stays.
1 new issue
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| extended = Utils::QueryStringParser.parse_search_extended(args) | ||
| searched = collection.searched_fields(search, extended) | ||
|
|
||
| searched&.each do |field| |
There was a problem hiding this comment.
🔴 Critical services/permissions.rb:302
When searched_fields returns nil, the search is accepted without any related-collection permission checks, allowing searches against unreadable fields to leak information through result presence. nil denotes unknown fields for custom replacers and native child searches, but searched&.each treats it as no fields; fail closed by raising ForbiddenError when the footprint is unknown.
- searched&.each do |field|
+ raise ForbiddenError, "Unable to determine searchable fields for permission check." if searched.nil?
+
+ searched.each do |field|Also found in 1 other location(s)
packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb:65
searched_fieldsreturnsnilwhen@replaceris installed or the child collection searches natively. The permission guard consumes this withsearched&.each, soniladds no usages and the search is allowed without checking any target collection. A custom replacement or native search that reaches a field on an unreadable related collection therefore retains the search-based data inference vulnerability this change is intended to close; an unknown field set must fail closed rather than be represented as an empty check.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb around line 302:
When `searched_fields` returns `nil`, the search is accepted without any related-collection permission checks, allowing searches against unreadable fields to leak information through result presence. `nil` denotes unknown fields for custom replacers and native child searches, but `searched&.each` treats it as no fields; fail closed by raising `ForbiddenError` when the footprint is unknown.
Also found in 1 other location(s):
- packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb:65 -- `searched_fields` returns `nil` when `@replacer` is installed or the child collection searches natively. The permission guard consumes this with `searched&.each`, so `nil` adds no usages and the search is allowed without checking any target collection. A custom replacement or native search that reaches a field on an unreadable related collection therefore retains the search-based data inference vulnerability this change is intended to close; an unknown field set must fail closed rather than be represented as an empty check.
There was a problem hiding this comment.
Two causes of nil are collapsed here, and they do not have the same status.
replace_search is a decided exemption, not an oversight. PRD-900 records it: the customer wrote the handler, and the caller controls the search string, not the fields it maps to — unlike filters={"field":"holder:national_id"}, they cannot aim at a column of their choosing. Authorizing the refined condition tree instead, which is the shape your fix implies, would check it after the scope has been intersected in and 403 a user out of their own collection whenever a legitimate scope references a related column. A test guards exactly that, and the same finding was answered on the node PR.
The native-search case is genuinely new here, and you are right that PRD-900 never covered it. Ruby's refine_filter only implements the search itself when the child collection is not natively searchable; otherwise the string goes down to the datasource. So searched_fields also answers nil for anything calling enable_search — datasource_rpc/collection.rb:21 and datasource_zendesk/collections/base_collection.rb:27. Nothing else in the repo sets it: ActiveRecord and Mongoid leave searchable false, so the decorator enumerates and the guard works normally.
Raising is not available as a fix, though. searched_fields returns nil for every search on an RPC or Zendesk collection, extended or not. Failing closed there does not harden the feature, it removes it for those two datasources.
The option worth weighing is narrower: refuse only when the caller asked for an extended search and the layer cannot say. An extended search is an explicit request to traverse relations, so refusing an unverifiable one is proportionate, and a plain search keeps working. Its cost is a visible change to the documented replace_search exemption — a customer whose users tick extended search would start getting a 403.
That is a product decision rather than a commit, so it is PRD-1024 with the analysis, the two call sites, and the RPC caveat that the far-end agent enforces its own permissions only once it carries PRD-900 itself.
|
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
|
The guard read filters, sorts and searches on every route, but a count applies no sort and a chart neither sort nor search, so a sort naming a denied collection refused a request that field could never reach. Four routes refused something they drop. Each route now names what it consumes.
1 participant
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.
Polymorphism changes the rule, not just the code
Unlike agent-nodejs, whose toolkit has no polymorphic relation type, this one has
PolymorphicManyToOne,PolymorphicOneToOneandPolymorphicOneToMany. APolymorphicManyToOneSchemacarriesforeign_collections— plural — and aforeign_key_type_fieldas its discriminant.So the leaf collection is no longer unique.
holder:*resolves topersonsorcompaniesdepending on the row, and the path carries no discriminant.leaf_collection_namestherefore returns a list, and the rule becomes: every possible target must be readable, one denied is enough to deny the path. Same semantics as the front's.everyoverreferenceModels.Two details the validator imposes, and which make this reachable rather than theoretical: a
PolymorphicManyToOnecan only be projected as<relation>:*— any sub-field is rejected upstream — butProjectionFactory.allincludes that:*. The default expansion is what exposes a polymorphic relation, and it is now pruned when a target is denied.PolymorphicOneToOneandPolymorphicOneToManycarry a singleforeign_collectionand go through the ordinary branch.One resolver, in the toolkit
agent-nodejs duplicates this resolution —
FieldPathUtilsin the agent,getLeafCollectionNamein the customizer — and it was the customizer copy that failed open, which a review caught late. HereUtils::FieldPathlives in the toolkit, shared by the agent and the search decorator, so the two halves of the check cannot disagree by construction.Its default is fail-closed: a prefix naming no relation raises rather than falling back to the collection being resolved. The caller pins that collection to readable, so a fallback would turn "this path does not resolve" into "this path is allowed".
Search: asked of the layer, never derived from the schema
The route does not enumerate searchable fields. It asks the collection what a given search string and extended flag will actually reach, and the search decorator answers against
child_collection— a field hidden by publication or renaming above it is still searched.Three answers, and the third is what makes it safe. A list of paths gets checked; an empty list has nothing to check;
nilmeans the layer cannot say and must never be read as "reaches nothing". Ruby has one morenilcase than node: besides a customerreplace_search, a natively searchable datasource (RPC, Zendesk) receives the search by delegation, so no enumeration made here would be true.Deliberate differences from agent-nodejs
count_relatedneeds no guard. It builds its filter from the scope alone and reads nothing from the caller, unlike its node counterpart.storedoes need one. Ruby re-reads the created record with the full expansion and serializes it, where node'screateserializes only caller-supplied primary keys.addwithoutreadon a related collection disclosed it.can?refetches the whole environment on every denial, unconditionally — there is noinstantCacheRefreshgate here. Since this change makes denial the steady state rather than the exception,read_permissionsdoes one cached pass and forces at most one refetch for the whole request.Routes covered
list,show,csv,count,list_related,csv_related, the chart routes, and the writes that serialize a record back with a projection of ours:update,update_fieldandstore. Those three are redacted, never refused — a write must not 403 because the row it just wrote carries a relation the caller cannot read.A
Countleaderboard names no path back to the collection it counts, sobrowseis asserted on it directly, on the relation's foreign collection rather than the through-collection a ManyToMany aggregates.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, and no front change absorbs that path — chart requests are not pruned client-side. Same sweep, on
browserather thanread.Basic segments and saved views need auditing. The front prunes projections but not filters or sorts, so a segment whose condition targets a belongsTo sub-field of a denied collection makes its whole list view 403 — and a saved view sorted on such a column fails on load with no user action at all. See PRD-1014 for the front-side error surfacing.
Any client requesting relation fields its role may not read 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.
Deliberately out of scope
Both are open in agent-nodejs too, and porting route-for-route would reproduce them:
filtersandsearchunchecked. Guarding it is a breaking change with the same profile as the rest of this rollout, so it needs its own sequencing decision.filters, plussearch.Two linkage disclosures remain, consistent with the leaf rule chosen deliberately:
with_pksre-adds a key per surviving relation, and the polymorphic linkage columns (holder_type,holder_id) stay — but those are columns ofcards, readable independently of this change.Tests
Toolkit 478, customizer 697, agent 1111 examples, rubocop clean on all three.
The new suites drive a real
Permissionsservice rather than a double, so the guards themselves are under test and not the stubs the route specs install — including the four polymorphic cases, the fail-closed resolver, and both sides of the search guard.One trap worth knowing before touching these specs: a missing stub on the permissions double makes the suite hang rather than fail. RSpec renders its unexpected-message error by inspecting the arguments, and a collection reaches its datasource, which reaches every collection, so the inspect never finishes. The shared context documents it.
Note
Redact unreadable fields from resource, CSV, and chart routes
read_permissions), redacts projections (redact_projection), and asserts readability of fields used in filters, sorts, and search (assert_can_read_query_fields).CsvGenerator.filter_headerto match the kept columns.Store,Update,UpdateField) use the newredacted_full_projectionhelper so created/updated records are returned with only readable fields.countaggregations now require browse permission on the target foreign collection.Macroscope summarized 7873281.