If, in your view, you are expecting params[:name] to be a string, but
actually rails has parsed it into {"."=>“1234”} (or something more
malicious), then currently
<%= sanitize(params[:name]) %> blows up because the hash does not
respond
the expected methods from the sanitize call.
I could put in code to check that the params values I am sanitizing are
strings, but it seems like it would be better for sanitize to handle
that,
and perhaps just return the empty string if the processing of the input
raises an exception.
If, in your view, you are expecting params[:name] to be a string, but
actually rails has parsed it into {"."=>“1234”} (or something more
malicious), then currently
<%= sanitize(params[:name]) %> blows up because the hash does not
respond
the expected methods from the sanitize call.
I could put in code to check that the params values I am sanitizing are
strings, but it seems like it would be better for sanitize to handle
that,
and perhaps just return the empty string if the processing of the input
raises an exception.
Hum. It seems to me that “blowing up” is the right thing to do in this
scenario. More precisely an exception should be raised indicating a
programmer mistake of passing an illegal argument to a method expecting
a string.
In this case it is user (hacker, scanner, etc.), not the programmer, who
has passed the illegal argument. I don’t think that should result in a
500
server error. To avoid that, either the programmer has to check each
input
parameter to make sure it is a string, or something like sanitize has to
make the parameter safe.
It’s unclear to me why you wouldn’t want a 500 ISE here. Silently
swallowing ArgumentError or NoMethodError is a terrible idea, since it
also
can obscure real bugs.
In this case it is user (hacker, scanner, etc.), not the programmer, who has
passed the illegal argument. I don’t think that should result in a 500
server error. To avoid that, either the programmer has to check each input
parameter to make sure it is a string, or something like sanitize has to
make the parameter safe.
It’s your problem and even more so your job to enforce types, not
sanitizes problem to enforce your type. Sanitize is a helper, it is
not part of your normal routing therefore it should not have to allow
you be more oblivious as to what is going on in your software and how
to handle and think about intentional/misintentional malicious users.
It is because I am trying to distinguish between “real bugs” and bad
input
(a 400 error) that I don’t want this to be a 500 error. I have a rescue
action that sends me an email when a 500 error occurs (though it limits
the
number it sends) because if there is a bug in the program that users are
running into, I want to know about it. I don’t want to get those alerts
every time someone intentionally sends bad input. It seems to me that
the
convenience of having sanitize just handle the exception outweighs the
possibility of missing bugs involving sanitize calls (which seems slight
to
me, though maybe I am not thinking of some use case).
Thanks for the suggestion of adding a rescue. I might just patch
sanitize
locally so I don’t have to type the rescue each time.
It is because I am trying to distinguish between “real bugs” and bad input
(a 400 error) that I don’t want this to be a 500 error. I have a rescue
action that sends me an email when a 500 error occurs (though it limits the
number it sends) because if there is a bug in the program that users are
running into, I want to know about it. I don’t want to get those alerts
every time someone intentionally sends bad input. It seems to me that the
convenience of having sanitize just handle the exception outweighs the
possibility of missing bugs involving sanitize calls (which seems slight to
me, though maybe I am not thinking of some use case).
His suggestion was not to rescue the error, it was an example of how
you could. To rescue any error in an app is asking for edge cases
unless they are specific errors for specific actions such as a
RoutingError. The proper way to handle this is to type the input and
return long before you even hit sanitize and have to rescue for: 1.)
Better performance on errors an 2.) Better ability to know how your
app is behaving.
On Wed, Sep 11, 2013 at 3:36 PM, Paul E. G. Lynch [email protected]
wrote:
If, in your view, you are expecting params[:name] to be a string, but
actually rails has parsed it into {“.”=>“1234”} (or something more
malicious)
Params are strings by definition; can you provide a test case/code
that demonstrates where this is not the case?
Not necessarily the case. For example the create and update actions in a
users_controller will likely contain the user model in the params hash
as a hash keyed by :user:
On Thu, Sep 12, 2013 at 11:52 AM, Robert W. [email protected]
wrote:
Params are strings by definition; can you provide a test case/code
that demonstrates where this is not the case?
Not necessarily the case. For example the create and update actions in a
users_controller will likely contain the user model in the params hash…
Meh, you’re right. I was thinking of the over-the-wire definition of
HTTP parameter name/value pairs but yes, at the controller level
Rails has already decided certain representations are more than
that.