Convert IP spoofing errors into client errors

Currently, IP spoofing attacks result in a 500 response, which counts
towards our error budget. Since they're caused by the properties of the
client request, it's appropriate to convert them into 400 errors.

We don't need to be too worried about the format of this response, as
only malicious users will see it.

changelogs/unreleased/218025-xff-is-a-400-error.yml

+---
+title: Convert IP spoofing errors into client errors
+merge_request: 33280
+author:
+type: other

config/application.rb

@@ -25,6 +25,7 @@ class Application < Rails::Application
     require_dependency Rails.root.join('lib/gitlab/middleware/read_only')
     require_dependency Rails.root.join('lib/gitlab/middleware/basic_health_check')
     require_dependency Rails.root.join('lib/gitlab/middleware/same_site_cookies')
+    require_dependency Rails.root.join('lib/gitlab/middleware/handle_ip_spoof_attack_error')
     require_dependency Rails.root.join('lib/gitlab/runtime')
 
     # Settings in config/environments/* take precedence over those specified here.
@@ -235,6 +236,8 @@ class Application < Rails::Application
 
     config.middleware.insert_before ActionDispatch::Cookies, ::Gitlab::Middleware::SameSiteCookies
 
+    config.middleware.insert_before ActionDispatch::RemoteIp, ::Gitlab::Middleware::HandleIpSpoofAttackError
+
     # Allow access to GitLab API from other domains
     config.middleware.insert_before Warden::Manager, Rack::Cors do
       headers_to_expose = %w[Link X-Total X-Total-Pages X-Per-Page X-Page X-Next-Page X-Prev-Page X-Gitlab-Blob-Id X-Gitlab-Commit-Id X-Gitlab-Content-Sha256 X-Gitlab-Encoding X-Gitlab-File-Name X-Gitlab-File-Path X-Gitlab-Last-Commit-Id X-Gitlab-Ref X-Gitlab-Size]

lib/gitlab/middleware/handle_ip_spoof_attack_error.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Middleware
+    # ActionDispatch::RemoteIp tries to set the `request.ip` for controllers by
+    # looking at the request IP and headers. It needs to see through any reverse
+    # proxies to get the right answer, but there are some security issues with
+    # that.
+    #
+    # Proxies can specify `Client-Ip` or `X-Forwarded-For`, and the security of
+    # that is determined at the edge. If both headers are present, it's likely
+    # that the edge is securing one, but ignoring the other. Rails blocks this,
+    # which is correct, because we don't know which header is the safe one - but
+    # we want the block to be a 400, rather than 500, error.
+    #
+    # This middleware needs to go before ActionDispatch::RemoteIp in the chain.
+    class HandleIpSpoofAttackError
+      attr_reader :app
+
+      def initialize(app)
+        @app = app
+      end
+
+      def call(env)
+        app.call(env)
+      rescue ActionDispatch::RemoteIp::IpSpoofAttackError => err
+        Gitlab::ErrorTracking.track_exception(err)
+
+        [400, { 'Content-Type' => 'text/plain' }, ['Bad Request']]
+      end
+    end
+  end
+end

spec/lib/gitlab/middleware/handle_ip_spoof_attack_error_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Middleware::HandleIpSpoofAttackError do
+  let(:spoof_error) { ActionDispatch::RemoteIp::IpSpoofAttackError.new('sensitive') }
+  let(:standard_error) { StandardError.new('error') }
+  let(:app) { -> (env) { env.is_a?(Exception) ? raise(env) : env } }
+
+  subject(:middleware) { described_class.new(app) }
+
+  it 'passes through the response from a valid upstream' do
+    expect(middleware.call(:response)).to eq(:response)
+  end
+
+  it 'translates an ActionDispatch::IpSpoofAttackError to a 400 response' do
+    expect(middleware.call(spoof_error))
+      .to eq([400, { 'Content-Type' => 'text/plain' }, ['Bad Request']])
+  end
+
+  it 'passes through the exception raised by an invalid upstream' do
+    expect { middleware.call(standard_error) }.to raise_error(standard_error)
+  end
+end

spec/requests/user_spoofs_ip_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'User spoofs their IP' do
+  it 'raises a 400 error' do
+    get '/nonexistent', headers: { 'Client-Ip' => '1.2.3.4', 'X-Forwarded-For' => '5.6.7.8' }
+
+    expect(response).to have_gitlab_http_status(:bad_request)
+    expect(response.body).to eq('Bad Request')
+  end
+end