Merge branch 'security-revert-19072-alt-17-4-17-3' into '17-3-stable-ee'

External webhook token should be set

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/4511



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Co-authored-by: Ash McKenzie <amckenzie@gitlab.com>

ee/lib/api/project_mirror.rb

@@ -18,9 +18,13 @@ def render_invalid_github_signature!
       end
       def valid_github_signature?
-        request.body.rewind
-        token        = project.external_webhook_token.to_s
+        token = project.external_webhook_token.to_s
+        # project.external_webhook_token should always exist when authenticating
+        # via headers['X-Hub-Signature']. If it doesn't exist, this could be
+        # an attempt to misuse.
+        return false if token.empty?
+        request.body.rewind
         payload_body = request.body.read
         signature    = 'sha1=' + OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), token, payload_body)

ee/spec/requests/api/project_mirror_spec.rb

@@ -311,6 +311,47 @@ def project_member(role, user)
       end
     end
+    context 'when attempting to authenticate via GitHub signature' do
+      before do
+        Grape::Endpoint.before_each do |endpoint|
+          allow(endpoint).to receive(:project).and_return(project_mirrored)
+        end
+
+        allow(project_mirrored).to receive(:external_webhook_token).and_return(external_webhook_token)
+      end
+
+      after do
+        Grape::Endpoint.before_each nil
+      end
+
+      subject(:request) do
+        params = { action: 'opened' }
+        spoofed_signature = "sha1=#{OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), '', 'action=opened')}"
+
+        do_post(params: params, headers: { 'X-Hub-Signature' => spoofed_signature })
+      end
+
+      context 'without a project.external_webhook_token' do
+        let(:external_webhook_token) { nil }
+
+        it 'returns a 401 status' do
+          request
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      context 'with a project.external_webhook_token' do
+        let(:external_webhook_token) { 'a-fake-token' }
+
+        it 'returns a 401 status' do
+          request
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+    end
+
     context 'when not authenticated' do
       before do
         Grape::Endpoint.before_each do |endpoint|