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

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rira100000000
Copy link
Contributor

@rira100000000 rira100000000 commented May 7, 2024

Issue

概要

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

変更確認方法

  1. feature/add_validation_to_broken_imageをローカルに取り込む
  2. foreman start -f Procfile.devでサーバーを立ち上げる
  3. 任意のユーザーでログインし、http://localhost:3000/current_user/editにアクセスする
  4. txtファイルを用意し、拡張子をjpgに修正する
  5. アバターに4で用意したファイルを選択し、更新するボタンを押下する
  6. エラーメッセージが表示されることを確認する

Screenshot

変更前

image

変更後

image

@rira100000000 rira100000000 changed the title アバター画像の形式を識別するようにした アバター画像の形式識別にruby-filemagic使うようにした May 7, 2024
@rira100000000 rira100000000 marked this pull request as ready for review May 7, 2024 06:41
@rira100000000
Copy link
Contributor Author

@unikounio
お疲れ様です。
こちらのPRのレビューをお願いしたいのですがご都合いかがでしょうか?

@rira100000000 rira100000000 requested a review from unikounio May 7, 2024 07:10
@rira100000000 rira100000000 self-assigned this May 7, 2024
@unikounio
Copy link
Contributor

@rira100000000 さん
お疲れ様です!
しばらくFBCの時間を安定して確保できる見通しが立たないので、別の方にお願いしていただいた方が良いかと思います。
申し訳ないです🙏

@rira100000000
Copy link
Contributor Author

@unikounio
承知しました~。

@kyokucho1989
お疲れ様です。
こちらのPRのレビューをお願いしたいのですがご都合いかがでしょうか?

@kyokucho1989
Copy link
Contributor

@rira100000000
了解しました!1週間ほど時間をください。

@kyokucho1989 kyokucho1989 requested review from kyokucho1989 and removed request for unikounio May 9, 2024 20:35
@@ -570,6 +570,14 @@ class UsersTest < ApplicationSystemTestCase
assert_match(/#{user.id}\.png$/, img.native['src'])
end

test 'can upload broken image as user avatar' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can not upload だと思いますー

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そのように修正します。

mime_type = fm.buffer(uploaded_avatar.read)
return if mime_type.start_with?('image/png', 'image/jpg', 'image/jpeg', 'image/gif', 'image/heic', 'image/heif')

errors.add(:avatar, 'はPNG, JPG, GIF, HEIC, HEIF形式にしてください')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このエラーメッセージは正確ではないと思います。
壊れたファイルでも拡張子はJPGだったりするので。
:avatar, 'は指定された拡張子(PNG, JPG, GIF, HEIC, HEIF形式)になっていないか、あるいは画像が破損している可能性があります' 
というふうに変更するのが良いかと。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そのように修正します。

@kyokucho1989
Copy link
Contributor

@rira100000000
レビューしました。
確認お願いしますー

@rira100000000
Copy link
Contributor Author

@kyokucho1989
連絡遅くなってしまってすいません!
修正が完了しましたので、再度確認をお願いします。

Copy link
Contributor

@kyokucho1989 kyokucho1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rira100000000
確認しました!
Approve しますー

@rira100000000
Copy link
Contributor Author

@kyokucho1989
ありがとうございます~!

@komagata
チームメンバーからApproveをいただいたので確認をお願いします。

@komagata
Copy link
Member

komagata commented May 28, 2024

@rira100000000 改めて仕様について考えてみてました。

そこで、最初にお話しした仕様と違ってしまって二度手間になってしまって大変もうしわけないのですが、アップロードのところはmime-typeだけのチェックにして、表示する場所で壊れたファイルであってもページ全体が表示されないのを防ぐような実装をお願いできれば幸いです。

なぜかというと、1つにはruby-magickのようなネイティブアプリと連携するものを使いたくないからです。
bootcampを開発する入門者の方が毎回大変になりますし、いろいろな環境で動かすときの障壁となります。1つ1つは大した問題ではありませんが、1つ許すとどんどん増えていって最終的には大きな問題になります。

もう1つにはmagicを使う方法(バイナリの中身を見て判別する方法)は原理的に完璧にはできず、どちらにしろ完璧にはできないので、webのアイコン画像のvalidationにつかうのはあまりみたこともなく、too muchなやり方だと思ったからです

いまさらになって大変もうしわけないですが、宜しくお願い致します。

@rira100000000
Copy link
Contributor Author

@komagata
お疲れ様です。
ruby-filemagicを使わないよう修正しましたので確認をお願いします。
MarcelというActiveStorageで使っているgemを使うことで、Mime-Typeを調べています。
Marcelを使うことで拡張子偽装したファイルを見破ることができ、libmagicのインストールも不要になりました。

validationをすり抜けた場合ですが、Vips::Errorのrescueが追加され、表示上の問題はmainでは解決されています。

@rira100000000 rira100000000 changed the title アバター画像の形式識別にruby-filemagic使うようにした アバター画像のバリデーションにMime-Typeを使うようにした Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants