feat(datasource-customizer): let replace_search take a field selection so a narrowed search is permission-checked - #382
Conversation
3 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| # 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) |
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (5)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
…on, and stop 403ing a blank search 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 <noreply@anthropic.com>
…n so a narrowed search is checked
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…able field
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 <noreply@anthropic.com>
…on, and stop 403ing a blank search 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 <noreply@anthropic.com>
…h field selection 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 <noreply@anthropic.com>
`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) <noreply@anthropic.com>
4917da5 to
540ea0b
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| def collect_search_usages(collection, search, search_extended, usages) | ||
| return if search.nil? || !collection.respond_to?(:searched_fields) | ||
| # The stack discards a blank search instead of running it, so there is nothing to authorize. | ||
| return if search.nil? || search.strip.empty? |
There was a problem hiding this comment.
🟠 High services/permissions.rb:334
A numeric search value such as 1234 raises NoMethodError in collect_search_usages instead of reaching the searchable collection or producing the route's request error. QueryStringParser.parse_search deliberately preserves non-string values, so only apply the blank-search check to strings.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb around line 334:
A numeric `search` value such as `1234` raises `NoMethodError` in `collect_search_usages` instead of reaching the searchable collection or producing the route's request error. `QueryStringParser.parse_search` deliberately preserves non-string values, so only apply the blank-search check to strings.
There was a problem hiding this comment.
Real bug, fixed at the source instead.
.strip is applied in three places — permissions.rb:334, search_collection_decorator.rb:36 and :103 — so guarding only the first would move the NoMethodError one layer down rather than remove it. parse_search now coerces the term and rejects a value that cannot be one (array, hash, boolean) with a BadRequestError, which closes all three.
On the premise: the spec pinning parse_search returning 1234 is titled "converts the query search parameter as string", so preserving the Integer was a bug its own name contradicted rather than a deliberate contract. The assertion now matches the title.
Fixed in ca75407.
|
One bug found, and it was in the addition this PR is built around. Narrowing it exposed a second problem worth naming, because it is the shape that hides this class of bug: Also fixed: the refusal now sits behind One thing deliberately reverted from the review's advice: the rubocop 840/0, and the twelve suites green on seed 1 (agent 1200, customizer 737, toolkit 483, rpc 169). |
`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) <noreply@anthropic.com>
| 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' |
christophebrun-forest
left a comment
christophebrun-forest
left a comment
There was a problem hiding this comment.
Read the full diff and ran the sharp parts rather than only re-reading them: bundle exec rubocop clean over the 236 lib files, forest_admin_datasource_customizer 737 examples / 0 failures, forest_admin_agent 1203 / 0 — the numbers in the description hold. The probes quoted below were run on a detached worktree, one against origin/main, one against ca75407.
The core of the feature is good. Unifying the footprint and the condition tree behind searchable_fields makes the invariant structural rather than only tested, and the spec comparing searched_fields to condition_tree.projection — numeric term included — is the right test for exactly that. Strict resolution at boot beats node's silent drop, and the end-to-end example through a booted customizer stack is what stops the search_handler? delegation from disappearing without a failing spec.
Four comments are inline (1 on parse_search, 2 on assert_extended_search_checkable, 5 on the DSL doc block, 6 on resolved_field). Three have no line to anchor to:
3. The "Behavioural change to be aware of" paragraph is not what ships
ConditionTreeFactory.group compacts before it counts, so union([nil, nil]) already returns ConditionTreeBranch('Or', []) — which is precisely what default_replacer's .map produced on main whenever a term matched no column. Probed on origin/main (v1.41.0), collection with Number columns only, search "abc":
MAIN: searchable=true condition_tree=#<ConditionTreeBranch @aggregator="Or", @conditions=[]>
And an empty Or is not "match none" downstream. ActiveRecord's apply_condition_tree iterates zero conditions and hands the query back untouched — probed on ca75407:
SQL FOR match_none => SELECT "cars".* FROM "cars"
Snowflake does the same (return nil if fragments.empty?), Mongoid emits {"$or" => []} which the server rejects, and Hasura is the only one that reads it as false.
So b16d759d changes behaviour only where searchable_fields is itself empty, and on the default datasource that case is unobservable. Suggest rewording the paragraph to what the commit actually does: the decorator now answers match_none instead of nil, and the datasources still have to translate an empty branch (point 7).
To be explicit, since the finding could read as an argument against the commit: keep it. filter_map without the match_none line would be a genuine regression — the first case above would fall back to union([]) -> nil -> every row — and the commit also carries the String normalisation of paths and implements_search?. The two lines belong together.
4. Make the breaking change discoverable before the first 403
An agent installing a replace_search block now answers 403 on an extended search, shipped in a minor. As it stands the customer learns this from a user hitting the 403. A logger.log('Warn', ...) at boot, naming the collection and pointing at the include_fields: form, is a few lines and changes what kind of event this is for them.
7. Separate ticket, and it outranks this PR: translate an empty branch in the datasources
Or + [] -> false, And + [] -> true. ActiveRecord: @query.none; Snowflake: 1 = 0; Mongoid: do not emit $or: []. Hasura is already correct.
This is pre-existing and live on main today — the first probe above is an ordinary user typing a word into the search bar of a numeric-only collection and getting every row — and it reaches well past search: match_ids([]) returns the same match_none, so a bulk delete or a non-global smart action on an empty selection builds a query with no WHERE on ActiveRecord. Worth opening whatever happens to this PR; it is also what will make b16d759d's promise true.
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
|
The three points with no line to anchor to. 3 — reworded in the description. The conclusion holds: that paragraph described a bug that was not there, and the commit should stay. One correction to the reasoning, because it changes what the paragraph should now say: an empty 4 — done in b692b1f. A block replacer logs a 7 — worth the ticket, but the blast radius needs correcting before it is prioritised. The translator half is real and reproduces as described. The consequence does not:
So the ticket is defence in depth — plus Mongoid, which would raise rather than over-match — not a live data-loss path. Filed as the latter it will read as an incident. One genuine gap in that decorator while it is in scope: |
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
|
All six actionable points are on the branch, and one of them cost more than it looked. Point 4 was the expensive one — the boot warning I added for it crashed the RPC agent. Your wording objection applied to the warning itself, too: it said "will be refused" while the refusal is also gated on Point 6 went further than you suggested, so push back if you disagree. A bare to-one relation is accepted and drops every path through it, as you proposed. But relaxing the check also started accepting names that can never match anything — Two of your own points landed in the specs rather than the code. The list in "every searchable field is excluded" named Points 1, 2, 5 are in 7f7bf0b and b692b1f, answered in their threads. Point 1 needed one thing beyond testing Point 7 stays yours. One thing to fold in while you are there: rubocop 840/0; 12 packages, 4254 examples, 0 failures on seed 1. |
christophebrun-forest
left a comment
christophebrun-forest
left a comment
There was a problem hiding this comment.
Re-reviewed at d3378a3f. Verified as CI runs it: bundle exec rubocop 840 files / no offenses, forest_admin_datasource_customizer 743, forest_admin_agent 1214, forest_admin_datasource_toolkit 485 — 0 failures.
I was wrong about point 3, and the correction matters
I claimed match_none was not honoured end-to-end, and that an ordinary search matching no column already returned every row on main. Both are false, and your EmptyCollectionDecorator note is right. It sits below the search layer and or_return_empty_set starts with conditions.empty?, so the tree is intercepted before any translator sees it. Probed through a booted customizer stack, numeric-only collection, search "abc", child list stubbed to return two records:
PROBE list result => []
The child's list is never consulted. So b16d759d does exactly what its message says, in the only case it changes (searchable_fields itself empty, where union([]) answered nil and nothing intercepted a nil), and it lands. My probe on the AR query builder was real but measured a layer nothing reaches through the stack — I read Or [] arriving at the translator, which it never does.
That also demotes point 7: the bulk-delete-with-empty-selection scenario I described goes through EmptyCollectionDecorator#delete too, so it is not the live bug I called it. A latent trap in the translators, worth the separate ticket you have, and it does not outrank this PR. Sorry for the misdirection.
What the round turned up
Three comments inline. The first is a real one, one method away from the fix you just made.
Also worth saying: the holder_note fixture is the right instinct — a prefix match with no sibling column to distinguish it is a test that cannot fail — and pinning count/csv on the flag's value rather than its presence caught a genuine hole, since the refusal could have been switched off on the export route with the suite green. Same for moving the search_handler? example into the package that owns it.
| # 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 | ||
| logger = ForestAdminAgent::Facades::Container.logger |
There was a problem hiding this comment.
This guard is right, and the reasoning in c4f3e790 is right — but the same file still calls the same facade unguarded at :272, inside get_fields:
ForestAdminAgent::Facades::Container.logger.log(
'Debug',
"We're not searching through #{self.name}.#{name} because it's a polymorphic relation. ..."
)So the crash you just fixed for the boot path is still live on the request path. An RPC agent doing an extended search on a collection carrying a polymorphic relation reaches it. Probed at d3378a3f, logger stubbed to nil:
PROBE RAISED: NoMethodError: undefined method 'log' for nil
Pre-existing rather than introduced here, but the file now has one guarded and one unguarded call to the same nil-able facade, and this commit's own message is the argument for guarding it. logger&.log matches what context_variables.rb:22, csv_generator_stream.rb:54 and the whole of datasource_rpc already do.
There was a problem hiding this comment.
Taken in fc2ffa4 rather than left to you — your Slack said you would do it before merge, but it is the same file I was editing for the exclusion point, so doing it in parallel would have put us on the same lines.
You are right that this is the live half: the boot path was what I guarded, and get_fields is the request path. The three calls in this file now all handle nil. A spec pins it with Container.logger stubbed to nil and an extended search reaching the polymorphic relation, and removing the &. fails it.
Two more in the same package are untouched, being outside this PR: publication_collection_decorator.rb:68 and compute_collection_decorator.rb:20. Same facade, same failure mode on an RPC agent.
| raise ForestException, "Cannot exclude '#{path}' from the search: the search does not read it" | ||
| end | ||
|
|
||
| def excludable?(path, schema) |
There was a problem hiding this comment.
Requiring searchable_field? here means an exclusion can turn into a boot failure, and it is the conservative configuration that breaks.
exclude_fields: ['ssn', 'internal_token'], written to keep PII out of the search bar, boots today. Let a schema change make one of those columns unsearchable — a type change to Json, an operator the datasource stops declaring — and the agent stops booting on the search does not read it, even though the developer's intent ("never search this") is now satisfied more completely than before. Nothing about that config became wrong; a column it names became less searchable.
The distinction I would draw is between the two failures you are currently treating alike:
- a name that resolves to nothing — a typo — should raise, as it does now, and for
include_fieldsthe same is true of an unsearchable column, because there the intent genuinely fails; - a name that resolves to a real field the search happens not to read is a no-op that still states intent, and stays correct as the schema moves. Accepting it, with a
Debugline if the silence is what bothered you, keeps the failure mode proportionate.
Your argument in c4f3e790 — "excluding it silently changes nothing, the configuration lied" — holds for the moment it is written. It stops holding the moment the schema moves under a config that was defensive on purpose.
There was a problem hiding this comment.
You are right, and I changed it even though you said it was fine as-is — the argument stands on its own.
The failure mode was disproportionate and it selected for the wrong configuration: exclude_fields: ['ssn'] boots today, a type change makes that column unsearchable, and the agent stops booting over a config whose intent just became more true. Nothing about it was wrong.
Your distinction is the one implemented in fc2ffa4: a name resolving to nothing is a typo and still raises, out of get_field_schema. A name resolving to a real field the search happens not to read is accepted and reported at Debug, so the silence that prompted the guard is gone without the boot failure. A path both selected and excluded keeps raising — a contradiction rather than a no-op, and no precedence between them is worth documenting.
| nil | ||
| end | ||
|
|
||
| return subset[key] if subset.is_a?(Hash) && !subset[key].nil? |
There was a problem hiding this comment.
Small one, on the same principle this helper was written for: '' in the subset body still wins over the query string.
PROBE body '' => ""
An explicit null no longer discards the term the URL carried, because dropping a search widens the result set — but an explicit '' does exactly that and is honoured. Reading '' as "explicit empty search" and null as "absent" is defensible; I just could not tell from the code whether it was decided or fell out of nil?. Worth a word either way, and it is academic today since no route reaches this branch — nothing calls parse_search with a body, get_record_selection builds its own filter and never applies the search its comment claims to.
There was a problem hiding this comment.
It fell out of nil?, it was not decided — so you found a real inconsistency rather than a documented choice. Fixed in fc2ffa4: '' is read as absent too.
The reason for the null case applies unchanged: discarding the term the URL carried widens the result set, and honouring an empty body value did exactly that. One rule now, stated once in the helper.
Agreed it is academic today — nothing calls parse_search with a body, and get_record_selection builds its own filter without applying the search its comment claims.
christophebrun-forest
left a comment
christophebrun-forest
left a comment
There was a problem hiding this comment.
- 1 important before merge: logging not retained
- 1 to validate: excludable, and okay to send as is
- 1 optional: the ''
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) <noreply@anthropic.com>
# [1.42.0](v1.41.0...v1.42.0) (2026-09-02) ### Features * **datasource-customizer:** let replace_search take a field selection so a narrowed search is permission-checked ([#382](#382)) ([add4b1b](add4b1b))
|
🎉 This PR is included in version 1.42.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
4 participants
fixes PRD-1078
Ports agent-nodejs #1852 and the refusal it narrows, #1840, so ruby ends up with node's search-permission behaviour rather than half of it.
Why
replace_searchtook a block only. A block picks its own fields, sosearched_fieldsanswersnilandcollect_search_usagesreturned without checking anything — a customized search was read entirely unchecked, extended included.That is not the same gap node had, and it is worth stating plainly because it inverts the framing. In node,
getSearchedFieldsreturningnullalready caused an extended search to be refused; #1852 exists to un-refuse the narrowable case. In ruby nothing was refused and nothing was checked. So this PR does both halves:replace_searchreplace_search(block)replace_search(include_fields:)replace_search, natively searchable childThe check is at collection granularity — there are no field-level permissions — so a selection naming only root columns can never 403; only relation paths can.
What
replace_searchnow also takes a field selection:Keywords rather than a hash, so a misspelt key raises where a hash would have carried it silently — the runtime equivalent of what
tscgives node. A block and a selection together, or neither, are refused rather than resolved by precedence.extendedis deliberately not a key: that flag is the caller's and comes from the request, so a customization must not pin it.The part worth reviewing
Both derivations are unified behind
searchable_fields, whichrefine_filterandsearched_fieldsnow share. A path the search reads without appearing in the footprint is a column read unchecked, so the two must not be allowed to drift. That is pinned in four specs against the paths the generated condition tree actually reads — include / exclude / only, plus a numeric term that matches more columns than a word.It is also structural rather than only tested:
enumerable_search?ishandler.nil? && implements_search?, andimplements_search?is the very predicaterefine_filterbranches on. The condition is spelt once.Strict field-name resolution
Names resolve through
Utils::Collection.get_field_schema, raising when the customization is applied. Node drops an unresolvable name silently, which empties the searchable set — that was raised in review on #1852 and left as pre-existing there; ruby has no such legacy. Resolution also refuses a path crossing anything but a to-one relation, so a polymorphic one is reported instead of silently skipped.Found by reviewing this branch adversarially
Probing the implementation rather than re-reading it turned up one sharp bug, in the second commit's own feature surface:
A search with no searchable field returned every row instead of none.
union([])answersnil, andintersectdrops a nil branch, so the filter came back carrying neither a condition nor a search. A customization meant to narrow widened the result to everything — reachable asonly_fields: [], orexclude_fieldscovering every searchable column.It was reachable with no customization at all too, on a collection whose columns are all unsearchable, and two existing specs pinned it while contradicting themselves in their own titles:
nilis not "return no record", it is "no restriction". Both now assertmatch_none, which is what their names always claimed, what the enum-value-not-found case a few lines above already returned, and what node returns here.What this actually changes (the paragraph here previously claimed such a search returned every row, which it did not):
ConditionTreeFactory.groupcompacts before it counts, so the old.mapalready producedConditionTreeBranch('Or', [])whenever a term matched no column — andEmptyCollectionDecorator, which sits below this layer and interceptslist/aggregate/update/delete, already reads an emptyOras match-none. So 0 rows came back onmaintoo.The commit replaces
nilwith an explicitmatch_noneand states the intent in code rather than in a comment. Behaviour changes only wheresearchable_fieldsis itself empty —only_fields: [], or anexclude_fieldscovering every searchable column — whereunion([])answerednilandintersectdropped it, widening the result to everything. Keep it:filter_mapwithout thematch_noneline would be a genuine regression, and the same commit carries theStringnormalisation of paths andimplements_search?.Worth knowing that the safety lives in
EmptyCollectionDecorator, not in the tree: the datasource translators still mistranslate an empty branch (ActiveRecord emits noWHERE, Snowflake the same, Mongoid emits$or: []which the server rejects; Hasura is correct). Nothing reaches them through the decorator stack, but that is a latent trap and it is being tracked separately.Also from the same pass: a symbol field name raised
NoMethodError: undefined method 'include?' for an instance of Symbolfrom inside the toolkit, defeating the point of resolving strictly to get a clear message. Paths are normalised to strings.Breaking in practice
An agent whose collection installs a
replace_searchblock now answers 403 on an extended search there, where it was served. Converting that block to a field selection restores it, checked per path.The refusal is scoped to a callable replacer. A natively searchable child — Zendesk declares one per collection, RPC mirrors whatever the remote agent declares — and a collection with no search decorator at all are served as before: they answer
nilbecause nothing in this stack builds the condition, not because a customization chose the fields.Shipped as a
fixrather than a major bump, matching how node shipped the same policy (fix(agent): refuse an extended search the stack cannot describe).Not in scope
refine_filter— out of proportion, same conclusion as node.refine_filter(caller, nil)raisesNoMethodError: the guard evaluatesfilter.overridewhenfilteris nil. Pre-existing, node hasfilter?.override. Untouched here because it is unrelated to this ticket and fixing it changes a behaviour (nil vs raise).Verified as CI runs it
bundle exec rubocopat the root: 840 files, no offensesBUNDLE_GEMFILE=Gemfile-test bundle exec rspec:forest_admin_datasource_customizer724,forest_admin_agent1179,forest_admin_datasource_toolkit478,forest_admin_datasource_rpc168 — 0 failuresEvery new guard was mutation-checked: disabling the extended-search refusal fails 3 specs, and reverting
enumerable_search?fails the end-to-end permission spec, so none of them pass vacuously.Definition of Done
General
Security
🤖 Generated with Claude Code
Note
Let
replace_searchaccept field selections and permission-check narrowed searchesreplace_searchDSL to accept field-selection arguments (include_fields,exclude_fields,only_fields) alongside the existing block form, with validation that rejects mixing block and selections, empty lists, or combiningonly_fieldswith include/exclude.SearchCollectionDecoratornow resolves and validates field selections at configuration time, builds a condition tree from the selected searchable fields, and reports accurate field footprints to the permissions layer viasearched_fields.Permissions.collect_search_usages) uses the footprint to permission-check selection-based extended searches; when a collection has a custom callable handler (search_handler?) and a permission system is enabled, extended searches raiseForbiddenErrorsince fields cannot be enumerated.parse_searchnow rejects non-string/numeric search values withBadRequestErrorand coerces numeric values to strings;parse_search_extendedcorrectly treatsfalse,0,'false', and empty string as falsy and lets action payload values override query string values.Changes since #382 opened
ForestAdminAgent::Utils::QueryStringParser.parse_searchto prioritize body search values over query string parameters, validate non-nil search values for searchability, and reject non-String/non-Numeric types including booleans [7f7bf0b]ForestAdminAgent::Services::Permissions.assert_extended_search_checkableto checkdescribes_own_search?beforepermission_system?[7f7bf0b]ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.searchable_fieldsto exclude entire relation trees when a relation name appears inexclude_fields, rather than only exact field name matches, and added private helper methodsexcluded?andexcluded_fieldto support hierarchical path exclusion [b692b1f]ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.assert_selection_resolvesto acceptexclude_fieldsentries that exist as field schemas without requiring them to be searchable or strictly resolvable [b692b1f]ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.replace_searchwhen a callable handler is configured, implemented via new private methodwarn_extended_search_refused[b692b1f]ForestAdminDatasourceCustomizer::CollectionCustomizerto clarify name resolution order, interaction withonly_fields, and extended search behavior with included relations [b692b1f]searched_fields_spec.rbfor warning logging with callable handlers, validation of non-searchable excluded fields, and hierarchical relation exclusion behavior [b692b1f]ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator[d3378a3]ForestAdminAgent::Utils::QueryStringParser[d3378a3]forest_admin_agentandforest_admin_datasource_customizerpackages [d3378a3]ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.excluded_fieldto log a debug message instead of raising an exception when excluding a field that the search does not read, and updated bothexcluded_fieldandget_fieldsmethods to use safe navigation when accessing the logger to prevent errors when no logger is configured [fc2ffa4]ForestAdminAgent::Utils::QueryStringParser.subset_or_queryto treat empty strings in the subset query as absent values, allowing query string parameters to take precedence [fc2ffa4]Macroscope summarized ca75407.