aboutsummaryrefslogtreecommitdiffstats
path: root/activestorage
diff options
context:
space:
mode:
authorRosa Gutierrez <rosa.ge@gmail.com>2018-01-04 19:35:54 +0100
committerRosa Gutierrez <rosa.ge@gmail.com>2018-01-05 16:32:32 +0100
commitd40284b1a44773b03d78ca67a888b94fd330d1b1 (patch)
treeac9cada273b492f231bf1b48d215e36ff78fde05 /activestorage
parent5a5014688873f1d6e1b66075eea8a4356b5a4d07 (diff)
downloadrails-d40284b1a44773b03d78ca67a888b94fd330d1b1.tar.gz
rails-d40284b1a44773b03d78ca67a888b94fd330d1b1.tar.bz2
rails-d40284b1a44773b03d78ca67a888b94fd330d1b1.zip
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.
Diffstat (limited to 'activestorage')
-rw-r--r--activestorage/CHANGELOG.md8
-rw-r--r--activestorage/app/models/active_storage/blob.rb9
-rw-r--r--activestorage/lib/active_storage.rb1
-rw-r--r--activestorage/lib/active_storage/engine.rb11
-rw-r--r--activestorage/test/models/blob_test.rb9
5 files changed, 36 insertions, 2 deletions
diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md
index c5171e7490..9ce177b462 100644
--- a/activestorage/CHANGELOG.md
+++ b/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.
diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 3b48ee72af..8fb53fa787 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -201,8 +201,8 @@ class ActiveStorage::Blob < ActiveRecord::Base
# 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 @@ class ActiveStorage::Blob < ActiveRecord::Base
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
diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb
index 01fd0f4415..3bd8ef664f 100644
--- a/activestorage/lib/active_storage.rb
+++ b/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
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index d5c71670ff..1f15ac2d2d 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -18,6 +18,16 @@ module ActiveStorage
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 @@ module ActiveStorage
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
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index f94e65ed77..fa8b935da0 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/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