Answer 500 when an error response cannot be rendered

[ericproulx]

Summary

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 — the failure happens after the handler has already returned its payload.

The trigger is client-controlled, which is what makes this more than a cosmetic issue. A rescue_from handler echoing request-derived bytes is enough:

class API < Grape::API
  format :json
  rescue_from(Missing) { |e| error!({ error: 'not_found', detail: e.message }, 404) }
  route_param(:id) { get { raise Missing, "no such thing: #{params[:id]}" } }
end

GET /%C3%28 — an invalid UTF-8 byte in the path — makes the JSON formatter raise JSON::GeneratorError, and the request dies instead of being answered. Any API whose handler interpolates request-derived text can be made to defeat its own error handling on demand.

scenario	before	after
formatter chokes on the payload	JSON::GeneratorError escapes the stack	500 {"error":"Internal Server Error"} in the API format
formatter broken outright	RuntimeError escapes the stack	500 text/plain 500 Internal Server Error
no rescue_from matches the exception	propagates	propagates (unchanged)
format has no error formatter	falls back to the default error formatter	unchanged

Approach

Guard the rendering in error_response. On failure, first retry the API's own format with the framework's InternalServerError — its message is a static string, so it cannot be what defeated the first attempt — and if that fails too, answer without a formatter at all. Both attempts call format_message directly rather than re-entering error_response, so the fallback cannot recurse.

This is the shape Rails already has. ActionDispatch::ShowExceptions#render_exception tries the application's own error rendering and falls back to a hardcoded [500, text/plain, "500 Internal Server Error\n..."] when that rendering is itself broken — same two tiers, same status, same content type, and it calls the second tier "failsafe" too.

The guard sits on the rendering rather than around run_rescue_handler. I tried the wider placement first; it swallowed things that must keep propagating, including the deprecation raised when a handler returns a Hash (error_spec.rb:95). Routing the retry through framework_default was also wrong — that goes back through run_rescue_handler, whose failure path redispatches into framework_default again, turning a clean RuntimeError into SystemStackError.

Swallowing an exception must not make it invisible

Answering 500 means an exception that used to propagate no longer does, and Grape is usually mounted inside a host stack that was reporting it. Setting env['grape.exception'] is not enough on its own: that key is Grape-private and nothing in the ecosystem reads it, so a rendering failure Sentry used to report as a raised exception would have quietly become an unremarkable 500.

So the exception is also published on env['rack.exception'] — the convention for an exception that was handled rather than raised, which sentry-ruby collects as env['rack.exception'] || env['sinatra.error'] — and the failure is written to rack.errors, so it reaches the server log even with no tracker installed. Measured with a tracker middleware mounted above Grape, using the repro above:

	tracker observes	client gets
master	raised: JSON::GeneratorError	500 text/html, the host's error page
this PR	rack.exception: JSON::GeneratorError	500 application/json

Rails writes to $stderr from its failsafe branch for the same reason: deferring the logging to the application is not an option there, because the application's own error rendering is precisely what broke.

This also changes one pre-existing path. safe_default — an exception raised inside a rescue_from block that nothing else handles — had the identical blind spot, setting only grape.exception. It now sets rack.exception as well. Fixing one of two identical paths seemed worse than fixing both, but it does mean this PR touches shipped behaviour beyond the rendering failure. Its deliberate silence is left alone, since there a rescue_from :internal_grape_exceptions handler can still own the response.

Backward compatibility

UPGRADING entry added. Tests asserting expect { get '/' }.to raise_error on a rendering failure no longer see it raised; error trackers above Grape keep reporting it via rack.exception with no application change. Exceptions that no rescue_from matches still propagate exactly as before.

Test plan

• Six examples in error_spec.rb covering both fallback tiers, grape.exception, rack.exception, rack.errors, and the invariant that an unrescued exception still propagates; verified the behavioural ones fail without the lib/ change.
• End-to-end check with a tracker middleware mounted above Grape (table above), on master and on this branch.
• Full RSpec suite passes locally (2575 examples, 0 failures).
• RuboCop clean.
• CI green.

🤖 Generated with Claude Code

[github-actions]

Danger Report

No issues found.

View run

[dblock]

If Grape is mounted inside another stack like Rails or Sinatra, which it usually is, this causes a different 500 than before. This makes me a little uneasy. Is there a strong argument to change the escape behavior?

[ericproulx]

You were right to push on this, and the concrete cost is worse than "a different 500": mounted in Rails, the exception stopped being reported. I had it on env['grape.exception'], but that's our own key — nothing reads it. I measured it with a tracker middleware above Grape:

	tracker observes
master	raised: JSON::GeneratorError
this PR, as you reviewed it	nothing

So a rendering failure Sentry used to report became an unremarkable 500. That's a real regression and it's fixed now: the exception is also published on env['rack.exception'] — the convention for an exception that was handled rather than raised, which sentry-ruby collects as env['rack.exception'] || env['sinatra.error'] — plus a write to rack.errors so it lands in the log with no tracker installed. Trackers report it again with no application change.

On whether there's a strong argument for changing the escape behavior — two:

The trigger is client-controlled. The repro is an invalid UTF-8 byte in the path, echoed through e.message into a rescue_from payload. Any API whose handler interpolates request-derived text can be made to defeat its own error handling by a crafted request. That's the part I'd weigh most.

Rails already does exactly this. ActionDispatch::ShowExceptions#render_exception tries the app's own error rendering and falls back to a hardcoded [500, text/plain, "500 Internal Server Error\n..."] when that rendering is itself broken — same two tiers, same status and content type, and it calls the fallback "failsafe" too. It also writes to $stderr there, for the reason above: you can't defer the logging to the application when the application's renderer is what broke. Sinatra is the counterexample — it propagates — but it also defaults raise_errors on, so it never claimed to contain errors the way rescue_from :all does.

Worth noting the swallow policy isn't new either: safe_default already answers a framework 500 for an unrecognised exception raised inside a rescue_from block. This PR closes the last hole in it rather than opening one.

One thing to flag, since it's scope I added after your review: safe_default had the same rack.exception blind spot, so it now sets that key too. Fixing one of two identical paths seemed worse than fixing both, but it does mean this touches a shipped path. Happy to split it out if you'd rather keep the diff to the rendering failure.

[ericproulx]

Split the safe_default change out into #2855 as suggested — this PR is now strictly the rendering failsafe. #2855 is based on this branch since it needs Grape::Env::RACK_EXCEPTION, so it rebases to nothing once this merges.

[dblock]

This still makes me uneasy. Grape is just a rack middleware, by attempting to rescue StandardError we abort the rack middleware stack as it's intended. But maybe I'm overthinking it?

Is there an easy way to allow for the old behavior?

Btw, copilot suggests unwrapping this a little like so:

def render_response(payload)
  rack_response(payload.status, payload.headers, format_message(payload))
rescue StandardError => error
  record_rendering_failure(error)
  render_failsafe_response
end

def render_failsafe_response
  headers = { Rack::CONTENT_TYPE => content_type }
  payload = failsafe_payload(headers)

  rack_response(FAILSAFE_STATUS, headers, format_message(payload))
rescue StandardError
  rack_response(
    FAILSAFE_STATUS,
    { Rack::CONTENT_TYPE => FAILSAFE_CONTENT_TYPE },
    FAILSAFE_MESSAGE
  )
end