Expand HandleNullBytes to handle malformed strings

Expands the HandleNullBytes middleware to also handle malformed strings.
Changes the middleware to return ActionController::BadRequest so we use
the default Rails bad request handling.

changelogs/unreleased/257822-null-byes-in-url.yml

+---
+title: Handle malformed strings in URL
+merge_request: 45701
+author:
+type: fixed

config/application.rb

@@ -28,7 +28,7 @@ module Gitlab
     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/middleware/handle_null_bytes')
+    require_dependency Rails.root.join('lib/gitlab/middleware/handle_malformed_strings')
     require_dependency Rails.root.join('lib/gitlab/runtime')
 
     # Settings in config/environments/* take precedence over those specified here.
@@ -254,7 +254,7 @@ module Gitlab
 
     config.middleware.insert_before ActionDispatch::RemoteIp, ::Gitlab::Middleware::HandleIpSpoofAttackError
 
-    config.middleware.use ::Gitlab::Middleware::HandleNullBytes
+    config.middleware.insert_after ActionDispatch::ActionableExceptions, ::Gitlab::Middleware::HandleMalformedStrings
 
     # Allow access to GitLab API from other domains
     config.middleware.insert_before Warden::Manager, Rack::Cors do

lib/gitlab/middleware/handle_null_bytes.rb → lib/gitlab/middleware/handle_malformed_strings.rb

@@ -2,9 +2,9 @@
 
 module Gitlab
   module Middleware
-    # There is no valid reason for a request to contain a null byte (U+0000)
+    # There is no valid reason for a request to contain a malformed string
     # so just return HTTP 400 (Bad Request) if we receive one
-    class HandleNullBytes
+    class HandleMalformedStrings
       NULL_BYTE_REGEX = Regexp.new(Regexp.escape("\u0000")).freeze
 
       attr_reader :app
@@ -14,18 +14,20 @@ module Gitlab
       end
 
       def call(env)
-        return [400, {}, ["Bad Request"]] if request_has_null_byte?(env)
+        return [400, { 'Content-Type' => 'text/plain' }, ['Bad Request']] if request_contains_malformed_string?(env)
 
         app.call(env)
       end
 
       private
 
-      def request_has_null_byte?(request)
-        return false if ENV['REJECT_NULL_BYTES'] == "1"
+      def request_contains_malformed_string?(request)
+        return false if ENV['DISABLE_REQUEST_VALIDATION'] == '1'
 
         request = Rack::Request.new(request)
 
+        return true if string_malformed?(request.path)
+
         request.params.values.any? do |value|
           param_has_null_byte?(value)
         end
@@ -39,7 +41,7 @@ module Gitlab
         depth += 1
 
         if value.respond_to?(:match)
-          string_contains_null_byte?(value)
+          string_malformed?(value)
         elsif value.respond_to?(:values)
           value.values.any? do |hash_value|
             param_has_null_byte?(hash_value, depth)
@@ -53,8 +55,11 @@ module Gitlab
         end
       end
 
-      def string_contains_null_byte?(string)
+      def string_malformed?(string)
         string.match?(NULL_BYTE_REGEX)
+      rescue ArgumentError
+        # If we're here, we caught a malformed string. Return true
+        true
       end
     end
   end

spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require "rack/test"
+
+RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
+  let(:null_byte) { "\u0000" }
+  let(:invalid_string) { "mal\xC0formed" }
+  let(:error_400) { [400, { 'Content-Type' => 'text/plain' }, ['Bad Request']] }
+  let(:app) { double(:app) }
+
+  subject { described_class.new(app) }
+
+  before do
+    allow(app).to receive(:call) do |args|
+      args
+    end
+  end
+
+  def env_for(params = {})
+    Rack::MockRequest.env_for('/', { params: params })
+  end
+
+  context 'in the URL' do
+    it 'rejects null bytes' do
+      # We have to create the env separately or Rack::MockRequest complains about invalid URI
+      env = env_for
+      env['PATH_INFO'] = "/someplace/witha#{null_byte}nullbyte"
+
+      expect(subject.call(env)).to eq error_400
+    end
+
+    it 'rejects malformed strings' do
+      # We have to create the env separately or Rack::MockRequest complains about invalid URI
+      env = env_for
+      env['PATH_INFO'] = "/someplace/with_an/#{invalid_string}"
+
+      expect(subject.call(env)).to eq error_400
+    end
+  end
+
+  context 'in params' do
+    shared_examples_for 'checks params' do
+      it 'rejects bad params in a top level param' do
+        env = env_for(name: "null#{problematic_input}byte")
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "rejects bad params for hashes with strings" do
+        env = env_for(name: { inner_key: "I am #{problematic_input} bad" })
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "rejects bad params for arrays with strings" do
+        env = env_for(name: ["I am #{problematic_input} bad"])
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "rejects bad params for arrays containing hashes with string values" do
+        env = env_for(name: [
+          {
+            inner_key: "I am #{problematic_input} bad"
+          }
+        ])
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "gives up and does not reject too deeply nested params" do
+        env = env_for(name: [
+          {
+            inner_key: { deeper_key: [{ hash_inside_array_key: "I am #{problematic_input} bad" }] }
+          }
+        ])
+
+        expect(subject.call(env)).not_to eq error_400
+      end
+    end
+
+    context 'with null byte' do
+      it_behaves_like 'checks params' do
+        let(:problematic_input) { null_byte }
+      end
+    end
+
+    context 'with malformed strings' do
+      it_behaves_like 'checks params' do
+        let(:problematic_input) { invalid_string }
+      end
+    end
+  end
+
+  context 'without problematic input' do
+    it "does not error for strings" do
+      env = env_for(name: "safe name")
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+
+    it "does not error with no params" do
+      env = env_for
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+  end
+end

spec/lib/gitlab/middleware/handle_null_bytes_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-require "rack/test"
-
-RSpec.describe Gitlab::Middleware::HandleNullBytes do
-  let(:null_byte) { "\u0000" }
-  let(:error_400) { [400, {}, ["Bad Request"]] }
-  let(:app) { double(:app) }
-
-  subject { described_class.new(app) }
-
-  before do
-    allow(app).to receive(:call) do |args|
-      args
-    end
-  end
-
-  def env_for(params = {})
-    Rack::MockRequest.env_for('/', { params: params })
-  end
-
-  context 'with null bytes in params' do
-    it 'rejects null bytes in a top level param' do
-      env = env_for(name: "null#{null_byte}byte")
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "responds with 400 BadRequest for hashes with strings" do
-      env = env_for(name: { inner_key: "I am #{null_byte} bad" })
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "responds with 400 BadRequest for arrays with strings" do
-      env = env_for(name: ["I am #{null_byte} bad"])
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "responds with 400 BadRequest for arrays containing hashes with string values" do
-      env = env_for(name: [
-        {
-          inner_key: "I am #{null_byte} bad"
-        }
-      ])
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "gives up and does not 400 with too deeply nested params" do
-      env = env_for(name: [
-        {
-          inner_key: { deeper_key: [{ hash_inside_array_key: "I am #{null_byte} bad" }] }
-        }
-      ])
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-  end
-
-  context 'without null bytes in params' do
-    it "does not respond with a 400 for strings" do
-      env = env_for(name: "safe name")
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-
-    it "does not respond with a 400 with no params" do
-      env = env_for
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-  end
-
-  context 'when disabled via env flag' do
-    before do
-      stub_env('REJECT_NULL_BYTES', '1')
-    end
-
-    it 'does not respond with a 400 no matter what' do
-      env = env_for(name: "null#{null_byte}byte")
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-  end
-end

spec/requests/user_sends_null_bytes_spec.rb → spec/requests/user_sends_malformed_strings_spec.rb

@@ -2,13 +2,19 @@
 
 require 'spec_helper'
 
-RSpec.describe 'User sends null bytes as params' do
+RSpec.describe 'User sends malformed strings as params' do
   let(:null_byte) { "\u0000" }
+  let(:invalid_string) { "mal\xC0formed" }
 
-  it 'raises a 400 error' do
+  it 'raises a 400 error with a null byte' do
     post '/nonexistent', params: { a: "A #{null_byte} nasty string" }
 
     expect(response).to have_gitlab_http_status(:bad_request)
-    expect(response.body).to eq('Bad Request')
+  end
+
+  it 'raises a 400 error with an invalid string' do
+    post '/nonexistent', params: { a: "A #{invalid_string} nasty string" }
+
+    expect(response).to have_gitlab_http_status(:bad_request)
   end
 end