From 84fb9e303e0b85ebde7ef7460e077670473074c8 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 28 Aug 2026 09:18:48 +0200 Subject: [PATCH 01/13] feat(datasource-customizer): let replace_search take a field selection so a narrowed search is checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes PRD-1078 `replace_search` took a block only. A block picks its own fields, so `searched_fields` answers nil and `collect_search_usages` returns without checking anything — a customized search is read entirely unchecked. It now also takes a field selection: collection.replace_search(include_fields: ['project:name'], exclude_fields: ['description']) A selection is a declarative list, so the footprint is computable. It is reported and checked per path, and a path the role may not read is refused by name. `extended` is deliberately not a key: that flag is the caller's and comes from the request. Keywords rather than a hash, so a misspelt key raises where a hash would have carried it silently; a block and a selection together, or neither, are refused rather than resolved by precedence. Both derivations are unified behind `searchable_fields`, which `refine_filter` and `searched_fields` now share — a path the search reads without appearing in the footprint is a column read unchecked, so the two cannot be allowed to drift. Four specs pin that invariant against the paths the generated condition tree actually reads, for include/exclude/only and for a numeric term that matches more columns than a word. Field names resolve strictly, through `Utils::Collection.get_field_schema`, raising when the customization is applied. Node drops an unresolvable name silently, which empties the searchable set and leaves the search matching nothing for good; that was raised in review there and left as pre-existing. Resolution also refuses a path crossing anything but a to-one relation, so a polymorphic one is reported rather than silently skipped. `enumerable_search?` now tracks the condition `refine_filter` tests rather than approximating it: the footprint is knowable whenever this layer builds the tree itself, which includes a field selection on a natively searchable collection — there the selection replaces the native search rather than narrowing it, and the customizer doc says so. Not in scope: ruby still serves an unknown footprint silently, including an extended search, where node refuses it. That parity gap would refuse requests that work today for every agent using `replace_search`, so it needs its own ticket rather than riding along here. Verified: forest_admin_datasource_customizer 721 examples / 0 failures, forest_admin_agent 1175 examples / 0 failures, rubocop clean on both. Co-Authored-By: Claude Opus 5 --- .../security/related_read_permissions_spec.rb | 43 ++++++ .../collection_customizer.rb | 41 +++++- .../search/search_collection_decorator.rb | 86 ++++++++++-- .../collection_customizer_spec.rb | 42 ++++++ .../decorators/search/searched_fields_spec.rb | 129 ++++++++++++++++++ 5 files changed, 324 insertions(+), 17 deletions(-) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 7c6b2f241..0a6fc1733 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -381,6 +381,49 @@ def searchable_cards(searched) expect { permissions.assert_can_read_query_fields(cards) }.not_to raise_error end + + # The examples above hand the guard a stubbed footprint to pin its policy. These drive a real + # search decorator instead, so what the decorator reports and what the guard does with it are + # checked together. + describe 'through a real replace_search field selection' do + def cards_searching(replacer) + decorator = ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.new( + cards, datasource + ) + decorator.replace_search(replacer) + + decorator + end + + it 'refuses an included relation path the caller cannot read' do + permissions = build_permissions([]) + collection = cards_searching({ include_fields: ['account:iban'] }) + + expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') } + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot search on 'account:iban': you are not allowed to read the 'accounts' collection." + ) + end + + it 'serves the same search once the collection the path reaches is readable' do + permissions = build_permissions(%w[accounts]) + collection = cards_searching({ include_fields: ['account:iban'] }) + + expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') }.not_to raise_error + end + + # The gap a field selection closes: a callable names no field, so nothing is checked and the + # same column is read for a role that cannot read the collection it belongs to. + it 'checks nothing of the same search once a callable picks the fields' do + permissions = build_permissions([]) + collection = cards_searching( + ->(value, _extended, _context) { { field: 'account:iban', operator: Operators::EQUAL, value: value } } + ) + + expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') }.not_to raise_error + end + end end describe '#read_permissions' do diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index 7cdab3d46..0c92a2efc 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -31,8 +31,45 @@ def disable_count push_customization { @stack.schema.get_collection(@name).override_schema(countable: false) } end - def replace_search(&definition) - push_customization { @stack.search.get_collection(@name).replace_search(definition) } + # Replace the behavior of the search bar, either with a block or with a field selection. + # + # A field selection narrows the same default search, so the agent knows which columns are read + # and checks them against the caller's read permissions: a path the role may not read is refused + # by name. Prefer it whenever it expresses what you need. On a collection whose datasource + # searches natively, it does not narrow that native search — it replaces it with the agent's own + # per-column one, restricted to the selection. + # + # A block is unrestricted, and pays for it: it picks its own fields, so the agent cannot tell + # which columns the search reads and checks none of them. + # + # +extended+ is deliberately not accepted: that flag is the caller's, and comes from the + # request. The block is handed the one the request carried. + # + # A field selection is stricter than the block it replaces, not looser. An included relation path + # is reported on a plain search too, so converting a block can start refusing a search for a role + # that cannot read the related collection. That is a migration to plan, not a mechanical rewrite. + # + # Example: + # collection.replace_search(include_fields: ['project:name'], exclude_fields: ['description']) + # collection.replace_search { |value, _extended, _context| { field: 'name', operator: Operators::CONTAINS, value: value } } + def replace_search(include_fields: nil, exclude_fields: nil, only_fields: nil, &definition) + selection = { + include_fields: include_fields, + exclude_fields: exclude_fields, + only_fields: only_fields + }.compact + + if definition && selection.any? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts either a block or a field selection, not both' + end + + if definition.nil? && selection.empty? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' + end + + push_customization { @stack.search.get_collection(@name).replace_search(definition || selection) } end # Disable the search bar diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 775d34567..c466549d2 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -19,8 +19,11 @@ def disable_search mark_schema_as_dirty end + # +replacer+ is either a callable, which picks its own fields, or a field selection naming + # the fields the default search reads. def replace_search(replacer) @replacer = replacer + assert_selection_resolves @disabled_search = false mark_schema_as_dirty end @@ -35,13 +38,14 @@ def refine_filter(caller, filter) # Implement search ourselves if @replacer || !@child_collection.schema[:searchable] - ctx = ForestAdminDatasourceCustomizer::Context::CollectionCustomizationContext.new(self, caller) - tree = default_replacer(filter.search, filter.search_extended) - - if @replacer - plain_tree = @replacer.call(filter.search, filter.search_extended, ctx) - tree = ConditionTreeFactory.from_plain_object(plain_tree) - end + tree = if handler + ctx = ForestAdminDatasourceCustomizer::Context::CollectionCustomizationContext.new(self, caller) + ConditionTreeFactory.from_plain_object( + handler.call(filter.search, filter.search_extended, ctx) + ) + else + search_condition_tree(filter.search, filter.search_extended) + end # Note that if no fields are searchable with the provided searchString, the conditions # array might be empty, which will create a condition returning zero records @@ -63,21 +67,44 @@ def refine_filter(caller, filter) # field the search cannot match — a number column for a word, a uuid column for anything # else — is left out rather than reported as reached. # - # +nil+ whenever this layer does not choose the fields — a replacer is installed, or the - # child collection searches natively — because then no enumeration made here is true. + # +nil+ whenever this layer does not choose the fields — a callable replacer is installed, or + # the child collection searches natively — because then no enumeration made here is true. A + # field selection is a declarative list, so it is answered rather than refused. def searched_fields(search, extended) return nil unless enumerable_search? return [] if insignificant_search?(search) - get_fields(extended).filter_map do |path, schema| + searchable_fields(extended).filter_map do |path, schema| searched_field(path) if build_condition(path, schema, search) end end private + def handler + @replacer.respond_to?(:call) ? @replacer : nil + end + + def field_selection + @replacer.respond_to?(:call) ? nil : @replacer + end + + # The footprint is knowable exactly when this layer builds the condition tree, which is the + # condition +refine_filter+ tests: a replacer is installed, or the child cannot search. Only + # a callable then picks fields no enumeration made here can name. def enumerable_search? - @replacer.nil? && !@child_collection.schema[:searchable] + handler.nil? && (!@replacer.nil? || !@child_collection.schema[:searchable]) + end + + # Resolved when the customization is applied rather than per request: a name that resolves + # to nothing would otherwise drop out of the searchable set unnoticed and leave the search + # matching nothing for good. + def assert_selection_resolves + selection = field_selection + + return if selection.nil? + + selected_paths(selection).each { |path| resolved_field(path) } end def insignificant_search?(search) @@ -93,16 +120,45 @@ def searched_field(path) } end - def default_replacer(search, extended) - searchable_fields = get_fields(extended) - - conditions = searchable_fields.map do |field, schema| + def search_condition_tree(search, extended) + conditions = searchable_fields(extended).map do |field, schema| build_condition(field, schema, search) end ConditionTreeFactory.union(conditions) end + # Both the condition tree +refine_filter+ builds and the footprint +searched_fields+ reports + # come from here: a path the search reads without appearing in the footprint is a column read + # unchecked. + # + # Keyed by path, so a field named twice — a selected one that is already a default — is + # searched and reported once. Defaults are merged first, which is what decides the order the + # footprint is reported in. + def searchable_fields(extended) + selection = field_selection || {} + only_fields = selection[:only_fields] + + defaults = only_fields ? {} : get_fields(extended).to_h + selected = (Array(only_fields) + Array(selection[:include_fields])).map { |path| resolved_field(path) } + + defaults.merge(selected.to_h).except(*Array(selection[:exclude_fields])) + end + + def selected_paths(selection) + Array(selection[:only_fields]) + + Array(selection[:include_fields]) + + Array(selection[:exclude_fields]) + end + + # Strict on purpose: this list is written by the developer, so a name that names nothing is a + # mistake to report, not a term to interpret. +get_field_schema+ names the field it could not + # resolve, and refuses a path crossing anything but a to-one relation — a polymorphic one + # included, which the search does not follow either. + def resolved_field(path) + [path, ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path)] + end + def build_condition(field, schema, search_string) column_type = schema.column_type enum_values = schema.enum_values diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb index b236e143c..3d1901f4d 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb @@ -200,6 +200,48 @@ module ForestAdminDatasourceCustomizer expect(search_collection.instance_variable_get(:@replacer)).to eq(condition) end + + it 'hands the search decorator the field selection it was given' do + stack = @datasource_customizer.stack + stack.apply_queued_customizations({}) + + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + customizer.replace_search(include_fields: ['title'], exclude_fields: ['id']) + stack.apply_queued_customizations({}) + + search_collection = @datasource_customizer.stack.search.get_collection('book') + + expect(search_collection.instance_variable_get(:@replacer)) + .to eq({ include_fields: ['title'], exclude_fields: ['id'] }) + end + + # `extended` is the caller's, and comes from the request: a customization pinning it would + # decide for every caller. + it 'refuses a key that is not one of the three field lists' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(extended: true) }.to raise_error(ArgumentError, /extended/) + end + + it 'refuses a block and a field selection at once, which would leave the winner implicit' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: ['title']) { |value| value } } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts either a block or a field selection, not both' + ) + end + + it 'refuses neither, which would replace the search with nothing' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' + ) + end end context 'when using disable_search' do diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index 24142426e..dcde383be 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -82,6 +82,135 @@ module Search expect(described_class.new(child, datasource).searched_fields('martin', true)).to be_nil end end + + describe '#searched_fields when a field selection narrows the default search' do + it 'answers the footprint instead of refusing to tell' do + decorated.replace_search(exclude_fields: ['pan_last4']) + + expect(decorated.searched_fields('martin', true)).to contain_exactly( + { path: 'holder:national_id', collections: ['holders'] } + ) + end + + it 'reports an included relation path on a plain search too, not only an extended one' do + decorated.replace_search(include_fields: ['holder:national_id']) + + expect(decorated.searched_fields('martin', false)).to contain_exactly( + { path: 'pan_last4', collections: ['cards'] }, + { path: 'holder:national_id', collections: ['holders'] } + ) + end + + it 'reports only the replaced set once only_fields is given' do + decorated.replace_search(only_fields: ['holder:national_id']) + + expect(decorated.searched_fields('martin', true)).to eq( + [{ path: 'holder:national_id', collections: ['holders'] }] + ) + end + + it 'reports an included path once, where it overlaps a default field' do + decorated.replace_search(include_fields: ['pan_last4']) + + expect(decorated.searched_fields('martin', false).map { |field| field[:path] }) + .to eq(['pan_last4']) + end + + it 'answers the footprint even when the datasource searches natively, which it replaces' do + child = datasource.get_collection('cards') + allow(child).to receive(:schema).and_return(child.schema.merge(searchable: true)) + decorator = described_class.new(child, datasource) + decorator.replace_search(only_fields: ['pan_last4']) + + expect(decorator.searched_fields('martin', true)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + + it 'still answers nothing it can be sure of when a callable chooses the fields' do + decorated.replace_search(->(search, _extended, _context) { { field: 'id', operator: 'equal', value: search } }) + + expect(decorated.searched_fields('martin', true)).to be_nil + end + end + + # The invariant the permission check rests on: a path the search reads without appearing in + # the footprint is a column read unchecked. + describe '#searched_fields against what the search actually reads' do + let(:caller) { instance_double(ForestAdminDatasourceToolkit::Components::Caller) } + + def paths_read(selection, search: 'martin', extended: true) + decorated.replace_search(**selection) + refined = decorated.refine_filter( + caller, + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + search: search, search_extended: extended + ) + ) + + refined.condition_tree.projection.uniq + end + + it 'covers every path an included relation path makes the search read' do + read = paths_read({ include_fields: ['holder:national_id'] }) + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + + expect(read).to include('holder:national_id') + expect(footprint).to include(*read) + end + + it 'covers every path the search reads once exclude_fields dropped one' do + read = paths_read({ exclude_fields: ['pan_last4'] }) + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + + expect(read).not_to include('pan_last4') + expect(read).to include('holder:national_id') + expect(footprint).to include(*read) + end + + it 'covers every path the search reads once only_fields replaced the set' do + read = paths_read({ only_fields: ['holder:national_id'] }) + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + + expect(read).to eq(['holder:national_id']) + expect(footprint).to include(*read) + end + + it 'reads nothing outside the footprint for a numeric term, which matches more columns' do + read = paths_read({ include_fields: ['holder:national_id'] }, search: '42') + footprint = decorated.searched_fields('42', true).map { |field| field[:path] } + + expect(read).to include('id', 'holder:id') + expect(footprint).to include(*read) + end + end + + describe '#replace_search with an unresolvable field selection' do + it 'names the field it cannot resolve rather than searching nothing for good' do + expect { decorated.replace_search(only_fields: ['pan_last_four']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + /Column not found cards.pan_last_four/ + ) + end + + # The list is written by the developer, so a name that names nothing is a mistake to + # report rather than one to guess at. + it 'refuses a name spelt in another case rather than resolving it fuzzily' do + expect { decorated.replace_search(include_fields: ['panLast4']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /panLast4/) + end + + it 'checks the excluded names too, which would otherwise exclude nothing' do + expect { decorated.replace_search(exclude_fields: ['nope']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) + end + + it 'refuses a path reaching through a relation the search cannot follow' do + expect { decorated.replace_search(include_fields: ['holder:nope']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) + end + end end end end From ac5b2c95556500c2132fb8afdbef67acebe7ddab Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 28 Aug 2026 09:26:40 +0200 Subject: [PATCH 02/13] fix(agent): refuse an extended search the stack cannot describe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes PRD-1078 `collect_search_usages` returned as soon as `searched_fields` answered nil, so a `replace_search` block's search was read entirely unchecked — the extended half included. node refuses that case (agent-nodejs#1840); ruby served it. An unknown footprint stays served on a plain search: the caller supplied only the text and aimed at nothing, the same category as a scope, and refusing would remove search from every customized collection. The extended flag is the caller's own, though — running one term with it off and on isolates exactly the rows matched through a relation, a bit per term on collections no check covered — so the exemption stops there. A collection with no `searched_fields` at all is read the same way: silence is not an empty footprint either. This refuses requests that are served today: an agent whose collection installs a `replace_search` block now answers 403 on an extended search there. Converting that block to the field selection the previous commit added restores it, checked per path. Shipped as a fix rather than a breaking change, matching how node shipped the same policy. Three specs pin the refusal — the extended half of a search served plain, a collection that cannot answer at all, and a real `replace_search` block against the field selection that is checked instead — and all three fail when the guard is disabled. Verified as CI runs it: root rubocop 840 files / no offenses; `BUNDLE_GEMFILE=Gemfile-test rspec` on forest_admin_agent 1179 examples, forest_admin_datasource_customizer 721, forest_admin_datasource_rpc 168, forest_admin_datasource_toolkit 478 — 0 failures. Co-Authored-By: Claude Opus 5 --- .../services/permissions.rb | 26 +++++++- .../security/related_read_permissions_spec.rb | 61 +++++++++++++++++-- 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index a6d8d4fdb..c23074c58 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -330,11 +330,15 @@ def usage(action, collection, path) end def collect_search_usages(collection, search, search_extended, usages) - return if search.nil? || !collection.respond_to?(:searched_fields) + return if search.nil? - searched = collection.searched_fields(search, search_extended) + searched = collection.searched_fields(search, search_extended) if collection.respond_to?(:searched_fields) - return if searched.nil? + if searched.nil? + assert_extended_search_checkable(collection, search_extended) + + return + end published = collection.datasource.collections @@ -344,6 +348,22 @@ def collect_search_usages(collection, search, search_extended, usages) end end + # An unknown footprint is a `replace_search` block choosing its own fields. On a plain search + # the caller aimed at nothing, so it is served — the same category as a scope, and refusing + # would remove search from every customized collection. The extended flag is the caller's own, + # though: running one term both ways isolates the rows matched through a relation, a bit per + # term on collections no check covered. The exemption stops there. + # + # A collection that cannot answer at all is read the same way: silence is not an empty + # footprint either. + def assert_extended_search_checkable(collection, search_extended) + return unless search_extended + + raise ForbiddenError, + "You cannot run an extended search on the '#{collection.name}' collection: the fields " \ + 'it reaches cannot be determined, so they cannot be checked against your permissions.' + end + # `searched_fields` answers below the publication layer — deliberately, so a field hidden by # renaming above it is still checked — so it can name a collection `remove_collection` took out # of the API. An extended search does reach through to it: the condition is built below diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 0a6fc1733..c7ff2358b 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -353,7 +353,7 @@ def searchable_cards(searched) ) end - # A replaced search: the handler picks the fields, the caller only supplies the text. + # A replaced search: the block picks the fields, the caller only supplies the text. it 'serves the request when the stack cannot say what a search reaches' do permissions = build_permissions([]) @@ -361,12 +361,41 @@ def searchable_cards(searched) .not_to raise_error end + # The flag is the caller's: the same term with it off and on differs by exactly the rows + # matched through a relation, so an unverifiable traversal is refused where a plain search is + # served. + it 'refuses the extended half of that same search' do + permissions = build_permissions([]) + + expect do + permissions.assert_can_read_query_fields( + searchable_cards(nil), search: 'martin', search_extended: true + ) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot run an extended search on the 'cards' collection: the fields it reaches " \ + 'cannot be determined, so they cannot be checked against your permissions.' + ) + end + it 'checks nothing on a collection that cannot answer what a search reaches' do permissions = build_permissions([]) expect { permissions.assert_can_read_query_fields(cards, search: 'martin') }.not_to raise_error end + # Silence is not an empty footprint either: a collection with no `searched_fields` at all + # says as little as one answering nil. + it 'reads a collection that cannot answer at all as an unknown footprint too' do + permissions = build_permissions([]) + + expect { permissions.assert_can_read_query_fields(cards, search: 'martin', search_extended: true) } + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + /You cannot run an extended search on the 'cards' collection/ + ) + end + it 'accepts a filter once the collection it reaches is readable' do permissions = build_permissions(%w[accounts]) @@ -413,9 +442,10 @@ def cards_searching(replacer) expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') }.not_to raise_error end - # The gap a field selection closes: a callable names no field, so nothing is checked and the - # same column is read for a role that cannot read the collection it belongs to. - it 'checks nothing of the same search once a callable picks the fields' do + # What a field selection buys over a callable: the callable names no field, so a plain search + # reads the same column unchecked for a role that cannot read the collection it belongs to, + # and its extended half is refused outright rather than checked. + it 'serves a plain search a callable describes, which names no field to check' do permissions = build_permissions([]) collection = cards_searching( ->(value, _extended, _context) { { field: 'account:iban', operator: Operators::EQUAL, value: value } } @@ -423,6 +453,29 @@ def cards_searching(replacer) expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') }.not_to raise_error end + + it 'refuses the extended search of that callable, where the selection is checked instead' do + permissions = build_permissions(%w[accounts]) + collection = cards_searching( + ->(value, _extended, _context) { { field: 'account:iban', operator: Operators::EQUAL, value: value } } + ) + + expect do + permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + /You cannot run an extended search on the 'cards' collection/ + ) + end + + it 'serves the extended search of the equivalent field selection' do + permissions = build_permissions(%w[accounts]) + collection = cards_searching({ include_fields: ['account:iban'] }) + + expect do + permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) + end.not_to raise_error + end end end From b16d759dd1c34d212bd82033f91fa3e2488525bb Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 28 Aug 2026 09:36:18 +0200 Subject: [PATCH 03/13] fix(datasource-customizer): match nothing when a search has no searchable field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the two commits above found this, and it is the sharp one. `search_condition_tree` handed `ConditionTreeFactory.union` an empty list whenever no field was searchable. `union` answers nil for an empty list, and `intersect` drops a nil branch — so the filter came back carrying neither a condition nor a search, and the request answered *every* row for a term that matched nothing. A narrowing customization that resolved to an empty set widened the result to everything. Reachable straight from the feature: `only_fields: []`, or `exclude_fields` covering every searchable column. Also reachable without any customization, on a collection whose columns are all unsearchable — two existing specs pinned that, and both said so in their own titles while asserting the opposite: it 'adds a condition to not return record if it is the only one filter' expect(refined_filter).to have_attributes(condition_tree: nil) `nil` is not "return no record", it is "no restriction". Those two specs now assert `match_none`, which is what their names always claimed and what node returns here. The enum-value-not-found case a few lines above already answered `match_none`, so this also removes an inconsistency between two identical situations. Note this changes what a search returns on collections that have no searchable column at all, with no `replace_search` involved. Also from the same review: - a symbol field name raised `NoMethodError: undefined method 'include?' for an instance of Symbol` from inside the toolkit, defeating the point of resolving strictly to get a clear error. Field paths are normalised to strings, so `only_fields: [:pan_last4]` resolves like the string does. - `refine_filter` returns early instead of nesting, and the predicate it tests is now the `implements_search?` that `enumerable_search?` reuses, so the footprint and the tree cannot drift on a condition spelt twice. Comments that restated the code are gone with it: the ones `implements_search?` and `match_none` now say in code, and the public doc trimmed to what a caller of `replace_search` has to know. Verified as CI runs it: root rubocop 840 files / no offenses; Gemfile-test rspec on forest_admin_datasource_customizer 724, forest_admin_agent 1179, forest_admin_datasource_toolkit 478, forest_admin_datasource_rpc 168 — 0 failures. Co-Authored-By: Claude Opus 5 --- .../collection_customizer.rb | 21 ++--- .../search/search_collection_decorator.rb | 83 +++++++++---------- .../search_collection_decorator_spec.rb | 13 ++- .../decorators/search/searched_fields_spec.rb | 32 +++++++ 4 files changed, 87 insertions(+), 62 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index 0c92a2efc..53648f7a8 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -33,21 +33,18 @@ def disable_count # Replace the behavior of the search bar, either with a block or with a field selection. # - # A field selection narrows the same default search, so the agent knows which columns are read - # and checks them against the caller's read permissions: a path the role may not read is refused - # by name. Prefer it whenever it expresses what you need. On a collection whose datasource - # searches natively, it does not narrow that native search — it replaces it with the agent's own - # per-column one, restricted to the selection. + # A field selection is checked against the caller's read permissions, because the agent can tell + # which columns it reads: a path the role may not read is refused by name. A block cannot be, so + # a plain search on it is served unchecked and an extended one is refused outright. # - # A block is unrestricted, and pays for it: it picks its own fields, so the agent cannot tell - # which columns the search reads and checks none of them. + # Converting a block to a selection is stricter, not looser: an included relation path is checked + # on a plain search too, so a role that cannot read that collection starts being refused. # - # +extended+ is deliberately not accepted: that flag is the caller's, and comes from the - # request. The block is handed the one the request carried. + # +extended+ is not accepted, so that a customization cannot pin a flag the caller owns; the + # block is handed the one the request carried. # - # A field selection is stricter than the block it replaces, not looser. An included relation path - # is reported on a plain search too, so converting a block can start refusing a search for a role - # that cannot read the related collection. That is a migration to plan, not a mechanical rewrite. + # On a natively searchable datasource, a selection replaces that search with the agent's own + # per-column one rather than narrowing it. # # Example: # collection.replace_search(include_fields: ['project:name'], exclude_fields: ['description']) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index c466549d2..38f02e5b5 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -19,8 +19,6 @@ def disable_search mark_schema_as_dirty end - # +replacer+ is either a callable, which picks its own fields, or a field selection naming - # the fields the default search reads. def replace_search(replacer) @replacer = replacer assert_selection_resolves @@ -36,28 +34,20 @@ def refine_filter(caller, filter) # Search string is not significant return filter.override({ search: nil }) if !filter || !filter.search || filter.search.strip&.empty? - # Implement search ourselves - if @replacer || !@child_collection.schema[:searchable] - tree = if handler - ctx = ForestAdminDatasourceCustomizer::Context::CollectionCustomizationContext.new(self, caller) - ConditionTreeFactory.from_plain_object( - handler.call(filter.search, filter.search_extended, ctx) - ) - else - search_condition_tree(filter.search, filter.search_extended) - end - - # Note that if no fields are searchable with the provided searchString, the conditions - # array might be empty, which will create a condition returning zero records - # (this is the desired behavior). - return filter.override({ - condition_tree: ConditionTreeFactory.intersect([filter.condition_tree, tree]), - search: nil - }) - end - # Let sub-collection deal with the search - filter + return filter unless implements_search? + + tree = if handler + ctx = ForestAdminDatasourceCustomizer::Context::CollectionCustomizationContext.new(self, caller) + ConditionTreeFactory.from_plain_object(handler.call(filter.search, filter.search_extended, ctx)) + else + search_condition_tree(filter.search, filter.search_extended) + end + + filter.override({ + condition_tree: ConditionTreeFactory.intersect([filter.condition_tree, tree]), + search: nil + }) end # Answers against +@child_collection+, which is what the search actually reads: a field @@ -68,8 +58,7 @@ def refine_filter(caller, filter) # else — is left out rather than reported as reached. # # +nil+ whenever this layer does not choose the fields — a callable replacer is installed, or - # the child collection searches natively — because then no enumeration made here is true. A - # field selection is a declarative list, so it is answered rather than refused. + # the child collection searches natively — because then no enumeration made here is true. def searched_fields(search, extended) return nil unless enumerable_search? return [] if insignificant_search?(search) @@ -89,16 +78,16 @@ def field_selection @replacer.respond_to?(:call) ? nil : @replacer end - # The footprint is knowable exactly when this layer builds the condition tree, which is the - # condition +refine_filter+ tests: a replacer is installed, or the child cannot search. Only - # a callable then picks fields no enumeration made here can name. + def implements_search? + !@replacer.nil? || !@child_collection.schema[:searchable] + end + def enumerable_search? - handler.nil? && (!@replacer.nil? || !@child_collection.schema[:searchable]) + handler.nil? && implements_search? end - # Resolved when the customization is applied rather than per request: a name that resolves - # to nothing would otherwise drop out of the searchable set unnoticed and leave the search - # matching nothing for good. + # Resolved now rather than per request, so a name that resolves to nothing is reported here + # instead of leaving the search matching nothing. def assert_selection_resolves selection = field_selection @@ -121,40 +110,42 @@ def searched_field(path) end def search_condition_tree(search, extended) - conditions = searchable_fields(extended).map do |field, schema| + conditions = searchable_fields(extended).filter_map do |field, schema| build_condition(field, schema, search) end + return ConditionTreeFactory.match_none if conditions.empty? + ConditionTreeFactory.union(conditions) end # Both the condition tree +refine_filter+ builds and the footprint +searched_fields+ reports # come from here: a path the search reads without appearing in the footprint is a column read # unchecked. - # - # Keyed by path, so a field named twice — a selected one that is already a default — is - # searched and reported once. Defaults are merged first, which is what decides the order the - # footprint is reported in. def searchable_fields(extended) selection = field_selection || {} only_fields = selection[:only_fields] defaults = only_fields ? {} : get_fields(extended).to_h - selected = (Array(only_fields) + Array(selection[:include_fields])).map { |path| resolved_field(path) } + selected = field_paths(only_fields) + field_paths(selection[:include_fields]) - defaults.merge(selected.to_h).except(*Array(selection[:exclude_fields])) + defaults + .merge(selected.to_h { |path| resolved_field(path) }) + .except(*field_paths(selection[:exclude_fields])) end def selected_paths(selection) - Array(selection[:only_fields]) + - Array(selection[:include_fields]) + - Array(selection[:exclude_fields]) + field_paths(selection[:only_fields]) + + field_paths(selection[:include_fields]) + + field_paths(selection[:exclude_fields]) + end + + def field_paths(names) + Array(names).map(&:to_s) end - # Strict on purpose: this list is written by the developer, so a name that names nothing is a - # mistake to report, not a term to interpret. +get_field_schema+ names the field it could not - # resolve, and refuses a path crossing anything but a to-one relation — a polymorphic one - # included, which the search does not follow either. + # Strict where an end-user term would be interpreted: this list is written by the developer, + # so a name that names nothing is a mistake to report. def resolved_field(path) [path, ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path)] end diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb index 46fb7afcd..b98f5766d 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb @@ -219,13 +219,18 @@ module Search end end - context 'when the given field is a column' do - it 'adds a condition to return records matching the search value' do + context 'when the collection carries no searchable column' do + # Used to answer an empty filter, which `intersect` reads as no restriction at all: every + # row came back for a term that matched nothing. + it 'matches nothing rather than every record' do filter = Filter.new(search: 'a search value') search_collection_decorator = described_class.new(@collection_user, datasource) refined_filter = search_collection_decorator.refine_filter(caller, filter) - expect(refined_filter.to_h).to eq(Filter.new.to_h) + expect(refined_filter).to have_attributes( + search: nil, + condition_tree: have_attributes(aggregator: 'Or', conditions: []) + ) end end @@ -600,7 +605,7 @@ module Search refined_filter = search_collection_decorator.refine_filter(caller, filter) expect(refined_filter).to have_attributes( search: nil, - condition_tree: nil + condition_tree: have_attributes(aggregator: 'Or', conditions: []) ) end end diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index dcde383be..9094ff6cb 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -185,6 +185,30 @@ def paths_read(selection, search: 'martin', extended: true) end end + describe '#refine_filter when the selection leaves no field searchable' do + let(:caller) { instance_double(ForestAdminDatasourceToolkit::Components::Caller) } + + def refined(selection) + decorated.replace_search(selection) + decorated.refine_filter( + caller, + ForestAdminDatasourceToolkit::Components::Query::Filter.new(search: 'martin') + ) + end + + # An empty union is nil, and `intersect` drops a nil: the search would carry no condition + # at all and the request would answer every row instead of none. + it 'matches nothing rather than everything once only_fields is empty' do + expect(refined({ only_fields: [] }).condition_tree) + .to have_attributes(aggregator: 'Or', conditions: []) + end + + it 'matches nothing rather than everything once every searchable field is excluded' do + expect(refined({ exclude_fields: %w[id pan_last4 holder_id holder:id holder:national_id] }).condition_tree) + .to have_attributes(aggregator: 'Or', conditions: []) + end + end + describe '#replace_search with an unresolvable field selection' do it 'names the field it cannot resolve rather than searching nothing for good' do expect { decorated.replace_search(only_fields: ['pan_last_four']) } @@ -201,6 +225,14 @@ def paths_read(selection, search: 'martin', extended: true) .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /panLast4/) end + it 'accepts a symbol, which resolves to the same column as the string' do + decorated.replace_search(only_fields: [:pan_last4]) + + expect(decorated.searched_fields('martin', false)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + it 'checks the excluded names too, which would otherwise exclude nothing' do expect { decorated.replace_search(exclude_fields: ['nope']) } .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) From 5aa6bcc4de435050e78da3ba30937506cac83923 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 28 Aug 2026 09:51:13 +0200 Subject: [PATCH 04/13] fix(datasource-customizer): refuse a bare relation in a field selection, and stop 403ing a blank search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by Macroscope on #382, both confirmed by probing rather than reading. `only_fields: ['holder']` passed validation and then raised `NoMethodError: undefined method 'column_type' for ManyToOneSchema`, from `searched_fields` and from `refine_filter` alike. `get_field_schema` only requires a to-one relation of the segments it *crosses*; it happily returns the relation itself when it is the last one, so the assumption that resolution guarantees a column was wrong. `resolved_field` now rejects a non-column leaf and names the type it found, at customization time like every other bad name. `collect_search_usages` refused `?search=&searchExtended=1`: a blank search is not nil, so it reached the footprint check and 403'd on a collection whose footprint is unknown — a request that runs no search at all. `refine_filter` discards a blank search rather than running it, so there is nothing to authorize; the guard now matches, whitespace included. node skips the empty string here because `''` is falsy in JS, but not `' '`, so this is slightly tighter than node rather than a port of it. Both guards are mutation-checked: each fails exactly its own spec when disabled. Left as is: qlty flags `replace_search` for having four parameters. The three keywords are the point — they are what makes a misspelt key raise, which a hash or `**options` would swallow, and a spec pins that. Four is the count of a deliberate signature, not of a smell. Verified as CI runs it: root rubocop 840 files / no offenses; Gemfile-test rspec on forest_admin_agent 1180, forest_admin_datasource_customizer 725, forest_admin_datasource_toolkit 478 — 0 failures. Co-Authored-By: Claude Opus 5 --- .../lib/forest_admin_agent/services/permissions.rb | 3 ++- .../security/related_read_permissions_spec.rb | 14 ++++++++++++++ .../search/search_collection_decorator.rb | 12 ++++++++++-- .../decorators/search/searched_fields_spec.rb | 11 +++++++++++ 4 files changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index c23074c58..0c8d16dab 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -330,7 +330,8 @@ def usage(action, collection, path) end def collect_search_usages(collection, search, search_extended, usages) - return if search.nil? + # The stack discards a blank search instead of running it, so there is nothing to authorize. + return if search.nil? || search.strip.empty? searched = collection.searched_fields(search, search_extended) if collection.respond_to?(:searched_fields) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index c7ff2358b..15a5a7964 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -378,6 +378,20 @@ def searchable_cards(searched) ) end + # `refine_filter` discards a blank search instead of running it, so refusing one would 403 a + # request that searches nothing. + it 'serves a blank search with the extended flag on, which runs no search at all' do + permissions = build_permissions([]) + + ['', ' '].each do |blank| + expect do + permissions.assert_can_read_query_fields( + searchable_cards(nil), search: blank, search_extended: true + ) + end.not_to raise_error + end + end + it 'checks nothing on a collection that cannot answer what a search reaches' do permissions = build_permissions([]) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 38f02e5b5..8685acb7a 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -4,6 +4,7 @@ module Search class SearchCollectionDecorator < ForestAdminDatasourceToolkit::Decorators::CollectionDecorator include ForestAdminDatasourceToolkit::Schema include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + include ForestAdminDatasourceToolkit::Exceptions POLYMORPHIC_TYPES = %w[PolymorphicManyToOne PolymorphicOneToOne].freeze TO_ONE_RELATIONS = %w[ManyToOne OneToOne].freeze @@ -145,9 +146,16 @@ def field_paths(names) end # Strict where an end-user term would be interpreted: this list is written by the developer, - # so a name that names nothing is a mistake to report. + # so a name that names nothing, or names a relation the search cannot compare a term to, is a + # mistake to report. def resolved_field(path) - [path, ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path)] + schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) + + unless schema.type == 'Column' + raise ForestException, "Cannot search on '#{path}': a #{schema.type} is not a column" + end + + [path, schema] end def build_condition(field, schema, search_string) diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index 9094ff6cb..ec217f291 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -242,6 +242,17 @@ def refined(selection) expect { decorated.replace_search(include_fields: ['holder:nope']) } .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) end + + # `get_field_schema` only requires a to-one relation of the segments it crosses, so a bare + # relation resolves to a RelationSchema and reached `build_condition`, which asks it for a + # `column_type` it does not have. + it 'refuses a bare relation, which carries no term to compare' do + expect { decorated.replace_search(only_fields: ['holder']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot search on 'holder': a ManyToOne is not a column" + ) + end end end end From 04b7ae380e1ceb287073158fdd2f4b6ab7dbb9d7 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 28 Aug 2026 09:55:04 +0200 Subject: [PATCH 05/13] test(datasource-customizer): pin the polymorphic behaviour of a search field selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asked whether the polymorphic case was validated, it was not: the claim that resolution refuses a polymorphic path rested on the same reading of `get_field_schema` that the bare-relation bug had just proved incomplete. Probed rather than re-read, and the behaviour does hold — but nothing pinned it, and one of the four cases only works because of the previous commit: - a path crossing a polymorphic relation raises `Unexpected field type PolymorphicManyToOne`, in include and in exclude alike - naming one bare raises `a PolymorphicManyToOne is not a column`. Before the previous commit this crashed with `NoMethodError` on `column_type`, exactly like the ManyToOne case that was reported - an extended search keeps its targets out of both the footprint and the condition tree, so no footprint entry ever carries the several collections `leaf_collection_names` answers for a polymorphic leaf — which is what lets the permission check stay a single-collection question Also fixes a trap in the specs added earlier: `caller` with no `let` in scope is `Kernel#caller`, so those examples were passing a backtrace to `refine_filter`. Unused on that path, so they passed while testing less than they read as doing. The `let` is hoisted to the top-level describe and the two duplicates dropped. Verified as CI runs it: root rubocop 840 files / no offenses; Gemfile-test rspec on forest_admin_datasource_customizer 729, forest_admin_agent 1180 — 0 failures. Co-Authored-By: Claude Opus 5 --- .../decorators/search/searched_fields_spec.rb | 92 ++++++++++++++++++- 1 file changed, 88 insertions(+), 4 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index ec217f291..ef02ca79d 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -9,6 +9,8 @@ module Search describe SearchCollectionDecorator do subject(:decorated) { described_class.new(datasource.get_collection('cards'), datasource) } + let(:caller) { instance_double(ForestAdminDatasourceToolkit::Components::Caller) } + let(:datasource) do build_datasource_with_collections( [ @@ -137,8 +139,6 @@ module Search # The invariant the permission check rests on: a path the search reads without appearing in # the footprint is a column read unchecked. describe '#searched_fields against what the search actually reads' do - let(:caller) { instance_double(ForestAdminDatasourceToolkit::Components::Caller) } - def paths_read(selection, search: 'martin', extended: true) decorated.replace_search(**selection) refined = decorated.refine_filter( @@ -185,9 +185,93 @@ def paths_read(selection, search: 'martin', extended: true) end end - describe '#refine_filter when the selection leaves no field searchable' do - let(:caller) { instance_double(ForestAdminDatasourceToolkit::Components::Caller) } + describe 'when the collection carries a polymorphic relation' do + let(:datasource) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'pan_last4' => build_column(column_type: 'String', filter_operators: [Operators::I_CONTAINS]), + 'holder_id' => build_column(column_type: 'Number'), + 'holder_type' => build_column(column_type: 'String'), + 'holder' => Relations::PolymorphicManyToOneSchema.new( + foreign_key: 'holder_id', + foreign_key_type_field: 'holder_type', + foreign_collections: %w[persons companies], + foreign_key_targets: { 'persons' => 'id', 'companies' => 'id' } + ) + } + } + ), + build_collection( + name: 'persons', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'national_id' => build_column(column_type: 'String', filter_operators: [Operators::I_CONTAINS]) + } + } + ), + build_collection( + name: 'companies', + schema: { fields: { 'id' => build_numeric_primary_key } } + ) + ] + ) + end + + before do + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return( + instance_double(ForestAdminAgent::Services::LoggerService, log: nil) + ) + end + it 'refuses a path crossing it, which the search cannot follow' do + expect { decorated.replace_search(include_fields: ['holder:national_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'Unexpected field type PolymorphicManyToOne: cards.holder' + ) + end + + it 'refuses it named bare, which carries no term to compare' do + expect { decorated.replace_search(only_fields: ['holder']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot search on 'holder': a PolymorphicManyToOne is not a column" + ) + end + + it 'refuses excluding a path crossing it, rather than excluding nothing' do + expect { decorated.replace_search(exclude_fields: ['holder:national_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'Unexpected field type PolymorphicManyToOne: cards.holder' + ) + end + + # Its targets stay out of both, so no footprint entry ever needs the several collections + # `leaf_collection_names` would answer for a polymorphic leaf. + it 'keeps its columns out of the footprint and out of what the search reads' do + decorated.replace_search(include_fields: ['pan_last4']) + + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + refined = decorated.refine_filter( + caller, + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + search: 'martin', search_extended: true + ) + ) + + expect(footprint).to eq(['pan_last4']) + expect(refined.condition_tree.projection.uniq).to eq(['pan_last4']) + end + end + + describe '#refine_filter when the selection leaves no field searchable' do def refined(selection) decorated.replace_search(selection) decorated.refine_filter( From 540ea0b212a7bd0d219bf2c01db6298b3db8caa9 Mon Sep 17 00:00:00 2001 From: Matt Date: Mon, 31 Aug 2026 18:09:19 +0200 Subject: [PATCH 06/13] fix(agent): narrow the extended-search refusal to a block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `searched_fields` answers nil for three unrelated reasons, and only one is a footprint this stack could have described: a `replace_search` block chose the fields. Refusing the other two took extended search off every natively searchable collection — Zendesk declares one per collection, RPC mirrors whatever the remote agent declares — for a check the datasource never let us make anyway. Gated on `permission_system?` as well: `can?` allows everything without one, so the refusal was the single denial no grant could lift. `search_handler?` is delegated on `CollectionDecorator` the way `searched_fields` is. Without it the predicate never reaches the search decorator from the top of the stack and the refusal fires for nobody. The new spec drives a booted `DatasourceCustomizer` rather than a decorator built by hand, and it is the only one of the 42 that catches that delegation going missing. `searchExtended` read `false`, `'false'`, `'FALSE'`, `''` and `0` as extended, and `||` discarded a real `false` in the select-all subset query for whatever the query string carried. It only widened a search before; it now gates a refusal. An empty field list, or `only_fields` alongside another list, installed a search matching nothing behind a bar the schema still advertises. A selected path is held to the same `searchable_field?` bar the defaults pass, at boot only: the request-time check rested on a schema mutation the RPC refresh cannot produce, and turned drift into a 500 that leaked the developer message to the client. The unchecked native search on RPC-backed collections is pre-existing and left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/permissions.rb | 19 +++-- .../utils/query_string_parser.rb | 16 ++-- .../routes/resources/count_spec.rb | 2 +- .../routes/resources/csv_spec.rb | 2 +- .../routes/resources/list_spec.rb | 9 ++- .../security/related_read_permissions_spec.rb | 79 +++++++++++++++---- .../utils/query_string_parser_spec.rb | 31 ++++++++ .../forest_admin_agent/spec/spec_helper.rb | 8 +- .../collection_customizer.rb | 31 +++++--- .../search/search_collection_decorator.rb | 39 +++++---- .../collection_customizer_spec.rb | 51 ++++++++++++ .../decorators/search/searched_fields_spec.rb | 26 ++++++ .../decorators/collection_decorator.rb | 6 ++ 13 files changed, 260 insertions(+), 59 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index 0c8d16dab..ee94fdb15 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -349,22 +349,25 @@ def collect_search_usages(collection, search, search_extended, usages) end end - # An unknown footprint is a `replace_search` block choosing its own fields. On a plain search - # the caller aimed at nothing, so it is served — the same category as a scope, and refusing - # would remove search from every customized collection. The extended flag is the caller's own, - # though: running one term both ways isolates the rows matched through a relation, a bit per - # term on collections no check covered. The exemption stops there. - # - # A collection that cannot answer at all is read the same way: silence is not an empty - # footprint either. + # Only a callable `replace_search` is a footprint this stack could have described and did not; a + # native child search, or no search decorator at all, builds no condition here. A plain search is + # served in either case — the extended flag is the caller's own, so the exemption stops there. def assert_extended_search_checkable(collection, search_extended) return unless search_extended + return unless permission_system? + return unless describes_own_search?(collection) raise ForbiddenError, "You cannot run an extended search on the '#{collection.name}' collection: the fields " \ 'it reaches cannot be determined, so they cannot be checked against your permissions.' end + # Reaches the search decorator only through `CollectionDecorator#search_handler?`: drop that + # delegation and this answers false for every collection. + def describes_own_search?(collection) + collection.respond_to?(:search_handler?) && collection.search_handler? + end + # `searched_fields` answers below the publication layer — deliberately, so a field hidden by # renaming above it is still checked — so it can name a collection `remove_collection` took out # of the API. An extended search does reach through to it: the condition is built below diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index 07301f4b0..b9bb089c1 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -10,6 +10,7 @@ class QueryStringParser DEFAULT_ITEMS_PER_PAGE = '15'.freeze DEFAULT_PAGE_TO_SKIP = '1'.freeze POLYMORPHIC_TARGET_WILDCARD = '*'.freeze + FALSY_SEARCH_EXTENDED = [nil, false, 0, '0', 'false', ''].freeze def self.parse_condition_tree(collection, args) filters = begin @@ -239,12 +240,15 @@ def self.parse_search(collection, args) end def self.parse_search_extended(args) - extended = args.dig(:params, :data, :attributes, :all_records_subset_query, - :searchExtended) || args.dig(:params, :searchExtended) - - return false if extended.nil? - - extended != '0' + # `key?` rather than `||`: a real +false+ here is falsy, and would lose to the query string. + subset = args.dig(:params, :data, :attributes, :all_records_subset_query) + extended = if subset.is_a?(Hash) && subset.key?(:searchExtended) + subset[:searchExtended] + else + args.dig(:params, :searchExtended) + end + + !FALSY_SEARCH_EXTENDED.include?(extended.is_a?(String) ? extended.downcase : extended) end def self.parse_sort(collection, args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb index 89d1bb663..9288ba3d1 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb @@ -55,7 +55,7 @@ module Resources ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count count.handle_request(args) - expect(read_guard_calls[:query_fields]).to eq([{ collection: 'user', applies: %i[filter search] }]) + expect(read_guard_calls[:query_fields]).to eq([{ collection: 'user', applies: %i[filter search search_extended] }]) end context 'when collection is countable' do diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb index b1e0b0fc6..0aac7c971 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb @@ -66,7 +66,7 @@ module Resources csv.handle_request(args) expect(read_guard_calls[:query_fields]).to eq( - [{ collection: 'user', applies: %i[filter sort search] }] + [{ collection: 'user', applies: %i[filter sort search search_extended] }] ) expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: false }]) end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb index 13d0529eb..c9f4eadfd 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb @@ -79,10 +79,17 @@ module Resources list.handle_request(args) expect(read_guard_calls[:query_fields]).to eq( - [{ collection: 'user', applies: %i[filter sort search] }] + [{ collection: 'user', applies: %i[filter sort search search_extended] }] ) end + it 'hands the guard the extended flag it parsed, not a default' do + args[:params][:searchExtended] = '1' + list.handle_request(args) + + expect(read_guard_calls[:search_extended]).to eq([true]) + end + it 'refuses a projection the caller named on its own collection' do args[:params][:fields] = { 'user' => 'id,first_name' } list.handle_request(args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 15a5a7964..cf511b60d 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -248,13 +248,13 @@ def leaf(field) Nodes::ConditionTreeLeaf.new(field, Operators::EQUAL, 'FR76') end - def searchable_cards(searched) + def searchable_cards(searched, search_handler: false) double = instance_double( - ForestAdminDatasourceToolkit::Decorators::CollectionDecorator, + ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator, name: 'cards', datasource: datasource ) - allow(double).to receive(:searched_fields).and_return(searched) + allow(double).to receive_messages(searched_fields: searched, search_handler?: search_handler) double end @@ -357,8 +357,11 @@ def searchable_cards(searched) it 'serves the request when the stack cannot say what a search reaches' do permissions = build_permissions([]) - expect { permissions.assert_can_read_query_fields(searchable_cards(nil), search: 'martin') } - .not_to raise_error + expect do + permissions.assert_can_read_query_fields( + searchable_cards(nil, search_handler: true), search: 'martin' + ) + end.not_to raise_error end # The flag is the caller's: the same term with it off and on differs by exactly the rows @@ -369,7 +372,7 @@ def searchable_cards(searched) expect do permissions.assert_can_read_query_fields( - searchable_cards(nil), search: 'martin', search_extended: true + searchable_cards(nil, search_handler: true), search: 'martin', search_extended: true ) end.to raise_error( ForestAdminAgent::Http::Exceptions::ForbiddenError, @@ -386,7 +389,7 @@ def searchable_cards(searched) ['', ' '].each do |blank| expect do permissions.assert_can_read_query_fields( - searchable_cards(nil), search: blank, search_extended: true + searchable_cards(nil, search_handler: true), search: blank, search_extended: true ) end.not_to raise_error end @@ -398,16 +401,12 @@ def searchable_cards(searched) expect { permissions.assert_can_read_query_fields(cards, search: 'martin') }.not_to raise_error end - # Silence is not an empty footprint either: a collection with no `searched_fields` at all - # says as little as one answering nil. - it 'reads a collection that cannot answer at all as an unknown footprint too' do + # Refusing here would 403 every collection the search decorator does not sit on. + it 'serves the extended search of a collection that cannot answer at all' do permissions = build_permissions([]) expect { permissions.assert_can_read_query_fields(cards, search: 'martin', search_extended: true) } - .to raise_error( - ForestAdminAgent::Http::Exceptions::ForbiddenError, - /You cannot run an extended search on the 'cards' collection/ - ) + .not_to raise_error end it 'accepts a filter once the collection it reaches is readable' do @@ -490,6 +489,58 @@ def cards_searching(replacer) permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) end.not_to raise_error end + + # Refusing here would take extended search off every natively searchable datasource. + it 'serves an extended search the child collection runs natively' do + permissions = build_permissions([]) + native = datasource.get_collection('cards') + allow(native).to receive(:schema).and_return(native.schema.merge(searchable: true)) + collection = ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.new( + native, datasource + ) + + expect(collection.searched_fields('martin', true)).to be_nil + expect do + permissions.assert_can_read_query_fields(collection, search: 'martin', search_extended: true) + end.not_to raise_error + end + + # The only example that goes through the stack the routes hand the guard: without it, the + # delegation going missing looks exactly like the refusal being correctly narrowed. + it 'refuses a callable extended search through a booted customizer stack' do + permissions = build_permissions([]) + customizer = ForestAdminDatasourceCustomizer::DatasourceCustomizer.new + customizer.add_datasource(datasource, {}) + customizer.customize_collection('cards') do |collection| + collection.replace_search do |value, _extended, _context| + { field: 'pan_last4', operator: Operators::EQUAL, value: value } + end + end + top = customizer.datasource({}).get_collection('cards') + + expect(top).not_to be_a(ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator) + expect(top.searched_fields('martin', true)).to be_nil + expect do + permissions.assert_can_read_query_fields(top, search: 'martin', search_extended: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + /You cannot run an extended search on the 'cards' collection/ + ) + end + + # `can?` allows everything without a permission system, so a refusal there would be the one + # denial no grant could lift. + it 'serves the callable extended search when no permission system is enabled' do + permissions = described_class.new(caller) + allow(permissions).to receive(:permission_system?).and_return(false) + collection = cards_searching( + ->(value, _extended, _context) { { field: 'pan_last4', operator: Operators::EQUAL, value: value } } + ) + + expect do + permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) + end.not_to raise_error + end end end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index cd7ee0144..ca66dfc65 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1232,6 +1232,37 @@ module Utils expect(described_class.parse_search_extended(args)).to be(false) end + + [false, 0, 'false', 'FALSE', ''].each do |falsy| + it "reads #{falsy.inspect} as not extended" do + expect(described_class.parse_search_extended({ params: { searchExtended: falsy } })).to be(false) + end + end + + it 'reads an absent parameter as not extended' do + expect(described_class.parse_search_extended({ params: {} })).to be(false) + end + + it 'keeps reading an unrecognised value as extended, as it always did' do + expect(described_class.parse_search_extended({ params: { searchExtended: 'yes' } })).to be(true) + end + + it 'lets the subset query say no while the query string still carries the flag' do + args = { + params: { + searchExtended: '1', + data: { attributes: { all_records_subset_query: { searchExtended: false } } } + } + } + + expect(described_class.parse_search_extended(args)).to be(false) + end + + it 'falls back to the query string when the subset query does not name the flag' do + args = { params: { searchExtended: '1', data: { attributes: { all_records_subset_query: {} } } } } + + expect(described_class.parse_search_extended(args)).to be(true) + end end describe 'parse_sort' do diff --git a/packages/forest_admin_agent/spec/spec_helper.rb b/packages/forest_admin_agent/spec/spec_helper.rb index edf93e0f9..c659ee7fe 100644 --- a/packages/forest_admin_agent/spec/spec_helper.rb +++ b/packages/forest_admin_agent/spec/spec_helper.rb @@ -44,10 +44,12 @@ # `applies` is derived from the keys the route actually passed, so it cannot drift from the query the # route builds: a component it drops is one it does not pass. -READ_GUARD_QUERY_KEYS = { filter: :condition_tree, sort: :sort, search: :search }.freeze +READ_GUARD_QUERY_KEYS = { + filter: :condition_tree, sort: :sort, search: :search, search_extended: :search_extended +}.freeze RSpec.shared_context 'with readable related collections' do - let(:read_guard_calls) { { query_fields: [], projections: [] } } + let(:read_guard_calls) { { query_fields: [], projections: [], search_extended: [] } } before do allow(permissions).to receive(:assert_can_read_query_fields) do |collection, **options| @@ -55,6 +57,8 @@ collection: collection.name, applies: READ_GUARD_QUERY_KEYS.select { |_name, key| options.key?(key) }.keys } + # `applies` only says the key was passed; the refusal turns on its value. + read_guard_calls[:search_extended] << options[:search_extended] if options.key?(:search_extended) end allow(permissions).to receive(:assert_can_read_usages) allow(permissions).to receive(:redact_projection) do |collection, projection, **options| diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index 53648f7a8..d98ec6bbd 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -31,20 +31,14 @@ def disable_count push_customization { @stack.schema.get_collection(@name).override_schema(countable: false) } end - # Replace the behavior of the search bar, either with a block or with a field selection. + # On a natively searchable datasource, a selection does not narrow that search — it replaces it + # with the agent's own per-column one. # - # A field selection is checked against the caller's read permissions, because the agent can tell - # which columns it reads: a path the role may not read is refused by name. A block cannot be, so - # a plain search on it is served unchecked and an extended one is refused outright. + # A relation path into a collection the role cannot read is refused; own columns carry no check, + # there being no field-level permissions. A block names nothing, so its extended half is refused. # - # Converting a block to a selection is stricter, not looser: an included relation path is checked - # on a plain search too, so a role that cannot read that collection starts being refused. - # - # +extended+ is not accepted, so that a customization cannot pin a flag the caller owns; the - # block is handed the one the request carried. - # - # On a natively searchable datasource, a selection replaces that search with the agent's own - # per-column one rather than narrowing it. + # Resolved in declaration order, at boot: put +replace_search+ after the +add_field+ / + # +add_relation+ calls it depends on. # # Example: # collection.replace_search(include_fields: ['project:name'], exclude_fields: ['description']) @@ -66,6 +60,19 @@ def replace_search(include_fields: nil, exclude_fields: nil, only_fields: nil, & 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' end + empty = selection.select { |_name, names| Array(names).empty? } + if empty.any? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + "replace_search cannot take an empty #{empty.keys.join(" or ")}: use disable_search to " \ + 'turn the search bar off' + end + + if only_fields && (include_fields || exclude_fields) + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or ' \ + 'exclude_fields' + end + push_customization { @stack.search.get_collection(@name).replace_search(definition || selection) } end diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 8685acb7a..f97ad0584 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -21,8 +21,8 @@ def disable_search end def replace_search(replacer) + assert_selection_resolves(replacer) @replacer = replacer - assert_selection_resolves @disabled_search = false mark_schema_as_dirty end @@ -69,6 +69,10 @@ def searched_fields(search, extended) end end + def search_handler? + !handler.nil? + end + private def handler @@ -87,14 +91,12 @@ def enumerable_search? handler.nil? && implements_search? end - # Resolved now rather than per request, so a name that resolves to nothing is reported here - # instead of leaving the search matching nothing. - def assert_selection_resolves - selection = field_selection - - return if selection.nil? + def assert_selection_resolves(replacer) + return if replacer.nil? || replacer.respond_to?(:call) - selected_paths(selection).each { |path| resolved_field(path) } + field_paths(replacer[:only_fields]).each { |path| selected_field(path) } + field_paths(replacer[:include_fields]).each { |path| selected_field(path) } + field_paths(replacer[:exclude_fields]).each { |path| resolved_field(path) } end def insignificant_search?(search) @@ -135,12 +137,6 @@ def searchable_fields(extended) .except(*field_paths(selection[:exclude_fields])) end - def selected_paths(selection) - field_paths(selection[:only_fields]) + - field_paths(selection[:include_fields]) + - field_paths(selection[:exclude_fields]) - end - def field_paths(names) Array(names).map(&:to_s) end @@ -158,6 +154,21 @@ def resolved_field(path) [path, schema] end + # `get_fields` skips such a column silently; here it is refused, because a Number or UUID column + # reaching `build_condition` gets an EQUAL leaf its datasource never declared. + def selected_field(path) + resolved = resolved_field(path) + schema = resolved.last + + unless searchable_field?(schema) + raise ForestException, + "Cannot search on '#{path}': its #{schema.column_type} column declares no filter " \ + 'operator a search term can use' + end + + resolved + end + def build_condition(field, schema, search_string) column_type = schema.column_type enum_values = schema.enum_values diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb index 3d1901f4d..bf3ff27fc 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb @@ -242,6 +242,57 @@ module ForestAdminDatasourceCustomizer 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' ) end + + # It also re-enables a bar `disable_search` had switched off. + it 'refuses an empty field list, which says nothing and disables nothing' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: []) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search cannot take an empty only_fields: use disable_search to turn the search bar off' + ) + end + + it 'refuses only_fields alongside include_fields, where only would stop meaning only' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: ['title'], include_fields: ['id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or exclude_fields' + ) + end + + it 'refuses only_fields alongside exclude_fields, which could empty the selected set' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: ['title'], exclude_fields: ['title']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or exclude_fields' + ) + end + + it 'refuses an empty include_fields too, not only an empty only_fields' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(include_fields: []) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search cannot take an empty include_fields: use disable_search to turn the search bar off' + ) + end + + it 'names every empty list it was given, not just the first' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(include_fields: [], exclude_fields: []) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + /empty include_fields or exclude_fields/ + ) + end end context 'when using disable_search' do diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index ef02ca79d..e2e272809 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -337,6 +337,32 @@ def refined(selection) "Cannot search on 'holder': a ManyToOne is not a column" ) end + + it 'refuses a column no search term can match, which the defaults already skip' do + expect { decorated.replace_search(include_fields: ['holder_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot search on 'holder_id': its Number column declares no filter operator a search term can use" + ) + end + + it 'still accepts excluding that column, which only has to exist to be dropped' do + decorated.replace_search(exclude_fields: ['holder_id']) + + expect(decorated.searched_fields('martin', false)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + + it 'leaves the previous selection in place when it refuses a new one' do + decorated.replace_search(only_fields: ['pan_last4']) + + expect { decorated.replace_search(only_fields: ['nope']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) + expect(decorated.searched_fields('martin', false)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end end end end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb index b6ffd92c4..50271df64 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb @@ -85,6 +85,12 @@ def searched_fields(search, extended) @child_collection.searched_fields(search, extended) end + def search_handler? + return false unless @child_collection.is_a?(CollectionDecorator) + + @child_collection.search_handler? + end + protected def mark_schema_as_dirty From ca75407ba3dffeef2b7b78a561555b26dab878bd Mon Sep 17 00:00:00 2001 From: Matt Date: Mon, 31 Aug 2026 18:28:21 +0200 Subject: [PATCH 07/13] fix(agent): coerce a search term rather than strip a number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parse_search` returned whatever the request carried, and three places strip it: the permission guard, `refine_filter`, and `insignificant_search?`. A number reaching any of them raised NoMethodError, so guarding one would have moved the crash rather than removed it. It is coerced at the source now, and a value that cannot be one term — an array, a hash, a boolean — is a request error instead of a search for the string an Array prints as. The spec pinning `parse_search` returning 1234 is titled "converts the query search parameter as string", so the preservation was a bug its own name contradicted. The assertion matches the title again. Also extracts the `replace_search` validation, which qlty flagged for complexity, and states the only_fields rule as `selection.keys != [:only_fields]` rather than enumerating the lists it cannot join. Co-Authored-By: Claude Opus 5 (1M context) --- .../utils/query_string_parser.rb | 9 +++- .../utils/query_string_parser_spec.rb | 13 ++++- .../collection_customizer.rb | 47 ++++++++++--------- 3 files changed, 45 insertions(+), 24 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index b9bb089c1..3b4a450f6 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -236,7 +236,14 @@ def self.parse_search(collection, args) raise BadRequestError, 'Collection is not searchable' if search && !collection.is_searchable? - search + if search && !search.is_a?(String) && !search.is_a?(Numeric) + raise BadRequestError, 'Search must be a string or a number' + end + + # Every layer that consumes this strips it: the permission guard, `refine_filter`, and + # `insignificant_search?`. A number reaching them raises, and `?search[]=x` would search the + # string an Array prints as. + search&.to_s end def self.parse_search_extended(args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index ca66dfc65..ec2ea83b0 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1200,7 +1200,18 @@ module Utils it 'converts the query search parameter as string' do args = { params: { search: 1234 } } - expect(described_class.parse_search(collection_user, args)).to eq(1234) + expect(described_class.parse_search(collection_user, args)).to eq('1234') + end + + # Every consumer strips it, so a value that cannot be one term is a request error rather than + # a `NoMethodError` deeper in the stack — or a search for the string an Array prints as. + [['x'], { a: '1' }, true].each do |value| + it "refuses #{value.inspect}, which cannot be a search term" do + args = { params: { search: value } } + + expect { described_class.parse_search(collection_user, args) } + .to raise_error(Http::Exceptions::BadRequestError, 'Search must be a string or a number') + end end it 'works when passed in the body (actions)' do diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index d98ec6bbd..f4de14931 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -50,28 +50,7 @@ def replace_search(include_fields: nil, exclude_fields: nil, only_fields: nil, & only_fields: only_fields }.compact - if definition && selection.any? - raise ForestAdminDatasourceToolkit::Exceptions::ForestException, - 'replace_search accepts either a block or a field selection, not both' - end - - if definition.nil? && selection.empty? - raise ForestAdminDatasourceToolkit::Exceptions::ForestException, - 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' - end - - empty = selection.select { |_name, names| Array(names).empty? } - if empty.any? - raise ForestAdminDatasourceToolkit::Exceptions::ForestException, - "replace_search cannot take an empty #{empty.keys.join(" or ")}: use disable_search to " \ - 'turn the search bar off' - end - - if only_fields && (include_fields || exclude_fields) - raise ForestAdminDatasourceToolkit::Exceptions::ForestException, - 'replace_search accepts only_fields on its own, not alongside include_fields or ' \ - 'exclude_fields' - end + assert_search_replacement(selection, definition, only_fields: only_fields) push_customization { @stack.search.get_collection(@name).replace_search(definition || selection) } end @@ -351,6 +330,30 @@ def override_delete(&handler) private + def assert_search_replacement(selection, definition, only_fields:) + if definition && selection.any? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts either a block or a field selection, not both' + end + + if definition.nil? && selection.empty? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' + end + + empty = selection.select { |_name, names| Array(names).empty? } + if empty.any? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + "replace_search cannot take an empty #{empty.keys.join(" or ")}: use disable_search to " \ + 'turn the search bar off' + end + + return unless only_fields && selection.keys != [:only_fields] + + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or exclude_fields' + end + def push_customization(&customization) @stack.queue_customization(customization) From 7f7bf0b3cd8de9d46770d7eb2dddb5fa025cfdba Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 2 Sep 2026 10:35:43 +0200 Subject: [PATCH 08/13] fix(agent): refuse a boolean search term, reorder the guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both guards in `parse_search` tested truthiness, so `false` skipped the type check and `Collection is not searchable` alike, and `&.` stops on `nil` only — it became the term "false". Testing `nil` closes that, and the body is now read by presence like the flag beside it: a boolean can only arrive through the select-all query, where `||` discarded it before either guard saw it. `describes_own_search?` is local while `permission_system?` fetches `/liana/v4/permissions/environment`. In the previous order every extended search on a collection with an unknown footprint reached that fetch — every Zendesk collection, every RPC-backed one, and every collection carrying no search decorator — where `read_permissions` only paid for it when a relation path existed. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/permissions.rb | 2 +- .../utils/query_string_parser.rb | 20 +++++++++++++------ .../security/related_read_permissions_spec.rb | 10 ++++++++++ .../utils/query_string_parser_spec.rb | 14 ++++++++++++- 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index ee94fdb15..332f94dd6 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -354,8 +354,8 @@ def collect_search_usages(collection, search, search_extended, usages) # served in either case — the extended flag is the caller's own, so the exemption stops there. def assert_extended_search_checkable(collection, search_extended) return unless search_extended - return unless permission_system? return unless describes_own_search?(collection) + return unless permission_system? raise ForbiddenError, "You cannot run an extended search on the '#{collection.name}' collection: the fields " \ diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index 3b4a450f6..595c2a7c2 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -232,18 +232,26 @@ def self.parse_export_pagination(limit) end def self.parse_search(collection, args) - search = args.dig(:params, :data, :attributes, :all_records_subset_query, :search) || args.dig(:params, :search) + # Presence, not truth, for the same reason as the flag below: `false` is a value here, and + # +||+ would read it as absent. + subset = args.dig(:params, :data, :attributes, :all_records_subset_query) + search = if subset.is_a?(Hash) && subset.key?(:search) + subset[:search] + else + args.dig(:params, :search) + end + + return nil if search.nil? - raise BadRequestError, 'Collection is not searchable' if search && !collection.is_searchable? + raise BadRequestError, 'Collection is not searchable' unless collection.is_searchable? - if search && !search.is_a?(String) && !search.is_a?(Numeric) + unless search.is_a?(String) || search.is_a?(Numeric) raise BadRequestError, 'Search must be a string or a number' end # Every layer that consumes this strips it: the permission guard, `refine_filter`, and - # `insignificant_search?`. A number reaching them raises, and `?search[]=x` would search the - # string an Array prints as. - search&.to_s + # `insignificant_search?`. + search.to_s end def self.parse_search_extended(args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index cf511b60d..46f3b2350 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -409,6 +409,16 @@ def searchable_cards(searched, search_handler: false) .not_to raise_error end + # `permission_system?` fetches `/liana/v4/permissions/environment` cold; `describes_own_search?` + # is local. Every extended search on a nil footprint reached that fetch. + it 'does not reach the permission system for a search it will not refuse' do + permissions = build_permissions([]) + + permissions.assert_can_read_query_fields(cards, search: 'martin', search_extended: true) + + expect(permissions).not_to have_received(:permission_system?) + end + it 'accepts a filter once the collection it reaches is readable' do permissions = build_permissions(%w[accounts]) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index ec2ea83b0..5dd89af53 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1205,7 +1205,7 @@ module Utils # Every consumer strips it, so a value that cannot be one term is a request error rather than # a `NoMethodError` deeper in the stack — or a search for the string an Array prints as. - [['x'], { a: '1' }, true].each do |value| + [['x'], { a: '1' }, true, false].each do |value| it "refuses #{value.inspect}, which cannot be a search term" do args = { params: { search: value } } @@ -1214,6 +1214,18 @@ module Utils end end + it 'refuses a body value the query string would otherwise mask' do + args = { + params: { + search: 'searched argument', + data: { attributes: { all_records_subset_query: { search: false } } } + } + } + + expect { described_class.parse_search(collection_user, args) } + .to raise_error(Http::Exceptions::BadRequestError, 'Search must be a string or a number') + end + it 'works when passed in the body (actions)' do args = { params: { From b692b1f008e9026823ceda89732d03a0d063c3bc Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 2 Sep 2026 10:38:06 +0200 Subject: [PATCH 09/13] fix(search): exclude a relation named bare `exclude_fields: ['holder']` raised, so "extended search, but not through this relation" meant listing the target's columns and remembering to revisit that list every time it gained one. An excluded name never carried `include_fields` constraint anyway, only having to exist to be dropped, so naming a relation now drops every path through it. A block replacer logs a Warn at boot naming the collection and the `include_fields:` form. The refusal ships in a minor, and without this the first sign of it is a user hitting the 403. The DSL block was missing three surprises, each in the same direction: an included relation path is read on a plain search too, `only_fields` makes `extended` inert, and names resolve below this layer while `rename_field` sits above it, so a field renamed to `title` is still named `name` here. Co-Authored-By: Claude Opus 5 (1M context) --- .../collection_customizer.rb | 8 +++-- .../search/search_collection_decorator.rb | 28 +++++++++++++++-- .../decorators/search/searched_fields_spec.rb | 31 +++++++++++++++++++ 3 files changed, 63 insertions(+), 4 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index f4de14931..2789f9180 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -37,8 +37,12 @@ def disable_count # A relation path into a collection the role cannot read is refused; own columns carry no check, # there being no field-level permissions. A block names nothing, so its extended half is refused. # - # Resolved in declaration order, at boot: put +replace_search+ after the +add_field+ / - # +add_relation+ calls it depends on. + # An included relation path is read on a plain search too, so +searchExtended=0+ no longer means + # no relation traversal; with +only_fields+, +extended+ becomes inert entirely. + # + # Names resolve against the collection below this layer, and +rename_field+ sits above it: a field + # renamed +name+ -> +title+ is named +name+ here. Resolved in declaration order, at boot, so put + # +replace_search+ after the +add_field+ / +add_relation+ calls it depends on. # # Example: # collection.replace_search(include_fields: ['project:name'], exclude_fields: ['description']) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index f97ad0584..66f8892f9 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -24,6 +24,7 @@ def replace_search(replacer) assert_selection_resolves(replacer) @replacer = replacer @disabled_search = false + warn_extended_search_refused if handler mark_schema_as_dirty end @@ -75,6 +76,17 @@ def search_handler? private + # Said at boot rather than left to the first user who trips the 403, since a block installed + # before this version was served. + def warn_extended_search_refused + ForestAdminAgent::Facades::Container.logger.log( + 'Warn', + "An extended search on #{name} will be refused: a `replace_search` block names no field, " \ + 'so the agent cannot check what it reads against the caller\'s permissions. Declaring the ' \ + 'search with `replace_search(include_fields: [...])` makes it checkable.' + ) + end + def handler @replacer.respond_to?(:call) ? @replacer : nil end @@ -96,7 +108,7 @@ def assert_selection_resolves(replacer) field_paths(replacer[:only_fields]).each { |path| selected_field(path) } field_paths(replacer[:include_fields]).each { |path| selected_field(path) } - field_paths(replacer[:exclude_fields]).each { |path| resolved_field(path) } + field_paths(replacer[:exclude_fields]).each { |path| excluded_field(path) } end def insignificant_search?(search) @@ -132,9 +144,21 @@ def searchable_fields(extended) defaults = only_fields ? {} : get_fields(extended).to_h selected = field_paths(only_fields) + field_paths(selection[:include_fields]) + excluded = field_paths(selection[:exclude_fields]) + defaults .merge(selected.to_h { |path| resolved_field(path) }) - .except(*field_paths(selection[:exclude_fields])) + .reject { |path, _schema| excluded?(path, excluded) } + end + + # An excluded relation drops every path through it: naming the target's columns one by one + # would have to be revisited each time it gains one. + def excluded?(path, excluded) + excluded.any? { |name| path == name || path.start_with?("#{name}:") } + end + + def excluded_field(path) + ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) end def field_paths(names) diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index e2e272809..b79a2490c 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -293,6 +293,29 @@ def refined(selection) end end + describe '#replace_search with a callable replacer' do + let(:logger) { instance_double(ForestAdminAgent::Services::LoggerService, log: nil) } + + before { allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) } + + it 'warns at boot that an extended search there will be refused' do + decorated.replace_search(->(value, _extended, _context) { { field: 'pan_last4', value: value } }) + + expect(logger).to have_received(:log).with( + 'Warn', + 'An extended search on cards will be refused: a `replace_search` block names no field, ' \ + "so the agent cannot check what it reads against the caller's permissions. Declaring the " \ + 'search with `replace_search(include_fields: [...])` makes it checkable.' + ) + end + + it 'says nothing for a field selection, which is checkable' do + decorated.replace_search(include_fields: ['holder:national_id']) + + expect(logger).not_to have_received(:log) + end + end + describe '#replace_search with an unresolvable field selection' do it 'names the field it cannot resolve rather than searching nothing for good' do expect { decorated.replace_search(only_fields: ['pan_last_four']) } @@ -346,6 +369,14 @@ def refined(selection) ) end + it 'excludes every path through a relation named bare, which no column list could keep up with' do + decorated.replace_search(exclude_fields: ['holder']) + + expect(decorated.searched_fields('martin', true)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + it 'still accepts excluding that column, which only has to exist to be dropped' do decorated.replace_search(exclude_fields: ['holder_id']) From c4f3e7900dede473acc113d7856409b7cf06d112 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 2 Sep 2026 11:31:32 +0200 Subject: [PATCH 10/13] fix(search): survive a nil logger, refuse an inert exclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The boot warning added for a block replacer reached `ForestAdminAgent::Facades::Container.logger`, which is nil in the RPC agent: it subclasses `AgentFactory`, and Singleton gives the subclass its own instance, so the base container is never built there. An RPC agent with a `replace_search` block therefore stopped booting, on a NoMethodError naming nothing about search. The maintainers already knew the facade returns nil outside the plain agent — `sinatra_extension` writes `logger&.log`. A warning must not become a boot failure. Its wording was wrong too, in both the log line and the public docstring: the refusal is also gated on `permission_system?`, so an agent with permissions disabled printed a warning about a 403 that never fires. `exclude_fields` accepted names it could never match. Only own columns and depth-1 to-one paths are ever searched, so `['lines']`, a depth-2 path, a polymorphic relation named bare, or a column no term can match were taken and silently ignored — the configuration lied. They are refused now, and a path both selected and excluded raises rather than letting one side win silently: exclude-then-merge drops a column the developer named, merge-then-exclude makes an excluded one searchable, and neither is a contract worth documenting. Two specs went with it. One listed `holder_id` among "every searchable field" though the search never read it. The other asserted the unmodified default footprint, so it passed whether or not the exclusion did anything. Co-Authored-By: Claude Opus 5 (1M context) --- .../collection_customizer.rb | 3 +- .../search/search_collection_decorator.rb | 56 +++++++++---- .../decorators/search/searched_fields_spec.rb | 80 ++++++++++++++++--- 3 files changed, 114 insertions(+), 25 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index 2789f9180..bce6492de 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -35,7 +35,8 @@ def disable_count # with the agent's own per-column one. # # A relation path into a collection the role cannot read is refused; own columns carry no check, - # there being no field-level permissions. A block names nothing, so its extended half is refused. + # there being no field-level permissions. A block names nothing, so its extended half is refused + # where permissions are enabled. # # An included relation path is read on a plain search too, so +searchExtended=0+ no longer means # no relation traversal; with +only_fields+, +extended+ becomes inert entirely. diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 66f8892f9..c73b5c3f2 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -76,14 +76,21 @@ def search_handler? private - # Said at boot rather than left to the first user who trips the 403, since a block installed - # before this version was served. + # Warned at boot, not left to the first caller who trips the 403: blocks installed before this + # version were served. Nil-guarded because the RPC agent runs its own +AgentFactory+ subclass, + # so the base facade's container is never built there — a customization must not become a boot + # failure over a warning. def warn_extended_search_refused - ForestAdminAgent::Facades::Container.logger.log( + logger = ForestAdminAgent::Facades::Container.logger + + return if logger.nil? + + logger.log( 'Warn', - "An extended search on #{name} will be refused: a `replace_search` block names no field, " \ - 'so the agent cannot check what it reads against the caller\'s permissions. Declaring the ' \ - 'search with `replace_search(include_fields: [...])` makes it checkable.' + "An extended search on #{name} is refused where permissions are enabled: a " \ + '`replace_search` block names no field, so the agent cannot check what it reads against ' \ + "the caller's permissions. Declaring the search with " \ + '`replace_search(include_fields: [...])` makes it checkable.' ) end @@ -106,9 +113,18 @@ def enumerable_search? def assert_selection_resolves(replacer) return if replacer.nil? || replacer.respond_to?(:call) - field_paths(replacer[:only_fields]).each { |path| selected_field(path) } - field_paths(replacer[:include_fields]).each { |path| selected_field(path) } - field_paths(replacer[:exclude_fields]).each { |path| excluded_field(path) } + selected = field_paths(replacer[:only_fields]) + field_paths(replacer[:include_fields]) + excluded = field_paths(replacer[:exclude_fields]) + + selected.each { |path| selected_field(path) } + excluded.each { |path| excluded_field(path) } + + overlap = selected.select { |path| excluded?(path, excluded) } + + return if overlap.empty? + + raise ForestException, + "Cannot both search and exclude #{overlap.map { |path| "'#{path}'" }.join(", ")}" end def insignificant_search?(search) @@ -151,14 +167,26 @@ def searchable_fields(extended) .reject { |path, _schema| excluded?(path, excluded) } end - # An excluded relation drops every path through it: naming the target's columns one by one - # would have to be revisited each time it gains one. def excluded?(path, excluded) excluded.any? { |name| path == name || path.start_with?("#{name}:") } end + # Unlike a selected path, a bare to-one relation is legal here: it drops every path through it, + # where naming the target's columns would have to be revisited each time it gains one. What is + # refused is a name the search never reads either way — a to-many, a depth-2 path, a column no + # term can match — because excluding it silently changes nothing. def excluded_field(path) - ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) + schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) + + return schema if excludable?(path, schema) + + raise ForestException, "Cannot exclude '#{path}' from the search: the search does not read it" + end + + def excludable?(path, schema) + return path.count(':') <= 1 && searchable_field?(schema) if schema.type == 'Column' + + !path.include?(':') && TO_ONE_RELATIONS.include?(schema.type) end def field_paths(names) @@ -178,8 +206,8 @@ def resolved_field(path) [path, schema] end - # `get_fields` skips such a column silently; here it is refused, because a Number or UUID column - # reaching `build_condition` gets an EQUAL leaf its datasource never declared. + # A Number, Enum or UUID column reaching `build_condition` gets an EQUAL leaf its datasource + # never declared, and `get_fields` skips it silently — so a named one is refused instead. def selected_field(path) resolved = resolved_field(path) schema = resolved.last diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index b79a2490c..d3a1bc28e 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -288,27 +288,79 @@ def refined(selection) end it 'matches nothing rather than everything once every searchable field is excluded' do - expect(refined({ exclude_fields: %w[id pan_last4 holder_id holder:id holder:national_id] }).condition_tree) + expect(refined({ exclude_fields: %w[id pan_last4 holder:id holder:national_id] }).condition_tree) .to have_attributes(aggregator: 'Or', conditions: []) end end + # The fixture above has no searchable column whose name the relation only prefixes, so the + # `:` boundary in the exclusion cannot be told apart there. + describe '#replace_search excluding a relation whose name prefixes a column' do + let(:datasource) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'holder_note' => build_column(column_type: 'String', + filter_operators: [Operators::I_CONTAINS]), + 'holder_id' => build_column(column_type: 'Number'), + 'holder' => build_many_to_one(foreign_collection: 'holders', foreign_key: 'holder_id') + } + } + ), + build_collection( + name: 'holders', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'national_id' => build_column(column_type: 'String', + filter_operators: [Operators::I_CONTAINS]) + } + } + ) + ] + ) + end + + it 'keeps a sibling column the relation name only prefixes' do + decorated.replace_search(exclude_fields: ['holder']) + + expect(decorated.searched_fields('martin', true)).to eq( + [{ path: 'holder_note', collections: ['cards'] }] + ) + end + end + describe '#replace_search with a callable replacer' do let(:logger) { instance_double(ForestAdminAgent::Services::LoggerService, log: nil) } before { allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) } - it 'warns at boot that an extended search there will be refused' do + it 'warns at boot that an extended search there is refused where permissions are on' do decorated.replace_search(->(value, _extended, _context) { { field: 'pan_last4', value: value } }) expect(logger).to have_received(:log).with( 'Warn', - 'An extended search on cards will be refused: a `replace_search` block names no field, ' \ - "so the agent cannot check what it reads against the caller's permissions. Declaring the " \ - 'search with `replace_search(include_fields: [...])` makes it checkable.' + 'An extended search on cards is refused where permissions are enabled: a ' \ + '`replace_search` block names no field, so the agent cannot check what it reads against ' \ + "the caller's permissions. Declaring the search with " \ + '`replace_search(include_fields: [...])` makes it checkable.' ) end + # The RPC agent runs its own `AgentFactory` subclass, so the base facade's container is never + # built there and `logger` answers nil. + it 'installs the block anyway where no logger is resolvable' do + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(nil) + + expect { decorated.replace_search(->(value, _extended, _context) { { field: 'pan_last4', value: value } }) } + .not_to raise_error + expect(decorated.searched_fields('martin', true)).to be_nil + end + it 'says nothing for a field selection, which is checkable' do decorated.replace_search(include_fields: ['holder:national_id']) @@ -377,12 +429,20 @@ def refined(selection) ) end - it 'still accepts excluding that column, which only has to exist to be dropped' do - decorated.replace_search(exclude_fields: ['holder_id']) + it 'refuses excluding a column the search never reads, which would change nothing' do + expect { decorated.replace_search(exclude_fields: ['holder_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot exclude 'holder_id' from the search: the search does not read it" + ) + end - expect(decorated.searched_fields('martin', false)).to eq( - [{ path: 'pan_last4', collections: ['cards'] }] - ) + it 'refuses searching and excluding the same path, rather than letting one win silently' do + expect { decorated.replace_search(include_fields: ['holder:national_id'], exclude_fields: ['holder']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot both search and exclude 'holder:national_id'" + ) end it 'leaves the previous selection in place when it refuses a new one' do From 84e1f546f88fade8ebaa686e3bc4723f356ab417 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 2 Sep 2026 11:32:11 +0200 Subject: [PATCH 11/13] fix(agent): read an explicit null subset search as absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Testing the subset query by key presence fixed a `false` losing to the query string, but it also let an explicit null there win — dropping the term the URL carried, which widens a result set rather than narrowing it. Only a present, non-nil body value masks the query string now. The precedence block was duplicated between the search and the flag, each with its own comment saying the same thing, one of them by cross-reference. It is one helper, so the rule is stated once, and the digs are guarded the way `parse_search_extended`'s neighbour 220 lines above already guards its own: `?data[attributes][]=1` walked `Hash#dig` into an Array and answered 500 instead of 400. Drops the note claiming every consumer strips the term. The three sites named do strip, but only to test significance — `build_condition` hands the unstripped string to the condition leaf, so a padded term is what the datasource compares. Co-Authored-By: Claude Opus 5 (1M context) --- .../utils/query_string_parser.rb | 38 ++++++++++-------- .../utils/query_string_parser_spec.rb | 39 +++++++++++++++++-- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index 595c2a7c2..11b04efc7 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -231,15 +231,27 @@ def self.parse_export_pagination(limit) Page.new(offset: 0, limit: limit&.to_i) end + # Presence, not truth: with +||+ a +false+ in the select-all body would silently lose to the + # query string. An explicit +null+ there is read as absent rather than as "no search", so it + # cannot widen a result set by discarding the term the URL carried. + def self.subset_or_query(args, key) + subset = begin + args.dig(:params, :data, :attributes, :all_records_subset_query) + rescue StandardError + nil + end + + return subset[key] if subset.is_a?(Hash) && !subset[key].nil? + + begin + args.dig(:params, key) + rescue StandardError + nil + end + end + def self.parse_search(collection, args) - # Presence, not truth, for the same reason as the flag below: `false` is a value here, and - # +||+ would read it as absent. - subset = args.dig(:params, :data, :attributes, :all_records_subset_query) - search = if subset.is_a?(Hash) && subset.key?(:search) - subset[:search] - else - args.dig(:params, :search) - end + search = subset_or_query(args, :search) return nil if search.nil? @@ -249,19 +261,11 @@ def self.parse_search(collection, args) raise BadRequestError, 'Search must be a string or a number' end - # Every layer that consumes this strips it: the permission guard, `refine_filter`, and - # `insignificant_search?`. search.to_s end def self.parse_search_extended(args) - # `key?` rather than `||`: a real +false+ here is falsy, and would lose to the query string. - subset = args.dig(:params, :data, :attributes, :all_records_subset_query) - extended = if subset.is_a?(Hash) && subset.key?(:searchExtended) - subset[:searchExtended] - else - args.dig(:params, :searchExtended) - end + extended = subset_or_query(args, :searchExtended) !FALSY_SEARCH_EXTENDED.include?(extended.is_a?(String) ? extended.downcase : extended) end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index 5dd89af53..6550cd7d6 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1197,10 +1197,43 @@ module Utils expect(described_class.parse_search(collection_user, args)).to eq('searched argument') end - it 'converts the query search parameter as string' do - args = { params: { search: 1234 } } + [[1234, '1234'], [12.5, '12.5']].each do |value, expected| + it "converts #{value.inspect} to #{expected.inspect}" do + args = { params: { search: value } } + + expect(described_class.parse_search(collection_user, args)).to eq(expected) + end + end + + it 'falls back to the query string when the subset query does not name a search' do + args = { params: { search: 'searched argument', data: { attributes: { all_records_subset_query: {} } } } } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end - expect(described_class.parse_search(collection_user, args)).to eq('1234') + # An explicit null there must not discard the term the URL carried: dropping a search widens + # the result set. + it 'reads an explicit null in the subset query as absent, not as no search' do + args = { + params: { + search: 'searched argument', + data: { attributes: { all_records_subset_query: { search: nil } } } + } + } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + it 'falls back to the query string when the subset query is not a hash' do + args = { params: { search: 'searched argument', data: { attributes: { all_records_subset_query: 'nope' } } } } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + it 'answers nil rather than raising when the body is shaped unexpectedly' do + args = { params: { search: 'searched argument', data: { attributes: [1] } } } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') end # Every consumer strips it, so a value that cannot be one term is a request error rather than From d3378a3ff144e658305f80d117af57b1bf2590f8 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 2 Sep 2026 11:32:45 +0200 Subject: [PATCH 12/13] test: pin what the search guard is actually wired to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `count` and `csv` asserted only that they passed the extended flag, not its value. Replacing the parsed flag with a literal `false` in either route left the suite green, so the refusal this PR exists to add could be switched off on the bulk-export route unnoticed. `list` already had the value pinned; the collector was shared, the examples were not. `CollectionDecorator#search_handler?` had no spec in the package that owns it. Stubbing its body dead passed the toolkit's own suite, and only one example in another package caught it — so the normal workflow of editing the toolkit and running the toolkit ships the fail-open that already happened once. The blank-search example looped two spellings inside one `it`, so a failure never named which one, and the exclusion fixture had no searchable column whose name the relation merely prefixes: dropping the colon from the prefix match passed everything. Co-Authored-By: Claude Opus 5 (1M context) --- .../routes/resources/count_spec.rb | 8 ++++++++ .../routes/resources/csv_spec.rb | 7 +++++++ .../security/related_read_permissions_spec.rb | 6 +++--- .../decorators/collection_decorator_spec.rb | 18 ++++++++++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb index 9288ba3d1..4751140fd 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb @@ -58,6 +58,14 @@ module Resources expect(read_guard_calls[:query_fields]).to eq([{ collection: 'user', applies: %i[filter search search_extended] }]) end + it 'hands the guard the extended flag it parsed, not a default' do + ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count + args[:params][:searchExtended] = '1' + count.handle_request(args) + + expect(read_guard_calls[:search_extended]).to eq([true]) + end + context 'when collection is countable' do it 'return an serialized content' do ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb index 0aac7c971..cacdc8c7b 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb @@ -71,6 +71,13 @@ module Resources expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: false }]) end + it 'hands the guard the extended flag it parsed, not a default' do + args[:params][:searchExtended] = '1' + csv.handle_request(args) + + expect(read_guard_calls[:search_extended]).to eq([true]) + end + context 'when call csv' do it 'returns a streaming export csv' do # Create a mock enumerator that yields CSV data diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 46f3b2350..a1529a341 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -383,10 +383,10 @@ def searchable_cards(searched, search_handler: false) # `refine_filter` discards a blank search instead of running it, so refusing one would 403 a # request that searches nothing. - it 'serves a blank search with the extended flag on, which runs no search at all' do - permissions = build_permissions([]) + ['', ' '].each do |blank| + it "serves #{blank.inspect} with the extended flag on, which runs no search at all" do + permissions = build_permissions([]) - ['', ' '].each do |blank| expect do permissions.assert_can_read_query_fields( searchable_cards(nil, search_handler: true), search: blank, search_extended: true diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb index 2117d3e4f..c7657dcb8 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb @@ -18,6 +18,24 @@ module Decorators datasource.add_collection(@collection_book) end + # The permission layer probes this with `respond_to?` on the top of a 26-layer stack, so a + # missing link answers "no customization" instead of raising. Owned here, tested here. + context 'when search_handler? is called' do + it 'forwards to the child decorator, which is where the search layer answers' do + child = described_class.new(@collection_book, @collection_book.datasource) + allow(child).to receive(:search_handler?).and_return(true) + decorator = described_class.new(child, @collection_book.datasource) + + expect(decorator.search_handler?).to be true + end + + it 'answers false at the bottom, where the child is a plain collection' do + decorator = described_class.new(@collection_book, @collection_book.datasource) + + expect(decorator.search_handler?).to be false + end + end + context 'when native_driver is called' do it 'returns the native driver' do allow(@collection_book).to receive(:native_driver).and_return('a native driver') From fc2ffa48d61ae14067d4a711e50cae85bd2d8b8e Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 2 Sep 2026 15:34:28 +0200 Subject: [PATCH 13/13] fix(search): guard the request-path logger, report an inert exclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nil-logger crash was only half fixed. `get_fields` calls the same base facade unguarded, and that is the request path: an RPC agent running an extended search on a collection carrying a polymorphic relation reached it and answered 500. The three calls in this file now all handle nil. Requiring a selected column's searchability of an excluded one turned schema drift into a boot failure, and it was the defensive configuration that broke: `exclude_fields: ['ssn']` written to keep PII out of the search bar stopped the agent booting the day that column turned unsearchable — the one change that made the intent more true, not less. A name resolving to nothing is still a typo and still raises; a name resolving to a real field the search happens not to read is reported at Debug instead. A path both selected and excluded keeps raising, being a contradiction rather than a no-op. An explicit empty string in the select-all body no longer wins over the query string either. A null there was already read as absent because discarding the term the URL carried widens the result set, and `''` did exactly that while being honoured. Co-Authored-By: Claude Opus 5 (1M context) --- .../utils/query_string_parser.rb | 6 ++-- .../utils/query_string_parser_spec.rb | 13 ++++++++ .../search/search_collection_decorator.rb | 20 ++++++++---- .../decorators/search/searched_fields_spec.rb | 31 +++++++++++++++---- 4 files changed, 55 insertions(+), 15 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index 11b04efc7..4b6bb0492 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -232,8 +232,8 @@ def self.parse_export_pagination(limit) end # Presence, not truth: with +||+ a +false+ in the select-all body would silently lose to the - # query string. An explicit +null+ there is read as absent rather than as "no search", so it - # cannot widen a result set by discarding the term the URL carried. + # query string. An explicit +null+ or +''+ there is read as absent rather than as "no search", + # so neither can widen a result set by discarding the term the URL carried. def self.subset_or_query(args, key) subset = begin args.dig(:params, :data, :attributes, :all_records_subset_query) @@ -241,7 +241,7 @@ def self.subset_or_query(args, key) nil end - return subset[key] if subset.is_a?(Hash) && !subset[key].nil? + return subset[key] if subset.is_a?(Hash) && !subset[key].nil? && subset[key] != '' begin args.dig(:params, key) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index 6550cd7d6..e7f115fac 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1224,6 +1224,19 @@ module Utils expect(described_class.parse_search(collection_user, args)).to eq('searched argument') end + # Honouring it would discard the term the URL carried, which widens the result set — the same + # reason an explicit null is read as absent. + it 'reads an empty string in the subset query as absent too' do + args = { + params: { + search: 'searched argument', + data: { attributes: { all_records_subset_query: { search: '' } } } + } + } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + it 'falls back to the query string when the subset query is not a hash' do args = { params: { search: 'searched argument', data: { attributes: { all_records_subset_query: 'nope' } } } } diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index c73b5c3f2..311f78b81 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -172,15 +172,23 @@ def excluded?(path, excluded) end # Unlike a selected path, a bare to-one relation is legal here: it drops every path through it, - # where naming the target's columns would have to be revisited each time it gains one. What is - # refused is a name the search never reads either way — a to-many, a depth-2 path, a column no - # term can match — because excluding it silently changes nothing. + # where naming the target's columns would have to be revisited each time it gains one. + # + # A name the search does not read is reported rather than refused. An exclusion states an + # intent that only becomes more true as the schema moves — a column turning unsearchable + # satisfies "never search this" — so refusing it would stop the agent booting over a + # configuration that was defensive on purpose. A typo still raises, from +get_field_schema+. def excluded_field(path) schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) - return schema if excludable?(path, schema) + unless excludable?(path, schema) + ForestAdminAgent::Facades::Container.logger&.log( + 'Debug', + "Excluding '#{path}' from the search on #{name} changes nothing: the search does not read it" + ) + end - raise ForestException, "Cannot exclude '#{path}' from the search: the search does not read it" + schema end def excludable?(path, schema) @@ -269,7 +277,7 @@ def get_fields(extended) fields.push([name, field]) if field.type == 'Column' && searchable_field?(field) if POLYMORPHIC_TYPES.include?(field.type) && extended - ForestAdminAgent::Facades::Container.logger.log( + ForestAdminAgent::Facades::Container.logger&.log( 'Debug', "We're not searching through #{self.name}.#{name} because it's a polymorphic relation. " \ "You can override the default search behavior with 'replace_search'. " \ diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index d3a1bc28e..7b0996ddc 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -253,6 +253,15 @@ def paths_read(selection, search: 'martin', extended: true) ) end + # The request path, not the boot path: an RPC agent resolves nothing from the base facade, so + # an extended search reaching this relation must not 500 over a Debug line. + it 'searches on where no logger is resolvable' do + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(nil) + + expect(decorated.searched_fields('martin', true).map { |field| field[:path] }) + .to eq(['pan_last4']) + end + # Its targets stay out of both, so no footprint entry ever needs the several collections # `leaf_collection_names` would answer for a polymorphic leaf. it 'keeps its columns out of the footprint and out of what the search reads' do @@ -429,12 +438,22 @@ def refined(selection) ) end - it 'refuses excluding a column the search never reads, which would change nothing' do - expect { decorated.replace_search(exclude_fields: ['holder_id']) } - .to raise_error( - ForestAdminDatasourceToolkit::Exceptions::ForestException, - "Cannot exclude 'holder_id' from the search: the search does not read it" - ) + # Refusing it would stop the agent booting the day a column named defensively turns + # unsearchable — the one configuration that only became more correct. + it 'reports rather than refuses excluding a column the search never reads' do + logger = instance_double(ForestAdminAgent::Services::LoggerService, log: nil) + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) + + expect { decorated.replace_search(exclude_fields: ['holder_id']) }.not_to raise_error + expect(logger).to have_received(:log).with( + 'Debug', + "Excluding 'holder_id' from the search on cards changes nothing: the search does not read it" + ) + end + + it 'still raises on a name that resolves to nothing, which is a typo' do + expect { decorated.replace_search(exclude_fields: ['holder_ids']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /holder_ids/) end it 'refuses searching and excluding the same path, rather than letting one win silently' do