Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord::Base.connection_handler.while_preventing_writes does not prevent all writes #39132

Closed
eikes opened this issue May 3, 2020 · 2 comments

Comments

@eikes
Copy link
Contributor

eikes commented May 3, 2020

The ActiveRecord::Base.connection_handler.while_preventing_writes method does not prevent all writes as the name suggests.

This should be mentioned in the method documentation, particularly because in the original Changelog the author @eileencodes does mention this.

Description

While looking for a way to safely expose a sql query parameter I stumbled upon the while_preventing_writes method. I was surprised that I wouldn't need a another readonly DB user. Not quite sure if this would really work, I did some digging and found that, at least in Postgresql, this method is not sufficient to block all writes.

Steps to reproduce

All of the following queries do write to a Postgresql database in a while_preventing_writes block:

Explain analyze

Postgresql let's you not only explain a query, you can also explain and analyze a query which actually runs it and also analyzes it:

ActiveRecord::Base.connection_handler.while_preventing_writes do
  ActiveRecord::Base.connection.execute "EXPLAIN ANALYZE INSERT INTO roles_users (role_id, user_id) VALUES (1, 1234)"
  ActiveRecord::Base.connection.execute "EXPLAIN ANALYZE UPDATE users SET role = 'admin' WHERE id = 10"
  ActiveRecord::Base.connection.execute "EXPLAIN ANALYZE DELETE FROM users WHERE id = 26"
end

Nested comments

Postgres has a peculiar comment syntax which lets you nest comments within other comments:

/* comment with nesting: /* nested comment */ */

That is why these comments look like SELECT queries to the regular expression READ_QUERY which is supposed to guard against writes, but to the SQL parser they are comments:

ActiveRecord::Base.connection_handler.while_preventing_writes do
  ActiveRecord::Base.connection.execute "/*/**/SELECT*/ INSERT INTO roles_users (role_id, user_id) VALUES (1, 1234)"
  ActiveRecord::Base.connection.execute "/*/**/SELECT*/ UPDATE users SET role = 'admin' WHERE id = 10"
  ActiveRecord::Base.connection.execute "/*/**/SELECT*/ DELETE FROM users WHERE id = 26"
end

Select into

Finally postgres let's you create tables using the SELECT INTO statement, which starts the query with a SELECT but then goes on to write the result into a new table. While it cannot write into an existing table, technically it's still a write query:

ActiveRecord::Base.connection_handler.while_preventing_writes do
  ActiveRecord::Base.connection.execute "SELECT * INTO users_new FROM users"
end

Expected behavior

The writing queries are not applied to the database.

Actual behavior

The queries are written to the database. The regular expression which is supposed to guard against write queries does not catch the write statements.

Discussion

I suspect that parsing SQL with regexes is like parsing HTML with regexes: it cannot work. I suspect that @eileencodes was aware of the limitations of this regular expression approach and mentioned them in the Changelog at the time.

I did spend a fair amount of time trying to fix these issues and I believe that a somewhat more complex Regex will be able to catch some of these issues, but not all.

Considering that the nested comments can have any level of depth, I don't think that they can be solved with Regexes. You will need to parse the SQL and strip out the comments entirely to safeguard against those.

The SELECT INTO issue is complex as well, as there can be a lot of statements between the SELECT and the INTO some of which may be strings or comments, so the regular expression approach is probably not going to work here as well.

I have tests and a fix for the EXPLAIN ANALYZE queries, if you want me to I can open a pull request for those. I will add a pull request for the documentation change.

System configuration

  • Rails master
  • ruby 2.7.0p0
  • vagrant@rails-dev-box
@rails-bot
Copy link

rails-bot bot commented Aug 5, 2020

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-0-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Aug 5, 2020
@rails-bot rails-bot bot closed this as completed Aug 12, 2020
@eikes
Copy link
Contributor Author

eikes commented Aug 12, 2020

I find it somewhat frustrating, that I put considerable effort into pointing out this potentially dangerous misnomer and it does not get addressed. The proposed change in the PR is a minuscule addition to the documentation which may save someone from relying on this method to prevent SQL injections.

eileencodes added a commit that referenced this issue Sep 21, 2020
Adds a note to clarify that `while_preventing_writes` is not meant to be
a replacement for a readonly replica user.

Closes #39132 and #39133

Co-authored-by: Eike Send <eike.send@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants