Merge branch 'bvl-reject-more-invalid-utf8' into 'master'

Reject incomplete multibyte strings and other invalid UTF8

See merge request gitlab-org/gitlab!47658

changelogs/unreleased/bvl-reject-more-invalid-utf8.yml

+---
+title: Reject incomplete multibyte chars in UTF8 params
+merge_request: 47658
+author:
+type: fixed

lib/gitlab/middleware/handle_malformed_strings.rb

@@ -89,8 +89,12 @@ module Gitlab
       def string_malformed?(string)
         # We're using match rather than include, because that will raise an ArgumentError
         # when  the string contains invalid UTF8
-        string.match?(NULL_BYTE_REGEX)
-      rescue ArgumentError
+        #
+        # We try to encode the string from ASCII-8BIT to UTF8. If we failed to do
+        # so for certain characters in the string, those chars are probably incomplete
+        # multibyte characters.
+        string.encode(Encoding::UTF_8).match?(NULL_BYTE_REGEX)
+      rescue ArgumentError, Encoding::UndefinedConversionError
         # If we're here, we caught a malformed string. Return true
         true
       end

spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb

@@ -62,18 +62,38 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
   context 'in authorization headers' do
     let(:problematic_input) { null_byte }
 
+    shared_examples 'rejecting invalid input' do
       it 'rejects problematic input in the password' do
         env = env_for.merge(auth_env("username", "password#{problematic_input}encoded", nil))
 
         expect(subject.call(env)).to eq error_400
       end
 
-    it 'rejects problematic input in the password' do
-      env = env_for.merge(auth_env("username#{problematic_input}", "password#{problematic_input}encoded", nil))
+      it 'rejects problematic input in the username' do
+        env = env_for.merge(auth_env("username#{problematic_input}", "passwordencoded", nil))
 
         expect(subject.call(env)).to eq error_400
       end
 
+      it 'rejects problematic input in non-basic-auth tokens' do
+        env = env_for.merge('HTTP_AUTHORIZATION' => "GL-Geo hello#{problematic_input}world")
+
+        expect(subject.call(env)).to eq error_400
+      end
+    end
+
+    it_behaves_like 'rejecting invalid input' do
+      let(:problematic_input) { null_byte }
+    end
+
+    it_behaves_like 'rejecting invalid input' do
+      let(:problematic_input) { invalid_string }
+    end
+
+    it_behaves_like 'rejecting invalid input' do
+      let(:problematic_input) { "\xC3" }
+    end
+
     it 'does not reject correct non-basic-auth tokens' do
       # This token is known to include a null-byte when we were to try to decode it
       # as Base64, while it wasn't encoded at such.
@@ -84,12 +104,6 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
 
       expect(subject.call(env)).not_to eq error_400
     end
-
-    it 'rejects problematic input in non-basic-auth tokens' do
-      env = env_for.merge('HTTP_AUTHORIZATION' => "GL-Geo hello#{problematic_input}world")
-
-      expect(subject.call(env)).to eq error_400
-    end
   end
 
   context 'in params' do