active_record: Type cast booleans and durations for string columns. #16069
Conversation
| when ::String then ::String.new(value) | ||
| when true then "1" | ||
| when false then "0" | ||
| else super |
There was a problem hiding this comment.
Could we just to_s everything at this point? Or whitelist the things that we shouldn't?
AFAICS, something along those lines is necessary in order to honour type_cast_for_database's contract: we're obliged to accept any value, but only permitted to return an instance of one of a few classes.
There was a problem hiding this comment.
I want that to be what happens, but its not possible yet.
There was a problem hiding this comment.
Ideally the contract is that we only accept values that type_cast_from_user or type_cast_from_database return. Method like where and update_all bypass normal type casting at the moment.
The reason this can't to_s everything yet is because of some weirdness in serialized attributes which I plan on looking into today.
There was a problem hiding this comment.
(I think where is the only method that is "safe" to pass user input to which doesn't go through type_cast_from_user)
There was a problem hiding this comment.
Yup, it would definitely be safer if we could to_s by default.
|
... or is the problem that we're not going through any |
|
|
||
| def test_where_with_integer_for_binary_column | ||
| count = Binary.where(:data => 0).count | ||
| assert_equal 0, count |
There was a problem hiding this comment.
This is failing on mysql: https://travis-ci.org/rails/rails/jobs/29233923#L192 (but not mysql2)
There was a problem hiding this comment.
Yup, I initially added the .to_s before unpack was called in ActiveRecord::Type::Binary::Data#hex so that I could test binary columns, but updated this pull request to call .to_s in the ActiveRecord::Type::Binary::Data constructor instead to fix this vulnerability that this test uncovered in the mysql adapter. I think the difference is that the mysql adapter uses type casting without quoting, so was skipping the hex method which was called for quoting.
active_record: Type cast booleans and durations for string columns.
|
Thanks for checking & rebasing this! 💛 @sgrif I'll leave you to opine on and, if necessary, adjust exactly where the casting happens... but these tests sure sound like things we want passing. |
|
I'm concerned that this sets a precedent for what we cast in |
|
"Oh, you're trying to look up a string column by an interger? I should probably cast the column. Casting the lookup value sounds like too much work, let's just compare everything to 0 instead. |
" Scumbag mysql...
|
Thanks for the quick review and merge! |
@rafaelfranca, @sgrif, @arthurnn this is a basically a rebase of pull #14894 which was closed without the problem being completely fixed.
Problem
Pull #16037 partially fixed the problem is described in Potential Query Manipulation with Common Rails Practises by quoting Numeric types, however, that pull request missing the need to quote boolean and ActiveSupport::Duration types.
For example:
can be exploited by passing in
falseforparams[:token]through a JSON body causing the queryto match any login_token that starts with a non-digit character.
Quoting for these additional types was properly handled in mistakenly closed pull request #14894.
Solution
ActiveRecord::Type::String#type_cast_for_database has been updated to handle these types. Tests are included for these missed types as well as the numeric types which are already quoted properly to prevent regressions.