diff --git a/CHANGELOG.md b/CHANGELOG.md index 0049465b9..ce3e03407 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ * [#2837](https://github.com/ruby-grape/grape/pull/2837): Document the definition-time rejection of an `Array`/`Set` of an uncoercible element type introduced by #2817, and pin it with specs - [@ericproulx](https://github.com/ericproulx). * [#2836](https://github.com/ruby-grape/grape/pull/2836): Define `#hash` alongside the `eql?`/`==` pairs on `Grape::Endpoint`, `Grape::Util::InheritableSetting`, `Grape::Middleware::Stack::Middleware`, `Grape::ServeStream::StreamResponse` and `Grape::ServeStream::FileBody`, so equal objects hash alike in a `Hash`, `Set` or `uniq` - [@ericproulx](https://github.com/ericproulx). * [#2835](https://github.com/ruby-grape/grape/pull/2835): Return the compiled instance from `Grape::API::Instance.compile!` so `call` and `recognize_path` no longer re-read `@instance`, which a concurrent `change!` could nil between the two reads - [@ericproulx](https://github.com/ericproulx). +* [#2849](https://github.com/ruby-grape/grape/pull/2849): Remove `http_digest`, which has had no strategy behind it since 2.0.0 and raised on the first request rather than when the API was defined (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes @@ -55,6 +56,17 @@ * [#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). +* [#2839](https://github.com/ruby-grape/grape/pull/2839): Tag path params as UTF-8 instead of leaving them ASCII-8BIT, so they compare equal to the non-ASCII literals an API declares (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). +* [#2840](https://github.com/ruby-grape/grape/pull/2840): Answer 500 instead of letting an exception escape the middleware stack when an error response cannot be rendered (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). +* [#2841](https://github.com/ruby-grape/grape/pull/2841): Stop `use`, `helpers`, `rescue_from` and other registrations declared below a route from reaching it when an earlier registration had seeded the same key (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). +* [#2842](https://github.com/ruby-grape/grape/pull/2842): Warn at definition time when a `rescue_from` class is already covered by one registered earlier in the same scope, since the later handler never runs - [@ericproulx](https://github.com/ericproulx). +* [#2843](https://github.com/ruby-grape/grape/pull/2843): Let `rescue_from :grape_exceptions` take precedence over a catch-all registered as a class, so Grape errors keep their own status (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). +* [#2844](https://github.com/ruby-grape/grape/pull/2844): Look an entity up by the presented object's own class before treating it as a collection, so `represent` is no longer skipped for models that respond to `#first` or `#klass` - [@ericproulx](https://github.com/ericproulx). +* [#2845](https://github.com/ruby-grape/grape/pull/2845): Render a `redirect` message as the plain text its content type announces, instead of letting the API's formatter re-encode it - [@ericproulx](https://github.com/ericproulx). +* [#2846](https://github.com/ruby-grape/grape/pull/2846): Keep a `:version` path capture in `params` when the API declares no version, instead of always dropping it as Grape's own - [@ericproulx](https://github.com/ericproulx). +* [#2847](https://github.com/ruby-grape/grape/pull/2847): Match `Accept` media types case-insensitively, so a differently-cased header still negotiates the content type and resolves a vendor version - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 3.3.5 (2026-07-30) diff --git a/README.md b/README.md index efae8b900..f90cd37c9 100644 --- a/README.md +++ b/README.md @@ -2737,6 +2737,19 @@ rescue_from :grape_exceptions do |e| end ``` +The opt-in takes precedence over a catch-all handler, whether that catch-all is written as `rescue_from :all` or as a class such as `rescue_from StandardError`. So a validation failure stays a `400` rather than becoming whatever the catch-all returns: + +```ruby +class Twitter::API < Grape::API + rescue_from StandardError do + error!('server error', 500) + end + rescue_from :grape_exceptions # validation errors still answer 400 +end +``` + +A handler registered for a specific Grape exception class is more precise than the opt-in and still wins, so `rescue_from Grape::Exceptions::ValidationErrors` keeps its own handler. The opt-in only outranks handlers that matched through a non-Grape ancestor. + You can also rescue specific exceptions. ```ruby @@ -2747,6 +2760,17 @@ end In this case ```UserDefinedError``` must be inherited from ```StandardError```. +When several classes could match, the one registered **first** in a scope wins — as with the clauses of a Ruby `rescue`. Register the more specific class before the broader one, or the narrower handler never runs: + +```ruby +class Twitter::API < Grape::API + rescue_from ArgumentError do ... end # matched first + rescue_from StandardError do ... end # everything else +end +``` + +Grape warns when a `rescue_from` is registered for a class an earlier one in the same scope already covers. This is about ordering within a scope; a handler in a nested namespace or a mounted API always takes precedence over one inherited from an enclosing scope, whatever the classes are. + Notice that you could combine these two approaches (rescuing custom errors takes precedence). For example, it's useful for handling all exceptions except Grape validation errors. ```ruby diff --git a/UPGRADING.md b/UPGRADING.md index ab4838229..f0b9cb076 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,123 @@ Upgrading Grape ### Upgrading to >= 4.0.0 +#### Path params are tagged UTF-8 instead of ASCII-8BIT + +Params captured from the request path — `route_param`, `:id`-style segments, splats — now come back tagged `UTF-8`. They used to carry the `ASCII-8BIT` encoding of Rack's `PATH_INFO`, because Mustermann decodes the path against that raw string and nothing re-tagged the result. Query and body params were already `UTF-8`, since Rack tags those itself. + +**Why UTF-8.** Nothing obliges a client to send it — HTTP treats the request target as octets, and Rack's SPEC has CGI keys carry non-ASCII as `ASCII-8BIT`. UTF-8 is a convention rather than a guarantee. But it is the convention Rack itself already applies to everything *except* the path: `Rack::QueryParser#unescape` decodes the query string and form bodies with `URI.decode_www_form_component(string, Encoding::UTF_8)`, which tags the result without validating it. + +One request carrying the same invalid octets in three places, before this change: + +| source | encoding | `valid_encoding?` | +| --- | --- | --- | +| query — `?q=%C3%28` | `UTF-8` | `false` | +| form body — `form=%C3%28` | `UTF-8` | `false` | +| path — `/%C3%28` | **`ASCII-8BIT`** | `true` | + +The bytes are equally malformed in all three; only the label differed. The path reads `true` merely because `ASCII-8BIT` considers every byte sequence valid. So this change is not Grape adopting an outside convention — it is Grape agreeing with the library handing it the request. The path param was the odd one out only because Mustermann decodes against `PATH_INFO` directly and never had Rack's `unescape` applied to it. + +Grape does exactly what Rack does: re-tag, do not validate. The bytes are untouched, so octets that are *not* UTF-8 stay detectably invalid rather than being scrubbed into something the client never sent. (Rails takes the same approach for path captures — `ActionDispatch::Journey::Router` force-encodes each one to UTF-8 after unescaping.) + +**What this fixes.** An API's own declarations used to disagree with themselves depending on where a value arrived from — a binary string never equals the UTF-8 literal it was written as: + +```ruby +params { requires :id, type: String, values: ['café'] } +``` + +| request | before | 4.0 | +| --- | --- | --- | +| `GET /?id=café` (query) | `200` | unchanged | +| `GET /café` (path) | `400 "id does not have a valid value"` | `200` | + +The same held for `same_as`, `except_values`, and any comparison an endpoint made against a non-ASCII literal. Serialization was affected too: a non-ASCII path param rendered into a JSON response emitted an encoding warning from the `json` gem, and is slated to raise there in json 3.0. + +**What can break.** Only the encoding tag changes; the bytes are untouched, and an invalid byte sequence stays invalid rather than being scrubbed. Comparisons against pure-ASCII strings are unaffected. Code that relied on a path param being binary — concatenating one with genuinely binary data, for instance — can now raise `Encoding::CompatibilityError`, and should call `.b` on the param to opt back into binary: + +```ruby +params[:id].b + binary_blob +``` + +An application that worked around the old behavior with its own `force_encoding(Encoding::UTF_8)` needs no change; that call is now a no-op. +#### A failed error rendering answers 500 instead of escaping the middleware stack + +When Grape could not render an error response — an error formatter handed a payload it cannot serialize, most often — the exception escaped every middleware above Grape and reached the application server. Rendering runs inside `Grape::Middleware::Error#call!`'s own `rescue` clause, so that clause did not cover it. + +Grape now answers `500` instead: first retrying the API's format with the framework's own `Internal Server Error` message, then falling back to a bare `text/plain` body if even that cannot be rendered. + +**What can break.** Code that observed these exceptions by letting them propagate — an error tracker mounted as Rack middleware above Grape, or a test asserting `expect { get '/' }.to raise_error` — no longer sees them. The exception is exposed on the rack env instead, under the same key the existing unrecognised-error path uses: + +```ruby +env[Grape::Env::GRAPE_EXCEPTION] # => the exception that defeated rendering +``` + +Exceptions that no `rescue_from` matches still propagate exactly as before; only rendering failures changed. +#### `use`, `helpers`, `rescue_from` and other registrations no longer reach routes defined above them + +A route captures the middleware, helpers, callbacks and rescue handlers registered above it. That was already true most of the time, but not always: `Grape::Util::InheritableSetting#point_in_time_copy` copied a scope's stackable store and its rescue-handler maps shallowly, so the nested Arrays and Hashes stayed shared with the scope. A registration added *after* an endpoint was defined therefore still reached that endpoint — but only when the key already held at least one registration when the endpoint was defined, since otherwise the scope allocated a fresh store only for itself. + +The outcome depended on something the API never expressed: + +```ruby +class A < Grape::API + use Middleware1 + get('/x') { } # endpoint defined here + use Middleware2 # applied to GET /x +end + +class B < Grape::API + get('/x') { } # endpoint defined here + use Middleware2 # NOT applied to GET /x +end +``` + +`A` and `B` state the same thing and behaved differently. Both now behave like `B`. The same held for `rescue_from` declared below a route. + +**What can break.** An API that declares `use` (or `helpers`, a filter such as `before`, or `rescue_from`) below its routes and relies on it applying to them. That arrangement only ever worked when an earlier registration for the same key happened to seed the stack, so it was never dependable, but code written against it will now see the middleware or helper silently not run. + +**The fix is to move the registration above the routes it should cover**, which is where Grape's documentation has always placed it: + +```ruby +class A < Grape::API + use Middleware1 + use Middleware2 + get('/x') { } +end +``` + +Nothing changes for the ordinary arrangement — registrations declared before a route, or inherited from an enclosing namespace or a mounting API, still apply exactly as before, including values an enclosing scope gains after the nested scope was created. +#### `rescue_from :grape_exceptions` now outranks a catch-all class handler + +`rescue_from :grape_exceptions` is an opt-in to keep Grape's own errors rendering with their own status — a validation failure answers `400` rather than whatever the application's catch-all returns. + +It only ever worked against `rescue_from :all`. Written as a class instead, a catch-all is a *registered* handler, which `Grape::Middleware::Error` consults first, and Grape's exceptions are `StandardError`s — so the opt-in was silently inert: + +```ruby +rescue_from StandardError do + error!('server error', 500) +end +rescue_from :grape_exceptions +``` + +| request | before | 4.0 | +| --- | --- | --- | +| fails parameter validation | **500** `server error` | `400` | +| raises an application error | `500` `server error` | unchanged | + +**What can break.** An API that registers a catch-all as a class *and* opts into `:grape_exceptions` will now answer Grape's own status for Grape's own errors, where it previously answered the catch-all's. That is what the opt-in asks for, so the change makes the two spellings agree — but a client or test asserting the catch-all's status for a validation failure will see the new one. + +Precedence is unchanged in every other case. A handler registered for a specific Grape exception class is more precise than the opt-in and still wins: + +```ruby +rescue_from Grape::Exceptions::ValidationErrors do + error!('unprocessable', 422) # still runs +end +rescue_from StandardError { ... } +rescue_from :grape_exceptions +``` + +Application errors still reach the catch-all, `rescue_from :all` behaves as before, and `Grape::Exceptions::InvalidVersionHeader` is still never rescued, so version cascading keeps working. An API that does not use `rescue_from :grape_exceptions` is unaffected. + #### `Array`/`Set` of an unsupported type is rejected when the API is defined Declaring a collection whose element type Grape cannot coerce — `type: Array[Foo]` or `type: Set[Foo]` where `Foo` is neither a primitive, a structure, nor a valid custom type — now raises as soon as the `params` block is evaluated, i.e. while the API class is being loaded: @@ -304,16 +421,16 @@ rescue_from MyError, with: :other_handler Calls that only use one meta selector or only use exception classes (the documented forms) are unaffected. -#### `auth`, `http_basic` and `http_digest` now take keyword arguments +#### `auth` and `http_basic` now take keyword arguments -`Grape::Middleware::Auth::DSL#auth`, `#http_basic` and `#http_digest` now accept their options as keyword arguments instead of a positional `Hash`. Calls using bare keyword syntax or a block are unaffected: +`Grape::Middleware::Auth::DSL#auth` and `#http_basic` now accept their options as keyword arguments instead of a positional `Hash`. Calls using bare keyword syntax or a block are unaffected: ```ruby http_basic realm: 'API' do |u, p| # ... end -auth :http_digest, realm: 'API', opaque: 'secret', &proc +auth :my_strategy, realm: 'API', &proc ``` Passing a positional options `Hash` still works but is deprecated and will be removed in a future release: @@ -321,13 +438,33 @@ Passing a positional options `Hash` still works but is deprecated and will be re ```ruby # deprecated http_basic({ realm: 'API' }) -auth :http_digest, { realm: 'API', opaque: 'secret' } +auth :my_strategy, { realm: 'API' } # preferred http_basic(realm: 'API') -auth :http_digest, realm: 'API', opaque: 'secret' +auth :my_strategy, realm: 'API' ``` +#### `http_digest` is removed + +`Grape::Middleware::Auth::DSL#http_digest` is gone. Calling it now raises `NoMethodError` while the API class is being defined. + +Nothing it could reach has existed since **2.0.0**, which removed `Rack::Auth::Digest` along with Grape's `:http_digest` strategy ([#2361](https://github.com/ruby-grape/grape/pull/2361)) after Rack 3 dropped digest authentication. The method survived that removal and kept recording its settings happily, so an API declaring `http_digest` still booted — and then raised `Grape::Exceptions::UnknownAuthStrategy` on the *first request*, from inside the middleware build, as an uncaught exception rather than a response. Failing while the class is defined is the point of removing it. + +**If you registered your own `:http_digest` strategy**, it still works; call `auth` directly: + +```ruby +Grape::Middleware::Auth::Strategies.add(:http_digest, MyDigestStrategy, ->(settings) { [settings[:realm]] }) + +class API < Grape::API + auth :http_digest, realm: 'API Authorization', opaque: 'secret' do |username| + # ... + end +end +``` + +The removed method supplied two defaults that `auth` does not, so pass them explicitly if you were relying on them: `realm` defaulted to `'API Authorization'`, and `opaque` to `'secret'` (nested inside `realm` when `realm` was itself a Hash). + #### Middleware options now route through per-class `Options` `Data` value objects `Grape::Middleware::Error`, `Grape::Middleware::Formatter`, and `Grape::Middleware::Versioner::Base` each declare an `Options` `Data.define` and route their `**options` kwargs through it on `initialize`. This means **unknown kwargs now raise `ArgumentError`** instead of being silently swallowed: diff --git a/lib/grape/dsl/entity.rb b/lib/grape/dsl/entity.rb index 9dbb42d99..735753a16 100644 --- a/lib/grape/dsl/entity.rb +++ b/lib/grape/dsl/entity.rb @@ -50,11 +50,39 @@ def present(*args, root: nil, with: nil, **options) # @param object [Object] the object to locate the Entity class for # @return [Class] the located Entity class, or nil if none is found def entity_class_for_obj(object) - klass = object_class(object) + entity_for_class(object.class) || entity_for_class(element_class(object)) + end + + private + + # The class standing in for a collection or wrapper: ActiveRecord::Relation + # and the like expose #klass, anything else falls back to the class of its + # first element. + # + # Consulted only once the object's own class has come up empty, because + # both tests are duck-typed and plenty of single objects answer them — + # a Struct is Enumerable, so it responds to #first, and so does any model + # that includes Enumerable. Asking this first meant `represent Model, + # with: Entity` was silently ignored for those, the entity for the + # *element* type being looked up instead. Deferring it also keeps #first + # from being called at all when the object resolves on its own class. + # + # @param object [Object] the object to represent. + # @return [Class, nil] + def element_class(object) + return object.klass if object.respond_to?(:klass) + + object.first.class if object.respond_to?(:first) + end + + # @param klass [Class, nil] the class to look an entity up for. + # @return [Class, nil] the registered or conventionally named entity. + def entity_for_class(klass) + return if klass.nil? representations = inheritable_setting.representations if representations - potential = klass.ancestors.detect { |potential| representations.key?(potential) } + potential = klass.ancestors.detect { |ancestor| representations.key?(ancestor) } return representations[potential] if potential && representations[potential] end @@ -65,18 +93,6 @@ def entity_class_for_obj(object) entity if entity.respond_to?(:represent) end - private - - # Resolves the class used to look up the Entity for +object+. - # @param object [Object] the object to represent. - # @return [Class] the object's collection element class, wrapped class, or its own class. - def object_class(object) - return object.klass if object.respond_to?(:klass) - return object.first.class if object.respond_to?(:first) - - object.class - end - # @param entity_class [Class] the entity class to use for representation. # @param object [Object] the object to represent. # @param options [Hash] additional options forwarded to the entity's `represent` call. diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 2a48fed36..b46ff99b9 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -52,7 +52,12 @@ def redirect(url, permanent: false, body: nil) body_message ||= "This resource has been moved temporarily to #{url}." end header 'Location', url + # The message is plain text, so say so and render it as such. Setting + # only the header left the body to the API's own formatter, which on a + # JSON API returned the sentence wrapped in quotes under a text/plain + # content type. content_type 'text/plain' + api_format :txt body body_message end diff --git a/lib/grape/middleware/auth/dsl.rb b/lib/grape/middleware/auth/dsl.rb index 52829cd22..5c62673c0 100644 --- a/lib/grape/middleware/auth/dsl.rb +++ b/lib/grape/middleware/auth/dsl.rb @@ -22,19 +22,6 @@ def http_basic(*legacy_options, **options, &) auth(:http_basic, **options, &) end - def http_digest(*legacy_options, **options, &) - options = merge_legacy_auth_options(:http_digest, legacy_options, options) - options[:realm] ||= 'API Authorization' - - if options[:realm].respond_to?(:values_at) - options[:realm][:opaque] ||= 'secret' - else - options[:opaque] ||= 'secret' - end - - auth(:http_digest, **options, &) - end - private # @deprecated Passing a positional options Hash is deprecated; pass diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index f12fa398e..a7db81c9b 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -48,6 +48,13 @@ def initialize( def_delegator :rescue_options, :backtrace, :include_backtrace def_delegator :rescue_options, :original_exception, :include_original_exception + # Emitted by {#failsafe_response} once even the framework's own message + # could not be rendered. Deliberately built without a formatter, an i18n + # lookup or anything else that could be the thing that is broken. + FAILSAFE_STATUS = 500 + FAILSAFE_MESSAGE = '500 Internal Server Error' + FAILSAFE_CONTENT_TYPE = 'text/plain' + def call!(env) @env = env error_response(catch(:error) { return @app.call(@env) }) @@ -87,7 +94,10 @@ def format_message(error) end def find_handler(klass) - registered_rescue_handler(klass) || + registered_entry = registered_rescue_handler_entry(klass) + + grape_exceptions_precedence_handler(klass, registered_entry) || + registered_entry&.last || rescue_handler_for_grape_exception(klass) || rescue_handler_for_any_class(klass) || raise @@ -104,7 +114,45 @@ def error_response(error = nil) backtrace: raw.backtrace || raw.original_exception&.backtrace || [] ) env[Grape::Env::API_ENDPOINT].status(payload.status) # error! may not have been called - rack_response(payload.status, payload.headers, format_message(payload)) + begin + rack_response(payload.status, payload.headers, format_message(payload)) + rescue StandardError => e + failsafe_response(e) + end + end + + # Last resort for an error response that could not be rendered — an error + # formatter handed a payload it cannot serialize, typically. Rendering runs + # inside #call!'s rescue clause, so it is not covered by that rescue and + # anything raised here would escape the entire middleware stack. Grape has + # committed to answering with an error by this point, so it answers with + # one that does not depend on the payload rather than dropping the request. + # + # First retry the API's own format with the framework's InternalServerError, + # whose message is a static string and so cannot be what defeated the first + # attempt. Should even that fail — a wholesale broken formatter, rather than + # one payload it choked on — drop the formatter entirely. Both attempts call + # {#format_message} directly rather than re-entering {#error_response}, so + # this path cannot recurse. + # + # The exception is exposed on the rack env so upstream middleware (loggers, + # error trackers) can still observe what actually went wrong. + def failsafe_response(error) + env[Grape::Env::GRAPE_EXCEPTION] = error + headers = { Rack::CONTENT_TYPE => content_type } + rack_response(FAILSAFE_STATUS, headers, format_message(failsafe_payload(headers))) + rescue StandardError + rack_response(FAILSAFE_STATUS, { Rack::CONTENT_TYPE => FAILSAFE_CONTENT_TYPE }, FAILSAFE_MESSAGE) + end + + def failsafe_payload(headers) + Grape::Exceptions::ErrorResponse.new( + status: FAILSAFE_STATUS, + message: Grape::Exceptions::InternalServerError.new.message, + headers:, + backtrace: [], + original_exception: nil + ) end def default_rescue_handler(exception) @@ -118,16 +166,51 @@ def default_rescue_handler(exception) end def registered_rescue_handler(klass) - rescue_handler_from(base_only_rescue_handlers) { |err| klass == err } || - rescue_handler_from(rescue_handlers) { |err| klass <= err } + registered_rescue_handler_entry(klass)&.last + end + + # The matched entry rather than just its handler, so callers can tell + # *which* class matched — see {#grape_exceptions_precedence_handler}. + # @return [Array(Class, #call), nil] + def registered_rescue_handler_entry(klass) + rescue_handler_entry_from(base_only_rescue_handlers) { |err| klass == err } || + rescue_handler_entry_from(rescue_handlers) { |err| klass <= err } end - def rescue_handler_from(handlers) + def rescue_handler_entry_from(handlers) error, handler = handlers&.find { |err, _handler| yield(err) } return unless error - handler || method(:default_rescue_handler) + [error, handler || method(:default_rescue_handler)] + end + + # +rescue_from :grape_exceptions+ is an opt-in to keep Grape's own errors + # rendering with their own status — a validation failure stays a 400 + # instead of becoming whatever the app's catch-all returns. + # + # It only ever worked against +rescue_from :all+, which lives in + # +all_rescue_handler+ and is consulted last. Spelled as a class instead, + # +rescue_from StandardError+ is a *registered* handler, matched first, + # and Grape's exceptions are StandardErrors — so the opt-in silently did + # nothing and validation errors came back as 500s either way. + # + # Let it win over a handler that only matched through a non-Grape + # ancestor. One registered for a Grape exception class is more specific + # than the opt-in and still wins, so an explicit + # +rescue_from Grape::Exceptions::ValidationErrors+ keeps its handler. + # + # InvalidVersionHeader is left alone: it must keep reaching Rack so the + # next versioned route is tried. + def grape_exceptions_precedence_handler(klass, registered_entry) + return unless rescue_grape_exceptions + return unless klass <= Grape::Exceptions::Base + return if klass == Grape::Exceptions::InvalidVersionHeader + + matched, = registered_entry + return if matched.nil? || matched <= Grape::Exceptions::Base + + grape_exceptions_rescue_handler || method(:error_response) end def rescue_handler_for_grape_exception(klass) diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 0eaf2c9cb..26336d1a1 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -156,11 +156,15 @@ def format_from_extension extension if content_type_for(extension) end + # Media types are case-insensitive (RFC 9110 §8.3.1) but the registered + # ones are spelled in lower case and Rack matches them literally, so an + # `Accept: TEXT/PLAIN` found nothing and fell through to the default + # format — the client quietly got something other than what it asked for. def format_from_header accept_header = try_scrub(env['HTTP_ACCEPT']) return if accept_header.blank? || accept_header == ALL_MEDIA_TYPES - media_type = Rack::Utils.best_q_match(accept_header, mime_types.keys) + media_type = Rack::Utils.best_q_match(accept_header.downcase, mime_types.keys) mime_types[media_type] if media_type end end diff --git a/lib/grape/request.rb b/lib/grape/request.rb index c56237e1d..e016169ed 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -168,8 +168,7 @@ def cookies? def make_params params = @params_builder.call(rack_params) - routing_args = env[Grape::Env::GRAPE_ROUTING_ARGS] - filtered = routing_args&.except(:version, :route_info) + filtered = routing_args_as_params(env[Grape::Env::GRAPE_ROUTING_ARGS]) return params if filtered.blank? params.deep_merge!(filtered) @@ -177,6 +176,31 @@ def make_params raise Grape::Exceptions::RequestError end + # The routing args carry two things that are not request params: + # +:route_info+, which is always Grape's own, and +:version+, which is only + # Grape's own when the API declared a version — that is captured as a path + # segment and exposed through +env['api.version']+ instead. + # + # An API that declares no version can legitimately name a param +:version+ + # (`route_param :version`, `get '/:version'`), and that capture belongs to + # the application. Dropping it unconditionally left `params[:version]` nil + # on a route that had matched, losing the segment silently. + def routing_args_as_params(routing_args) + return if routing_args.nil? + return routing_args.except(:version, :route_info) if grape_owns_version?(routing_args) + + routing_args.except(:route_info) + end + + # A route reports a +version+ only when the API declared one, which is the + # case where the captured segment is Grape's rather than the application's. + def grape_owns_version?(routing_args) + return false unless routing_args.key?(:version) + + route = routing_args[:route_info] + route.respond_to?(:version) && !route.version.nil? + end + # Uses a plain `each_header` block instead of `each_header.with_object`: # `with_object` can only pass the block one value plus the memo, so the # `k, v` pair would be boxed into a throwaway Array on every header. A diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index 7afa40bc1..4f3e5e55b 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -49,7 +49,7 @@ def params_for(input) parsed = pattern.params(input) return unless parsed - parsed.compact.symbolize_keys + parsed.compact.symbolize_keys.transform_values! { |value| tag_utf8(value) } end protected @@ -60,6 +60,33 @@ def convert_to_head_request! private + # Mustermann decodes path captures out of +PATH_INFO+, which Rack hands us + # tagged ASCII-8BIT, so path params came back binary while Rack tags query + # and body params UTF-8. That split makes an API's own declarations + # disagree with themselves: `values: ['café']` matched `?id=café` but not + # `/café`, since a binary string never equals the UTF-8 literal it was + # written as. + # + # Re-tag as UTF-8. Nothing obliges a client to send UTF-8: the request + # target is octets to HTTP, and Rack's SPEC has CGI keys carry non-ASCII + # as ASCII-8BIT. But UTF-8 is what browsers percent-encode with, what an + # IRI maps to, and what Rails settles on — ActionDispatch::Journey::Router + # force_encodes every path capture to UTF-8 after unescaping it. + # + # Only the encoding changes; the bytes are untouched. Octets that are not + # UTF-8 therefore stay invalid and are caught downstream rather than being + # silently scrubbed into something the client never sent. + def tag_utf8(value) + # String first: every capture but a multi-splat is one. + if value.is_a?(String) + value.encoding == Encoding::UTF_8 ? value : (+value).force_encoding(Encoding::UTF_8) + elsif value.is_a?(Array) + value.map { |element| tag_utf8(element) } + else + value + end + end + def upcase_method(method) method_s = method.to_s Grape::HTTP_SUPPORTED_METHODS.detect { |m| m.casecmp(method_s).zero? } || method_s.upcase diff --git a/lib/grape/util/inheritable_setting.rb b/lib/grape/util/inheritable_setting.rb index 21593255f..f202b5cb8 100644 --- a/lib/grape/util/inheritable_setting.rb +++ b/lib/grape/util/inheritable_setting.rb @@ -384,6 +384,7 @@ def base_only_rescue_handlers def add_rescue_handlers(mapping, subclasses:) @rescue_handler_maps ||= {} own = (@rescue_handler_maps[subclasses ? :rescue_handlers : :base_only_rescue_handlers] ||= {}) + ShadowedRescueHandlers.warn_about(own, mapping) if subclasses own.merge!(mapping) { |_klass, registered, _new| registered } end @@ -778,11 +779,14 @@ def merged_rescue_handlers(key) def copy_state_from(source) @namespace = source.namespace.dup @namespace_inheritable = source.namespace_inheritable&.dup - # Shallow, matching the store this replaced: the per-key Arrays stay - # shared with the source, so a registration made on the source after - # the copy was taken is still visible through it. - @stackable_values = source.stackable_values&.dup - @rescue_handler_maps = source.rescue_handler_maps&.dup + # The nested stores are duped too, not just the Hash holding them. A + # copy is a point in time: what the source registers afterwards must not + # reach it. Sharing them made that depend on whether the key already + # held a registration — `use` below a route applied to it when some + # earlier `use` had seeded the Array, and did nothing otherwise; the + # same went for `rescue_from` below a route. + @stackable_values = source.stackable_values&.transform_values(&:dup) + @rescue_handler_maps = source.rescue_handler_maps&.transform_values(&:dup) @route = source.route.clone end diff --git a/lib/grape/util/media_type.rb b/lib/grape/util/media_type.rb index e93956b12..e4f041f19 100644 --- a/lib/grape/util/media_type.rb +++ b/lib/grape/util/media_type.rb @@ -7,6 +7,10 @@ class MediaType # based on the HTTP Accept header with the pattern: # application/vnd.:vendor-:version+:format + # + # Matched against a down-cased media type: they are case-insensitive + # (RFC 9110 §8.3.1), while a vendor and version are declared in the DSL + # in the case they will be compared in. VENDOR_VERSION_HEADER_REGEX = /\Avnd\.(?[a-z0-9.\-_!^]+?)(?:-(?[a-z0-9*.]+))?(?:\+(?[a-z0-9*\-.]+))?\z/ def initialize(type:, subtype:) @@ -41,7 +45,7 @@ def best_quality(header, available_media_types) def parse(media_type) return if media_type.blank? - type, subtype = media_type.split('/', 2) + type, subtype = media_type.downcase.split('/', 2) return if type.blank? || subtype.blank? new(type:, subtype:) @@ -50,14 +54,17 @@ def parse(media_type) def match?(media_type) return false if media_type.blank? - subtype = media_type.split('/', 2).last + subtype = media_type.downcase.split('/', 2).last return false if subtype.blank? VENDOR_VERSION_HEADER_REGEX.match?(subtype) end + # The available types are registered in lower case and Rack matches them + # literally, so the header has to be down-cased to be compared against + # them at all. def best_quality_media_type(header, available_media_types) - header.blank? ? available_media_types.first : Rack::Utils.best_q_match(header, available_media_types) + header.blank? ? available_media_types.first : Rack::Utils.best_q_match(header.downcase, available_media_types) end end diff --git a/lib/grape/util/shadowed_rescue_handlers.rb b/lib/grape/util/shadowed_rescue_handlers.rb new file mode 100644 index 000000000..b5180adc5 --- /dev/null +++ b/lib/grape/util/shadowed_rescue_handlers.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Grape + module Util + # Diagnostics for +rescue_from+ registrations that can never run. + # + # Middleware::Error resolves with +find+, so within a scope the first + # matching class wins and one registered for a class an earlier handler + # already covers is dead code — silently, before this warned: + # + # rescue_from StandardError do ... end # wins + # rescue_from ArgumentError do ... end # never runs + # + # Warn rather than reorder: which should win is the author's call, and + # +rescue_from :all+ (consulted only after the registered handlers) already + # offers "broad first, specific still wins" to anyone who wants it. + module ShadowedRescueHandlers + module_function + + # @param registered [Hash] the scope's own handlers, in registration order + # @param mapping [Hash] the handlers being registered now + # @return [void] + # + # Only a scope's own registrations are compared: across scopes the nearest + # one deliberately wins, so an inner +rescue_from StandardError+ shadowing + # an outer +rescue_from ArgumentError+ is the documented behaviour rather + # than a mistake. Classes sharing a handler object are skipped too, since + # +rescue_from A, B+ registers one handler for both and the entry that + # loses to the other changes nothing. + def warn_about(registered, mapping) + return if registered.empty? + + mapping.each do |klass, handler| + covered_by, = registered.find { |already, existing| klass <= already && !existing.equal?(handler) } + next unless covered_by + + warn(message_for(klass, covered_by)) + end + end + + def message_for(klass, covered_by) + return "Grape: rescue_from #{klass} was already registered in this scope; the first handler is kept and this one will never run." if klass == covered_by + + "Grape: rescue_from #{klass} will never run — #{covered_by} was registered earlier in the same scope " \ + 'and is matched first. Register the more specific class before the broader one.' + end + end + end +end 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/api_spec.rb b/spec/grape/api_spec.rb index ff3454f15..9d36c991f 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -177,6 +177,78 @@ subject.represent represent_object, with: dummy_presenter_klass expect(subject.inheritable_setting.representations).to eq(represent_object => dummy_presenter_klass) end + + # Both the collection tests are duck-typed, and plenty of single objects + # answer them, so a registered entity used to be skipped for those in favour + # of the entity for whatever #first returned. + context 'when the presented object also looks like a collection' do + let(:entity) do + Class.new do + def self.represent(object, **) + { presented: object.class.name.to_s } + end + end + end + + def present_with(api, model, object, entity) + api.format :json + api.represent model, with: entity + api.get('/') { present object } + end + + it 'uses the entity registered for a Struct' do + model = Struct.new(:name) + present_with(subject, model, model.new('x'), entity) + + get '/' + expect(JSON.parse(last_response.body)).to eq('presented' => model.name.to_s) + end + + it 'uses the entity registered for an Enumerable model' do + model = Class.new do + include Enumerable + + def each(&) = [1, 2].each(&) + end + present_with(subject, model, model.new, entity) + + get '/' + expect(JSON.parse(last_response.body)).to eq('presented' => model.name.to_s) + end + + it 'uses the entity registered for an object exposing #klass' do + model = Class.new do + def klass = String + end + present_with(subject, model, model.new, entity) + + get '/' + expect(JSON.parse(last_response.body)).to eq('presented' => model.name.to_s) + end + + it 'still resolves an array through its element class' do + model = Class.new + present_with(subject, model, [model.new], entity) + + get '/' + expect(JSON.parse(last_response.body)).to eq('presented' => 'Array') + end + + # A relation resolves through #klass, so #first must not be reached. + it 'does not call #first when the object resolves without it' do + model = Class.new + relation = Class.new do + def initialize(klass) = (@klass = klass) + attr_reader :klass + + def first = raise('#first should not have been called') + end + present_with(subject, model, relation.new(model), entity) + + expect { get '/' }.not_to raise_error + expect(last_response.status).to eq(200) + end + end end describe '.namespace' do @@ -309,6 +381,68 @@ expect(last_response.body).to eq('{"foo":1234}') end end + + context 'with a non-ascii segment' do + it 'tags the param as UTF-8 rather than leaving it binary' do + subject.route_param :id do + get { params[:id].encoding.name } + end + + get '/caf%C3%A9' + expect(last_response.body).to eq('UTF-8') + end + + it 'tags every captured segment' do + subject.namespace :a do + route_param :one do + route_param :two do + get { [params[:one].encoding.name, params[:two].encoding.name].join(',') } + end + end + end + + get '/a/caf%C3%A9/th%C3%A9' + expect(last_response.body).to eq('UTF-8,UTF-8') + end + + it 'tags a splat capture' do + subject.get('/files/*path') { params[:path].encoding.name } + + get '/files/a/b/caf%C3%A9' + expect(last_response.body).to eq('UTF-8') + end + + # An unnamed splat captures into an Array rather than a String. + it 'tags every element of an unnamed splat capture' do + subject.get('/files/*') { params[:splat].map { |s| s.encoding.name }.join(',') } + + get '/files/caf%C3%A9' + expect(last_response.body).to eq('UTF-8') + end + + # A path param used to arrive binary while the same value arrived UTF-8 + # through the query string, so an API's own declaration disagreed with + # itself depending on where the value came from. + it 'compares equal to the utf-8 literal the API declares' do + subject.params { requires :id, type: String, values: ['café'] } + subject.route_param :id do + get { 'matched' } + end + + get '/caf%C3%A9' + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('matched') + end + + it 'leaves the bytes untouched' do + subject.route_param :id do + get { params[:id] } + end + + get '/caf%C3%A9' + expect(last_response.body.b).to eq('caf%C3%A9'.b.gsub('%C3%A9', "\xC3\xA9".b)) + end + end end describe '.route' do @@ -1517,6 +1651,48 @@ def call(env) expect(last_response.body).to eql 'hello' end + # A route captures the middleware registered above it. Whether some + # earlier `use` had already seeded the scope's stack must not change that. + context 'when declared below a route' do + it 'does not apply to that route' do + subject.get('/') { env['phony.args'].inspect } + subject.use phony_middleware, 'too-late' + + get '/' + expect(last_response.body).to eql 'nil' + end + + it 'does not apply to that route when an earlier use seeded the stack' do + subject.use phony_middleware, 'in-time' + subject.get('/') { env['phony.args'].flatten.inspect } + subject.use phony_middleware, 'too-late' + + get '/' + expect(last_response.body).to eql ['in-time'].inspect + end + end + end + + describe '.rescue_from declared below a route' do + let(:boom) { Class.new(StandardError) } + + it 'does not apply to that route' do + error_class = boom + subject.get('/') { raise error_class } + subject.rescue_from(error_class) { error!('too late', 480) } + + expect { get '/' }.to raise_error(error_class) + end + + it 'does not apply to that route when an earlier rescue_from seeded the map' do + error_class = boom + subject.rescue_from(ArgumentError) { error!('in time', 481) } + subject.get('/') { raise error_class } + subject.rescue_from(error_class) { error!('too late', 480) } + + expect { get '/' }.to raise_error(error_class) + end + it 'adds a block if one is given' do block = -> {} subject.use phony_middleware, &block @@ -2529,6 +2705,59 @@ def rescue_all_errors expect(last_response).to be_forbidden expect(last_response.body).to eq('Redefined Error') end + + # The opt-in exists to keep Grape's own errors rendering with their own + # status. A catch-all registered as a class is matched before it, and + # Grape's exceptions are StandardErrors, so it used to be silently inert + # unless the catch-all happened to be spelled `rescue_from :all`. + context 'with a catch-all class handler' do + subject(:app) do + Class.new(Grape::API) do + format :json + rescue_from(StandardError) { error!({ h: 'catch-all' }, 500) } + rescue_from(:grape_exceptions) { |e| error!({ h: 'grape' }, e.status) } + params { requires :n, type: Integer } + get('/validated') { 'ok' } + get('/boom') { raise ArgumentError, 'app error' } + end + end + + it 'keeps a validation failure a 400' do + get '/validated' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq({ h: 'grape' }.to_json) + end + + it 'still sends the application’s own errors to the catch-all' do + get '/boom' + + expect(last_response.status).to eq(500) + expect(last_response.body).to eq({ h: 'catch-all' }.to_json) + end + end + + it 'lets a handler registered for a grape exception class win over the opt-in' do + subject.rescue_from(Grape::Exceptions::ValidationErrors) { error!('specific', 422) } + subject.rescue_from(StandardError) { error!('catch-all', 500) } + subject.params { requires :n, type: Integer } + subject.get('/validated') { 'ok' } + + get '/validated' + + expect(last_response.status).to eq(422) + expect(last_response.body).to eq('specific') + end + + it 'does not intercept an exception that is not a grape exception' do + subject.rescue_from(StandardError) { error!('catch-all', 500) } + subject.get('/boom') { raise ArgumentError } + + get '/boom' + + expect(last_response.status).to eq(500) + expect(last_response.body).to eq('catch-all') + end end describe '.error_format' do @@ -4183,6 +4412,32 @@ def my_method end end + # Media types are case-insensitive (RFC 9110 §8.3.1). The registered ones are + # spelled in lower case and matched literally, so a differently-cased Accept + # used to find nothing: content negotiation fell through to the default format + # and header versioning behaved as though no version had been asked for. + describe 'a differently-cased Accept header' do + it 'still negotiates the content type' do + subject.content_type :json, 'application/json' + subject.content_type :txt, 'text/plain' + subject.default_format :json + subject.get('/x') { { a: 1 } } + + get '/x', {}, 'HTTP_ACCEPT' => 'TEXT/PLAIN' + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('text/plain') + end + + it 'still resolves the version of a vendor media type' do + subject.version 'v1', using: :header, vendor: 'twitter' + subject.format :json + subject.get('/x') { env[Grape::Env::API_VERSION] } + + get '/x', {}, 'HTTP_ACCEPT' => 'APPLICATION/VND.TWITTER-V1+JSON' + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('v1'.to_json) + end + end + describe '.format' do context ':txt' do before do diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 17cadc2d3..6535b918b 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -667,6 +667,38 @@ def handle_argument_error get '/hey' expect(last_response.body).to eq 'test body' end + + # The message is announced as text/plain, so it has to be rendered as such + # whatever the API's own format is. Left to the JSON formatter it came back + # as a quoted JSON string under a text/plain content type. + context 'when the API declares a format of its own' do + before do + subject.format :json + subject.get('/hey') { redirect '/ha' } + end + + it 'renders the message as plain text' do + get '/hey' + + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('text/plain') + expect(last_response.body).to eq 'This resource has been moved temporarily to /ha.' + end + + it 'renders an overridden body as plain text too' do + subject.get('/there') { redirect '/ha', body: 'go away' } + + get '/there' + expect(last_response.body).to eq 'go away' + end + + it 'leaves the format of other routes alone' do + subject.get('/plain') { { a: 1 } } + + get '/plain' + expect(last_response.headers[Rack::CONTENT_TYPE]).to eq('application/json') + expect(last_response.body).to eq({ a: 1 }.to_json) + end + end end describe 'NameError' do diff --git a/spec/grape/middleware/auth/dsl_spec.rb b/spec/grape/middleware/auth/dsl_spec.rb index 612c511c8..29d9e2775 100644 --- a/spec/grape/middleware/auth/dsl_spec.rb +++ b/spec/grape/middleware/auth/dsl_spec.rb @@ -4,12 +4,15 @@ subject { Class.new(Grape::API) } let(:block) { -> {} } + # #auth records whatever it is given; the strategy behind the label is looked + # up when the middleware is built. A label with no built-in sugar method keeps + # that distinction visible. let(:settings) do { opaque: 'secret', proc: block, realm: 'API Authorization', - type: :http_digest + type: :custom } end @@ -17,7 +20,7 @@ it 'sets auth parameters' do expect(subject.base_instance).to receive(:use).with(Grape::Middleware::Auth::Base, settings) - subject.auth :http_digest, realm: settings[:realm], opaque: settings[:opaque], &settings[:proc] + subject.auth :custom, realm: settings[:realm], opaque: settings[:opaque], &settings[:proc] expect(subject.auth).to eq(settings) end @@ -25,10 +28,10 @@ expect(subject.base_instance).to receive(:use).with(Grape::Middleware::Auth::Base, settings) expect(subject.base_instance).to receive(:use).with(Grape::Middleware::Auth::Base, settings.merge(realm: 'super_secret')) - subject.auth :http_digest, realm: settings[:realm], opaque: settings[:opaque], &settings[:proc] + subject.auth :custom, realm: settings[:realm], opaque: settings[:opaque], &settings[:proc] first_settings = subject.auth - subject.auth :http_digest, realm: 'super_secret', opaque: settings[:opaque], &settings[:proc] + subject.auth :custom, realm: 'super_secret', opaque: settings[:opaque], &settings[:proc] expect(subject.auth).to eq(settings.merge(realm: 'super_secret')) expect(subject.auth.object_id).not_to eq(first_settings.object_id) @@ -42,29 +45,13 @@ end end - describe '.http_digest' do - context 'when realm is a hash' do - it 'sets auth parameters' do - subject.http_digest realm: { realm: 'my_realm', opaque: 'my_opaque' }, &settings[:proc] - expect(subject.auth).to eq(realm: { realm: 'my_realm', opaque: 'my_opaque' }, type: :http_digest, proc: block) - end - end - - context 'when realm is not hash' do - it 'sets auth parameters' do - subject.http_digest realm: 'my_realm', opaque: 'my_opaque', &settings[:proc] - expect(subject.auth).to eq(realm: 'my_realm', type: :http_digest, proc: block, opaque: 'my_opaque') - end - end - end - describe 'deprecated positional options Hash' do it 'deprecates a positional Hash for `auth` but still works when silenced' do - expect { subject.auth :http_digest, { realm: 'r', opaque: 'o' }, &block } + expect { subject.auth :custom, { realm: 'r', opaque: 'o' }, &block } .to raise_error(ActiveSupport::DeprecationException, /positional options Hash to `auth`/) - Grape.deprecator.silence { subject.auth :http_digest, { realm: 'r', opaque: 'o' }, &block } - expect(subject.auth).to eq(realm: 'r', opaque: 'o', type: :http_digest, proc: block) + Grape.deprecator.silence { subject.auth :custom, { realm: 'r', opaque: 'o' }, &block } + expect(subject.auth).to eq(realm: 'r', opaque: 'o', type: :custom, proc: block) end it 'deprecates a positional Hash for `http_basic` but still works when silenced' do @@ -74,13 +61,5 @@ Grape.deprecator.silence { subject.http_basic({ realm: 'my_realm' }, &block) } expect(subject.auth).to eq(realm: 'my_realm', type: :http_basic, proc: block) end - - it 'deprecates a positional Hash for `http_digest` but still works when silenced' do - expect { subject.http_digest({ realm: 'my_realm' }, &block) } - .to raise_error(ActiveSupport::DeprecationException, /positional options Hash to `http_digest`/) - - Grape.deprecator.silence { subject.http_digest({ realm: 'my_realm' }, &block) } - expect(subject.auth).to eq(realm: 'my_realm', opaque: 'secret', type: :http_digest, proc: block) - end end end diff --git a/spec/grape/middleware/error_spec.rb b/spec/grape/middleware/error_spec.rb index 320a68d61..5edf8be84 100644 --- a/spec/grape/middleware/error_spec.rb +++ b/spec/grape/middleware/error_spec.rb @@ -99,6 +99,73 @@ def self.call(_env) end end + # Rendering happens inside #call!'s rescue clause, so it is not covered by + # that rescue: without a failsafe an error formatter that raises takes the + # exception straight out through every middleware above. + describe 'when the error response cannot be rendered' do + subject(:response) do + get '/' + last_response + end + + context 'and the formatter chokes on the payload' do + let(:app) do + Class.new(Grape::API) do + format :json + + rescue_from(:all) { |e| error!({ detail: e.message }, 404) } + + # A message the JSON formatter cannot serialize. + get('/') { raise StandardError, +"bad \xC3 byte".b } + end + end + + it 'answers with the framework message in the API format' do + expect(response.status).to eq(500) + expect(response.headers[Rack::CONTENT_TYPE]).to include('application/json') + expect(JSON.parse(response.body)).to eq('error' => 'Internal Server Error') + end + + it 'exposes the rendering failure on the rack env' do + get '/' + expect(last_request.env[Grape::Env::GRAPE_EXCEPTION]).to be_a(StandardError) + end + end + + context 'and the formatter is broken outright' do + let(:app) do + Class.new(Grape::API) do + format :json + + error_formatter :json, ->(**) { raise 'formatter is broken' } + rescue_from(:all) { error!({ detail: 'nope' }, 404) } + + get('/') { raise StandardError, 'boom' } + end + end + + it 'drops the formatter rather than recursing' do + expect(response.status).to eq(500) + expect(response.headers[Rack::CONTENT_TYPE]).to include('text/plain') + expect(response.body).to eq('500 Internal Server Error') + end + end + + context 'and nothing rescues the original exception' do + let(:app) do + Class.new(Grape::API) do + format :json + + get('/') { raise ArgumentError, 'kaboom' } + end + end + + it 'keeps propagating it' do + expect { get '/' }.to raise_error(ArgumentError, 'kaboom') + end + end + end + describe 'when a rescue_from block raises' do subject(:response) do get '/' diff --git a/spec/grape/request_spec.rb b/spec/grape/request_spec.rb index 19675178f..f1566960d 100644 --- a/spec/grape/request_spec.rb +++ b/spec/grape/request_spec.rb @@ -50,13 +50,29 @@ let(:routing_args) do { version: '123', - route_info: '456', + route_info: instance_double(Grape::Router::Route, version: route_version), c: 'ccc' } end - it 'cuts version and route_info' do - expect(request.params).to eq(ActiveSupport::HashWithIndifferentAccess.new(a: '123', b: 'xyz', c: 'ccc')) + context 'when the route carries a version of its own' do + let(:route_version) { 'v1' } + + it 'cuts version and route_info' do + expect(request.params).to eq(ActiveSupport::HashWithIndifferentAccess.new(a: '123', b: 'xyz', c: 'ccc')) + end + end + + # Without a declared version the captured segment is the application's: + # `route_param :version` on an unversioned API has to reach the endpoint. + context 'when the route carries no version' do + let(:route_version) { nil } + + it 'cuts only route_info' do + expect(request.params).to eq( + ActiveSupport::HashWithIndifferentAccess.new(a: '123', b: 'xyz', c: 'ccc', version: '123') + ) + end end end diff --git a/spec/grape/util/inheritable_setting_spec.rb b/spec/grape/util/inheritable_setting_spec.rb index 729866879..7fa22f360 100644 --- a/spec/grape/util/inheritable_setting_spec.rb +++ b/spec/grape/util/inheritable_setting_spec.rb @@ -227,6 +227,62 @@ subject.add_rescue_handlers({ StandardError => :child }, subclasses: true) expect(subject.rescue_handlers).to eq(StandardError => :child) end + + # Middleware::Error resolves with #find, so the first matching class in a + # scope wins and a narrower one registered after it is dead code. + describe 'shadowing warnings' do + def add(mapping, subclasses: true) + subject.add_rescue_handlers(mapping, subclasses:) + end + + it 'warns when a broader class was registered first' do + add({ StandardError => :broad }) + + expect { add({ ArgumentError => :narrow }) } + .to output(/rescue_from ArgumentError will never run — StandardError was registered earlier/).to_stderr + end + + it 'warns when the same class is registered twice' do + add({ ArgumentError => :first }) + + expect { add({ ArgumentError => :second }) } + .to output(/rescue_from ArgumentError was already registered in this scope/).to_stderr + end + + it 'does not warn when the narrower class was registered first' do + add({ ArgumentError => :narrow }) + + expect { add({ StandardError => :broad }) }.not_to output.to_stderr + end + + it 'does not warn for unrelated classes' do + add({ ArgumentError => :one }) + + expect { add({ TypeError => :two }) }.not_to output.to_stderr + end + + # `rescue_from A, B` registers one handler for both, so the entry that + # loses to the other changes nothing. + it 'does not warn when both classes share a handler' do + expect { add({ StandardError => :shared, ArgumentError => :shared }) }.not_to output.to_stderr + end + + # Exact-match handlers are consulted before the subclass-matching ones and + # never match a descendant, so they cannot shadow each other. + it 'does not warn for exact-match handlers' do + add({ StandardError => :broad }, subclasses: false) + + expect { add({ ArgumentError => :narrow }, subclasses: false) }.not_to output.to_stderr + end + + # Across scopes the nearest registration deliberately wins. + it 'does not warn about a handler inherited from an enclosing scope' do + parent = described_class.new.tap { |s| s.add_rescue_handlers({ StandardError => :outer }, subclasses: true) } + subject.inherit_from parent + + expect { add({ ArgumentError => :inner }) }.not_to output.to_stderr + end + end end describe '#route' do @@ -332,6 +388,68 @@ expect(cloned_obj.helpers).to eq [:namespace_stackable_foo_bar] end + # The case above registers only on the parent, so the copy never shares an + # Array with `subject` and passes even when the per-key Arrays are shared. + # Here the key already holds one of `subject`'s own registrations when the + # copy is taken, which is what made the later one leak into it. + context 'when the scope already registered the key itself' do + subject(:setting) do + described_class.new.tap do |settings| + settings.inherit_from parent + settings.add_helper(:own_before_copy) + end + end + + let!(:cloned_obj) { setting.point_in_time_copy } + + it 'does not leak a later registration into the copy' do + setting.add_helper(:own_after_copy) + + expect(setting.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy own_after_copy] + expect(cloned_obj.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy] + end + + it 'does not leak the copy’s own registration back to the source' do + cloned_obj.add_helper(:only_on_copy) + + expect(setting.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy] + expect(cloned_obj.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy only_on_copy] + end + + it 'keeps sibling copies independent' do + sibling = setting.point_in_time_copy + cloned_obj.add_helper(:only_on_first) + + expect(sibling.helpers).to eq %i[namespace_stackable_foo_bar own_before_copy] + end + end + + # Same shape as the stackable case: the per-kind Hashes inside + # @rescue_handler_maps have to be duped, not just the Hash holding them. + context 'when the scope already registered a rescue handler' do + subject(:setting) do + described_class.new.tap do |settings| + settings.add_rescue_handlers({ ArgumentError => :before_copy }, subclasses: true) + end + end + + let!(:cloned_obj) { setting.point_in_time_copy } + + it 'does not leak a later handler into the copy' do + setting.add_rescue_handlers({ TypeError => :after_copy }, subclasses: true) + + expect(setting.rescue_handlers).to eq(ArgumentError => :before_copy, TypeError => :after_copy) + expect(cloned_obj.rescue_handlers).to eq(ArgumentError => :before_copy) + end + + it 'does not leak a later base-only handler into the copy' do + setting.add_rescue_handlers({ TypeError => :after_copy }, subclasses: false) + + expect(setting.base_only_rescue_handlers).to eq(TypeError => :after_copy) + expect(cloned_obj.base_only_rescue_handlers).to be_blank + end + end + it 'decouples route values' do expect(cloned_obj.route[:route_thing]).to eq :route_foo_bar diff --git a/spec/grape/util/media_type_spec.rb b/spec/grape/util/media_type_spec.rb index 09e328513..ae2456f93 100644 --- a/spec/grape/util/media_type_spec.rb +++ b/spec/grape/util/media_type_spec.rb @@ -41,6 +41,25 @@ it_behaves_like 'MediaType' end end + + # Media types are case-insensitive (RFC 9110 §8.3.1); the vendor pattern is + # written in lower case, so anything else used to parse as no vendor at all. + context 'when the header is not in lower case' do + subject(:media_type) { described_class.parse(header) } + + let(:header) { 'APPLICATION/VND.TEST-V1+JSON' } + + it 'parses the vendor, version and format' do + expect(media_type.vendor).to eq('test') + expect(media_type.version).to eq('v1') + expect(media_type.format).to eq('json') + end + + it 'down-cases the type and subtype' do + expect(media_type.type).to eq('application') + expect(media_type.subtype).to eq('vnd.test-v1+json') + end + end end describe '.match?' do 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