SQLインジェクションを回避して、SQL内のカラムを動的に変える

##概要
①飲食店クラスがあって、ランチの時間帯とディナーの時間帯を絞り込むメソッドがある
②内部的には、引数に始まりの時間と終わりの時間を受け取って、その条件に合うレコードを取得する
③両者はロジックがほぼ一緒で、違うのはwhere句内のカラムだけ
④同じロジックは書きたくないので、なるべく共通化させたい！

# 飲食店クラス
class Restaurant < ApplicationRecord
  # 飲食店のランチの時間を絞り込むメソッド
  scope :where_by_between_lunch_time, lambda { |start_date, end_date|
    if start_date.present? && end_date.present?
      where('lunch_time BETWEEN ? AND ?', start_date, end_date)
    elsif start_date.present?
      where('lunch_time >= :date', date: start_date)
    elsif end_date.present?
      where('lunch_time <= :date', date: end_date)
    end
  }
  
  # 飲食店のディナーの時間を絞り込むメソッド
  scope :where_by_between_dinner_time, lambda { |start_date, end_date|
    if start_date.present? && end_date.present?
      where('dinner_time BETWEEN ? AND ?', start_date, end_date)
    elsif start_date.present?
      where('dinner_time >= :date', date: start_date)
    elsif end_date.present?
      where('dinner_time <= :date', date: end_date)
    end
  }
end

対策①　引数の検証を行うように

まず、調べていくとカラムを動的に変えようとするとSQLインジェクションの恐れがあることに気付く。
(SQLインジェクションがわからない方はこちらを参考に)

なのでSQLインジェクションにならないように、最初はこの記事を参考に引数の検証を行うように変更！

# 飲食店クラス
class Restaurant < ApplicationRecord
  ALLOWED_COLUMNS = %w(lunch_time dinner_time).freeze

  scope :where_by_between_date, lambda { |column, start_date, end_date|
    # 定数で予め受け取る想定のカラム名を定義して、それ以外が来たらエラー
    unless column.in?(ALLOWED_COLUMNS)
      raise ArgumentError, "Invalid argument: #{column}"
    end

    if start_date.present? && end_date.present?
      where("#{column} BETWEEN ? AND ?", start_date, end_date)
    elsif start_date.present?
      where("#{column} >= :date", date: start_date)
    elsif end_date.present?
      where("#{column} <= :date", date: end_date)
    end
  }
end

確かに、これだと良さそう・・・

しかし、これにしてもBrakemanで怒られてしまう。。。
(Brakemanは、簡単に言うとRailsアプリケーション内のセキュリティをチェックしてくれるGemです)

対策②　サニタイズ処理をする

そんな中、Railsのドキュメントを見ているとquote_column_nameというメソッドを発見！
使えそう！！

ふむふむ。SQLインジェクションにならないようにサニタイズをしてくれるメソッドっぽい。

# 飲食店クラス
class Restaurant < ApplicationRecord
  scope :where_by_between_date, lambda { |column, start_date, end_date|
    # sqlインジェクションにならないようにサニタイズ処理をする
    sanitized_column = ActiveRecord::Base.connection.quote_column_name(column)

    if start_date.present? && end_date.present?
      where("#{sanitized_column} BETWEEN ? AND ?", start_date, end_date)
    elsif start_date.present?
      where("#{sanitized_column} >= :date", date: start_date)
    elsif end_date.present?
      where("#{sanitized_column} <= :date", date: end_date)
    end
  }
end

これだとBrakemanで怒られなくなったし、カラムを動的に変えることができるようになった！！

リファクタ

ここからはリファクタ対応。

元々はこのwhere_by_between_dateにカラム名を第一引数に渡す想定だったけど、
今後呼び出される側のロジックが変更される可能性も想定して、抽象scopeに変更。

# 飲食店クラス
class Restaurant < ApplicationRecord
  # 抽象スコープ
  scope :where_by_between_date, lambda { |column, start_date, end_date|
    # sqlインジェクションにならないようにサニタイズ処理をする
    sanitized_column = ActiveRecord::Base.connection.quote_column_name(column)
    if start_date.present? && end_date.present?
      where("#{sanitized_column} BETWEEN ? AND ?", start_date, end_date)
    elsif start_date.present?
      where("#{sanitized_column} >= :date", date: start_date)
    elsif end_date.present?
      where("#{sanitized_column} <= :date", date: end_date)
    end
  }

  # 下記スコープで抽象スコープを呼び出す形に
  scope :where_by_between_lunch_time, lambda { |start_date, end_date|
    where_by_between_date(:lunch_time, start_date, end_date)
  }

  # 下記スコープで抽象スコープを呼び出す形に
  scope :where_by_between_dinner_time, lambda { |start_date, end_date|
    where_by_between_date(:dinner_time, start_date, end_date)
  }
end

これだとメソッド名も直感的なのでよりわかりやすくなったし、
なにより今後の変更にも強くなった気がする！

追記

対策③　ActiveRecordのメソッドを使う

ご指摘ありがとうございました！

よく考えれば、そもそも生SQLに埋め込まないでActiveRecordのメソッドを使えば問題なかったですね

難しく考え過ぎていました。。。

（まあ、でもRailsのサニタイズ処理に関して勉強になったのでヨシ）

# 飲食店クラス
class Restaurant < ApplicationRecord

  START_DATE_INFINITY = '1000-01-01'
  END_DATE_INFINITY = '3000-01-01'
  
  # 対象のテーブルで不明なcolumnにはUndefinedColumnが出る
  scope :between_date, lambda { |column, start_date, end_date|
    if start_date.present? && end_date.present?
      where(column => start_date..end_date)
    elsif start_date.present?
      # end_dateが無いなら、遠い未来までの範囲
      where(column => start_date..END_DATE_INFINITY)
    elsif end_date.present?
      # start_dateが無いなら、遠い過去までの範囲
      where(column => START_DATE_INFINITY..end_date)
    end
  }
end

おまけ

現在、Float::INFINITYは範囲指定の終わりにしか使用できないんですよね。（Rubyのバグ）

ですが、、、issueにこのように書いてありました！

Ruby 2.6 supports endless range, and ruby 2.7 will support beginless range.

つまり今後リリースされるであろう、Ruby2.7だと始まりに対してもInfinityを使えるようになるみたいです！

その際はこんな感じで書けますね！

# 飲食店クラス
class Restaurant < ApplicationRecord
  scope :between_date, lambda { |column, start_date, end_date|
    if start_date.present? && end_date.present?
      where(column => start_date..end_date)
    elsif start_date.present?
      where(column => start_date.to_time..Float::INFINITY)
    elsif end_date.present?
      # 2.7以降使用できる
      where(column => -Float::INFINITY..end_date.to_time)
    end
  }
end

こっちの方が個人的には綺麗に見えるけど、可読性は微妙かな・・・

[参考記事]
https://qiita.com/jnchito/items/e3a144b07f578cda5ee2
https://www.kagoya.jp/howto/network/sql-injection/
https://api.rubyonrails.org/v4.2/classes/ActiveRecord/ConnectionAdapters/Quoting.html
https://bugs.ruby-lang.org/issues/12961