Merge branch 'safe-content-type' into 'master'

Explain why we mangle blob content types

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/9079

See merge request !2956

3 files changed, 23 insertions, 12 deletions

diff --git a/app/controllers/projects/avatars_controller.rb b/app/controllers/projects/avatars_controller.rb
index f7e6bb34443..b64dbbd89ce 100644
--- a/app/controllers/projects/avatars_controller.rb
+++ b/app/controllers/projects/avatars_controller.rb
@@ -1,4 +1,6 @@
 class Projects::AvatarsController < Projects::ApplicationController
+  include BlobHelper
+
   before_action :project
 
   def show
@@ -7,7 +9,7 @@ class Projects::AvatarsController < Projects::ApplicationController
       headers['X-Content-Type-Options'] = 'nosniff'
       headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
       headers['Content-Disposition'] = 'inline'
-      headers['Content-Type'] = @blob.content_type
+      headers['Content-Type'] = safe_content_type(@blob)
       head :ok # 'render nothing: true' messes up the Content-Type
     else
       render_404
diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index 87b4d08da0e..d9723acb1d9 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -1,6 +1,7 @@
 # Controller for viewing a file's raw
 class Projects::RawController < Projects::ApplicationController
   include ExtractsPath
+  include BlobHelper
 
   before_action :require_non_empty_project
   before_action :assign_ref_vars
@@ -17,7 +18,7 @@ class Projects::RawController < Projects::ApplicationController
       else
         headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
         headers['Content-Disposition'] = 'inline'
-        headers['Content-Type'] = get_blob_type
+        headers['Content-Type'] = safe_content_type(@blob)
         head :ok # 'render nothing: true' messes up the Content-Type
       end
     else
@@ -27,16 +28,6 @@ class Projects::RawController < Projects::ApplicationController
 
   private
 
-  def get_blob_type
-    if @blob.text?
-      'text/plain; charset=utf-8'
-    elsif @blob.image?
-      @blob.content_type
-    else
-      'application/octet-stream'
-    end
-  end
-
   def send_lfs_object
     lfs_object = find_lfs_object
 
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 7143a744869..7f63a2e2cb4 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -134,4 +134,22 @@ module BlobHelper
     blob.data = Loofah.scrub_fragment(blob.data, :strip).to_xml
     blob
   end
+
+  # If we blindly set the 'real' content type when serving a Git blob we
+  # are enabling XSS attacks. An attacker could upload e.g. a Javascript
+  # file to a Git repository, trick the browser of a victim into
+  # downloading the blob, and then the 'application/javascript' content
+  # type would tell the browser to execute the attacker's Javascript. By
+  # overriding the content type and setting it to 'text/plain' (in the
+  # example of Javascript) we tell the browser of the victim not to
+  # execute untrusted data.
+  def safe_content_type(blob)
+    if blob.text?
+      'text/plain; charset=utf-8'
+    elsif blob.image?
+      blob.content_type
+    else
+      'application/octet-stream'
+    end
+  end
 end