Commit fac1e0e8 authored by Douwe Maan's avatar Douwe Maan

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
parents 7d41e4dc cf2c5396
class Projects::AvatarsController < Projects::ApplicationController class Projects::AvatarsController < Projects::ApplicationController
include BlobHelper
before_action :project before_action :project
def show def show
...@@ -7,7 +9,7 @@ class Projects::AvatarsController < Projects::ApplicationController ...@@ -7,7 +9,7 @@ class Projects::AvatarsController < Projects::ApplicationController
headers['X-Content-Type-Options'] = 'nosniff' headers['X-Content-Type-Options'] = 'nosniff'
headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob)) headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
headers['Content-Disposition'] = 'inline' 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 head :ok # 'render nothing: true' messes up the Content-Type
else else
render_404 render_404
......
# Controller for viewing a file's raw # Controller for viewing a file's raw
class Projects::RawController < Projects::ApplicationController class Projects::RawController < Projects::ApplicationController
include ExtractsPath include ExtractsPath
include BlobHelper
before_action :require_non_empty_project before_action :require_non_empty_project
before_action :assign_ref_vars before_action :assign_ref_vars
...@@ -17,7 +18,7 @@ class Projects::RawController < Projects::ApplicationController ...@@ -17,7 +18,7 @@ class Projects::RawController < Projects::ApplicationController
else else
headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob)) headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
headers['Content-Disposition'] = 'inline' 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 head :ok # 'render nothing: true' messes up the Content-Type
end end
else else
...@@ -27,16 +28,6 @@ class Projects::RawController < Projects::ApplicationController ...@@ -27,16 +28,6 @@ class Projects::RawController < Projects::ApplicationController
private 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 def send_lfs_object
lfs_object = find_lfs_object lfs_object = find_lfs_object
......
...@@ -134,4 +134,22 @@ module BlobHelper ...@@ -134,4 +134,22 @@ module BlobHelper
blob.data = Loofah.scrub_fragment(blob.data, :strip).to_xml blob.data = Loofah.scrub_fragment(blob.data, :strip).to_xml
blob blob
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment