I think there’s an opportunity to reduce additional false positives for Dangerous Query Method deprecations/errors, but am cognizant of the trade-off with not writing a sql parser as well as how long it’s existed as is.
The first potential opportunity would be allowing nested functions. Similar to Allow column name with function (e.g. `length(title)`) as safe SQL string by kamipo · Pull Request #36448 · rails/rails · GitHub, it seems reasonable to allow functions that accept other functions (e.g.
length(trim(title))). As an example (though all DB adapters seem like they’d be able to be changed similarly), the change in
activerecord/lib/active_record/connection_adapters/abstract/quoting.rb could look like:
- ((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\) + ((?:\w+\.)?\w+ | \w+\((?:|\g<2>)\))
Collation seems to me, less reasonable within the rails codebase, though I’m also happy to add this in a PR if I should. In that case, I might need help implementing this outside of the postgres adapter though. In the simplest case, this might look like adding
above the ASC/DESC portion of the regex.
- Would either or both of these be things we’d want in rails, or are they too niche/are we happier leaving them as it’s been through multiple rails versions?
- Whether or not we want them in rails, I’d appreciate concerns about either opening up injection vectors I’m not thinking about?
- PR accepting non-nested functions: Allow column name with function (e.g. `length(title)`) as safe SQL string by kamipo · Pull Request #36448 · rails/rails · GitHub
- Deep background on deprecation and false positives: "Dangerous query method" deprecation warning matches `random()` function call · Issue #32995 · rails/rails · GitHub
COLUMN_NAMEfor the first and
Thanks for any thoughts!