From cf8b8ff99b26d0a1f90be289cea08344bb8baff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Thu, 6 Dec 2018 21:22:39 +0000 Subject: Add feature flag for workhorse content type calculation --- GITLAB_WORKHORSE_VERSION | 2 +- Gemfile | 3 + Gemfile.lock | 3 +- Gemfile.rails4.lock | 3 +- app/controllers/concerns/snippets_actions.rb | 2 + app/controllers/concerns/uploads_actions.rb | 1 + app/controllers/projects/jobs_controller.rb | 18 ++++- app/helpers/blob_helper.rb | 4 ++ app/helpers/workhorse_helper.rb | 9 +++ changelogs/unreleased/fj-clean-content-headers.yml | 5 ++ lib/gitlab/workhorse.rb | 1 + .../projects/avatars_controller_spec.rb | 35 +++++++-- spec/controllers/projects/jobs_controller_spec.rb | 58 ++++++++++++--- spec/controllers/projects/raw_controller_spec.rb | 74 +++++++++++++++---- spec/controllers/projects/wikis_controller_spec.rb | 82 ++++++++++++++++++---- spec/controllers/snippets_controller_spec.rb | 21 ++++++ 16 files changed, 277 insertions(+), 44 deletions(-) create mode 100644 changelogs/unreleased/fj-clean-content-headers.yml diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index ba7f754d0c3..18bb4182dd0 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -7.4.0 +7.5.0 diff --git a/Gemfile b/Gemfile index bc53b70d2f6..cd2e5df3255 100644 --- a/Gemfile +++ b/Gemfile @@ -263,6 +263,9 @@ gem 'ace-rails-ap', '~> 4.1.0' # Detect and convert string character encoding gem 'charlock_holmes', '~> 0.7.5' +# Detect mime content type from content +gem 'mimemagic', '~> 0.3.2' + # Faster blank gem 'fast_blank' diff --git a/Gemfile.lock b/Gemfile.lock index 8a9e023c8ee..608d1814127 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -459,7 +459,7 @@ GEM mime-types (3.2.2) mime-types-data (~> 3.2015) mime-types-data (3.2018.0812) - mimemagic (0.3.0) + mimemagic (0.3.2) mini_magick (4.8.0) mini_mime (1.0.1) mini_portile2 (2.3.0) @@ -1051,6 +1051,7 @@ DEPENDENCIES loofah (~> 2.2) mail_room (~> 0.9.1) method_source (~> 0.8) + mimemagic (~> 0.3.2) mini_magick minitest (~> 5.7.0) mysql2 (~> 0.4.10) diff --git a/Gemfile.rails4.lock b/Gemfile.rails4.lock index 924086cfd68..ad10511c366 100644 --- a/Gemfile.rails4.lock +++ b/Gemfile.rails4.lock @@ -456,7 +456,7 @@ GEM mime-types (3.2.2) mime-types-data (~> 3.2015) mime-types-data (3.2018.0812) - mimemagic (0.3.0) + mimemagic (0.3.2) mini_magick (4.8.0) mini_mime (1.0.1) mini_portile2 (2.3.0) @@ -1042,6 +1042,7 @@ DEPENDENCIES loofah (~> 2.2) mail_room (~> 0.9.1) method_source (~> 0.8) + mimemagic (~> 0.3.2) mini_magick minitest (~> 5.7.0) mysql2 (~> 0.4.10) diff --git a/app/controllers/concerns/snippets_actions.rb b/app/controllers/concerns/snippets_actions.rb index 8c22490700c..014232a7d05 100644 --- a/app/controllers/concerns/snippets_actions.rb +++ b/app/controllers/concerns/snippets_actions.rb @@ -10,6 +10,8 @@ module SnippetsActions def raw disposition = params[:inline] == 'false' ? 'attachment' : 'inline' + workhorse_set_content_type! + send_data( convert_line_endings(@snippet.content), type: 'text/plain; charset=utf-8', diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 5912fffc058..0eea0cdd50f 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -38,6 +38,7 @@ module UploadsActions return render_404 unless uploader + workhorse_set_content_type! send_upload(uploader, attachment: uploader.filename, disposition: disposition) end diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb index 3ecf94c008e..c58b30eace7 100644 --- a/app/controllers/projects/jobs_controller.rb +++ b/app/controllers/projects/jobs_controller.rb @@ -140,15 +140,22 @@ class Projects::JobsController < Projects::ApplicationController def raw if trace_artifact_file + workhorse_set_content_type! send_upload(trace_artifact_file, send_params: raw_send_params, redirect_params: raw_redirect_params) else build.trace.read do |stream| if stream.file? + workhorse_set_content_type! send_file stream.path, type: 'text/plain; charset=utf-8', disposition: 'inline' 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 @@ -201,4 +208,13 @@ class Projects::JobsController < Projects::ApplicationController def build_path(build) project_job_path(build.project, build) 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 diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 638744a1426..bd42f00944f 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -140,6 +140,8 @@ module BlobHelper Gitlab::Sanitizers::SVG.clean(data) 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 # are enabling XSS attacks. An attacker could upload e.g. a Javascript # file to a Git repository, trick the browser of a victim into @@ -161,6 +163,8 @@ module BlobHelper end 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' inline ? 'inline' : 'attachment' diff --git a/app/helpers/workhorse_helper.rb b/app/helpers/workhorse_helper.rb index 49c08dce96c..e9fc39e451b 100644 --- a/app/helpers/workhorse_helper.rb +++ b/app/helpers/workhorse_helper.rb @@ -6,8 +6,13 @@ module WorkhorseHelper # Send a Git blob through Workhorse def send_git_blob(repository, blob, inline: true) headers.store(*Gitlab::Workhorse.send_git_blob(repository, blob)) + headers['Content-Disposition'] = content_disposition(blob, inline) headers['Content-Type'] = safe_content_type(blob) + + # If enabled, this will override the values set above + workhorse_set_content_type! + render plain: "" end @@ -40,4 +45,8 @@ module WorkhorseHelper def set_workhorse_internal_api_content_type headers['Content-Type'] = Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE end + + def workhorse_set_content_type! + headers[Gitlab::Workhorse::DETECT_HEADER] = "true" if Feature.enabled?(:workhorse_set_content_type) + end end diff --git a/changelogs/unreleased/fj-clean-content-headers.yml b/changelogs/unreleased/fj-clean-content-headers.yml new file mode 100644 index 00000000000..59e25ca6578 --- /dev/null +++ b/changelogs/unreleased/fj-clean-content-headers.yml @@ -0,0 +1,5 @@ +--- +title: Added feature flag to signal content headers detection by Workhorse +merge_request: 22667 +author: +type: added diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index e1f777e9cd1..da22ea9cf5c 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -13,6 +13,7 @@ module Gitlab INTERNAL_API_REQUEST_HEADER = 'Gitlab-Workhorse-Api-Request'.freeze NOTIFICATION_CHANNEL = 'workhorse:notifications'.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 # bytes https://tools.ietf.org/html/rfc4868#section-2.6 diff --git a/spec/controllers/projects/avatars_controller_spec.rb b/spec/controllers/projects/avatars_controller_spec.rb index 14059cff74c..5a77a7ac06f 100644 --- a/spec/controllers/projects/avatars_controller_spec.rb +++ b/spec/controllers/projects/avatars_controller_spec.rb @@ -26,12 +26,37 @@ describe Projects::AvatarsController do context 'when the avatar is stored in the repository' do let(:filepath) { 'files/images/logo-white.png' } - it 'sends the avatar' do - subject + context 'when feature flag workhorse_set_content_type is' do + before do + stub_feature_flags(workhorse_set_content_type: flag_value) + end - 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:') + context 'enabled' do + let(:flag_value) { true } + + 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 diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index da3d658d061..51a7cc63cef 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -838,23 +838,48 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do context "when job has a trace artifact" do let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } - it 'returns a trace' do - response = subject + context 'when feature flag workhorse_set_content_type is' do + before do + stub_feature_flags(workhorse_set_content_type: flag_value) + end - 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) + context 'enabled' do + let(:flag_value) { true } + + 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 context "when job has a trace file" do let(:job) { create(:ci_build, :trace_live, pipeline: pipeline) } - it "send a trace file" do + it 'sends a trace file' 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(/^inline/) expect(response.body).to eq("BUILD TRACE") end end @@ -866,12 +891,27 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do job.update_column(:trace, "Sample trace") end - it "send a trace file" do + it 'sends a trace file' 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("Sample trace") + 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('Sample trace') + end + + context 'when trace format is not text/plain' do + before do + job.update_column(:trace, '') + 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 diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index 6b658bf5295..d3cd15fbcd7 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -14,26 +14,74 @@ describe Projects::RawController do context 'regular filename' do let(:filepath) { 'master/README.md' } - it 'delivers ASCII file' do - subject - - 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::SEND_DATA_HEADER]).to start_with('git-blob:') + context 'when feature flag workhorse_set_content_type is' do + before do + stub_feature_flags(workhorse_set_content_type: flag_value) + + subject + end + + 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 context 'image header' do let(:filepath) { 'master/files/images/6049019_460s.jpg' } - it 'sets image content type header' do - subject + context 'when feature flag workhorse_set_content_type is' do + 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.header['Content-Type']).to eq('image/jpeg') - expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') + 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 nil + expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') + end + end end end diff --git a/spec/controllers/projects/wikis_controller_spec.rb b/spec/controllers/projects/wikis_controller_spec.rb index 6d75152857b..b974d927856 100644 --- a/spec/controllers/projects/wikis_controller_spec.rb +++ b/spec/controllers/projects/wikis_controller_spec.rb @@ -52,24 +52,56 @@ describe Projects::WikisController do 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 } context 'when file is an image' do let(:file_name) { 'dk.png' } - it 'renders the content inline' do - expect(response.headers['Content-Disposition']).to match(/^inline/) - end + context 'when feature flag workhorse_set_content_type is' do + before do + stub_feature_flags(workhorse_set_content_type: flag_value) + + subject + end - context 'when file is a svg' do - let(:file_name) { 'unsanitized.svg' } + context 'enabled' do + let(:flag_value) { true } - it 'renders the content as an attachment' do - expect(response.headers['Content-Disposition']).to match(/^attachment/) + it 'delivers the image' 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 "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 @@ -77,8 +109,32 @@ describe Projects::WikisController do context 'when file is a pdf' do let(:file_name) { 'git-cheat-sheet.pdf' } - it 'sets the content type to application/octet-stream' do - expect(response.headers['Content-Type']).to eq 'application/octet-stream' + context 'when feature flag workhorse_set_content_type is' do + 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 diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 9effe47ab05..957bab638b1 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -437,7 +437,10 @@ describe SnippetsController do end context 'when signed in user is the author' do + let(:flag_value) { false } + before do + stub_feature_flags(workhorse_set_content_type: flag_value) get :raw, id: personal_snippet.to_param end @@ -451,6 +454,24 @@ describe SnippetsController do expect(response.header['Content-Disposition']).to match(/inline/) 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 -- cgit v1.2.1