Arel.sqlを付けるだけじゃダメ!? Railsで"Dangerous query method …”の警告が出たときの対応方法

TL; DR（長いので最初に結論）

この記事で言いたいことはこちらです ↓

Rails 5.2で"Dangerous query method ..."の警告が出たからといって、機械的にArel.sqlを適用してはいけません。
警告が出たらまず、該当したコードにSQLインジェクションの危険性がないか見極めることが大事です。
もしその危険性がある場合は、入力値を検証するなどして危険性を取り除いてから、Arel.sqlを適用してください。

「え？何を言ってるのかさっぱりわかりません」という人は、この続きを読んでください。

はじめに

Rails 5.1で開発していたRailsアプリをRails 5.2にアップグレードすると、以下のような警告メッセージが大量に出力されることがあります。

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "LOWER(books.title)". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). (called from block in class:Book at /Users/jnito/dev/sandbox/arel-sql-sandbox/app/models/book.rb:5)

「うおー、なんじゃこりゃ！？」と思ってエラーメッセージでググると、以下のように「Arel.sqlメソッドでSQLを囲んでください」という解決策が見つかるはずです。

# Rails 5.1以前の場合
Book.order("LOWER(books.title)")

# Rails 5.2の場合
Book.order(Arel.sql("LOWER(books.title)"))

実際、このようにすると警告は消えます。
しかし、「そうか、警告が出たらArel.sqlで囲めばいいんだな」と考えて機械的に適用していくのは とても危険 です。

というわけで、この記事ではArel.sqlを機械的に適用するのがなぜ危険なのか、そしてこの警告が出たらどのように対応するのが適切なのかについて説明します。

まずチェック！あなたはこの記事を読むべき人？読まなくていい人？

本文へ進む前に、あなたがこの記事を読むべき人かそうでないかを確認しておきましょう。

以下の質問に対する答えがすべてYESになった人は、この記事を読む必要はありません。
逆に、ひとつでもNOがついた人は、この記事をちゃんと読むべき人です。

• SQLインジェクションが何かを理解しているか？
• 警告メッセージの内容をちゃんと理解しているか？
• Rails 5.2から上記の警告が入るようになった背景や目的を理解しているか？
• 上記の警告が出た場合、1つずつ警告が出たコードを確認しながら修正しているか？（機械的、盲目的に修正していないか？）

いかがでしょう？意外と全部YESになった人は少ないんじゃないでしょうか。

ひとつでもNOが付いた人はこのまま本文を読み進めてください。

知識1: SQLインジェクションを理解する

この記事を読むためにはいくつかの前提知識が必要になるので、それを順に説明していきます。

"Dangerous query method ..."の警告メッセージは、SQLインジェクションと呼ばれるセキュリティリスクに関連しています。 ですので、まず、SQLインジェクションが何かを理解しなければなりません。

SQLインジェクションを知らない方は、Railsガイドの以下の記事をしっかり読んで理解してから、この記事に戻ってきてください。

Rails セキュリティガイド - SQLインジェクション | Rails ガイド

なお、SQLインジェクションに限らず、Railsガイドの「Rails セキュリティガイド」の章はRails開発者なら全員必読ですので、章全体をちゃんと読んで理解しておきましょう。

知識2: 警告メッセージの内容を理解する

次に、冒頭で示した"Dangerous query method ..."の警告メッセージをもう一度見ていきます。

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "LOWER(books.title)". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). (called from block in class:Book at /Users/jnito/dev/sandbox/arel-sql-sandbox/app/models/book.rb:5)

ちょっとごちゃごちゃして、日本人には読みづらいですね。なので、ついつい「ググって解決」で終わらせたくなる気持ちは理解できますが、ここで一度、ちゃんとメッセージの内容を理解しておきましょう。

上のメッセージを翻訳すると次のようになります（読みやすくするために改行も入れています）。

非推奨な実装に関する警告:
危険なクエリメソッド（引数として生SQLが渡されたメソッド）が属性名以外の引数で呼び出されました: "LOWER(books.title)"
属性名以外の引数はRails 6.0では使用できなくなります。
このメソッドにはユーザーによって入力された値（リクエストパラメータやモデルに保存された値など）を渡すべきではありません。
既知の安全な値であれば、Arel.sql()を使ってその値をラップしてから渡してください。
（メソッドが呼び出された場所の情報）

しかし、このメッセージを読んでもいまいちピンと来ないかもしれません。そこで、この変更が導入された背景や目的を確認していきましょう。

知識3: この警告が出るようになった背景や目的を理解する

この警告は以下のpull requestで導入されました。

Disallow raw SQL in dangerous AR methods by mastahyeti · Pull Request #27947 · rails/rails

簡単にまとめると、以下のようなコードはSQLインジェクションの危険があるのでそれを防止する、というのがこのpull requestの目的です。

class PostsController < ApplicationController
  def index
    @posts = Post.order(params[:order_by]).limit(10)
  end
end

上のコードは先ほどの警告メッセージでいうところの「ユーザーによって入力された値（params[:order_by]）」を「危険なクエリメソッド（order）」に渡したパターンに該当します。

ですから、これをこうやってArel.sqlで囲めば、ほら安全・・・

# いやいや、ちょっと待って！！
Post.order(Arel.sql(params[:order_by]))

ではありません！！！

これだとSQLインジェクションの危険性は残ったままになります。

Arel.sqlを付けることは一種の安全宣言

そもそも、"Dangerous query method ..."の警告は「その呼び出し方は古いから新しい書き方に直せ」と言っているのではありません。

そうではなく、「その値が安全かどうかを確認した上で、安全なら（＝つまり、既知の安全な値なら）、Arel.sqlで囲め」と言っているのです。

逆に言うと、安全でないなら、安全であることを保証できる何かしらの策を講じた上で、Arel.sqlで囲む必要があります。

# 何らかの方法で入力値を検証したり無害化して安全を保証する
order_by = validate_or_sanitize(params[:order_by])
Posts.order(Arel.sql(order_by))

Arel.sqlで囲むのはある意味、「私（開発者）はこの引数が安全であることを確認しましたよ」という一種の安全宣言です。
（もちろん、本当は文字列からArel::Nodes::SqlLiteralのインスタンスに変換している、というのが正確な説明になりますが）

そして、"Dangerous query method ..."の警告は、「まだ安全かどうか確認が済んでいないコードを見つけたから、ちゃんとチェックしてね」とRailsが警告しているようなもの、と考えた方がいいかもしれません。

よって、機械的、盲目的にArel.sqlを適用して警告を消していくのは間違いである、ということになります。

さて、ここまでがこの記事を読むために必要な前提知識です。
このあとさらに、この警告の詳細と適切な対応方法について深く掘り下げていきます。

警告が出ても特別な対策が不要なケース

"Dangerous query method ..."の警告はorder、reorder、pluckに対して、生SQLの文字列を渡したときに発生します。ただし、文字列以外の値（シンボルやハッシュ）を渡した場合や、文字列にModelの属性名しか含まれていない場合は警告が出ません。

# 文字列ではないので警告が出ない
Book.order(:title)
Book.order(title: :desc)

# 文字列を渡しているが、Modelの属性名（テーブルのカラム名）しか
# 含まれないので警告が出ない
Book.order("books.title")

警告が出るのは以下のように属性名以外の文字列（たとえばSQL関数など）が含まれていた場合です。

# LOWER関数を使ってタイトルを全部小文字にしてから並び替える
# => LOWERは属性名以外の文字列なので警告が出る
Book.order("LOWER(books.title)")
# DEPRECATION WARNING: Dangerous query method ...

しかし、"LOWER(books.title)"はただの文字列リテラルであり、危険な値が混入する余地がありません。
こういったケースでは単純にArel.sqlを適用しても問題ないことになります。

# 危険な値が混入する余地がないので、そのままArel.sqlで囲むだけで良い
Book.order(Arel.sql("LOWER(books.title)"))

対策が必要な（＝実際にSQLインジェクションが発生しうる）ケース

一方、先ほども見たように、リクエストパラメータなど誰がどのような値を入力するかわからない値については、何らかの対策を講じた上でArel.sqlを適用する必要があります。

以下はSQLインジェクションが発生しうるコード例です。

class BooksController < ApplicationController
  def index
    # リクエストパラメータには誰がどんな値を入れてくるのかわからないので危険！！
    # => こういうケースでは機械的にArel.sqlを適用してはいけない
    @books = Book.order(params[:order_by])
  end
end

SQLインジェクションの実例を見てみる

実際の危険性を理解してもらうために、上のようなコードでどのようなセキュリティ問題（SQLインジェクション）が発生するのか説明しましょう。

（注: 以下のSQLインジェクションの例は https://rails-sqli.org/ に載っていたサンプルコードを参考にしています。）

たとえば、booksテーブルに以下のようなデータが保存されていたとします（説明を簡単にするために、正規化はあえて崩してあります）。

title	author_name	author_email
Alice's book	Alice	[e-mail]
Bob's book	Bob	[e-mail]
Carol's book	Carol	[e-mail]

このとき、params[:order_by]に次のような値が渡されると、「メールアドレスが"x"で始まる著者（つまりCarolの本）」を先頭に持ってくることができます。

-- メールアドレスの1文字目が'x'なら0、それ以外なら1を返す、SQLのCASE式
CASE SUBSTR(books.author_email, 1, 1)
  WHEN 'x' THEN
    0 
  ELSE
    1 
END

以下はrails consoleで試した場合の実行例です。

sql = <<SQL
CASE SUBSTR(books.author_email, 1, 1)
  WHEN 'x' THEN
    0 
  ELSE
    1 
END
SQL

# 引数sqlの値がそのままORDER BY句に渡される
Book.order(sql).to_sql
#=> DEPRECATION WARNING: Dangerous query method ...
#   SELECT "books".*
#   FROM "books"
#   ORDER BY
#     CASE SUBSTR(books.author_email, 1, 1)
#       WHEN 'x' THEN
#         0 
#       ELSE
#         1 
#     END

# Carolのメールアドレスが"x"で始まるため、"Carol's book"が先頭に来る
# （ただし、Arel.sqlで囲んでいないため、警告も一緒に出る）
Book.order(sql).first
#=> DEPRECATION WARNING: Dangerous query method ...
#   #<Book id: 3, title: "Carol's book", author_name: "Carol", author_email: "[e-mail]", created_at: "2018-12-15 03:31:02", updated_at: "2018-12-15 03:31:02">

このような場合、画面上は本のタイトルと著者名しか出していなくても、悪意のあるユーザーは意図的に並び順を自由に操作するリクエストパラメータを送り込むことで、（非常に手間はかかりますが）著者のメールアドレスを推測することができてしまいます。

Arel.sqlを付けたからといって、SQLインジェクションはなくならない

そして、このようなケースではArel.sqlを機械的に適用してもまったく意味がありません。
Arel.sql自体には悪意のあるコードを見抜いて実行を止めるような機能はないため（単に安全宣言するだけ）、警告が出なくなるだけでSQLインジェクションの危険性は残ったままになってしまいます。

sql = <<SQL
-- 先ほどと同じなので省略
SQL

# Arel.sqlで囲んでも生成されるSQLは同じ
Book.order(Arel.sql(sql)).to_sql
#=> SELECT "books".*
#   FROM "books"
#   ORDER BY
#     CASE SUBSTR(books.author_email, 1, 1)
#       WHEN 'x' THEN
#         0 
#       ELSE
#         1 
#     END

# 生成されるSQLが同じなので、"Carol's book"が先頭に来る動きも変わらない
# （警告が消えるだけ）
Book.order(Arel.sql(sql)).first
#=> #<Book id: 3, title: "Carol's book", author_name: "Carol", author_email: "[e-mail]", created_at: "2018-12-15 03:31:02", updated_at: "2018-12-15 03:31:02">

対応方法の一例: リクエストパラメータを検証してSQLインジェクションを防止する

無用なリスクを増やさないよう、ユーザー入力によって作られた文字列は基本的に、orderのようなクエリメソッドに直接渡すべきではありません。

しかし、どうしてもそのような実装が必要になった場合は、次のように入力値を検証する対策が考えられます（セキュリティ対策上、この方法が最善かどうかは議論の余地がありそうですが）。

class BooksController < ApplicationController
  # 並び替えで指定できるカラムはtitleとauthor_nameだけに限定する
  ALLOWED_ATTRIBUTES = %w(title author_name).freeze

  def index
    # リクエストパラメータが安全であることを検証した上で、Arel.sqlで囲む
    order_by = validate_order_by(params[:order_by])
    @books = Book.order(Arel.sql(order_by))
  end

  private

  def validate_order_by(text)
    unless text.in?(ALLOWED_ATTRIBUTES)
      # 事前に定めた属性名以外はエラーとする
      raise ArgumentError, "Attribute not allowed: #{text}"
    end
    text
  end
end

コラム: 「セレクトボックスだったら好き勝手に文字列を送信されるはずがない」は間違い

もしかすると、プログラミング初心者の方は「テキストボックスになっているならわかるけど、僕のRailsアプリはこんなふうにセレクトボックスになっているから大丈夫！」と高をくくっている人がいるかもしれません。

ですが、残念ながらテキストボックスであろうとセレクトボックスであろうと、HTTPの知識がある人なら自由に文字列を送信できます。
なんなら、テキストボックスやセレクトボックスが表示されていなくても、任意のリクエストパラメータを送信できます。

このあたりの話はWebアプリケーションにおけるセキュリティ対策の基本ですので、「え、そうなの！？」とビックリした人は書籍等でしっかり勉強しておきましょう。

参考書籍: 体系的に学ぶ 安全なWebアプリケーションの作り方 第2版

さらに: 警告が出ない = 絶対に安全、とは限らない

先ほども述べたとおり、"Dangerous query method ..."の警告は「まだ安全かどうか確認が済んでいないコードを見つけたから、ちゃんとチェックしてね」とRailsが警告しているようなものです。

ですが、警告が出ていないからといって、ここで述べたようなSQLインジェクションの危険性がなくなった、と言い切ることはできません。そのような例を以下に3つ挙げます。

例1: 何も確認せずにArel.sqlを付けていった場合

まず、すでに述べたように、危険なコードに対しても機械的にArel.sqlを適用してしまった場合は、セキュリティ上の問題があるにもかかわらず、わざわざ「大丈夫でーす。問題ありませーん！👌」と自分で安全宣言したことと変わりません。

# これではまったく意味がない！！（間違った安全宣言）
@books = Book.order(Arel.sql(params[:order_by]))

ですが、ここまで読まれた方はこの点は大丈夫かと思います。

例2: テストコードが書かれていなかった場合

次に、"Dangerous query method ..."の警告は実際にそのコードを動かさないと出力されません。

この警告に気づくのはRSpecやMinitestで書いたテストを実行するときだと思いますが、テストを全く書いていなかったり、ちょうどそこだけテストパターンが漏れていたりすると、警告は出力されません。

運が良ければ、アプリケーションを実行している最中にコンソールやログに警告が出力されて気づく可能性もあります。
ですが、それでもテストコードの実行に比べると気づく確率は低いのではないでしょうか。

例3: テストコードはあるが、警告が出ないテストパターンしかない場合

最後に、動的な文字列を渡したからといって、必ずしも警告が出るわけではない、という点にも注意が必要です。

「警告が出ても特別な対策が不要なケース」の項で述べたとおり、orderのようなメソッドに文字列を渡しても、そこにModelの属性名（テーブルのカラム名）しか含まれていなかった場合はエラーが出ません。

# リクエストパラメータに入っているのはModelの属性名だけ
params[:order_by]
#=> "books.title"

# この場合はコードを実行しても警告は出ない
# （だが、SQLインジェクションの危険性は十分ある）
@books = Book.order(params[:order_by])

ですので、たとえテストコードが書かれていたとしても、「警告が出ないテストパターン」しか用意していなかった場合は、SQLインジェクションの危険性を見過ごしてしまうことになります。

備考: Rails 6なら大丈夫かも？

ただし、警告メッセージにも書いてあるとおり、Rails 6（本記事執筆時点では未リリース）では警告ではなくエラーになるそうなので、Rails 6であれば例2や例3のようなパターンでSQLインジェクションが起きることは未然に防げそうです。

オプション: Brakemanのような静的コード解析ツールを併用する

「警告が出ない = 絶対に安全、とは限らない」の項で挙げた問題点については、Brakemanのような静的コード解析ツールを導入すると、かなり高い確率で検知できます。

ためしに、Brakeman 4.3.1で以下のようなコードを含む、Railsアプリケーションを検証してみました。
（あくまで検証用のコードなので、ロジック自体に意味はありません）

class BooksController < ApplicationController
  def index
    @books =
      if params[:arel_sql]
        Book.order(Arel.sql(params[:order_by]))
      else
        Book.order(params[:order_by])
      end
  end
end

すると、次のように5行目、7行目双方の問題点を報告してくれました。

$ bundle exec brakeman
Loading scanner...
（省略）

== Warning Types ==

SQL Injection: 2

== Warnings ==

Confidence: High
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Book.order(params[:order_by])
File: app/controllers/book_controller.rb
Line: 7

Confidence: High
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Arel.sql(params[:order_by])
File: app/controllers/book_controller.rb
Line: 5

Circle CIのようなCIツールで毎回Brakemanを実行するようにしておけば、うっかり見落としていたSQLインジェクションの危険性も（そしてさらに、それ以外のセキュリティ上のリスクも！）検知できるはずです。

個人的な見解: でもぶっちゃけ、毎回Arel.sqlを入れる対応ってどうなん？？

さて、ここまで長々とRails 5.2から発生する"Dangerous query method ..."の警告と、その対応方法について説明してきましたが、こうやってあらためてみると、正直、あんまりイケてないなーという気がします。

ここがイケてない: 誤検知が多い、修正結果が美しくない

というのも、まず誤検知（と呼ぶのは変かもしれないけど）が多すぎます。
以下のようなコードは明らかに問題がないのに、いちいち警告されてArel.sqlを付けなければいけません。

# このコードはセキュリティ上、全く問題がないのに警告が出る
Book.order("LOWER(books.title)")
#=> DEPRECATION WARNING: Dangerous query method ...

Arel.sqlを付けていくのは面倒くさいですし、なんか見た目にも美しくありません。

# 警告は消えるものの、面倒くさいし、なんか美しくない（個人の意見です）
Book.order(Arel.sql("LOWER(books.title)"))

せめて、こんな感じならまだ良かったかなとも思うのですが。

# String#html_safeみたいな感じで対応できたら若干スマート？
Book.order("LOWER(books.title)".sql_safe)

ここがイケてない: メリット < デメリットになってしまっている気がする

そして、「警告が出ない = 絶対に安全、とは限らない」の項でも述べたとおり、「機械的に適用されてしまうと全く意味がない」「実行しないと警告が出ない」という点も少し残念です。

SQLインジェクションを理解していない人ほど、機械的にArel.sqlを付けてしまいそうですし、SQLインジェクションを理解している人は元から危険なコードを書かないので、Arel.sqlを付けるぶんだけ余計な手間だけが増えてしまいます。

結局のところ、この警告がうまく機能するのは「SQLインジェクションを理解している人が、ついうっかり危険なコードを書いてしまったとき」ぐらいかもしれません。

もちろん、SQLインジェクションを理解していない人も、この警告を見てSQLインジェクションについて正しく理解するようになる、という効果は期待できます。
ですが、個人的にはそういう人に限って「Arel.sqlを付けたらいいんだな、よし理解した」で終わってしまいそうな気がしています。

参考: Railsのissueにおける同様の議論

実際、このあたりの話はRailsのissueでもいろいろと議論されているようです。

"Dangerous query method" deprecation warning matches `random()` function call · Issue #32995 · rails/rails

まとめ

というわけで、この記事ではRails 5.2から発生するようになった"Dangerous query method ..."の警告の適切な対処方法について説明してきました。

簡単にまとめると、 「機械的にArel.sqlを適用するな。警告が出たらまず、SQLインジェクションの危険性がないか見極めろ」 ということになります。

「しまった、今まで何も考えずにArel.sqlを付けてきた！」という人は、もう一度Arel.sqlでgrep検索してSQLインジェクションの危険性がないか、再チェックしてください。

参考: 本記事の執筆時に作ったサンプルコードはこちらに置いています。