diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-06-29 14:09:54 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-06-29 14:09:54 +0000 |
commit | 37823295027da50ff5bc14df482b8cba09bf41b4 (patch) | |
tree | b2a9e1deb265b777cb20cb6b4c512be955153a3b | |
parent | 6bea43795252f980eeee7ce67413ef440da88a31 (diff) | |
download | gitlab-ce-37823295027da50ff5bc14df482b8cba09bf41b4.tar.gz |
Add latest changes from gitlab-org/security/gitlab@15-1-stable-ee
-rw-r--r-- | app/graphql/resolvers/ci/config_resolver.rb | 2 | ||||
-rw-r--r-- | config/initializers/net_http_response_patch.rb | 46 | ||||
-rw-r--r-- | doc/update/index.md | 3 | ||||
-rw-r--r-- | lib/gitlab/buffered_io.rb | 6 | ||||
-rw-r--r-- | spec/graphql/resolvers/ci/config_resolver_spec.rb | 100 | ||||
-rw-r--r-- | spec/initializers/net_http_response_patch_spec.rb | 79 | ||||
-rw-r--r-- | spec/lib/gitlab/buffered_io_spec.rb | 64 |
7 files changed, 219 insertions, 81 deletions
diff --git a/app/graphql/resolvers/ci/config_resolver.rb b/app/graphql/resolvers/ci/config_resolver.rb index 6f861012d83..1f486c47771 100644 --- a/app/graphql/resolvers/ci/config_resolver.rb +++ b/app/graphql/resolvers/ci/config_resolver.rb @@ -12,7 +12,7 @@ module Resolvers Should not be requested more than once per request. MD - authorize :read_pipeline + authorize :create_pipeline argument :project_path, GraphQL::Types::ID, required: true, diff --git a/config/initializers/net_http_response_patch.rb b/config/initializers/net_http_response_patch.rb new file mode 100644 index 00000000000..4ffe227a3fd --- /dev/null +++ b/config/initializers/net_http_response_patch.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Net + class HTTPResponse + # rubocop: disable Cop/LineBreakAfterGuardClauses + # rubocop: disable Cop/LineBreakAroundConditionalBlock + # rubocop: disable Layout/EmptyLineAfterGuardClause + # rubocop: disable Style/AndOr + # rubocop: disable Style/CharacterLiteral + # rubocop: disable Style/InfiniteLoop + + # Original method: + # https://github.com/ruby/ruby/blob/v2_7_5/lib/net/http/response.rb#L54-L69 + # + # Our changes: + # - Pass along the `start_time` to `Gitlab::BufferedIo`, so we can raise a timeout + # if reading the headers takes too long. + # - Limit the regexes to avoid ReDoS attacks. + def self.each_response_header(sock) + start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) + key = value = nil + while true + line = sock.is_a?(Gitlab::BufferedIo) ? sock.readuntil("\n", true, start_time) : sock.readuntil("\n", true) + line = line.sub(/\s{0,10}\z/, '') + break if line.empty? + if line[0] == ?\s or line[0] == ?\t and value + # :nocov: + value << ' ' unless value.empty? + value << line.strip + # :nocov: + else + yield key, value if key + key, value = line.strip.split(/\s{0,10}:\s{0,10}/, 2) + raise Net::HTTPBadResponse, 'wrong header line format' if value.nil? + end + end + yield key, value if key + end + # rubocop: enable Cop/LineBreakAfterGuardClauses + # rubocop: enable Cop/LineBreakAroundConditionalBlock + # rubocop: enable Layout/EmptyLineAfterGuardClause + # rubocop: enable Style/AndOr + # rubocop: enable Style/CharacterLiteral + # rubocop: enable Style/InfiniteLoop + end +end diff --git a/doc/update/index.md b/doc/update/index.md index 416adb621d0..dcdcf8f01ae 100644 --- a/doc/update/index.md +++ b/doc/update/index.md @@ -463,6 +463,9 @@ and [Helm Chart deployments](https://docs.gitlab.com/charts/). They come with ap - If you run external PostgreSQL, particularly AWS RDS, [check you have a PostgreSQL bug fix](#postgresql-segmentation-fault-issue) to avoid the database crashing. +- Unauthenticated requests to the [`ciConfig` GraphQL field](../api/graphql/reference/index.md#queryciconfig) are no longer supported. + Before you upgrade to GitLab 15.1, add an [access token](../api/index.md#authentication) to your requests. + The user creating the token must have [permission](../user/permissions.md) to create pipelines in the project. ### 15.0.0 diff --git a/lib/gitlab/buffered_io.rb b/lib/gitlab/buffered_io.rb index 91d5d8acc52..b76c8191fa2 100644 --- a/lib/gitlab/buffered_io.rb +++ b/lib/gitlab/buffered_io.rb @@ -14,6 +14,7 @@ module Gitlab HEADER_READ_TIMEOUT = 20 + # rubocop: disable Style/RedundantBegin # rubocop: disable Style/RedundantReturn # rubocop: disable Cop/LineBreakAfterGuardClauses # rubocop: disable Layout/EmptyLineAfterGuardClause @@ -21,9 +22,7 @@ module Gitlab # Original method: # https://github.com/ruby/ruby/blob/cdb7d699d0641e8f081d590d06d07887ac09961f/lib/net/protocol.rb#L190-L200 override :readuntil - def readuntil(terminator, ignore_eof = false) - start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) - + def readuntil(terminator, ignore_eof = false, start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)) begin until idx = @rbuf.index(terminator) if (elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - start_time) > HEADER_READ_TIMEOUT @@ -39,6 +38,7 @@ module Gitlab return rbuf_consume(@rbuf.size) end end + # rubocop: disable Style/RedundantBegin # rubocop: enable Style/RedundantReturn # rubocop: enable Cop/LineBreakAfterGuardClauses # rubocop: enable Layout/EmptyLineAfterGuardClause diff --git a/spec/graphql/resolvers/ci/config_resolver_spec.rb b/spec/graphql/resolvers/ci/config_resolver_spec.rb index 7a6104fc503..dc030b1313b 100644 --- a/spec/graphql/resolvers/ci/config_resolver_spec.rb +++ b/spec/graphql/resolvers/ci/config_resolver_spec.rb @@ -7,24 +7,13 @@ RSpec.describe Resolvers::Ci::ConfigResolver do describe '#resolve' do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :repository, creator: user, namespace: user.namespace) } + let_it_be(:project) { create(:project, :repository) } let_it_be(:sha) { nil } let_it_be(:content) do File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci_includes.yml')) end - let(:ci_lint) do - ci_lint_double = instance_double(::Gitlab::Ci::Lint) - allow(ci_lint_double).to receive(:validate).and_return(fake_result) - - ci_lint_double - end - - before do - allow(::Gitlab::Ci::Lint).to receive(:new).and_return(ci_lint) - end - subject(:response) do resolve(described_class, args: { project_path: project.full_path, content: content, sha: sha }, @@ -51,52 +40,77 @@ RSpec.describe Resolvers::Ci::ConfigResolver do end end - context 'with a valid .gitlab-ci.yml' do - context 'with a sha' do - let(:sha) { '1231231' } + context 'when the user can create a pipeline' do + let(:ci_lint) do + ci_lint_double = instance_double(::Gitlab::Ci::Lint) + allow(ci_lint_double).to receive(:validate).and_return(fake_result) - it_behaves_like 'a valid config file' + ci_lint_double end - context 'without a sha' do - it_behaves_like 'a valid config file' + before do + allow(::Gitlab::Ci::Lint).to receive(:new).and_return(ci_lint) + + project.add_developer(user) end - end - context 'with an invalid .gitlab-ci.yml' do - let(:content) { 'invalid' } + context 'with a valid .gitlab-ci.yml' do + context 'with a sha' do + let(:sha) { '1231231' } - let(:fake_result) do - Gitlab::Ci::Lint::Result.new( - jobs: [], - merged_yaml: content, - errors: ['Invalid configuration format'], - warnings: [], - includes: [] - ) + it_behaves_like 'a valid config file' + end + + context 'without a sha' do + it_behaves_like 'a valid config file' + end end - it 'responds with errors about invalid syntax' do - expect(response[:status]).to eq(:invalid) - expect(response[:errors]).to eq(['Invalid configuration format']) + context 'with an invalid .gitlab-ci.yml' do + let(:content) { 'invalid' } + + let(:fake_result) do + Gitlab::Ci::Lint::Result.new( + jobs: [], + merged_yaml: content, + errors: ['Invalid configuration format'], + warnings: [], + includes: [] + ) + end + + it 'responds with errors about invalid syntax' do + expect(response[:status]).to eq(:invalid) + expect(response[:errors]).to match_array(['Invalid configuration format']) + end end - end - context 'with an invalid SHA' do - let_it_be(:sha) { ':' } + context 'with an invalid SHA' do + let_it_be(:sha) { ':' } - let(:ci_lint) do - ci_lint_double = instance_double(::Gitlab::Ci::Lint) - allow(ci_lint_double).to receive(:validate).and_raise(GRPC::InvalidArgument) + let(:ci_lint) do + ci_lint_double = instance_double(::Gitlab::Ci::Lint) + allow(ci_lint_double).to receive(:validate).and_raise(GRPC::InvalidArgument) - ci_lint_double + ci_lint_double + end + + it 'logs the invalid SHA to Sentry' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception) + .with(GRPC::InvalidArgument, sha: ':') + + response + end end + end - it 'logs the invalid SHA to Sentry' do - expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception) - .with(GRPC::InvalidArgument, sha: ':') + context 'when the user cannot create a pipeline' do + before do + project.add_guest(user) + end - response + it 'returns an error stating that the user cannot access the linting' do + expect(response).to be_instance_of(::Gitlab::Graphql::Errors::ResourceNotAvailable) end end end diff --git a/spec/initializers/net_http_response_patch_spec.rb b/spec/initializers/net_http_response_patch_spec.rb new file mode 100644 index 00000000000..3bd0d8c3907 --- /dev/null +++ b/spec/initializers/net_http_response_patch_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Net::HTTPResponse patch header read timeout' do + describe '.each_response_header' do + let(:server_response) do + <<~EOS + Content-Type: text/html + Header-Two: foo + + Hello World + EOS + end + + before do + stub_const('Gitlab::BufferedIo::HEADER_READ_TIMEOUT', 0.1) + end + + subject(:each_response_header) { Net::HTTPResponse.each_response_header(socket) { |k, v| } } + + context 'with Net::BufferedIO' do + let(:socket) { Net::BufferedIO.new(StringIO.new(server_response)) } + + it 'does not forward start time to the socket' do + allow(socket).to receive(:readuntil).and_call_original + expect(socket).to receive(:readuntil).with("\n", true) + + each_response_header + end + + context 'when the response contains many consecutive spaces' do + before do + expect(socket).to receive(:readuntil).and_return( + "a: #{' ' * 100_000} b", + '' + ) + end + + it 'has no regex backtracking issues' do + Timeout.timeout(1) do + each_response_header + end + end + end + end + + context 'with Gitlab::BufferedIo' do + let(:mock_io) { StringIO.new(server_response) } + let(:socket) { Gitlab::BufferedIo.new(mock_io) } + + it 'forwards start time to the socket' do + allow(socket).to receive(:readuntil).and_call_original + expect(socket).to receive(:readuntil).with("\n", true, kind_of(Numeric)) + + each_response_header + end + + context 'when the response contains an infinite number of headers' do + before do + read_counter = 0 + + allow(mock_io).to receive(:read_nonblock) do + read_counter += 1 + raise 'Test did not raise HeaderReadTimeout' if read_counter > 10 + + sleep 0.01 + +"Yet-Another-Header: foo\n" + end + end + + it 'raises a timeout error' do + expect { each_response_header }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, + /Request timed out after reading headers for 0\.[0-9]+ seconds/) + end + end + end + end +end diff --git a/spec/lib/gitlab/buffered_io_spec.rb b/spec/lib/gitlab/buffered_io_spec.rb index f8896abd46e..c6939b819e2 100644 --- a/spec/lib/gitlab/buffered_io_spec.rb +++ b/spec/lib/gitlab/buffered_io_spec.rb @@ -1,54 +1,50 @@ -# rubocop:disable Style/FrozenStringLiteralComment +# frozen_string_literal: true + require 'spec_helper' RSpec.describe Gitlab::BufferedIo do describe '#readuntil' do - let(:never_ending_tcp_socket) do - Class.new do - def initialize(*_) - @read_counter = 0 - end + let(:mock_io) { StringIO.new('a') } + let(:start_time) { Process.clock_gettime(Process::CLOCK_MONOTONIC) } - def setsockopt(*_); end + before do + stub_const('Gitlab::BufferedIo::HEADER_READ_TIMEOUT', 0.1) + end - def closed? - false - end + subject(:readuntil) do + Gitlab::BufferedIo.new(mock_io).readuntil('a', false, start_time) + end - def close - true - end + it 'does not raise a timeout error' do + expect { readuntil }.not_to raise_error + end - def to_io - StringIO.new('Hello World!') - end + context 'when the response contains infinitely long headers' do + before do + read_counter = 0 - def write_nonblock(data, *_) - data.size - end + allow(mock_io).to receive(:read_nonblock) do |buffer_size, *_| + read_counter += 1 + raise 'Test did not raise HeaderReadTimeout' if read_counter > 10 - def read_nonblock(buffer_size, *_) sleep 0.01 - @read_counter += 1 - - raise 'Test did not raise HeaderReadTimeout' if @read_counter > 10 - 'H' * buffer_size end end - end - before do - stub_const('Gitlab::BufferedIo::HEADER_READ_TIMEOUT', 0.1) - end + it 'raises a timeout error' do + expect { readuntil }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, /Request timed out after reading headers for 0\.[0-9]+ seconds/) + end - subject(:readuntil) do - Gitlab::BufferedIo.new(never_ending_tcp_socket.new).readuntil('a') - end + context 'when not passing start_time' do + subject(:readuntil) do + Gitlab::BufferedIo.new(mock_io).readuntil('a', false) + end - it 'raises a timeout error' do - expect { readuntil }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, /Request timed out after reading headers for 0\.[0-9]+ seconds/) + it 'raises a timeout error' do + expect { readuntil }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, /Request timed out after reading headers for 0\.[0-9]+ seconds/) + end + end end end end -# rubocop:enable Style/FrozenStringLiteralComment |