画像のバリデーションをMimeTypeを用いるものに変更

[reckyy]

Issue

• 壊れた画像ファイルをユーザーアイコンに設定できてしまう #7487

概要

riraさんが作成したPRの概要から引用させていただきます。

アバター画像のバリデーションにMarcelを使ったMime-Typeのチェックを加えました。
受け入れるべきファイル形式であるかをファイルの実体から判別するようになるため、拡張子偽装したファイルや形式判別不可能なファイルをアバターとして登録することを防ぐことができます。

• アバター画像のバリデーションにMime-Typeを使うようにした #7759

変更確認方法

1. fix/broken-images-can-be-registered をローカルに取り込む
2. 任意のユーザーでログインする
3. http://localhost:3000/current_user/edit にアクセス
4. 壊れた画像をアバター画像に設定し、更新ができない（エラーが出る）ことを確認。

Screenshot

変更前

変更後

[reckyy]

@MikotoMakizuru
お疲れ様です！
レビューをお願いしたくご連絡いたしました。急ぎではありません。
もしご都合厳しければ、教えていただけますと幸いです。
よろしくお願いいたします。

[reckyy]

avatarではなく、uploaded_avatarにvalidationをかけているのは理由があります。
理由
uploaded_avatarはアップロードされたファイル自体を指す。（#ActionDispatch::Http::UploadedFile:hogehoge)これはIO互換の操作が可能。
ただavatarに一度代入されてしまうと、ActiveStorageにアタッチされたデータになる（#ActiveStorage::Attached::One:hogehoge）
後者だと、ファイルの実体に対するcheckができなくなってしまう。
そのため、uploaded_avatarとして切り出しアップロードされた画像自体にバリデーションをかけています。

[MikotoMakizuru]

@reckyy 今から見ます〜

[MikotoMakizuru]

分けられるのであれば、拡張子が指定以外のものか、画像が破損しているか、別々のバリデーションメッセージのほうがユーザにとってわかりやすいかなと思いました。

[reckyy]

@MikotoMakizuru
ご提案ありがとうございます。
回答としては、分けるのは難しいため今の仕様になっています。
なぜかというと、

• mime_typeがnilの場合でも必ず破損しているわけではない（テキストファイルなど）
• 何かアップロードに問題が発生した（画像が部分的にしかアップロードされなかったとか？）

のように、画像が破損しているか拡張子が規定から外れているかの判別は難しいためです。

[MikotoMakizuru]

何かアップロードに問題が発生した（画像が部分的にしかアップロードされなかったとか？）

どのようなときのこのこと言っているのか分からないですが、画像処理の gem を使うといいかもしれません。

1. gem 追加

Gemefile

gem 'rmagick'

2. インストール

$ bundle

3. バリデーション処理実装

user.rb

# frozen_string_literal: true

require 'rmagick'

class User < ApplicationRecord
  （省略）
  def validate_uploaded_avatar_content_type
    return unless uploaded_avatar
    allowed_extensions = %w[png jpg jpeg gif heic heif]
    file_extension = File.extname(uploaded_avatar.original_filename).delete('.').downcase

    unless allowed_extensions.include?(file_extension)
      errors.add(:avatar, 'はPNG, JPG, JPEG, GIF, HEIC, HEIF形式にしてください')
      return
    end

    begin
      Magick::Image.read(uploaded_avatar.tempfile.path)
    rescue Magick::ImageMagickError
      errors.add(:avatar, 'が破損している可能性があります')
    end
  end
end

動作確認

2024-12-05.2.11.25.mov

使った gem：https://github.com/rmagick/rmagick

あと、

• jpeg は指定 OK なのにバリデーションメッセージに「JPEG」が含まれていないのが気になりました。
• rubocop というコミットコメントは指摘されるような気がします。

[reckyy]

@MikotoMakizuru
今外出中なのですが、取り急ぎ返信だけさせていただきます〜。

どのようなときのこのこと言っているのか分からないですが

思いつかなかったので曖昧な例を挙げてしまいました、すみません。
こちらについてはご放念ください。

画像処理の gem を使うといいかもしれません。

ご提案、そして動画まで添付頂きありがとうございます！
ご提案頂いたgemは、数ヶ月前にImageMagickからlibvipsに移行した都合上、非推奨だと思われます。
ruby-vipsを使えば出来るかもですが、後述のように現在は不要だと考えています。

1点ご相談なのですが、自分としてはエラーメッセージを細分化する処理を実装するのは少し過剰かなと考えています。
画像が壊れているかどうかの判別は完璧にはできないと思っているからです。
理由はkomagataさんのコメントに同じです。

#7759 (comment)

そのため、このエラーメッセージの細分化については現時点では一旦保留とし、最終的な判断はkomagataさんのレビュー時に仰ぐ形で進めたいと思っていますが、いかがでしょうか。

jpeg は指定 OK なのにバリデーションメッセージに「JPEG」が含まれていないのが気になりました。

含めておきます。

rubocop というコミットコメントは指摘されるような気がします。

「Rubocopの指摘を修正」に変更しておきます。

[MikotoMakizuru]

確かにバリデーション1つで gem を入れるのはやりすぎな気もしますね、仕様を komagata さんに相談する形でレビュー進めていただいて問題ありません。
また、指定された拡張子以外のファイル選択を出来ないようにすることで、model で考慮すべきことも減らせると思うので、その辺の仕様も相談してみると良いと思います！

[MikotoMakizuru]

@reckyy 1点コメントさせていただきました〜