Handle param-parsing errors from Rack that happen low down the stack

[greysteil]

Move ActionDispatch::ShowExceptions and ActionDispatch::DebugExceptions lower down the default middleware stack so they can catch Rack errors raised from Rack::MethodOverride. Also adds the two errors Rack recommends integrators serve 400s for (Rack::Utils::ParameterTypeError and Rack::Utils::InvalidParameterError) to the rescue_responses hash in ExceptionWrapper.

We've been seeing some Rack::Utils::InvalidParameterErrors coming from Rack::MethodOverride (spec included), and it feels like they should be handled by Rails' exception handling, or by any custom exception_apps.

[greysteil]

@rafaelfranca - I'm not sure who to ask to review this. Any advice? Doesn't look like anyone's touched the default middleware stack since 2013!

[matthewd]

👎

If we do this, the exception handlers now exist outside the logger, and are not going to see the correctly overridden method. (And miss out on the request ID, which at least some error reporting mechanisms surely use.)

An application can obviously choose to make this trade-off locally, but I think it's fairer to default to invoking the error handler in a more realistic-looking environment. (I also suspect you'd need to be Rather Careful Indeed to write a handler that would function normally on a request so badly-formed that Rack::MethodOverride chokes.)

[greysteil]

@matthewd - since the env is mutated by ActionDispatch::RequestId and by Rack::MethodOverride you'll still have the new values as long as the request made it past those middleware classes. It's only if the request bombs out early that they'll be missing, in which case you probably still want to catch the error, no?

[greysteil]

Also, Rack::MethodOverride chokes in a relatively polite way - it calls Rack::Utils.parse_query, which will raise InvalidParameterError if anything goes wrong. That's pretty easy to catch and serve a 400, hence the Rack recommendation to do so

[greysteil]

Actually, there's no reason ActionDispatch::RequestId can't come before the exception handlers - it won't cause Rack to parse the user's params, so it's safe. I've added a commit above to keep it before the exception handlers.

@matthewd - does any of the above change your opinion?

[matthewd]

Relying on the "it's all just a big hash" situation to allow invoked-later middleware to munge the request out from under us makes me feel dirty. I also wouldn't count on it working with any future "rack 2", though that's not a direct concern today.

I think I'd rather treat Rack::MethodOverride as too low-level to be allowed to raise such an error: call it a bug that it doesn't rescue and silently continue processing.

But I'll back off to neutral. And I'm 👍 on the ExceptionWrapper change.

[greysteil]

@matthewd - great, thanks. I've put in a PR to Rack to silently continue processing there - let's see what the outcome is from that.

[matthewd]

It appears Team Rack has kindly merged your PR (and backported to 1-6-stable).

If you want to trim this down to just the ExceptionWrapper change, this should be good to go too. 💙

[greysteil]

@matthewd - updated, thanks! ❤️

[matthewd]

❤️ 💚 💙 💛 💜