ruby-grape / grape / 32106065653

97%

master: 97%

Build Type

Pull #2840

github

Committed by ericproulx

Commit Message

Answer 500 when an error response cannot be rendered

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. This is the
shape ActionDispatch::ShowExceptions#render_exception already has in Rails,
down to the text/plain last resort.

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.

Swallowing an exception must not make it invisible. Grape put the exception on
env['grape.exception'], but that is a Grape-private key no tracker reads, so a
rendering failure that Sentry used to report as a raised exception would have
become an unremarkable 500. Publish it on env['rack.exception'] as well — the
convention for an exceptio... (continued)

Pull Request Pull Request #2840: Answer 500 when an error response cannot be rendered

Coverage Stats

1216 of 1307 branches covered (93.04%)

Branch coverage included in aggregate %.

23 of 23 new or added lines in 3 files covered. (100.0%)

5 existing lines in 1 file now uncovered.

3855 of 3922 relevant lines covered (98.29%)

26873.01 hits per line