Merge branch '257822-fix-handlenullbytes-with-escaped-chars' into 'master'

Fix HandleMalformedStrings middleware

See merge request gitlab-org/gitlab!46502

lib/gitlab/middleware/handle_malformed_strings.rb

@@ -26,13 +26,20 @@ module Gitlab
 
         request = Rack::Request.new(request)
 
-        return true if string_malformed?(request.path)
+        return true if malformed_path?(request.path)
 
         request.params.values.any? do |value|
           param_has_null_byte?(value)
         end
       end
 
+      def malformed_path?(path)
+        string_malformed?(Rack::Utils.unescape(path))
+      rescue ArgumentError
+        # Rack::Utils.unescape raised this, path is malformed.
+        true
+      end
+
       def param_has_null_byte?(value, depth = 0)
         # Guard against possible attack sending large amounts of nested params
         # Should be safe as deeply nested params are highly uncommon.

spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb

@@ -5,7 +5,9 @@ require "rack/test"
 
 RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
   let(:null_byte) { "\u0000" }
+  let(:escaped_null_byte) { "%00" }
   let(:invalid_string) { "mal\xC0formed" }
+  let(:escaped_invalid_string) { "mal%c0formed" }
   let(:error_400) { [400, { 'Content-Type' => 'text/plain' }, ['Bad Request']] }
   let(:app) { double(:app) }
 
@@ -30,6 +32,14 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
       expect(subject.call(env)).to eq error_400
     end
 
+    it 'rejects escaped null bytes' do
+      # We have to create the env separately or Rack::MockRequest complains about invalid URI
+      env = env_for
+      env['PATH_INFO'] = "/someplace/withan#{escaped_null_byte}escaped 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
@@ -37,6 +47,14 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
 
       expect(subject.call(env)).to eq error_400
     end
+
+    it 'rejects escaped 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/#{escaped_invalid_string}"
+
+      expect(subject.call(env)).to eq error_400
+    end
   end
 
   context 'in params' do