Replace pg_search with activerecord query #351
Conversation
| @@ -40,6 +26,17 @@ def status(param) | |||
| param == 'active' ? where(active: true) : where(active: false) | |||
| end | |||
|
|
|||
| def keyword(k) | |||
| tsquery = <<-SQL | |||
| (to_tsquery('english', ''' ' || '#{k}' || ' ''' || ':*')) | |||
There was a problem hiding this comment.
This seems to introduce an SQL injection vector since #{k} is not escaped in any way. I know that tsquery is only interpolated into the SELECT and WHERE clause below, but I believe this is still exploitable.
There was a problem hiding this comment.
Good catch! I believe I just need to replace '#{k}' with sanitize(k).
There was a problem hiding this comment.
I think sanitize is for HTML. One of the methods in here might help though: http://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html
There was a problem hiding this comment.
Actually, I guess you're talking about this one: http://apidock.com/rails/ActiveRecord/Sanitization/ClassMethods/sanitize
Not sure why it's not listed on the previous page.
There was a problem hiding this comment.
Apparently it has a #:nodoc: on it: https://github.com/rails/rails/blob/d5902c9e7eaba4db4e79c464d623a7d7e6e2d0e3/activerecord/lib/active_record/sanitization.rb#L11
There was a problem hiding this comment.
Ha! I was just gonna say the same thing. I wonder why. From what I can tell, all it does is place quotes around the query, which is the same as what I have now: '#{k}'.
There was a problem hiding this comment.
Do you have an example of how this can be exploited so I can test it?
There was a problem hiding this comment.
I was just re-reading the Rails Security Guide a couple days ago and it has some good examples: http://guides.rubyonrails.org/security.html#sql-injection
It would take me a while to figure one out, but think you'd basically want to close the quoted string and the function call to to_tsquery, add in whatever other fields are needed plus a new FROM clause, then use -- to comment out the rest of the generated query.
The generated SQL is still going to be a SELECT clause, so an attacker probably couldn't destroy the DB with a DELETE, but I think it would be possible to construct a query that exposes the admins or users table.
There was a problem hiding this comment.
Looks like it eventually delegates here in the case of a String: https://github.com/rails/rails/blob/64b09823e6a6b1e19218d3fd815bb65cd2e44f1e/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb#L39
Why:
We're only using the pg_search gem for the keyword search, and the
query that it generates is not that complicated. Upgrading to the
latest version of the pg_search gem breaks our tests. So, instead of
having to rely on a third party, we can generate the same query
ourselves using activerecord, and reduce our dependencies with the
same stone.
How:
To figure out what the proper query was, I fired up the Rails console
and ran `Location.keyword('food')`. That gave me the SQL query I
needed, which I then converted to activerecord so that the result
would return an ActiveRecord::Relation that can be chained with other
scopes.
monfresh
force-pushed
the
replace-pg-search
branch
from
7b6e11c to
ae238e8
Compare
|
@md5 Check out the latest changes. I made the query even simpler thanks to help from my 18F colleagues, and sanitized the query in the |
|
LGTM |
Replace pg_search with activerecord query
Why:
We're only using the pg_search gem for the keyword search, and the query that it generates is not that complicated. Upgrading to the latest version of the pg_search gem breaks our tests. So, instead of having to rely on a third party, we can generate the same query ourselves using activerecord, and reduce our dependencies with the same stone.
How:
To figure out what the proper query was, I fired up the Rails console and ran
Location.keyword('food'). That gave me the SQL query I needed, which I then converted to activerecord so that the result would return an ActiveRecord::Relation that can be chained with other scopes.