I’m going to try for a constructive comment, but first I’m having trouble getting past the assertion that
The most common pattern that I’ve seen across many codebases is a form that manages multiple selections…
Followed by a completely un-sanitized call to constantize on an http param?! The entire world has been beating the drum of “never trust the client” and “sanitize everything” since the 90’s. How confident are you that this is the most common pattern, because it reads like lunacy.
I know the focus of your post wasn’t this sort of architectural detail, so this isn’t a criticism. It would be even simpler to define it via a hash, maybe as a constant somewhere, and use #fetch to select the correct class. It feels much more “tell, don’t ask” and you get the exception raising for free (i.e. no conditional nil check):
# Some file where a constant definition is appropriate.
ALERTS = {
'info' => InfoAlert,
'warn' => WarnAlert,
'error' => ErrorAlert
}
# The controller code that builds an alert.
ALERTS.fetch(params.dig(:alert, :type)).new(params.dig(:alert, :value))
A further refinement might wrap the constant in a method call to some place that handles settings related to Alerts and performs the fetch. ALL_CAPS constants always feel weird in Ruby (to me anyway).
(edited to swap two sentences into a more logical order and clean up some grammar)
@swifthand You’re completely correct re: never trust the client and such. I’m a big fan of this quote by thomas ptacek:
I am wary of software security advice that leads with “don’t trust user input”, or revolves around “validate user input”. That principle has been the core of software security strategy for going on 20 years, and has bought us very little. In the real world, we have to tstart by acknowledging that the verb “trust” is situational, and that in some circumstances virtually all user input is “trusted” somehow. You could phrase this less tactfully as “Validate user input? No shit? Now what?”
The security industry has banged the drum of sanitize everything and yet this stuff still slips by! And I’m in agreement with you that the majority of code bases aren’t written like the example I posted, however I’ve seen variations of that pattern in live code bases. Shitty code exists everywhere! Likely not written by those that read these types of posts on reddit / lobsters / et al.
Agreed. I am perplexed about String#constantize’s docs not having a big warning about it being a possible code injection vector, instructing that the string should always be validated. Or at least being tagged as “insecure/unsafe” in some way.
Skimming through the Rails docs, it seems that these security matters are not given the importance they should. Even ActiveRecord’s whereonly says:
Note that building your own string from user input may expose your application to injection attacks if not done properly. As an alternative, it is recommended to use one of the following methods.
I’m going to try for a constructive comment, but first I’m having trouble getting past the assertion that
Followed by a completely un-sanitized call to
constantize
on an http param?! The entire world has been beating the drum of “never trust the client” and “sanitize everything” since the 90’s. How confident are you that this is the most common pattern, because it reads like lunacy.I know the focus of your post wasn’t this sort of architectural detail, so this isn’t a criticism. It would be even simpler to define it via a hash, maybe as a constant somewhere, and use
#fetch
to select the correct class. It feels much more “tell, don’t ask” and you get the exception raising for free (i.e. no conditional nil check):A further refinement might wrap the constant in a method call to some place that handles settings related to Alerts and performs the fetch. ALL_CAPS constants always feel weird in Ruby (to me anyway).
(edited to swap two sentences into a more logical order and clean up some grammar)
@swifthand You’re completely correct re: never trust the client and such. I’m a big fan of this quote by thomas ptacek:
The security industry has banged the drum of sanitize everything and yet this stuff still slips by! And I’m in agreement with you that the majority of code bases aren’t written like the example I posted, however I’ve seen variations of that pattern in live code bases. Shitty code exists everywhere! Likely not written by those that read these types of posts on reddit / lobsters / et al.
Agreed. I am perplexed about
String#constantize
’s docs not having a big warning about it being a possible code injection vector, instructing that the string should always be validated. Or at least being tagged as “insecure/unsafe” in some way.Skimming through the Rails docs, it seems that these security matters are not given the importance they should. Even ActiveRecord’s
where
only says: