Adversarial sweep: eleven longstanding fixes - #2850
Open
ericproulx wants to merge 11 commits into
Open
Conversation
Danger ReportNo issues found. |
ericproulx
force-pushed
the
fix/adversarial-sweep-2026-08
branch
from
August 1, 2026 14:07
89cbb50 to
ef13efa
Compare
ericproulx
changed the title
4 tasks
ericproulx
force-pushed
the
fix/adversarial-sweep-2026-08
branch
from
August 1, 2026 14:53
ef13efa to
2d28454
Compare
`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 <noreply@anthropic.com>
(cherry picked from commit adeb549)
Mustermann decodes path captures out of PATH_INFO, which Rack hands over
tagged ASCII-8BIT, and nothing re-tagged the result. Query and body params
arrive UTF-8 because Rack tags those itself, so the same value reached the
endpoint with a different encoding depending on where it came from.
That made an API's declarations disagree with themselves -- a binary string
never equals the UTF-8 literal it was written as:
params { requires :id, type: String, values: ['café'] }
GET /?id=café -> 200
GET /café -> 400 "id does not have a valid value"
The same held for same_as, except_values and any comparison an endpoint made
against a non-ASCII literal. It also leaked into serialization: a non-ASCII
path param rendered into a JSON response drew an encoding warning from the
json gem, which that gem says will become an error in json 3.0.
Re-tag in Route#params_for, the single funnel for path-extracted values.
Nothing obliges a client to send UTF-8 -- HTTP treats the request target as
octets, and Rack's SPEC has CGI keys carry non-ASCII as ASCII-8BIT -- so
UTF-8 is the convention rather than a guarantee: it 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 here: the bytes are
untouched, so octets that are not UTF-8 stay invalid and are still caught
downstream instead of being silently scrubbed into something the client
never sent. Unnamed splats capture into an Array, so those are walked too.
No extra allocations -- the captures are freshly built and unfrozen, so the
re-tag happens in place.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 80e9e99)
Grape::Middleware::Error#call! renders the error response from inside its own
rescue clause, so that clause never covered the rendering. An error formatter
that raised on the payload it was handed took the exception straight out
through every middleware above Grape and into the application server —
`rescue_from :all` did not help, because the failure happened after the
handler had already returned.
A rescue_from handler echoing request-derived bytes was enough to hit it:
rescue_from(Missing) { |e| error!({ detail: e.message }, 404) }
with an invalid UTF-8 byte in the path, the JSON formatter raised
JSON::GeneratorError and the request died rather than being answered.
Guard the rendering in error_response. On failure, 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; if that fails too — a
formatter broken outright rather than one payload it choked on — answer
without a formatter at all. Both attempts call format_message directly instead
of re-entering error_response, so the fallback cannot recurse.
The guard sits on the rendering rather than around run_rescue_handler on
purpose. Wrapping the handler call too would have swallowed things that must
keep propagating, the deprecation raised when a handler returns a Hash among
them.
Exceptions that no rescue_from matches still propagate unchanged; only
rendering failures are caught. The exception that defeated rendering is put on
env['grape.exception'], the key the existing unrecognised-error path already
uses, so upstream loggers can still observe it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 4b7a3b3)
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 source. A registration made after an endpoint was defined
therefore still reached that endpoint -- but only when the key already held a
registration at the time the copy was taken, since otherwise the writer
allocated a fresh store on the source alone.
That made the outcome depend on something the API never expressed:
use Middleware1
get('/x') { }
use Middleware2 # applied to GET /x
get('/x') { }
use Middleware2 # did NOT apply to GET /x
and the same for rescue_from:
rescue_from ArgumentError { }
get('/x') { raise Boom }
rescue_from Boom { } # rescued GET /x
get('/x') { raise Boom }
rescue_from Boom { } # did NOT rescue GET /x
Two APIs stating the same thing, behaving differently. Not a deliberate
"late registration" feature -- helpers defined after an endpoint already did
not leak, because they resolve down a different path.
Dup the nested stores as well as the Hash holding them. A copy is a point in
time: what the source registers afterwards must not reach it. Inheritance is
untouched -- it resolves by walking #parent, so a scope still sees values an
enclosing scope gains later, which is what the existing "decouples namespace
stackable values" spec actually exercised (its value lives on the parent, so
nothing was ever shared and it passed either way).
Boot cost is negligible: 300 endpoints under scopes carrying middleware,
helpers and filters allocate 1800 more objects (+0.4%) with no measurable
change in time, all at definition time.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 2599501)
Grape::Middleware::Error resolves a registered handler with #find, so within a
scope the first matching class wins. A handler registered for a class an
earlier one already covers is therefore dead code, and silently so:
rescue_from StandardError do ... end # wins
rescue_from ArgumentError do ... end # never runs
Swapping the two lines is all it takes, but nothing said so -- and the
neighbouring `rescue_from :all` behaves the other way round, since it is
consulted only after the registered handlers, so a specific class registered
after it still wins. Two documented ways of saying "catch everything",
ordering differently against a later specific handler.
Warn at definition time rather than reorder: which handler should win is the
author's call, and :all already covers "broad first, specific still wins".
The check lives in Grape::Util::ShadowedRescueHandlers, called from
#add_rescue_handlers where the scope's own registrations are known.
Compared within a scope only -- across scopes the nearest registration
deliberately wins, so an inner rescue_from StandardError shadowing an outer
rescue_from ArgumentError is the documented behaviour, not 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 changes nothing. Exact-match
handlers (rescue_subclasses: false) are consulted before the subclass-matching
ones and never match a descendant, so they cannot shadow each other either.
README gains the ordering rule, which was only implied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 8f6202a)
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, which lives in
all_rescue_handler and is consulted last. Written 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 still came back as 500s:
rescue_from StandardError { error!('server error', 500) }
rescue_from :grape_exceptions # inert
Let the opt-in win over a handler that only matched through a non-Grape
ancestor. A handler registered for a Grape exception class is more precise
than the opt-in and still wins, so an explicit
rescue_from Grape::Exceptions::ValidationErrors keeps its handler.
registered_rescue_handler gains an _entry variant returning the matched class
alongside its handler, since deciding this needs to know *which* class matched,
not just that one did. find_handler resolves the entry once and passes it down.
Application errors still reach the catch-all, rescue_from :all is unchanged,
and InvalidVersionHeader is left alone so it keeps reaching Rack and version
cascading still works.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit f67e86c)
Grape::DSL::Entity#object_class decided which class to look an entity up for
by duck-typing:
return object.klass if object.respond_to?(:klass)
return object.first.class if object.respond_to?(:first)
object.class
Both tests are answered by plenty of single objects, and the object's own
class was consulted only last. A Struct is Enumerable, so it responds to
#first; so does any model that includes Enumerable; and #klass is hardly
exclusive to ActiveRecord::Relation. For all of those, `represent Model,
with: Entity` was silently ignored and the entity for whatever #first
returned was looked up instead -- usually nothing, so the raw object was
serialized.
Try the object's own class first and fall back to the collection or wrapped
class only when that comes up empty. An Array still resolves through its
element class, since Array itself has no entity, and a relation still
resolves through #klass.
Deferring the fallback also stops #first from being called at all when the
object resolves on its own class, which matters for anything where taking
the first element is expensive or has side effects.
Longstanding: the duck-typing dates to 2014 (v0.10.0) and behaves the same
way in 3.3.4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit e70e858)
#redirect announces its message as text/plain and has done since it was
introduced in 2015 ("Redirect as plain text with optional message override"),
but it only set the header. The body was still handed to the API's own
formatter, so on a JSON API the sentence came back JSON-encoded:
format :json
get('/r') { redirect '/there' }
Content-Type: text/plain
"This resource has been moved temporarily to /there."
quotes included -- neither valid plain text nor something a client reading the
content type would expect. The existing specs missed it because they run on
the default :txt format, where the formatter is a no-op.
Set api.format alongside the header, the same lever an endpoint already has
via #api_format, so the message is rendered by the txt formatter whatever the
API declares. It is per-request env, so other routes on the same API are
untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 4e7d9d5)
Grape::Request#make_params dropped :version from the routing args
unconditionally, alongside :route_info. That is right when Grape put it
there -- a path-versioned API captures the version as a segment and exposes
it through env['api.version'] rather than params -- but it is not always
Grape's.
An API that declares no version can name a param :version:
route_param :version do
get { params[:version] } # => nil
end
The route matched and Mustermann captured the segment, but the value was
filtered out before the endpoint saw it, so params[:version] came back nil
and the key was absent from params entirely. Same for a bare
get '/:version'. Silent loss of a segment on a route that had matched.
A route reports a #version only when the API declared one, so use that to
tell the two apart: drop the capture when the route carries a version,
keep it otherwise. Path, header and param versioning are unaffected --
their routes all report a version, so :version stays out of params and
env['api.version'] remains the way to read it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 09abf2b)
Media types are case-insensitive (RFC 9110 section 8.3.1), but the registered
ones are spelled in lower case and matched literally, so a differently-cased
Accept header found nothing:
Accept: TEXT/PLAIN -> served application/json
Accept: APPLICATION/VND.TWITTER-V1+JSON -> api.version nil
Neither failed loudly. Content negotiation fell through to the default
format, and header versioning behaved as though no version had been asked
for, so the request was served by whichever version matched first -- the
client quietly got something other than what it asked for.
Three sites decided this, all comparing against lower-case registered types:
the formatter's Accept lookup, MediaType.best_quality_media_type, and the
vendor pattern in MediaType.parse / .match?. Down-case the incoming media
type at each. The vendor pattern stays lower-case, which is the case a
vendor and version are declared in and therefore compared in.
Grape already treats media types this way when deciding whether to escape an
error body (Middleware::Error#html_content_type?, from #2789).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 550dd62)
Nothing it could reach has existed since 2.0.0. #2361 removed Rack::Auth::Digest and Grape's :http_digest strategy after Rack 3 dropped digest authentication, but Grape::Middleware::Auth::DSL#http_digest survived and kept recording its settings, so an API declaring it 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. A misconfiguration only visible in production. The 4.0 UPGRADING notes still used `auth :http_digest, realm: 'API', opaque: 'secret'` as a worked example of a supported call, so the documentation pointed at it too. That section is rewritten, and the removal documented. Removing the sugar rather than validating the strategy when `auth` is called: #auth deliberately records whatever it is given and resolves the strategy when the middleware is built, which is what lets an application register its own. That contract is specified -- validating early breaks it, and would break registering a strategy after the API class is defined. `auth :http_digest` therefore still works for anyone who registered one; only the sugar is gone, along with the two defaults it supplied (realm 'API Authorization', opaque 'secret'), which UPGRADING spells out. The DSL specs used :http_digest as their example label for #auth itself. They now use a neutral :custom, which keeps the distinction the specs are actually about: #auth records a label, the strategy behind it is looked up later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 2e12a20)
ericproulx
force-pushed
the
fix/adversarial-sweep-2026-08
branch
from
August 1, 2026 21:21
2d28454 to
07516ee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The eleven changes from the 2026-08 adversarial sweep, gathered into one branch — #2838 through #2849 except #2848, one commit each, cherry-picked in order and applying cleanly.
Every one is longstanding, not a regression: each was reproduced on released 3.3.4 before being fixed. Each individual PR carries the full reasoning, benchmarks and rejected-alternatives; this one exists to review or land them together.
TypeError. Also givestype: Array[JSON]its element indices backvalues: ['café']matched?id=cafébut not/caférescue_from :alldid not helpuse/helpers/rescue_fromdeclared below a route still applied to it — but only when an earlier registration had seeded the storerescue_fromclass is already covered by one registered earlier in the same scope, making it dead coderescue_from :grape_exceptionswas inert against a catch-all written as a class, so validation failures still answered 500representwas silently skipped for any model answering#firstor#klass— aStruct, or anythingEnumerableredirectannouncedtext/plainand then let the API's formatter re-encode the bodyroute_param :versionon an API declaring no version reached the endpoint asnilAcceptmedia types were matched case-sensitively, soTEXT/PLAINsilently got the default formathttp_digest, which has had no strategy behind it since 2.0.0 and raised on the first request rather than at definitionContract changes
Five carry UPGRADING entries, plus a rewrite of the existing
authkeyword-arguments section::grape_exceptionsoutranks a catch-all class handler (Let rescue_from :grape_exceptions outrank a catch-all class handler #2843)http_digestremoved (Remove http_digest #2849)#2844, #2845, #2846, #2847 and #2848 need none: each only makes something work that previously could not, on inputs that had no working behaviour to depend on.
Supersedes
Closes #2838
Closes #2839
Closes #2840
Closes #2841
Closes #2842
Closes #2843
Closes #2844
Closes #2845
Closes #2846
Closes #2847
Closes #2849
Each of those carries the full reasoning for its fix — reproduction, benchmarks, rejected alternatives — and the CHANGELOG entries here keep their numbers, so the discussion stays reachable.
On the shape of this branch
Eleven commits rather than one, deliberately — squashing eleven unrelated fixes would throw away the per-fix rationale, and each message carries the reproduction and the reasoning. Every commit is
cherry-pick -x'd, so it references the commit it came from.The CHANGELOG entries keep their original PR numbers, which point at the individual discussions.
Perf
Suite time is flat. Interleaved A/B, four runs each, against
master:The +2.3% matches the +2.2% examples this branch adds — per-example cost is unchanged.
#2848 was dropped from this branch and closed. It made route declaration invalidate the compiled router, which only mattered when mutating an API that had already been compiled — declaring routes after
mount, afterhelpers, or after reading.routesall work on master today. That leaves runtime mutation of a live API, which is neither documented nor thread-safe, and the change cost O(n²) in exactly that case.Test plan
🤖 Generated with Claude Code