diff options
-rw-r--r-- | app/models/integrations/zentao.rb | 4 | ||||
-rw-r--r-- | config/initializers/rack_VULNDB-255039_patch.rb | 35 | ||||
-rw-r--r-- | lib/gitlab/set_cache.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/zentao/client.rb | 50 | ||||
-rw-r--r-- | spec/initializers/rack_VULNDB-255039_patch_spec.rb | 17 | ||||
-rw-r--r-- | spec/lib/gitlab/reactive_cache_set_cache_spec.rb | 14 | ||||
-rw-r--r-- | spec/lib/gitlab/zentao/client_spec.rb | 127 | ||||
-rw-r--r-- | spec/models/integrations/zentao_spec.rb | 20 |
8 files changed, 243 insertions, 28 deletions
diff --git a/app/models/integrations/zentao.rb b/app/models/integrations/zentao.rb index 53194089296..459756c865b 100644 --- a/app/models/integrations/zentao.rb +++ b/app/models/integrations/zentao.rb @@ -69,6 +69,10 @@ module Integrations } end + def client_url + api_url.presence || url + end + def self.to_param name.demodulize.downcase end diff --git a/config/initializers/rack_VULNDB-255039_patch.rb b/config/initializers/rack_VULNDB-255039_patch.rb new file mode 100644 index 00000000000..b613ed9bdb1 --- /dev/null +++ b/config/initializers/rack_VULNDB-255039_patch.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +if Gem.loaded_specs['rack'].version >= Gem::Version.new("3.0.0") + raise <<~ERR + This patch is unnecessary in Rack versions 3.0.0 or newer. + Please remove this file and the associated spec. + + See https://github.com/rack/rack/blob/main/CHANGELOG.md#security (issue #1733) + ERR +end + +# Patches a cache poisoning attack vector in Rack by not allowing semicolons +# to delimit query parameters. +# See https://github.com/rack/rack/issues/1732. +# +# Solution is taken from the same issue. +# +# The actual patch is due for release in Rack 3.0.0. +module Rack + class Request + Helpers.module_eval do + # rubocop: disable Naming/MethodName + def GET + if get_header(RACK_REQUEST_QUERY_STRING) == query_string + get_header(RACK_REQUEST_QUERY_HASH) + else + query_hash = parse_query(query_string, '&') # only allow ampersand here + set_header(RACK_REQUEST_QUERY_STRING, query_string) + set_header(RACK_REQUEST_QUERY_HASH, query_hash) + end + end + # rubocop: enable Naming/MethodName + end + end +end diff --git a/lib/gitlab/set_cache.rb b/lib/gitlab/set_cache.rb index feb2c3c1d7d..896e7e3f65e 100644 --- a/lib/gitlab/set_cache.rb +++ b/lib/gitlab/set_cache.rb @@ -68,6 +68,10 @@ module Gitlab with { |redis| redis.ttl(cache_key(key)) } end + def count(key) + with { |redis| redis.scard(cache_key(key)) } + end + private def with(&blk) diff --git a/lib/gitlab/zentao/client.rb b/lib/gitlab/zentao/client.rb index 0c2b3049670..a9e89b99a27 100644 --- a/lib/gitlab/zentao/client.rb +++ b/lib/gitlab/zentao/client.rb @@ -5,6 +5,10 @@ module Gitlab class Client Error = Class.new(StandardError) ConfigError = Class.new(Error) + RequestError = Class.new(Error) + + CACHE_MAX_SET_SIZE = 5_000 + CACHE_TTL = 1.month.freeze attr_reader :integration @@ -33,11 +37,21 @@ module Gitlab end def fetch_issues(params = {}) - get("products/#{zentao_product_xid}/issues", params) + get("products/#{zentao_product_xid}/issues", params).tap do |response| + mark_issues_as_seen_in_product(response['issues']) + end end def fetch_issue(issue_id) - raise Gitlab::Zentao::Client::Error, 'invalid issue id' unless issue_id_pattern.match(issue_id) + raise Error, 'invalid issue id' unless issue_id_pattern.match(issue_id) + + # Only return issues that are associated with the product configured in + # the integration. Due to a lack of available data in the ZenTao APIs, we + # can only determine if an issue belongs to a product if the issue was + # previously returned in the `#fetch_issues` call. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/360372#note_1016963713 + raise RequestError unless issue_seen_in_product?(issue_id) get("issues/#{issue_id}") end @@ -52,17 +66,15 @@ module Gitlab options = { headers: headers, query: params } response = Gitlab::HTTP.get(url(path), options) - raise Gitlab::Zentao::Client::Error, 'request error' unless response.success? + raise RequestError unless response.success? Gitlab::Json.parse(response.body) rescue JSON::ParserError - raise Gitlab::Zentao::Client::Error, 'invalid response format' + raise Error, 'invalid response format' end def url(path) - host = integration.api_url.presence || integration.url - - URI.parse(Gitlab::Utils.append_path(host, "api.php/v1/#{path}")) + URI.parse(Gitlab::Utils.append_path(integration.client_url, "api.php/v1/#{path}")) end def headers @@ -75,6 +87,30 @@ module Gitlab def zentao_product_xid integration.zentao_product_xid end + + def issue_ids_cache_key + @issue_ids_cache_key ||= [ + :zentao_product_issues, + OpenSSL::Digest::SHA256.hexdigest(integration.client_url), + zentao_product_xid + ].join(':') + end + + def issue_ids_cache + @issue_ids_cache ||= ::Gitlab::SetCache.new(expires_in: CACHE_TTL) + end + + def mark_issues_as_seen_in_product(issues) + return unless issues && issue_ids_cache.count(issue_ids_cache_key) < CACHE_MAX_SET_SIZE + + ids = issues.map { _1['id'] } + + issue_ids_cache.write(issue_ids_cache_key, ids) + end + + def issue_seen_in_product?(id) + issue_ids_cache.include?(issue_ids_cache_key, id) + end end end end diff --git a/spec/initializers/rack_VULNDB-255039_patch_spec.rb b/spec/initializers/rack_VULNDB-255039_patch_spec.rb new file mode 100644 index 00000000000..754ff2f10e0 --- /dev/null +++ b/spec/initializers/rack_VULNDB-255039_patch_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Rack VULNDB-255039' do + context 'when handling query params in GET requests' do + it 'does not treat semicolons as query delimiters' do + env = ::Rack::MockRequest.env_for('http://gitlab.com?a=b;c=1') + + query_hash = ::Rack::Request.new(env).GET + + # Prior to this patch, this was splitting around the semicolon, which + # would return {"a"=>"b", "c"=>"1"} + expect(query_hash).to eq({ "a" => "b;c=1" }) + end + end +end diff --git a/spec/lib/gitlab/reactive_cache_set_cache_spec.rb b/spec/lib/gitlab/reactive_cache_set_cache_spec.rb index f405b2ad86e..207ac1c0eaa 100644 --- a/spec/lib/gitlab/reactive_cache_set_cache_spec.rb +++ b/spec/lib/gitlab/reactive_cache_set_cache_spec.rb @@ -72,4 +72,18 @@ RSpec.describe Gitlab::ReactiveCacheSetCache, :clean_gitlab_redis_cache do it { is_expected.to be(true) } end end + + describe 'count' do + subject { cache.count(cache_prefix) } + + it { is_expected.to be(0) } + + context 'item added' do + before do + cache.write(cache_prefix, 'test_item') + end + + it { is_expected.to be(1) } + end + end end diff --git a/spec/lib/gitlab/zentao/client_spec.rb b/spec/lib/gitlab/zentao/client_spec.rb index 135f13e6265..b17ad867f0d 100644 --- a/spec/lib/gitlab/zentao/client_spec.rb +++ b/spec/lib/gitlab/zentao/client_spec.rb @@ -2,17 +2,21 @@ require 'spec_helper' -RSpec.describe Gitlab::Zentao::Client do - subject(:integration) { described_class.new(zentao_integration) } +RSpec.describe Gitlab::Zentao::Client, :clean_gitlab_redis_cache do + subject(:client) { described_class.new(zentao_integration) } let(:zentao_integration) { create(:zentao_integration) } def mock_get_products_url - integration.send(:url, "products/#{zentao_integration.zentao_product_xid}") + client.send(:url, "products/#{zentao_integration.zentao_product_xid}") + end + + def mock_fetch_issues_url + client.send(:url, "products/#{zentao_integration.zentao_product_xid}/issues") end def mock_fetch_issue_url(issue_id) - integration.send(:url, "issues/#{issue_id}") + client.send(:url, "issues/#{issue_id}") end let(:mock_headers) do @@ -29,13 +33,13 @@ RSpec.describe Gitlab::Zentao::Client do let(:zentao_integration) { nil } it 'raises ConfigError' do - expect { integration }.to raise_error(described_class::ConfigError) + expect { client }.to raise_error(described_class::ConfigError) end end context 'integration is provided' do it 'is initialized successfully' do - expect { integration }.not_to raise_error + expect { client }.not_to raise_error end end end @@ -50,7 +54,7 @@ RSpec.describe Gitlab::Zentao::Client do end it 'fetches the product' do - expect(integration.fetch_product(zentao_integration.zentao_product_xid)).to eq mock_response + expect(client.fetch_product(zentao_integration.zentao_product_xid)).to eq mock_response end end @@ -62,8 +66,8 @@ RSpec.describe Gitlab::Zentao::Client do it 'fetches the empty product' do expect do - integration.fetch_product(zentao_integration.zentao_product_xid) - end.to raise_error(Gitlab::Zentao::Client::Error, 'request error') + client.fetch_product(zentao_integration.zentao_product_xid) + end.to raise_error(Gitlab::Zentao::Client::RequestError) end end @@ -75,7 +79,7 @@ RSpec.describe Gitlab::Zentao::Client do it 'fetches the empty product' do expect do - integration.fetch_product(zentao_integration.zentao_product_xid) + client.fetch_product(zentao_integration.zentao_product_xid) end.to raise_error(Gitlab::Zentao::Client::Error, 'invalid response format') end end @@ -89,7 +93,7 @@ RSpec.describe Gitlab::Zentao::Client do end it 'responds with success' do - expect(integration.ping[:success]).to eq true + expect(client.ping[:success]).to eq true end end @@ -100,7 +104,69 @@ RSpec.describe Gitlab::Zentao::Client do end it 'responds with unsuccess' do - expect(integration.ping[:success]).to eq false + expect(client.ping[:success]).to eq false + end + end + end + + describe '#fetch_issues' do + let(:mock_response) { { 'issues' => [{ 'id' => 'story-1' }, { 'id' => 'bug-11' }] } } + + before do + WebMock.stub_request(:get, mock_fetch_issues_url) + .with(mock_headers).to_return(status: 200, body: mock_response.to_json) + end + + it 'returns the response' do + expect(client.fetch_issues).to eq(mock_response) + end + + describe 'marking the issues as seen in the product' do + let(:cache) { ::Gitlab::SetCache.new } + let(:cache_key) do + [ + :zentao_product_issues, + OpenSSL::Digest::SHA256.hexdigest(zentao_integration.client_url), + zentao_integration.zentao_product_xid + ].join(':') + end + + it 'adds issue ids to the cache' do + expect { client.fetch_issues }.to change { cache.read(cache_key) } + .from(be_empty) + .to match_array(%w[bug-11 story-1]) + end + + it 'does not add issue ids to the cache if max set size has been reached' do + cache.write(cache_key, %w[foo bar]) + stub_const("#{described_class}::CACHE_MAX_SET_SIZE", 1) + + client.fetch_issues + + expect(cache.read(cache_key)).to match_array(%w[foo bar]) + end + + it 'does not duplicate issue ids in the cache' do + client.fetch_issues + client.fetch_issues + + expect(cache.read(cache_key)).to match_array(%w[bug-11 story-1]) + end + + it 'touches the cache ttl every time issues are fetched' do + fresh_ttl = 1.month.to_i + + freeze_time do + client.fetch_issues + + expect(cache.ttl(cache_key)).to eq(fresh_ttl) + end + + travel_to(1.minute.from_now) do + client.fetch_issues + + expect(cache.ttl(cache_key)).to eq(fresh_ttl) + end end end end @@ -109,9 +175,9 @@ RSpec.describe Gitlab::Zentao::Client do context 'with invalid id' do let(:invalid_ids) { ['story', 'story-', '-', '123', ''] } - it 'returns empty object' do + it 'raises Error' do invalid_ids.each do |id| - expect { integration.fetch_issue(id) } + expect { client.fetch_issue(id) } .to raise_error(Gitlab::Zentao::Client::Error, 'invalid issue id') end end @@ -120,12 +186,31 @@ RSpec.describe Gitlab::Zentao::Client do context 'with valid id' do let(:valid_ids) { %w[story-1 bug-23] } - it 'fetches current issue' do - valid_ids.each do |id| - WebMock.stub_request(:get, mock_fetch_issue_url(id)) - .with(mock_headers).to_return(status: 200, body: { issue: { id: id } }.to_json) + context 'when issue has been seen on the index' do + before do + issues_body = { issues: valid_ids.map { { id: _1 } } }.to_json + + WebMock.stub_request(:get, mock_fetch_issues_url) + .with(mock_headers).to_return(status: 200, body: issues_body) + + client.fetch_issues + end + + it 'fetches the issue' do + valid_ids.each do |id| + WebMock.stub_request(:get, mock_fetch_issue_url(id)) + .with(mock_headers).to_return(status: 200, body: { issue: { id: id } }.to_json) + + expect(client.fetch_issue(id).dig('issue', 'id')).to eq id + end + end + end - expect(integration.fetch_issue(id).dig('issue', 'id')).to eq id + context 'when issue has not been seen on the index' do + it 'raises RequestError' do + valid_ids.each do |id| + expect { client.fetch_issue(id) }.to raise_error(Gitlab::Zentao::Client::RequestError) + end end end end @@ -135,7 +220,7 @@ RSpec.describe Gitlab::Zentao::Client do context 'api url' do shared_examples 'joins api_url correctly' do it 'verify url' do - expect(integration.send(:url, "products/1").to_s) + expect(client.send(:url, "products/1").to_s) .to eq("https://jihudemo.zentao.net/zentao/api.php/v1/products/1") end end @@ -157,7 +242,7 @@ RSpec.describe Gitlab::Zentao::Client do let(:zentao_integration) { create(:zentao_integration, url: 'https://jihudemo.zentao.net') } it 'joins url correctly' do - expect(integration.send(:url, "products/1").to_s) + expect(client.send(:url, "products/1").to_s) .to eq("https://jihudemo.zentao.net/api.php/v1/products/1") end end diff --git a/spec/models/integrations/zentao_spec.rb b/spec/models/integrations/zentao_spec.rb index 4ef977ba3d2..1a32453819d 100644 --- a/spec/models/integrations/zentao_spec.rb +++ b/spec/models/integrations/zentao_spec.rb @@ -81,4 +81,24 @@ RSpec.describe Integrations::Zentao do expect(zentao_integration.help).not_to be_empty end end + + describe '#client_url' do + subject(:integration) { build(:zentao_integration, api_url: api_url, url: 'url').client_url } + + context 'when api_url is set' do + let(:api_url) { 'api_url' } + + it 'returns the api_url' do + is_expected.to eq(api_url) + end + end + + context 'when api_url is not set' do + let(:api_url) { '' } + + it 'returns the url' do + is_expected.to eq('url') + end + end + end end |