😅

初めてテストしたら他のユーザーのデータを操作できることが発覚した話

2024/01/24に公開

Ruby

Rails

RSpec

tech

この記事について

個人開発に初めてテストを導入してみたところ、ログインさえしていれば他人の作成したレコードを勝手に操作できる大欠陥が見つかった話をまとめた記事になります。

筆者は未経験からエンジニアに転職するべくポートフォリオを作成中の身になりますので、記事に間違いなどがあれば、コメントで教えて頂けると幸いです。

環境

Ruby 3.2.2
Rails 7.0.6 (APIモード)
認証機能: Devise / Devise Token Auth
テスト: Rspec

今回扱うもの

モデル・テーブル

Userモデル(親)とPet(子)モデルがあり、petsテーブルには以下のとおり、ペットデータを作成したユーザーのidを外部キーとして持ちます。

pets
id
name
sex
user_id

コントローラー

ペットに関するシンプルなCRUD機能です。
各アクションを実行する前に、Deviseから提供されるヘルパーメソッドauthenticate_api_v1_user!にてユーザーがログインしているかどうかチェックします。
user_idは、createアクション時にログイン中ユーザーのidを自動で付与するため、ストロングパラメータpet_paramsには含めないようにします。

pets_controller.rb

class Api::V1::PetsController < ApplicationController
  # ログイン状態の確認
  before_action :authenticate_api_v1_user!

  def index
    user_id = current_api_v1_user.id
    pets = Pet.where(user_id: user_id)
    render json: pets, status: :ok
  end

  def show
    pet = Pet.find(params[:id])
    render json: pet, status: :ok
  end

  def create
    pet = Pet.new(pet_params)
    pet.user_id = current_api_v1_user.id

    if pet.save
      render json: pet, status: :created
    else
      render json: { errors: ['ペットの登録に失敗しました'] }, status: :unprocessable_entity
    end
  end

  def update
    pet = Pet.find(params[:id])
    if pet.update(pet_params)
      render json: pet, status: :ok
    else
      render json: { errors: ['ペットの更新に失敗しました'] }, status: :unprocessable_entity
    end
  end

  def destroy
    pet = Pet.find(params[:id])
    pet.destroy
    head :no_content
  end

  private

　def pet_params
  　params.require(:pet).permit(:name, :sex)
　end
end

初めてのテスト

実施したテスト

petsコントローラーにおける認証周りのテストでは、以下の3項目を確認しました。

非ログインユーザーは各アクションを実施できないこと
有効期限切れ等の無効なトークン情報を持つユーザーは各アクションを実施できないこと
ログイン中ユーザーは他のユーザーが作成したレコードに対して各アクションを実施できないこと

テストの結果

1、2については期待した結果が返り、無事テストが成功しました。
しかし、3について結果は以下のとおりでした。

index: ログイン中ユーザーのidと一致する「user_id」をキーとしてペットを検索するため、他のユーザーが作成したペット情報一覧を取得することはできなかった(成功)
show: ログインさえしていれば、他のユーザーが作成したペット詳細情報が取得できてしまった(失敗)
create: ログイン中ユーザーのidを「user_id」としてペットデータを作成するので、他のユーザーが作成したペットに関する操作は発生しなかった(成功)
update: ログインさえしていれば、他のユーザーが作成したペット詳細情報が更新できてしまった(失敗)
destroy: ログインさえしていれば、他のユーザーが作成したペット詳細情報が削除できてしまった(失敗)

やばい・・・これをリリース前に気づけて本当によかったです。

テスト後の修正

モデルの関連付けを利用する

ユーザーとペットは1対多の関係であるので、PetモデルはUserモデルにbelongs_toで関連付けられています。
今回はbelongs_toで追加されるメソッドを利用して、指定されたidのペットがそのユーザー作成のものであるかどうかチェックするロジックを追加し、修正を行います。

コントローラーの修正

show / update / destroyアクションを実施する前に、ログイン中ユーザーが作成したペットであることを確認するset_petメソッドを追加しました。具体的な処理の流れは以下のとおりです。

モデルの関連付けを利用して、ログイン中ユーザーの作成したペットデータの中に指定したidを持つペットが存在するかどうかをチェックする
存在する場合は、そのレコードをインスタンス変数@petに格納する
存在しない場合は、ステータスコード404とエラーメッセージを返す
show / update / destroyアクションでは、@petについて各操作を行う。

なお、404のエラーメッセージについては、指定したidが他のユーザーが作成したペットである事実には言及しないような文言にしています。

  def set_pet
    @pet = current_api_v1_user.pets.find_by(id: params[:id])
    record_not_found unless @pet
  end
  
  def record_not_found
    render json: { errors: ['指定されたペットが見つかりませんでした'] }, status: :not_found
  end

修正後のコード全体

pets_controller.rb

class Api::V1::PetsController < ApplicationController
  # ログイン状態の確認
  before_action :authenticate_api_v1_user!
  
  # ログイン中のユーザー所有のデータか確認
  before_action :set_pet, only: %i[show update destroy]
  
  # ステータスコード404の共通処理
  rescue_from ActiveRecord::RecordNotFound, with: :record_not_found

  def index
    user_id = current_api_v1_user.id
    pets = Pet.where(user_id: user_id)
    render json: pets, status: :ok
  end

  def show
    render json: @pet, status: :ok
  end

  def create
    pet = Pet.new(pet_params)
    pet.user_id = current_api_v1_user.id

    if pet.save
      render json: pet, status: :created
    else
      render json: { errors: ['ペットの登録に失敗しました'] }, status: :unprocessable_entity
    end
  end

  def update
    if @pet.update(pet_params)
      render json: @pet, status: :ok
    else
      render json: { errors: ['ペットの更新に失敗しました'] }, status: :unprocessable_entity
    end
  end

  def destroy
    @pet.destroy
    head :no_content
  end

  private

　def pet_params
  　params.require(:pet).permit(:name, :sex)
　end
 
  def set_pet
    @pet = current_api_v1_user.pets.find_by(id: params[:id])
    record_not_found unless @pet
  end

  def record_not_found
    render json: { errors: ['指定されたペットが見つかりませんでした'] }, status: :not_found
  end
end

再テストの結果

前述と同じテストを実施した結果、show / update / destroyアクションについても、ログインしていても、他のユーザーが作成したペットについては(無事に)操作が行えないようになり、テストが成功しました。

まとめ

初めてのテストでしたが、思わぬミスが見つかり(若干血の気が引いたものの)良い経験となりました。

同じようなことでお悩みの方や、初めてコントローラーを書くけどどうやって書いたら良いのかわからないというような方に、手早くこの情報が届くと嬉しいです。

今後の課題

今回はUserモデルとPetモデルの2つを扱いましたが、例えばPetモデルの下に別のモデル(例えばCareモデルやFoodモデルなど)があり、Userモデルと直接関連はないものの、外部キーとしてuser_idを持たせるべきかどうかという点についても気になりました。

テーブル設計についても今後学習を進めていければと思います。