Merge branch 'cwoolley-fix-akismet-server-options' into 'master'

Fix referrer option passed to Akismet client

See merge request gitlab-org/gitlab!63117

app/services/spam/akismet_service.rb

@@ -20,14 +20,18 @@ module Spam
         created_at: DateTime.current,
         author: owner_name,
         author_email: owner_email,
-        referer: options[:referer]
+        referrer: options[:referer]
       }
 
       begin
         is_spam, is_blatant = akismet_client.check(options[:ip_address], options[:user_agent], params)
         is_spam || is_blatant
+      rescue ArgumentError => e
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
+        false
       rescue StandardError => e
-        Gitlab::AppLogger.error("Unable to connect to Akismet: #{e}, skipping check")
+        Gitlab::ErrorTracking.track_exception(e)
+        Gitlab::AppLogger.error("Error during Akismet spam check, flagging as not spam: #{e}")
         false
       end
     end

spec/services/spam/akismet_service_spec.rb

@@ -4,12 +4,15 @@ require 'spec_helper'
 
 RSpec.describe Spam::AkismetService do
   let(:fake_akismet_client) { double(:akismet_client) }
+  let(:ip) { '1.2.3.4' }
+  let(:user_agent) { 'some user_agent' }
+  let(:referer) { 'some referer' }
 
   let_it_be(:text) { "Would you like to buy some tinned meat product?" }
   let_it_be(:spam_owner) { create(:user) }
 
   subject do
-    options = { ip_address: '1.2.3.4', user_agent: 'some user_agent', referrer: 'some referrer' }
+    options = { ip_address: ip, user_agent: user_agent, referer: referer }
     described_class.new(spam_owner.name, spam_owner.email, text, options)
   end
 
@@ -56,6 +59,21 @@ RSpec.describe Spam::AkismetService do
     it_behaves_like 'no activity if Akismet is not enabled', :spam?, :check
 
     context 'if Akismet is enabled' do
+      it 'correctly transforms options for the akismet client' do
+        expected_check_params = {
+          type: 'comment',
+          text: text,
+          created_at: anything,
+          author: spam_owner.name,
+          author_email: spam_owner.email,
+          # NOTE: The akismet_client needs the option to be named `:referrer`, not `:referer`
+          referrer: referer
+        }
+
+        expect(fake_akismet_client).to receive(:check).with(ip, user_agent, expected_check_params)
+        subject.spam?
+      end
+
       context 'the text is spam' do
         before do
           allow(fake_akismet_client).to receive(:check).and_return([true, false])
@@ -86,21 +104,33 @@ RSpec.describe Spam::AkismetService do
         end
       end
 
-      context 'if Akismet is not available' do
+      describe 'error handling' do
         before do
-          allow(fake_akismet_client).to receive(:check).and_raise(StandardError.new("oh noes!"))
+          allow(fake_akismet_client).to receive(:check).and_raise(error)
         end
 
+        context 'StandardError other than ArgumentError is raised' do
+          let(:error) { Akismet::Error.new("Lovely spam! Wonderful spam!") }
+
           specify do
             expect(subject.spam?).to be_falsey
           end
 
           it 'logs an error' do
-          expect(Gitlab::AppLogger).to receive(:error).with(/skipping check/)
+            expect(Gitlab::AppLogger).to receive(:error).with(/Error during Akismet.*flagging as not spam.*Lovely spam/)
 
             subject.spam?
           end
         end
+
+        context 'ArgumentError is raised in dev' do
+          let(:error) { ArgumentError }
+
+          it 'raises original error' do
+            expect { subject.spam? }.to raise_error(error)
+          end
+        end
+      end
     end
   end