Direct S3 upload with local store raises NoMethodError instead of being rejected

Contribute Bug
1

I found and resolved this issue myself after a client reported it. I used AI to generate the bug report because I was lazy and it does a better job than I do - and it found a core bug while doing that, which I manually verified.

Nothing big but it might make sense to see if this pattern is causing issues in other places as well.

It all started with someone having checked enable_direct_s3_uploads without actually configuring and enabling S3, which broke all uploads, returning 500 errors behind the scenes. I thought the 500 was worth investigating.

Summary

When enable_direct_s3_uploads is enabled while the active upload store is local, attempting an upload can raise:

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'

This appears to be caused by a combination of an invalid settings state and an overwritten before_action callback.

Configuration

The problematic state is effectively:

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

For local storage, Discourse.store does not implement:

signed_request_for_temporary_upload

which is expected, since that operation only makes sense for an external/S3 store.

Expected behaviour

If direct S3 uploads are enabled while the active store is local, the request should be rejected cleanly by external_store_check.

Ideally the invalid site setting combination should also be prevented or validated.

Actual behaviour

The request reaches ExternalUploadManager.create_direct_upload, which calls:

store.signed_request_for_temporary_upload(...)

on a FileStore::LocalStore, resulting in a NoMethodError.

Possible cause

ExternalUploadHelpers registers the following callback:

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

This should prevent the request from reaching the external upload code when the store is local.

However, UploadsController later registers another callback using the same filter method:

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

Rails treats repeated registration of the same callback as a redefinition, so the latter appears to replace the earlier only: conditions.

image

As a result, external_store_check is no longer run for generate_presigned_put, allowing a local store to reach the direct-upload code path.

Reproduction

  1. Configure a Discourse instance with local upload storage.
  2. Ensure:
SiteSetting.enable_s3_uploads = false
SiteSetting.enable_direct_s3_uploads = true
  1. Attempt to upload a file.
  2. Observe the NoMethodError from FileStore::LocalStore.

A console check of the state can be done with:

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

Suggested fix

The callback registrations should not overwrite each other.

For example, include the secure upload actions in the existing external_store_check callback:

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
  ]

Alternatively, use a separate callback method for the secure-upload actions.

It may also be worth adding validation so that enable_direct_s3_uploads cannot be enabled while S3/external uploads are disabled.

Workaround

For sites using local upload storage:

SiteSetting.enable_direct_s3_uploads = false

prevents the error.