ActiveStorage ProxyController sets Cache-Control headers on errors like FileNotFoundError

[thedarkside]

Steps to reproduce

Ive observed occasional broken representation images on our site. After digging a little into this issue it became clear that the proxy cache in front of our application has cached a broken image. A cache purge always fixed it.

After digging a little deeper i think i found the source. The proxy cache is getting a cache-control header instructing it to do so by the ActiveStorage::Representations::ProxyController!

Ive created a synthetic test which reproduces it. The response is successful with zero length and cache-control header set. The following test falsely passed!

    ActiveStorage::Service::DiskService.any_instance.expects(:download).raises(ActiveStorage::FileNotFoundError)
    
    get active_storage_image_url # "/rails/active_storage/representations/proxy/..."

    assert_response :ok
    assert_equal 'image/png', response.content_type
    assert_equal "0", response.headers['Content-Length']
    assert response.headers.key? 'ETag'
    assert_equal "max-age=3155695200, public", response.headers['Cache-Control']

Expected behavior

On an error like ActiveStorage::FileNotFoundError the response should not be successful nor should it instruct caches to cache that empty response.

Actual behavior

On an error like ActiveStorage::FileNotFoundError the response is still successful (Statuscode: 200) and a cache-control header is instructing caches to cache this empty response.

System configuration

Rails version: 7.0.8

Ruby version: 3.1.2

[JoeDupuis]

Here's a one file repro using the templates.
The test asserts for the incorrect behavior, I am not sure what the correct behavior should be. It probably should 404 or 500 with no caching.

It looks like it's affecting both Representations::ProxyController and Blobs::ProxyController. The following script tests against Blobs::ProxyController.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record/railtie"
require "active_storage/engine"
require "tmpdir"

db_file = Tempfile.new("sqlite")
ENV["DATABASE_URL"] ||= "sqlite3:#{db_file.path}"

class TestApp < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f

  config.root = __dir__
  config.hosts << "example.org"
  config.eager_load = false
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  config.active_storage.service = :local
  config.active_storage.service_configurations = {
    local: {
      root: Dir.tmpdir,
      service: "Disk"
    }
  }
end


Rails.application.initialize!

require ActiveStorage::Engine.root.join("db/migrate/20170806125915_create_active_storage_tables.rb").to_s

ActiveRecord::Schema.define do
  CreateActiveStorageTables.new.change

  create_table :users, force: true
end

class User < ActiveRecord::Base
  has_one_attached :profile
end

require "minitest/autorun"

class ActiveStorage::Service::DiskService
  def download(key, &block)
    raise ActiveStorage::FileNotFoundError.new "File not yet uploaded!"
  end
end


class BugTest < Minitest::Test
  include Rack::Test::Methods
  def test_upload_and_download
    get Rails.application.routes.url_helpers.rails_storage_proxy_path(file, only_path: true)

    assert last_response.ok?
    assert_equal 'text/plain', last_response.content_type
    assert last_response.headers.key? 'ETag'
    assert_equal "max-age=3155695200, public", last_response.headers['Cache-Control']
    assert_equal 0,  last_response.body.size
  end
  private
  def file
    User.create!(
      profile: {
        content_type: "text/plain",
        filename: "dummy.txt",
        io: ::StringIO.new("dummy"),
      }
    ).profile
  end

  def app
    Rails.application
  end
end

[JoeDupuis]

I thought it would be an easy fix, but the headers are already streamed by the time we call to download.
I'll take a deeper look soon™.

[JoeDupuis]

Pushed a potential fix #51288