Commit cf8b8ff9 authored by Francisco Javier López's avatar Francisco Javier López Committed by Nick Thomas

Add feature flag for workhorse content type calculation

parent c3bbad76
7.4.0 7.5.0
...@@ -263,6 +263,9 @@ gem 'ace-rails-ap', '~> 4.1.0' ...@@ -263,6 +263,9 @@ gem 'ace-rails-ap', '~> 4.1.0'
# Detect and convert string character encoding # Detect and convert string character encoding
gem 'charlock_holmes', '~> 0.7.5' gem 'charlock_holmes', '~> 0.7.5'
# Detect mime content type from content
gem 'mimemagic', '~> 0.3.2'
# Faster blank # Faster blank
gem 'fast_blank' gem 'fast_blank'
......
...@@ -459,7 +459,7 @@ GEM ...@@ -459,7 +459,7 @@ GEM
mime-types (3.2.2) mime-types (3.2.2)
mime-types-data (~> 3.2015) mime-types-data (~> 3.2015)
mime-types-data (3.2018.0812) mime-types-data (3.2018.0812)
mimemagic (0.3.0) mimemagic (0.3.2)
mini_magick (4.8.0) mini_magick (4.8.0)
mini_mime (1.0.1) mini_mime (1.0.1)
mini_portile2 (2.3.0) mini_portile2 (2.3.0)
...@@ -1051,6 +1051,7 @@ DEPENDENCIES ...@@ -1051,6 +1051,7 @@ DEPENDENCIES
loofah (~> 2.2) loofah (~> 2.2)
mail_room (~> 0.9.1) mail_room (~> 0.9.1)
method_source (~> 0.8) method_source (~> 0.8)
mimemagic (~> 0.3.2)
mini_magick mini_magick
minitest (~> 5.7.0) minitest (~> 5.7.0)
mysql2 (~> 0.4.10) mysql2 (~> 0.4.10)
......
...@@ -456,7 +456,7 @@ GEM ...@@ -456,7 +456,7 @@ GEM
mime-types (3.2.2) mime-types (3.2.2)
mime-types-data (~> 3.2015) mime-types-data (~> 3.2015)
mime-types-data (3.2018.0812) mime-types-data (3.2018.0812)
mimemagic (0.3.0) mimemagic (0.3.2)
mini_magick (4.8.0) mini_magick (4.8.0)
mini_mime (1.0.1) mini_mime (1.0.1)
mini_portile2 (2.3.0) mini_portile2 (2.3.0)
...@@ -1042,6 +1042,7 @@ DEPENDENCIES ...@@ -1042,6 +1042,7 @@ DEPENDENCIES
loofah (~> 2.2) loofah (~> 2.2)
mail_room (~> 0.9.1) mail_room (~> 0.9.1)
method_source (~> 0.8) method_source (~> 0.8)
mimemagic (~> 0.3.2)
mini_magick mini_magick
minitest (~> 5.7.0) minitest (~> 5.7.0)
mysql2 (~> 0.4.10) mysql2 (~> 0.4.10)
......
...@@ -10,6 +10,8 @@ module SnippetsActions ...@@ -10,6 +10,8 @@ module SnippetsActions
def raw def raw
disposition = params[:inline] == 'false' ? 'attachment' : 'inline' disposition = params[:inline] == 'false' ? 'attachment' : 'inline'
workhorse_set_content_type!
send_data( send_data(
convert_line_endings(@snippet.content), convert_line_endings(@snippet.content),
type: 'text/plain; charset=utf-8', type: 'text/plain; charset=utf-8',
......
...@@ -38,6 +38,7 @@ module UploadsActions ...@@ -38,6 +38,7 @@ module UploadsActions
return render_404 unless uploader return render_404 unless uploader
workhorse_set_content_type!
send_upload(uploader, attachment: uploader.filename, disposition: disposition) send_upload(uploader, attachment: uploader.filename, disposition: disposition)
end end
......
...@@ -140,15 +140,22 @@ class Projects::JobsController < Projects::ApplicationController ...@@ -140,15 +140,22 @@ class Projects::JobsController < Projects::ApplicationController
def raw def raw
if trace_artifact_file if trace_artifact_file
workhorse_set_content_type!
send_upload(trace_artifact_file, send_upload(trace_artifact_file,
send_params: raw_send_params, send_params: raw_send_params,
redirect_params: raw_redirect_params) redirect_params: raw_redirect_params)
else else
build.trace.read do |stream| build.trace.read do |stream|
if stream.file? if stream.file?
workhorse_set_content_type!
send_file stream.path, type: 'text/plain; charset=utf-8', disposition: 'inline' send_file stream.path, type: 'text/plain; charset=utf-8', disposition: 'inline'
else else
send_data stream.raw, type: 'text/plain; charset=utf-8', disposition: 'inline', filename: 'job.log' # In this case we can't use workhorse_set_content_type! and let
# Workhorse handle the response because the data is streamed directly
# to the user but, because we have the trace content, we can calculate
# the proper content type and disposition here.
raw_data = stream.raw
send_data raw_data, type: 'text/plain; charset=utf-8', disposition: raw_trace_content_disposition(raw_data), filename: 'job.log'
end end
end end
end end
...@@ -201,4 +208,13 @@ class Projects::JobsController < Projects::ApplicationController ...@@ -201,4 +208,13 @@ class Projects::JobsController < Projects::ApplicationController
def build_path(build) def build_path(build)
project_job_path(build.project, build) project_job_path(build.project, build)
end end
def raw_trace_content_disposition(raw_data)
mime_type = MimeMagic.by_magic(raw_data)
# if mime_type is nil can also represent 'text/plain'
return 'inline' if mime_type.nil? || mime_type.type == 'text/plain'
'attachment'
end
end end
...@@ -140,6 +140,8 @@ module BlobHelper ...@@ -140,6 +140,8 @@ module BlobHelper
Gitlab::Sanitizers::SVG.clean(data) Gitlab::Sanitizers::SVG.clean(data)
end end
# Remove once https://gitlab.com/gitlab-org/gitlab-ce/issues/36103 is closed
# and :workhorse_set_content_type flag is removed
# If we blindly set the 'real' content type when serving a Git blob we # 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 # are enabling XSS attacks. An attacker could upload e.g. a Javascript
# file to a Git repository, trick the browser of a victim into # file to a Git repository, trick the browser of a victim into
...@@ -161,6 +163,8 @@ module BlobHelper ...@@ -161,6 +163,8 @@ module BlobHelper
end end
def content_disposition(blob, inline) def content_disposition(blob, inline)
# Remove the following line when https://gitlab.com/gitlab-org/gitlab-ce/issues/36103
# is closed and :workhorse_set_content_type flag is removed
return 'attachment' if blob.extension == 'svg' return 'attachment' if blob.extension == 'svg'
inline ? 'inline' : 'attachment' inline ? 'inline' : 'attachment'
......
...@@ -6,8 +6,13 @@ module WorkhorseHelper ...@@ -6,8 +6,13 @@ module WorkhorseHelper
# Send a Git blob through Workhorse # Send a Git blob through Workhorse
def send_git_blob(repository, blob, inline: true) def send_git_blob(repository, blob, inline: true)
headers.store(*Gitlab::Workhorse.send_git_blob(repository, blob)) headers.store(*Gitlab::Workhorse.send_git_blob(repository, blob))
headers['Content-Disposition'] = content_disposition(blob, inline) headers['Content-Disposition'] = content_disposition(blob, inline)
headers['Content-Type'] = safe_content_type(blob) headers['Content-Type'] = safe_content_type(blob)
# If enabled, this will override the values set above
workhorse_set_content_type!
render plain: "" render plain: ""
end end
...@@ -40,4 +45,8 @@ module WorkhorseHelper ...@@ -40,4 +45,8 @@ module WorkhorseHelper
def set_workhorse_internal_api_content_type def set_workhorse_internal_api_content_type
headers['Content-Type'] = Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE headers['Content-Type'] = Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
end end
def workhorse_set_content_type!
headers[Gitlab::Workhorse::DETECT_HEADER] = "true" if Feature.enabled?(:workhorse_set_content_type)
end
end end
---
title: Added feature flag to signal content headers detection by Workhorse
merge_request: 22667
author:
type: added
...@@ -13,6 +13,7 @@ module Gitlab ...@@ -13,6 +13,7 @@ module Gitlab
INTERNAL_API_REQUEST_HEADER = 'Gitlab-Workhorse-Api-Request'.freeze INTERNAL_API_REQUEST_HEADER = 'Gitlab-Workhorse-Api-Request'.freeze
NOTIFICATION_CHANNEL = 'workhorse:notifications'.freeze NOTIFICATION_CHANNEL = 'workhorse:notifications'.freeze
ALLOWED_GIT_HTTP_ACTIONS = %w[git_receive_pack git_upload_pack info_refs].freeze ALLOWED_GIT_HTTP_ACTIONS = %w[git_receive_pack git_upload_pack info_refs].freeze
DETECT_HEADER = 'Gitlab-Workhorse-Detect-Content-Type'.freeze
# Supposedly the effective key size for HMAC-SHA256 is 256 bits, i.e. 32 # Supposedly the effective key size for HMAC-SHA256 is 256 bits, i.e. 32
# bytes https://tools.ietf.org/html/rfc4868#section-2.6 # bytes https://tools.ietf.org/html/rfc4868#section-2.6
......
...@@ -26,12 +26,37 @@ describe Projects::AvatarsController do ...@@ -26,12 +26,37 @@ describe Projects::AvatarsController do
context 'when the avatar is stored in the repository' do context 'when the avatar is stored in the repository' do
let(:filepath) { 'files/images/logo-white.png' } let(:filepath) { 'files/images/logo-white.png' }
it 'sends the avatar' do context 'when feature flag workhorse_set_content_type is' do
subject before do
stub_feature_flags(workhorse_set_content_type: flag_value)
end
expect(response).to have_gitlab_http_status(200) context 'enabled' do
expect(response.header['Content-Type']).to eq('image/png') let(:flag_value) { true }
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
it 'sends the avatar' do
subject
expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Disposition']).to eq('inline')
expect(response.header['Content-Type']).to eq 'image/png'
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
end
end
context 'disabled' do
let(:flag_value) { false }
it 'sends the avatar' do
subject
expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Type']).to eq('image/png')
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
end
end
end end
end end
......
...@@ -838,23 +838,48 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do ...@@ -838,23 +838,48 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
context "when job has a trace artifact" do context "when job has a trace artifact" do
let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
it 'returns a trace' do context 'when feature flag workhorse_set_content_type is' do
response = subject before do
stub_feature_flags(workhorse_set_content_type: flag_value)
end
expect(response).to have_gitlab_http_status(:ok) context 'enabled' do
expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8") let(:flag_value) { true }
expect(response.body).to eq(job.job_artifacts_trace.open.read)
it "sets #{Gitlab::Workhorse::DETECT_HEADER} header" do
response = subject
expect(response).to have_gitlab_http_status(:ok)
expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
expect(response.body).to eq(job.job_artifacts_trace.open.read)
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
end
end
context 'disabled' do
let(:flag_value) { false }
it 'returns a trace' do
response = subject
expect(response).to have_gitlab_http_status(:ok)
expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
expect(response.body).to eq(job.job_artifacts_trace.open.read)
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to be nil
end
end
end end
end end
context "when job has a trace file" do context "when job has a trace file" do
let(:job) { create(:ci_build, :trace_live, pipeline: pipeline) } let(:job) { create(:ci_build, :trace_live, pipeline: pipeline) }
it "send a trace file" do it 'sends a trace file' do
response = subject response = subject
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8") expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8")
expect(response.headers["Content-Disposition"]).to match(/^inline/)
expect(response.body).to eq("BUILD TRACE") expect(response.body).to eq("BUILD TRACE")
end end
end end
...@@ -866,12 +891,27 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do ...@@ -866,12 +891,27 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
job.update_column(:trace, "Sample trace") job.update_column(:trace, "Sample trace")
end end
it "send a trace file" do it 'sends a trace file' do
response = subject response = subject
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(response.headers["Content-Type"]).to eq("text/plain; charset=utf-8") expect(response.headers['Content-Type']).to eq('text/plain; charset=utf-8')
expect(response.body).to eq("Sample trace") expect(response.headers['Content-Disposition']).to match(/^inline/)
expect(response.body).to eq('Sample trace')
end
context 'when trace format is not text/plain' do
before do
job.update_column(:trace, '<html></html>')
end
it 'sets content disposition to attachment' do
response = subject
expect(response).to have_gitlab_http_status(:ok)
expect(response.headers['Content-Type']).to eq('text/plain; charset=utf-8')
expect(response.headers['Content-Disposition']).to match(/^attachment/)
end
end end
end end
......
...@@ -14,26 +14,74 @@ describe Projects::RawController do ...@@ -14,26 +14,74 @@ describe Projects::RawController do
context 'regular filename' do context 'regular filename' do
let(:filepath) { 'master/README.md' } let(:filepath) { 'master/README.md' }
it 'delivers ASCII file' do context 'when feature flag workhorse_set_content_type is' do
subject before do
stub_feature_flags(workhorse_set_content_type: flag_value)
expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8') subject
expect(response.header['Content-Disposition']) end
.to eq('inline')
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') context 'enabled' do
let(:flag_value) { true }
it 'delivers ASCII file' do
expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
expect(response.header['Content-Disposition']).to eq('inline')
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
end
end
context 'disabled' do
let(:flag_value) { false }
it 'delivers ASCII file' do
expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
expect(response.header['Content-Disposition']).to eq('inline')
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
end
end
end end
end end
context 'image header' do context 'image header' do
let(:filepath) { 'master/files/images/6049019_460s.jpg' } let(:filepath) { 'master/files/images/6049019_460s.jpg' }
it 'sets image content type header' do context 'when feature flag workhorse_set_content_type is' do
subject before do
stub_feature_flags(workhorse_set_content_type: flag_value)
end
context 'enabled' do
let(:flag_value) { true }
it 'leaves image content disposition' do
subject
expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Type']).to eq('image/jpeg')
expect(response.header['Content-Disposition']).to eq('inline')
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
end
end
context 'disabled' do
let(:flag_value) { false }
it 'sets image content type header' do
subject
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response.header['Content-Type']).to eq('image/jpeg') expect(response.header['Content-Type']).to eq('image/jpeg')
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') expect(response.header['Content-Disposition']).to eq('inline')
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
end
end
end end
end end
......
...@@ -52,24 +52,56 @@ describe Projects::WikisController do ...@@ -52,24 +52,56 @@ describe Projects::WikisController do
let(:path) { upload_file_to_wiki(project, user, file_name) } let(:path) { upload_file_to_wiki(project, user, file_name) }
before do
subject
end
subject { get :show, namespace_id: project.namespace, project_id: project, id: path } subject { get :show, namespace_id: project.namespace, project_id: project, id: path }
context 'when file is an image' do context 'when file is an image' do
let(:file_name) { 'dk.png' } let(:file_name) { 'dk.png' }
it 'renders the content inline' do context 'when feature flag workhorse_set_content_type is' do
expect(response.headers['Content-Disposition']).to match(/^inline/) before do
end stub_feature_flags(workhorse_set_content_type: flag_value)
subject
end
context 'when file is a svg' do context 'enabled' do
let(:file_name) { 'unsanitized.svg' } let(:flag_value) { true }
it 'renders the content as an attachment' do it 'delivers the image' do
expect(response.headers['Content-Disposition']).to match(/^attachment/) expect(response.headers['Content-Type']).to eq('image/png')
expect(response.headers['Content-Disposition']).to match(/^inline/)
expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
end
context 'when file is a svg' do
let(:file_name) { 'unsanitized.svg' }
it 'delivers the image' do
expect(response.headers['Content-Type']).to eq('image/svg+xml')
expect(response.headers['Content-Disposition']).to match(/^attachment/)
expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
end
end
end
context 'disabled' do
let(:flag_value) { false }
it 'renders the content inline' do
expect(response.headers['Content-Type']).to eq('image/png')
expect(response.headers['Content-Disposition']).to match(/^inline/)
expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
end
context 'when file is a svg' do
let(:file_name) { 'unsanitized.svg' }
it 'renders the content as an attachment' do
expect(response.headers['Content-Type']).to eq('image/svg+xml')
expect(response.headers['Content-Disposition']).to match(/^attachment/)
expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
end
end
end end
end end
end end
...@@ -77,8 +109,32 @@ describe Projects::WikisController do ...@@ -77,8 +109,32 @@ describe Projects::WikisController do
context 'when file is a pdf' do context 'when file is a pdf' do
let(:file_name) { 'git-cheat-sheet.pdf' } let(:file_name) { 'git-cheat-sheet.pdf' }
it 'sets the content type to application/octet-stream' do context 'when feature flag workhorse_set_content_type is' do
expect(response.headers['Content-Type']).to eq 'application/octet-stream' before do
stub_feature_flags(workhorse_set_content_type: flag_value)
subject
end
context 'enabled' do
let(:flag_value) { true }
it 'sets the content type to sets the content response headers' do
expect(response.headers['Content-Type']).to eq 'application/octet-stream'
expect(response.headers['Content-Disposition']).to match(/^inline/)
expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
end
end
context 'disabled' do
let(:flag_value) { false }
it 'sets the content response headers' do
expect(response.headers['Content-Type']).to eq 'application/octet-stream'
expect(response.headers['Content-Disposition']).to match(/^inline/)
expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq nil
end
end
end end
end end
end end
......
...@@ -437,7 +437,10 @@ describe SnippetsController do ...@@ -437,7 +437,10 @@ describe SnippetsController do
end end
context 'when signed in user is the author' do context 'when signed in user is the author' do
let(:flag_value) { false }
before do before do
stub_feature_flags(workhorse_set_content_type: flag_value)
get :raw, id: personal_snippet.to_param get :raw, id: personal_snippet.to_param
end end
...@@ -451,6 +454,24 @@ describe SnippetsController do ...@@ -451,6 +454,24 @@ describe SnippetsController do
expect(response.header['Content-Disposition']).to match(/inline/) expect(response.header['Content-Disposition']).to match(/inline/)
end end
context 'when feature flag workhorse_set_content_type is' do
context 'enabled' do
let(:flag_value) { true }
it "sets #{Gitlab::Workhorse::DETECT_HEADER} header" do
expect(response).to have_gitlab_http_status(200)
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
end
end
context 'disabled' do
it "does not set #{Gitlab::Workhorse::DETECT_HEADER} header" do
expect(response).to have_gitlab_http_status(200)
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to be nil
end
end
end
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