Proxying: sanitize Content-Type and Content-Disposition

Prevent XSS where unsafe content is served inline on the application origin.

Follows up on #34477. References 06ab7b2 and d40284b.

activestorage/app/controllers/concerns/active_storage/set_headers.rb

@@ -5,8 +5,8 @@ module ActiveStorage::SetHeaders #:nodoc:
 
   private
     def set_content_headers_from(blob)
-      response.headers["Content-Type"] = blob.content_type
+      response.headers["Content-Type"] = blob.content_type_for_serving
       response.headers["Content-Disposition"] = ActionDispatch::Http::ContentDisposition.format \
-        disposition: params[:disposition] || "inline", filename: blob.filename.sanitized
+        disposition: blob.forced_disposition_for_serving || params[:disposition] || "inline", filename: blob.filename.sanitized
     end
 end

activestorage/app/models/active_storage/blob.rb

@@ -181,10 +181,8 @@ def text?
   # the URL should only be exposed as a redirect from a stable, possibly authenticated URL. Hiding the
   # URL behind a redirect also allows you to change services without updating all URLs.
   def url(expires_in: ActiveStorage.service_urls_expire_in, disposition: :inline, filename: nil, **options)
-    filename = ActiveStorage::Filename.wrap(filename || self.filename)
-
-    service.url key, expires_in: expires_in, filename: filename, content_type: content_type_for_service_url,
-      disposition: forced_disposition_for_service_url || disposition, **options
+    service.url key, expires_in: expires_in, filename: ActiveStorage::Filename.wrap(filename || self.filename),
+      content_type: content_type_for_serving, disposition: forced_disposition_for_serving || disposition, **options
   end
 
   alias_method :service_url, :url
@@ -201,6 +199,16 @@ def service_headers_for_direct_upload
     service.headers_for_direct_upload key, filename: filename, content_type: content_type, content_length: byte_size, checksum: checksum
   end
 
+  def content_type_for_serving #:nodoc:
+    forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : content_type
+  end
+
+  def forced_disposition_for_serving #:nodoc:
+    if forcibly_serve_as_binary? || !allowed_inline?
+      :attachment
+    end
+  end
+
 
   # Uploads the +io+ to the service on the +key+ for this blob. Blobs are intended to be immutable, so you shouldn't be
   # using this method after a file has already been uploaded to fit with a blob. If you want to create a derivative blob,
@@ -309,16 +317,6 @@ def allowed_inline?
       ActiveStorage.content_types_allowed_inline.include?(content_type)
     end
 
-    def content_type_for_service_url
-      forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : content_type
-    end
-
-    def forced_disposition_for_service_url
-      if forcibly_serve_as_binary? || !allowed_inline?
-        :attachment
-      end
-    end
-
     def service_metadata
       if forcibly_serve_as_binary?
         { content_type: ActiveStorage.binary_content_type, disposition: :attachment, filename: filename }

activestorage/app/models/active_storage/variant.rb

@@ -90,6 +90,12 @@ def download(&block)
     service.download key, &block
   end
 
+  alias_method :content_type_for_serving, :content_type
+
+  def forced_disposition_for_serving #:nodoc:
+    nil
+  end
+
   # Returns the receiving variant. Allows ActiveStorage::Variant and ActiveStorage::Preview instances to be used interchangeably.
   def image
     self

activestorage/test/controllers/blobs/proxy_controller_test.rb

@@ -4,18 +4,24 @@
 require "database/setup"
 
 class ActiveStorage::Blobs::ProxyControllerTest < ActionDispatch::IntegrationTest
-  setup do
-    @blob = create_file_blob filename: "racecar.jpg"
-  end
-
   test "invalid signed ID" do
     get rails_service_blob_proxy_url("invalid", "racecar.jpg")
     assert_response :not_found
   end
 
   test "HTTP caching" do
-     get rails_storage_proxy_url(@blob)
-     assert_response :success
-     assert_equal "max-age=3155695200, public", response.headers["Cache-Control"]
-   end
+    get rails_storage_proxy_url(create_file_blob(filename: "racecar.jpg"))
+    assert_response :success
+    assert_equal "max-age=3155695200, public", response.headers["Cache-Control"]
+  end
+
+  test "forcing Content-Type to binary" do
+    get rails_storage_proxy_url(create_blob(content_type: "text/html"))
+    assert_equal "application/octet-stream", response.headers["Content-Type"]
+  end
+
+  test "forcing Content-Disposition to attachment" do
+    get rails_storage_proxy_url(create_blob(content_type: "application/zip"))
+    assert_match(/^attachment; /, response.headers["Content-Disposition"])
+  end
 end