Merge branch 'master' into guides_session_guidelines_2

6 files changed, 93 insertions, 21 deletions

diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb
index 7b5e989699..a723b4d56a 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
 
@@ -22,11 +23,11 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     assert_equal "Hello world!", response.body
   end
 
-  test "showing blob range inline" do
+  test "showing blob range" do
     blob = create_blob
     get blob.service_url, headers: { "Range" => "bytes=5-9" }
     assert_response :partial_content
-    assert_equal "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"]
+    assert_equal "attachment; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"]
     assert_equal "text/plain", response.headers["Content-Type"]
     assert_equal " worl", 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..1503f5fc50 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: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized), 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..af27946357 100644
--- a/activestorage/test/service/gcs_service_test.rb
+++ b/activestorage/test/service/gcs_service_test.rb
@@ -31,24 +31,59 @@ if SERVICE_CONFIGURATIONS[:gcs]
       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"))
+    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 "signed URL response headers" do
+    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))
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
 
-        url = @service.url(key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
+        @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"))
+    end
   end
 else
   puts "Skipping GCS Service tests because no GCS configuration was supplied"
diff --git a/activestorage/test/service/s3_service_test.rb b/activestorage/test/service/s3_service_test.rb
index 559aa028f2..0e5b06dd78 100644
--- a/activestorage/test/service/s3_service_test.rb
+++ b/activestorage/test/service/s3_service_test.rb
@@ -2,6 +2,7 @@
 
 require "service/shared_service_tests"
 require "net/http"
+require "database/setup"
 
 if SERVICE_CONFIGURATIONS[:s3]
   class ActiveStorage::Service::S3ServiceTest < ActiveSupport::TestCase
diff --git a/activestorage/test/service/shared_service_tests.rb b/activestorage/test/service/shared_service_tests.rb
index ca2490f2bc..b97030b320 100644
--- a/activestorage/test/service/shared_service_tests.rb
+++ b/activestorage/test/service/shared_service_tests.rb
@@ -46,6 +46,24 @@ module ActiveStorage::Service::SharedServiceTests
       end
     end
 
+    test "uploading with integrity and multiple keys" do
+      begin
+        key  = SecureRandom.base58(24)
+        data = "Something else entirely!"
+        @service.upload(
+          key,
+          StringIO.new(data),
+          checksum: Digest::MD5.base64digest(data),
+          filename: "racecar.jpg",
+          content_type: "image/jpg"
+        )
+
+        assert_equal data, @service.download(key)
+      ensure
+        @service.delete key
+      end
+    end
+
     test "downloading" do
       assert_equal FIXTURE_DATA, @service.download(@key)
     end