Add latest changes from gitlab-org/security/gitlab@15-3-stable-ee

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