From adeb5499fae69ec491b8e9fa012f982517924733 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Wed, 29 Jul 2026 22:52:26 +0200 Subject: [PATCH] Reject params nested deeper than the array scope declares `AttributesIterator#do_each` recursed into any element that was an Array. That recursion is required when the declaration itself nests array scopes -- `map_params` adds one level of nesting per element-iterating scope on the chain, so the params for the inner scope really are an array of arrays -- but nothing compared the incoming depth against the declared one. A request could therefore wrap its elements in extra arrays and have them silently unwrapped: params do requires :lines, type: Array do requires :book_id, type: String requires :qty, type: Integer end end `{"lines":[[{"book_id":"x","qty":1}]]}` passed validation, and `params[:lines]` / `declared` then handed the endpoint `[[{...}]]`. Any ordinary body assuming the declared shape (`params[:lines].sum { |l| l[:qty] }`) died with a `TypeError`, i.e. a 500 on malformed input. `[[]]` passed the same way. Each scope now records, at definition time, how many element-iterating scopes sit on its chain; the iterator descends that many levels less the one `Array.wrap` already consumes, and yields anything deeper as-is. The attribute validators then see a non-hash and fail it exactly as they do for any other unexpected element type, so these requests get the 400 they always should have. "Element-iterating" is a scope predicate rather than an `== Array` test, because `type: Array[JSON]` iterates elements just as much but evaluates to the Array *instance* `[JSON]` rather than the Array class. Excluding it also cost `Array[JSON]` its error indices: every element reported under the same bracket-less name, so `docs[1][name] is missing` came out as `docs[name] is missing`, and two failing elements deduped into one message. Sharing the predicate fixes that too -- `Array[JSON]` now reports per element exactly as `type: Array do` does. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 + lib/grape/validations/attributes_iterator.rb | 18 +++- lib/grape/validations/params_scope.rb | 26 ++++- spec/grape/validations/params_scope_spec.rb | 95 +++++++++++++++++++ .../validators/coerce_validator_spec.rb | 32 ++++++- 5 files changed, 165 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0049465b9..d6e57e613 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,8 @@ * [#2827](https://github.com/ruby-grape/grape/pull/2827): Make the `cascade` DSL getter return the configured value (`cascade false` read back as `true`) - [@ericproulx](https://github.com/ericproulx). * [#2829](https://github.com/ruby-grape/grape/pull/2829): Fix a cascading route handing over only to the last route registered for the path, making a middle version (3+ mounted versions with a catch-all) answer 406 - [@ericproulx](https://github.com/ericproulx). * [#2826](https://github.com/ruby-grape/grape/pull/2826): Fix `api.version` not being set for the root route of a path-versioned API (`GET /v1`) - [@ericproulx](https://github.com/ericproulx). +* [#2834](https://github.com/ruby-grape/grape/pull/2834): Restore the #2824 fix for cascaded routes leaking `route_info` and path captures, silently reverted by #2829 - [@ericproulx](https://github.com/ericproulx). +* [#2838](https://github.com/ruby-grape/grape/pull/2838): Reject request params nested in more arrays than the block declares, instead of silently unwrapping them and passing validation, and report `type: Array[JSON]` errors against the element that failed - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 3.3.5 (2026-07-30) diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index 578ca3fab..dbc119d27 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -10,6 +10,11 @@ class AttributesIterator def initialize(attrs, scope) @attrs = attrs @scope = scope + # How many times #do_each may descend into a nested array. The + # declaration allows one level per Array-typed scope on the chain, less + # the one +Array.wrap+ already consumes in #each. Anything deeper was + # put there by the request, not by the declaration. + @max_nesting = [scope.array_depth - 1, 0].max end def each(params, &) @@ -24,19 +29,24 @@ def do_each(params_to_process, original_params, parent_indices = [], &block) params_to_process.each_with_index do |resource_params, index| # when we get arrays of arrays it means that target element located inside array # we need this because we want to know parent arrays indices - if resource_params.is_a?(Array) + # + # Only descend as far as the declaration nests. A request that wraps + # its elements deeper than that is yielded as-is, so the attribute + # validators see a non-hash and fail it the same way any other + # unexpected element type does. + if resource_params.is_a?(Array) && parent_indices.size < @max_nesting do_each(resource_params, original_params, [index] + parent_indices, &block) next end - if @scope.type == Array + if @scope.iterates_elements? next unless original_params.is_a?(Array) # do not validate content of array if it isn't array store_indices(@scope, index, parent_indices) elsif original_params.is_a?(Array) # Lateral scope (no @element) whose params resolved to an array — - # delegate index tracking to the nearest array-typed ancestor so - # that full_name produces the correct bracketed index. + # delegate index tracking to the nearest element-iterating ancestor + # so that full_name produces the correct bracketed index. target = @scope.nearest_array_ancestor store_indices(target, index, parent_indices) if target end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index dcfd44095..8c810d914 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -3,7 +3,7 @@ module Grape module Validations class ParamsScope - attr_reader :parent, :type, :nearest_array_ancestor, :full_path + attr_reader :parent, :type, :nearest_array_ancestor, :array_depth, :full_path def qualifying_params ParamScopeTracker.current&.qualifying_params(self) @@ -78,6 +78,9 @@ def initialize(api:, element: nil, element_renamed: nil, parent: nil, optional: # configure_declared_params consumes it and clears @declared_params to nil. @declared_params = [] @full_path = build_full_path + # Read by the validators instantiated from the block below, so it has to + # be settled before the instance_eval. + @array_depth = find_array_depth instance_eval(&block) if block @@ -169,6 +172,17 @@ def nested? @parent && @element end + # Whether this scope's params resolve to one entry per element, which is + # what makes both an element index and a nesting level meaningful. + # + # +type: Array[JSON]+ counts as much as +type: Array+ does. It is easy to + # miss because it evaluates to the Array *instance* +[JSON]+ rather than + # the Array class, so an +== Array+ test quietly excluded it. + # @return [Boolean] + def iterates_elements? + @type == Array || @type == SPECIAL_JSON.last + end + # A lateral scope is subordinate to its parent, but its keys are at the # same level as its parent and thus is not contained within an element. # @return [Boolean] whether or not this scope is lateral @@ -320,10 +334,18 @@ def configure_declared_params def find_nearest_array_ancestor scope = @parent - scope = scope.parent while scope && scope.type != Array + scope = scope.parent while scope && !scope.iterates_elements? scope end + # Every element-iterating scope on the chain adds one level of nesting to + # what {#params} returns, because +map_params+ maps over the array it + # resolved from the parent. Counting them tells {AttributesIterator} how + # deep the declaration says the params for this scope may legitimately be. + def find_array_depth + (iterates_elements? ? 1 : 0) + (@parent&.array_depth || 0) + end + def validates(attrs, validations) process_oneof!(validations) if validations.key?(:oneof) spec = ValidationsSpec.from(validations) diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index a6733a772..b1df98d1e 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -761,6 +761,101 @@ def initialize(value) end end + context 'when the request nests its arrays deeper than the declaration' do + before do + subject.params do + requires :lines, type: Array do + requires :name, type: String + end + end + subject.post('/lines') { 'ok' } + end + + it 'accepts the declared shape' do + post '/lines', { lines: [{ name: 'x' }] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(201) + end + + # Without this the elements are silently unwrapped, validation passes, and + # the endpoint receives an Array where it declared a Hash. + it 'rejects elements wrapped in an extra array' do + post '/lines', { lines: [[{ name: 'x' }]] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('lines[0][name] is missing, lines[0][name] is invalid') + end + + it 'rejects elements wrapped in several extra arrays' do + post '/lines', { lines: [[[{ name: 'x' }]]] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + end + + it 'rejects an empty array element' do + post '/lines', { lines: [[]] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + end + end + + context 'when an array is declared inside a hash' do + before do + subject.params do + requires :outer, type: Hash do + requires :inner, type: Array do + requires :leaf, type: String + end + end + end + subject.post('/hash_array') { 'ok' } + end + + it 'accepts the declared shape' do + post '/hash_array', { outer: { inner: [{ leaf: 'x' }] } }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(201) + end + + it 'rejects elements wrapped in an extra array' do + post '/hash_array', { outer: { inner: [[{ leaf: 'x' }]] } }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + end + end + + context 'when arrays are nested in the declaration' do + before do + subject.params do + requires :a, type: Array do + requires :b, type: Array do + requires :c, type: String + end + end + end + subject.post('/nested_arrays') { 'ok' } + end + + it 'still descends as deep as the declaration nests' do + post '/nested_arrays', { a: [{ b: [{ c: 'x' }] }] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(201) + end + + it 'reports errors against the inner elements' do + post '/nested_arrays', { a: [{ b: [{}] }] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('a[0][b][0][c] is missing') + end + + it 'rejects one level deeper than declared' do + post '/nested_arrays', { a: [{ b: [[{ c: 'x' }]] }] }.to_json, 'CONTENT_TYPE' => 'application/json' + + expect(last_response.status).to eq(400) + end + end + context 'array without given' do before do subject.params do diff --git a/spec/grape/validations/validators/coerce_validator_spec.rb b/spec/grape/validations/validators/coerce_validator_spec.rb index 50083c16a..263e491f4 100644 --- a/spec/grape/validations/validators/coerce_validator_spec.rb +++ b/spec/grape/validations/validators/coerce_validator_spec.rb @@ -1011,13 +1011,41 @@ def self.parse(_val) expect(last_response).to be_successful expect(last_response.body).to eq("#{integer_class_name}.#{integer_class_name}") + # A bare object is coerced into a one-element array, so it reports + # against that element the same way an explicit array does. get '/', splines: '{"x":"4","y":"woof"}' expect(last_response).to be_bad_request - expect(last_response.body).to eq('splines[x] does not have a valid value') + expect(last_response.body).to eq('splines[0][x] does not have a valid value') get '/', splines: '[{"x":"4","y":"woof"}]' expect(last_response).to be_bad_request - expect(last_response.body).to eq('splines[x] does not have a valid value') + expect(last_response.body).to eq('splines[0][x] does not have a valid value') + end + + it 'reports Array[JSON] errors against the element that failed' do + subject.params do + requires :splines, type: Array[JSON] do + requires :x, type: Integer + end + end + subject.get('/') { 'ok' } + + get '/', splines: '[{"x":1},{},{"x":3}]' + expect(last_response).to be_bad_request + expect(last_response.body).to eq('splines[1][x] is missing') + end + + it 'reports every failing Array[JSON] element, not just one' do + subject.params do + requires :splines, type: Array[JSON] do + requires :x, type: Integer + end + end + subject.get('/') { 'ok' } + + get '/', splines: '[{},{}]' + expect(last_response).to be_bad_request + expect(last_response.body).to eq('splines[0][x] is missing, splines[1][x] is missing') end it "doesn't make sense using coerce_with" do