From 563879264bb4367633f9b7ae936044712737ef5d Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 9 Aug 2023 11:01:05 +0100 Subject: [PATCH] Patch ActionDispatch::Response to prevent X-Sendfile body parsing Patches ActionDispatch::Response to return a new custom derivative of ActionDispatch::Response::FileBody, which will prevent regressions being introduced that attempt to load the contents of files being returned as an X-Sendfile response, causing memory bloat and process locks. Changelog: performance --- app/controllers/application_controller.rb | 6 +- .../block_xsendfile_response_body_parsing.yml | 8 ++ .../rails_action_dispatch_http_patch.rb | 17 ++++ lib/gitlab/external_authorization.rb | 2 + .../testing/request_inspector_middleware.rb | 7 +- lib/gitlab/utils/file_body.rb | 64 +++++++++++++ .../lfs_storage_controller_spec.rb | 96 ++++++++++++++++--- spec/features/projects/jobs_spec.rb | 2 +- spec/lib/gitlab/utils/file_body_spec.rb | 58 +++++++++++ 9 files changed, 244 insertions(+), 16 deletions(-) create mode 100644 config/feature_flags/development/block_xsendfile_response_body_parsing.yml create mode 100644 config/initializers/rails_action_dispatch_http_patch.rb create mode 100644 lib/gitlab/utils/file_body.rb create mode 100644 spec/lib/gitlab/utils/file_body_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ef489a65575cb7..bc9737a2f062fe 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -198,7 +198,11 @@ def append_info_to_payload(payload) payload[:queue_duration_s] = request.env[::Gitlab::Middleware::RailsQueueDuration::GITLAB_RAILS_QUEUE_DURATION_KEY] - payload[:response_bytes] = response.body_parts.sum(&:bytesize) if Feature.enabled?(:log_response_length) + begin + payload[:response_bytes] = response.body_parts.sum(&:bytesize) if Feature.enabled?(:log_response_length) + rescue Gitlab::Utils::FileBody::AttemptedToReadBodyOfSendFileResponseError + # noop + end store_cloudflare_headers!(payload, request) end diff --git a/config/feature_flags/development/block_xsendfile_response_body_parsing.yml b/config/feature_flags/development/block_xsendfile_response_body_parsing.yml new file mode 100644 index 00000000000000..d1ef68d8df81f6 --- /dev/null +++ b/config/feature_flags/development/block_xsendfile_response_body_parsing.yml @@ -0,0 +1,8 @@ +--- +name: block_xsendfile_response_body_parsing +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128869 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/421688 +milestone: '16.3' +type: development +group: group::source code +default_enabled: false diff --git a/config/initializers/rails_action_dispatch_http_patch.rb b/config/initializers/rails_action_dispatch_http_patch.rb new file mode 100644 index 00000000000000..4be05e37e19756 --- /dev/null +++ b/config/initializers/rails_action_dispatch_http_patch.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +# This patch prevents accidental reads of X-Sendfile response bodies. +# By default Rails sets the response body to an instance of the FileBody +# class in actionpack/lib/action_dispatch/http/response.rb. +# +# We replace this with a call to our own implementation that blocks +# the reading of the response body on X-Sendfile responses. + +module ActionDispatch + class Response + def send_file(path) + commit! + @stream = Gitlab::Utils::FileBody.new(path, request.headers) + end + end +end diff --git a/lib/gitlab/external_authorization.rb b/lib/gitlab/external_authorization.rb index 25f8b7b3628424..84194627913655 100644 --- a/lib/gitlab/external_authorization.rb +++ b/lib/gitlab/external_authorization.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative "external_authorization/config" + module Gitlab module ExternalAuthorization extend ExternalAuthorization::Config diff --git a/lib/gitlab/testing/request_inspector_middleware.rb b/lib/gitlab/testing/request_inspector_middleware.rb index 3cbe97cd84cee6..1a307e54f7949b 100644 --- a/lib/gitlab/testing/request_inspector_middleware.rb +++ b/lib/gitlab/testing/request_inspector_middleware.rb @@ -40,7 +40,12 @@ def call(env) status, headers, body = @app.call(env) full_body = +'' - body.each { |b| full_body << b } + + begin + body.each { |b| full_body << b } + rescue Gitlab::Utils::FileBody::AttemptedToReadBodyOfSendFileResponseError + # noop + end request = Request.new( url: url, diff --git a/lib/gitlab/utils/file_body.rb b/lib/gitlab/utils/file_body.rb new file mode 100644 index 00000000000000..a12b7113a69b6a --- /dev/null +++ b/lib/gitlab/utils/file_body.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +# Reimplementation of ActionDispatch::Response::FileBody +# Used to block reading of X-Sendfile response bodies. +# +# Patched into Rails in config/initializers/rails_action_dispatch_http_patch.rb +# +# The only two ways in which Rack::SendFile detects when to send an X-Sendfile response are: +# - The request includes the X-Sendfile header +# - The response body responds to #to_path +# +# This class is only ever instantiated when setting the value of the body in the #send_file +# method, and so won't conflict with standard body responses. + +module Gitlab + module Utils + class FileBody < ActionDispatch::Response::FileBody + AttemptedToReadBodyOfSendFileResponseError = Class.new(StandardError) + + ERROR_TEXT = "parsing of an X-Sendfile response body in Ruby has been blocked" + + attr_reader :to_path, :headers + + def initialize(path, headers) + @to_path = path + @headers = headers + end + + def body + return super unless feature_flag_enabled? && sendfile_response? + + raise AttemptedToReadBodyOfSendFileResponseError, ERROR_TEXT + end + + def each + return super unless feature_flag_enabled? && sendfile_response? + + raise AttemptedToReadBodyOfSendFileResponseError, ERROR_TEXT + end + + private + + # This is loaded in an initializer, and though unlikely to be called there, + # we should ensure the feature flag system is set up before continuing. + def feature_flags_available? + ActiveRecord::Base.connection.active? && Feature::FlipperFeature.table_exists? # rubocop: disable Database/MultipleDatabases + rescue StandardError, ActiveRecord::NoDatabaseError + false + end + + def feature_flag_enabled? + feature_flags_available? && Feature.enabled?(:block_xsendfile_response_body_parsing) + end + + def sendfile_request? + headers.key?("HTTP_X_SENDFILE_TYPE") + end + + def sendfile_response? + to_path.present? && sendfile_request? + end + end + end +end diff --git a/spec/controllers/repositories/lfs_storage_controller_spec.rb b/spec/controllers/repositories/lfs_storage_controller_spec.rb index 672e6f1e85b784..8ae501dbbc130f 100644 --- a/spec/controllers/repositories/lfs_storage_controller_spec.rb +++ b/spec/controllers/repositories/lfs_storage_controller_spec.rb @@ -2,10 +2,21 @@ require 'spec_helper' -RSpec.describe Repositories::LfsStorageController do +RSpec.describe Repositories::LfsStorageController, feature_category: :source_code_management do using RSpec::Parameterized::TableSyntax include GitHttpHelpers + def temp_file + upload_path = LfsObjectUploader.workhorse_local_upload_path + file_path = "#{upload_path}/lfs" + + FileUtils.mkdir_p(upload_path) + File.write(file_path, 'test') + File.truncate(file_path, params[:size].to_i) + + UploadedFile.new(file_path, filename: File.basename(file_path), sha256: params[:oid]) + end + let_it_be(:project) { create(:project, :public) } let_it_be(:user) { create(:user) } let_it_be(:pat) { create(:personal_access_token, user: user, scopes: ['write_repository']) } @@ -23,9 +34,79 @@ stub_config(lfs: { enabled: lfs_enabled }) end + describe "GET #download" do + let(:headers) { workhorse_internal_api_request_header } + + let(:params) do + { + repository_path: "#{project.full_path}.git", + oid: "6b9765d3888aaec789e8c309eb05b05c3a87895d6ad70d2264bd7270fff665ac", + size: "6725030" + } + end + + let(:lfs_object) { instance_double("LfsObject", file: file) } + let(:file) do + instance_double("LfsObjectUploader", path: temp_file.path, exists?: true, file_storage?: true) + end + + before do + request.headers.merge!(extra_headers) + request.headers.merge!(headers) + + allow(LfsObject).to receive(:find_by_oid).and_return(lfs_object) + end + + subject do + get :download, params: params + end + + context "when sendfile header is present in the request" do + let(:extra_headers) do + { + "X-Sendfile-Type" => "X-Sendfile" + } + end + + it { is_expected.to be_successful } + + it "cannot return a file body" do + expect { subject.body_parts.sum(&:bytesize) }.to raise_error(Gitlab::Utils::FileBody::AttemptedToReadBodyOfSendFileResponseError) + end + end + + context "when sendfile header is not present in the request" do + let(:extra_headers) { {} } + + it { is_expected.to be_successful } + + it "can return a file body" do + expect(subject.body_parts.sum(&:bytesize)).to eq(6725030) + end + end + + context "when feature flag is disabled" do + let(:extra_headers) do + { + "X-Sendfile-Type" => "X-Sendfile" + } + end + + before do + stub_feature_flags(block_xsendfile_response_body_parsing: false) + end + + it { is_expected.to be_successful } + + it "can return a file body" do + expect(subject.body_parts.sum(&:bytesize)).to eq(6725030) + end + end + end + describe 'PUT #upload_authorize' do let(:headers) { workhorse_internal_api_request_header } - let(:extra_headers) { {} } + let(:extra_headers) { { 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(user.username, pat.token) } } before do request.headers.merge!(extra_headers) @@ -230,16 +311,5 @@ it_behaves_like 'returning response status', :not_implemented end - - def temp_file - upload_path = LfsObjectUploader.workhorse_local_upload_path - file_path = "#{upload_path}/lfs" - - FileUtils.mkdir_p(upload_path) - File.write(file_path, 'test') - File.truncate(file_path, params[:size].to_i) - - UploadedFile.new(file_path, filename: File.basename(file_path), sha256: params[:oid]) - end end end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index 67486b545c9a57..9429342cd04977 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -309,7 +309,7 @@ expect(artifact_request.response_headers['Content-Disposition']).to eq(%{attachment; filename="#{job.artifacts_file.filename}"; filename*=UTF-8''#{job.artifacts_file.filename}}) expect(artifact_request.response_headers['Content-Transfer-Encoding']).to eq("binary") expect(artifact_request.response_headers['Content-Type']).to eq("image/gif") - expect(artifact_request.body).to eq(job.artifacts_file.file.read.b) + expect(artifact_request.response_headers['X-Sendfile']).to eq(job.artifacts_file.file.path) end end diff --git a/spec/lib/gitlab/utils/file_body_spec.rb b/spec/lib/gitlab/utils/file_body_spec.rb new file mode 100644 index 00000000000000..4d8df3465a57be --- /dev/null +++ b/spec/lib/gitlab/utils/file_body_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Gitlab::Utils::FileBody, feature_category: :source_code_management do + let(:original_implementation) { ActionDispatch::Response::FileBody.new(file_path) } + let(:file_path) { Rails.root.join("spec/fixtures/sample.ico") } + + let(:headers) do + { + "HTTP_X_SENDFILE_TYPE" => "X-Sendfile" + } + end + + subject(:file_body) { described_class.new(file_path, headers) } + + describe "#body" do + subject { file_body.body } + + it "raises an error if called" do + expect { subject }.to raise_error(described_class::AttemptedToReadBodyOfSendFileResponseError, described_class::ERROR_TEXT) # rubocop: ignore Layout/LineLength + end + + context "when feature flag is disabled" do + before do + stub_feature_flags(block_xsendfile_response_body_parsing: false) + end + + it { is_expected.to eq(original_implementation.body) } + end + end + + describe "#each" do + subject { bytesize(file_body) } + + it "raises an error if called" do + expect { subject }.to raise_error(described_class::AttemptedToReadBodyOfSendFileResponseError, described_class::ERROR_TEXT) # rubocop: ignore Layout/LineLength + end + + context "when feature flag is disabled" do + before do + stub_feature_flags(block_xsendfile_response_body_parsing: false) + end + + it { is_expected.to eq bytesize(original_implementation) } + end + end + + def bytesize(file_body) + size = 0 + + file_body.each do |s| + size += s.bytesize + end + + size + end +end -- GitLab