ローカルストレージを併用したS3直接アップロードが拒否される代わりにNoMethodErrorを発生させる

貢献 バグ
1

あるクライアントからの報告を受けて、この問題を自分で発見し、解決しました。私はAIを使ってバグレポートを生成しました。理由は単純に怠惰だったことと、AIの方が私よりも良いレポートを作成できるからです。その過程で、AIはコア部分のバグを発見し、私はそれを手動で検証しました。

大きな問題ではありませんが、このパターンが他の場所でも問題を引き起こしていないか確認する価値があるかもしれません。

事の発端は、あるユーザーがS3を構成・有効化していないにもかかわらず enable_direct_s3_uploads をチェックしていたことで、これによりすべてのアップロードが破損し、裏側で500エラーが返されるようになったことです。私はこの500エラーは調査する価値があると考えました。

概要

アクティブなアップロードストアがローカルである状態で enable_direct_s3_uploads が有効化されている場合、アップロードの試行により以下が引き起こされる可能性があります：

NoMethodError (undefined method 'signed_request_for_temporary_upload' for an instance of FileStore::LocalStore)

app/services/external_upload_manager.rb:34:in 'ExternalUploadManager.create_direct_upload'
lib/external_upload_helpers.rb:56:in 'ExternalUploadHelpers#generate_presigned_put'
app/controllers/application_controller.rb:443:in 'block in ApplicationController#with_resolved_locale'
app/controllers/application_controller.rb:443:in 'ApplicationController#with_resolved_locale'

これは、無効な設定状態と上書きされた before_action コールバックの組み合わせによって引き起こされているようです。

設定

問題のある状態は実質的に以下の通りです：

SiteSetting.enable_direct_s3_uploads == true
SiteSetting.enable_s3_uploads == false
Discourse.store.is_a?(FileStore::LocalStore) == true

ローカルストレージの場合、Discourse.store は以下を実装していません：

signed_request_for_temporary_upload

これは想定された動作であり、この操作は外部/S3ストアに対してのみ意味があるためです。

期待される動作

アクティブなストアがローカルである状態で直接S3アップロードが有効化されている場合、リクエストは external_store_check によってクリーンに拒否されるべきです。

理想を言えば、無効なサイト設定の組み合わせも防止または検証されるべきです。

実際の動作

リクエストは ExternalUploadManager.create_direct_upload に到達し、そこで以下が呼び出されます：

store.signed_request_for_temporary_upload(...)

これは FileStore::LocalStore に対して行われるため、NoMethodError になります。

考えられる原因

ExternalUploadHelpers は以下のコールバックを登録しています：

before_action :external_store_check,
  only: %i[
    generate_presigned_put
    complete_external_upload
    create_multipart
    batch_presign_multipart_parts
    complete_multipart
    abort_multipart
  ]

これにより、ストアがローカルである場合、リクエストが外部アップロードコードに到達するのを防ぐはずです。

しかし、UploadsController は後で同じフィルタメソッドを使用して別のコールバックを登録しています：

before_action :external_store_check,
  only: %i[_show_secure_deprecated show_secure]

Railsは同じコールバックの繰り返し登録を再定義として扱いますため、後者は以前の only: 条件を置き換えているようです。

image

その結果、external_store_check は generate_presigned_put に対して実行されなくなり、ローカルストアが直接アップロードのコードパスに到達できるようになっています。

再現手順

  1. ローカルアップロードストレージでDiscourseインスタンスを設定します。
  2. 以下の状態を確認します：
SiteSetting.enable_s3_uploads = false
SiteSetting.enable_direct_s3_uploads = true
  1. ファイルのアップロードを試みます。
  2. FileStore::LocalStore からの NoMethodError を確認します。

状態のコンソールでの確認は以下で行えます：

[
  SiteSetting.enable_direct_s3_uploads,
  SiteSetting.enable_s3_uploads,
  Discourse.store.class,
  Discourse.store.external?
]

修正提案

コールバックの登録が互いに上書きするべきではありません。

例えば、セキュアアップロードアクションを既存の external_store_check コールバックに含めることができます：

before_action :external_store_check,
  only: %i[
    generate_presigned_put
    complete_external_upload
    create_multipart
    batch_presign_multipart_parts
    complete_multipart
    abort_multipart
    _show_secure_deprecated
    show_secure
  ]

あるいは、セキュアアップロードアクション用に別のコールバックメソッドを使用することもできます。

また、S3/外部アップロードが無効化されている状態で enable_direct_s3_uploads を有効化できないよう、検証を追加する価値があるかもしれません。

回避策

ローカルアップロードストレージを使用しているサイトの場合：

SiteSetting.enable_direct_s3_uploads = false

とすることで、エラーを防止できます。

「いいね！」 2
2

ありがとうございます。以下の通り修正されるはずです：

「いいね！」 1