Raise informative exceptions on invalid encodings #21924
Conversation
Prior to this change, given a route:
# config/routes.rb
get ':a' => "foo#bar"
If one pointed to http://example.com/%BE (param `a` has invalid encoding),
a `BadRequest` would be raised with the following non-informative message:
ActionController::BadRequest
From now on, `ActionController::InvalidParameterEncoding` is raised instead
and the message displayed is:
Invalid parameter encoding: hi => "\xBE
Fixes #21923.
|
r? @schneems (@rails-bot has picked a reviewer for you, use r? to override) |
|
|
agis
changed the title
|
This is still lacking tests; I will add them if it's on the right direction. A more backwards-compatible solution would be to skip the new exception class and just adapt P.S. I've targeted this to |
| @@ -59,7 +59,7 @@ def check_path_parameters! | |||
| path_parameters.each do |key, value| | |||
| next unless value.respond_to?(:valid_encoding?) | |||
| unless value.valid_encoding? | |||
| raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}" | |||
There was a problem hiding this comment.
Is not this the same thing that is being done in the new code but just with a different class? Why the different class is needed?
There was a problem hiding this comment.
Not exactly. The way BadRequest is designed, it acts as a wrapper for other exceptions. For example see it's intented usage here and here.
But in this case (ie. the removed line), it was used differently: By passing only the message as the type argument as we were doing, the actual message was lost, since the exception ends up just calling super(). Hence the non-informative message described in #21923.
That said, I understand that this change breaks backwards compatibility since people may already be rescuing BadRequest for encoding errors. So I see two options here:
- close this PR and re-open it based on
master - keep this PR open and adapt the
BadRequestclass it so that the same exception is raised but with the informative message.
As I don't think this is urgent, I would go with (1).
What do you think?
There was a problem hiding this comment.
@rafaelfranca I've updated my comment above, with some proposals. I believe this is something that we could and should fix, at least in master.
WDYT?
There was a problem hiding this comment.
There's definitely something to be fixed; this line clearly thinks it's specifying a message, which is not actually the case.
And yes, as the bot pointed out, we should be working against master -- any necessary backporting will be handled after that.
As for the choice of how we actually solve the problem, I'm not sure... I'm hesitant to go with the code as proposed, partly because of the compatibility change you've already mentioned, but also because this addresses the "exception can't be used 'normally' because it has magic / non-message parameters" problem by introducing a new exception class which also has magic / non-message parameters.
Finally, I wonder whether we should really be revisiting the whole original_exception arrangement, everywhere it's used, given that we can now rely on cause.
There was a problem hiding this comment.
@matthewd Thanks for the insight. Agreed so I'll open a new PR soon.
agis
changed the title
Prior to this change, given a route:
If one pointed to http://example.com/%BE (param
ahas invalid encoding),a
BadRequestwould be raised with the following non-informative message:From now on,
ActionController::InvalidParameterEncodingis raised insteadand the message displayed is:
Fixes #21923.