Merge branch '218025-xff-is-a-400-error' into 'master'

Convert IP spoofing errors into client errors

Closes #218025

See merge request gitlab-org/gitlab!33280

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 @@ module Gitlab
     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 @@ module Gitlab
 
     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