4 files changed, 78 insertions, 11 deletions
diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb
index 7b5e989699..0aa8648db3 100644
--- a/activestorage/test/controllers/disk_controller_test.rb
+++ b/activestorage/test/controllers/disk_controller_test.rb
@@ -5,11 +5,12 @@ require "database/setup"
 
 class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
   test "showing blob inline" do
-    blob = create_blob
+    blob = create_blob(filename: "hello.jpg", content_type: "image/jpg")
+
     get blob.service_url
     assert_response :ok
-    assert_equal "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"]
-    assert_equal "text/plain", response.headers["Content-Type"]
+    assert_equal "inline; filename=\"hello.jpg\"; filename*=UTF-8''hello.jpg", response.headers["Content-Disposition"]
+    assert_equal "image/jpg", response.headers["Content-Type"]
     assert_equal "Hello world!", response.body
   end
 
@@ -36,6 +37,10 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     blob.delete
 
     get blob.service_url
+  end
+
+  test "showing blob with invalid key" do
+    get rails_disk_service_url(encoded_key: "Invalid key", filename: "hello.txt")
     assert_response :not_found
   end
 
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index 1a6a89de56..0eac622d72 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/activestorage/test/models/blob_test.rb
@@ -121,12 +121,21 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "urls force attachment as content disposition for content types served as binary" do
+  test "urls force content_type to binary and attachment as content disposition for content types served as binary" do
     blob = create_blob(content_type: "text/html")
 
     freeze_time do
-      assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url
-      assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url(disposition: :inline)
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url(disposition: :inline)
+    end
+  end
+
+  test "urls force attachment as content disposition when the content type is not allowed inline" do
+    blob = create_blob(content_type: "application/zip")
+
+    freeze_time do
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url(disposition: :inline)
     end
   end
 
@@ -148,7 +157,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     arguments = [
       blob.key,
       expires_in: ActiveStorage.service_urls_expire_in,
-      disposition: :inline,
+      disposition: :attachment,
       content_type: blob.content_type,
       filename: blob.filename,
       thumb_size: "300x300",
@@ -183,9 +192,13 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
   end
 
   private
-    def expected_url_for(blob, disposition: :inline, filename: nil)
+    def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil)
       filename ||= blob.filename
-      query_string = { content_type: blob.content_type, disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized) }.to_param
-      "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}"
+      content_type ||= blob.content_type
+
+      query = { disposition: disposition.to_s + "; #{filename.parameters}", content_type: content_type }
+      key_params = { key: blob.key }.merge(query)
+
+      "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(key_params, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query.to_param}"
     end
 end
diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb
index 6577f1cd9f..8552080e7b 100644
--- a/activestorage/test/models/variant_test.rb
+++ b/activestorage/test/models/variant_test.rb
@@ -156,7 +156,7 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   test "service_url doesn't grow in length despite long variant options" do
     blob = create_file_blob(filename: "racecar.jpg")
     variant = blob.variant(font: "a" * 10_000).processed
-    assert_operator variant.service_url.length, :<, 525
+    assert_operator variant.service_url.length, :<, 726
   end
 
   test "works for vips processor" do
diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb
index 2ba2f8b346..73337ef8e1 100644
--- a/activestorage/test/service/gcs_service_test.rb
+++ b/activestorage/test/service/gcs_service_test.rb
@@ -31,6 +31,55 @@ if SERVICE_CONFIGURATIONS[:gcs]
       end
     end
 
+    test "upload with content_type and content_disposition" do
+      begin
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
+
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
+
+        url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html"))
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.content_type
+        assert_match /attachment;.*test.txt/, response["Content-Disposition"]
+      ensure
+        @service.delete key
+      end
+    end
+
+    test "upload with content_type" do
+      begin
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
+
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), content_type: "text/plain")
+
+        url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html"))
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.content_type
+        assert_match /inline;.*test.html/, response["Content-Disposition"]
+      ensure
+        @service.delete key
+      end
+    end
+
+    test "update metadata" do
+      begin
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.html"), content_type: "text/html")
+
+        @service.update_metadata(key, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
+        url = @service.url(key, expires_in: 2.minutes, disposition: :attachment, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html"))
+
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.content_type
+        assert_match /inline;.*test.txt/, response["Content-Disposition"]
+      ensure
+        @service.delete key
+      end
+    end
+
     test "signed URL generation" do
       assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/,
         @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain"))