Force content disposition to attachment for specific content types

In this way we avoid HTML, XML, SVG and other files that can be rendered
by the browser to be served inline by default. Depending on the origin
from where these files are served, this might lead to XSS
vulnerabilities, and in the best case, to more realistic phishing
attacks and open redirects.

We force it rather than falling back to it when other disposition is not
provided. Otherwise it would be possible for someone to force inline
just by passing `disposition=inline` in the URL.

The list of content types to be served as attachments is configurable.

activestorage/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Force `:attachment` disposition for specific, configurable content types.
+    This mitigates possible security issues such as XSS or phishing when
+    serving them inline. A list of such content types is included by default,
+    and can be configured via `content_types_to_serve_as_binary`.
+
+    *Rosa Gutierrez*
+
+
 ## Rails 5.2.0.beta2 (November 28, 2017) ##
 
 *   Fix the gem adding the migrations files to the package.

activestorage/app/models/active_storage/blob.rb

@@ -201,8 +201,8 @@ def representable?
   # with users. Instead, the +service_url+ should only be exposed as a redirect from a stable, possibly authenticated URL.
   # Hiding the +service_url+ behind a redirect also gives you the power to change services without updating all URLs. And
   # it allows permanent URLs that redirect to the +service_url+ to be cached in the view.
-  def service_url(expires_in: service.url_expires_in, disposition: "inline")
-    service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type
+  def service_url(expires_in: service.url_expires_in, disposition: :inline)
+    service.url key, expires_in: expires_in, disposition: forcibly_serve_as_binary? ? :attachment : disposition, filename: filename, content_type: content_type
   end
 
   # Returns a URL that can be used to directly upload a file for this blob on the service. This URL is intended to be
@@ -325,4 +325,9 @@ def analyzer
     def analyzer_class
       ActiveStorage.analyzers.detect { |klass| klass.accept?(self) } || ActiveStorage::Analyzer::NullAnalyzer
     end
+
+
+    def forcibly_serve_as_binary?
+      ActiveStorage.content_types_to_serve_as_binary.include?(content_type)
+    end
 end

activestorage/lib/active_storage.rb

@@ -43,4 +43,5 @@ module ActiveStorage
   mattr_accessor :analyzers, default: []
   mattr_accessor :paths, default: {}
   mattr_accessor :variable_content_types, default: []
+  mattr_accessor :content_types_to_serve_as_binary, default: []
 end

activestorage/lib/active_storage/engine.rb

@@ -18,6 +18,16 @@ class Engine < Rails::Engine # :nodoc:
     config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]
     config.active_storage.paths = ActiveSupport::OrderedOptions.new
     config.active_storage.variable_content_types = [ "image/png", "image/gif", "image/jpg", "image/jpeg", "image/vnd.adobe.photoshop" ]
+    config.active_storage.content_types_to_serve_as_binary = [
+      "text/html",
+      "text/javascript",
+      "image/svg+xml",
+      "application/postscript",
+      "application/x-shockwave-flash",
+      "text/xml",
+      "application/xml",
+      "application/xhtml+xml"
+    ]
 
     config.eager_load_namespaces << ActiveStorage
 
@@ -29,6 +39,7 @@ class Engine < Rails::Engine # :nodoc:
         ActiveStorage.analyzers  = app.config.active_storage.analyzers || []
         ActiveStorage.paths      = app.config.active_storage.paths || {}
         ActiveStorage.variable_content_types = app.config.active_storage.variable_content_types || []
+        ActiveStorage.content_types_to_serve_as_binary = app.config.active_storage.content_types_to_serve_as_binary || []
       end
     end
 

activestorage/test/models/blob_test.rb

@@ -41,6 +41,15 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
+  test "urls force 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)
+    end
+  end
+
   test "purge deletes file from external service" do
     blob = create_blob