From 33e595567115f508ff069ee4927e10eae49e87a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 19 Dec 2018 12:51:07 +0100 Subject: [PATCH] Removing workhorse_set_content_type feature flag Removing workhorse_set_content_type feature flag introduced in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22667 --- app/helpers/blob_helper.rb | 30 ------- app/helpers/workhorse_helper.rb | 5 +- .../projects/avatars_controller_spec.rb | 36 ++------ .../projects/jobs_controller_spec.rb | 35 ++------ .../projects/raw_controller_spec.rb | 73 +++------------- .../projects/wikis_controller_spec.rb | 85 ++++--------------- spec/controllers/snippets_controller_spec.rb | 22 +---- 7 files changed, 45 insertions(+), 241 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index bd42f00944f..4c8e1b209c0 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -140,36 +140,6 @@ 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 - # 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.extension == 'svg' - blob.mime_type - elsif blob.text? - 'text/plain; charset=utf-8' - elsif blob.image? - blob.content_type - else - 'application/octet-stream' - end - 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' - end - def ref_project @ref_project ||= @target_project || @project end diff --git a/app/helpers/workhorse_helper.rb b/app/helpers/workhorse_helper.rb index e9fc39e451b..bb5b1555dc4 100644 --- a/app/helpers/workhorse_helper.rb +++ b/app/helpers/workhorse_helper.rb @@ -7,8 +7,7 @@ module WorkhorseHelper 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) + headers['Content-Disposition'] = inline ? 'inline' : 'attachment' # If enabled, this will override the values set above workhorse_set_content_type! @@ -47,6 +46,6 @@ module WorkhorseHelper end def workhorse_set_content_type! - headers[Gitlab::Workhorse::DETECT_HEADER] = "true" if Feature.enabled?(:workhorse_set_content_type) + headers[Gitlab::Workhorse::DETECT_HEADER] = "true" end end diff --git a/spec/controllers/projects/avatars_controller_spec.rb b/spec/controllers/projects/avatars_controller_spec.rb index 40ab81395ea..95b7ae5885a 100644 --- a/spec/controllers/projects/avatars_controller_spec.rb +++ b/spec/controllers/projects/avatars_controller_spec.rb @@ -26,37 +26,13 @@ describe Projects::AvatarsController do context 'when the avatar is stored in the repository' do let(:filepath) { 'files/images/logo-white.png' } - context 'when feature flag workhorse_set_content_type is' do - before do - stub_feature_flags(workhorse_set_content_type: flag_value) - end + it 'sends the avatar' do + subject - 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 + expect(response).to have_gitlab_http_status(200) + expect(response.header['Content-Disposition']).to eq('inline') + 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 diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index e1133fdd562..7f65fe551e9 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -892,36 +892,13 @@ 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) } - context 'when feature flag workhorse_set_content_type is' do - before do - stub_feature_flags(workhorse_set_content_type: flag_value) - end + it "sets #{Gitlab::Workhorse::DETECT_HEADER} header" do + response = subject - 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 + 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 diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index 24e2441cf9d..cffdf30da6b 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -16,74 +16,27 @@ describe Projects::RawController do context 'regular filename' do let(:filepath) { 'master/README.md' } - context 'when feature flag workhorse_set_content_type is' do - before do - stub_feature_flags(workhorse_set_content_type: flag_value) + it 'delivers ASCII file' do + subject - 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 + 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 'image header' do let(:filepath) { 'master/files/images/6049019_460s.jpg' } - context 'when feature flag workhorse_set_content_type is' do - before do - stub_feature_flags(workhorse_set_content_type: flag_value) - end + it 'leaves image content disposition' do + subject - 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['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 + expect(response).to have_gitlab_http_status(200) + 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 diff --git a/spec/controllers/projects/wikis_controller_spec.rb b/spec/controllers/projects/wikis_controller_spec.rb index 341bf244397..b2f40231796 100644 --- a/spec/controllers/projects/wikis_controller_spec.rb +++ b/spec/controllers/projects/wikis_controller_spec.rb @@ -52,56 +52,24 @@ describe Projects::WikisController do let(:path) { upload_file_to_wiki(project, user, file_name) } - subject { get :show, params: { namespace_id: project.namespace, project_id: project, id: path } } + before do + get :show, params: { namespace_id: project.namespace, project_id: project, id: path } + end context 'when file is an image' do let(:file_name) { 'dk.png' } - context 'when feature flag workhorse_set_content_type is' do - before do - stub_feature_flags(workhorse_set_content_type: flag_value) + it 'delivers the image' do + expect(response.headers['Content-Disposition']).to match(/^inline/) + expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" + end - subject - end + context 'when file is a svg' do + let(:file_name) { 'unsanitized.svg' } - context 'enabled' do - let(:flag_value) { true } - - 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 + it 'delivers the image' do + expect(response.headers['Content-Disposition']).to match(/^inline/) + expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" end end end @@ -109,32 +77,9 @@ describe Projects::WikisController do context 'when file is a pdf' do let(:file_name) { 'git-cheat-sheet.pdf' } - 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 + it 'sets the content type to sets the content response headers' do + expect(response.headers['Content-Disposition']).to match(/^inline/) + expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" end end end diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 01a5161f775..d2a56518f65 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -437,10 +437,7 @@ 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, params: { id: personal_snippet.to_param } end @@ -455,22 +452,9 @@ 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 + 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 end