Merge branch 'revert-77f50948' into 'master'

Revert "Merge branch 'bvl-handle-invalid-headers' into 'master'"

See merge request gitlab-org/gitlab!47112

changelogs/unreleased/bvl-handle-invalid-headers.yml

----
-title: Handle nullbytes in auth headers
-merge_request: 46985
-author:
-type: fixed

lib/gitlab/middleware/handle_malformed_strings.rb

@@ -5,8 +5,6 @@ module Gitlab
     # 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 HandleMalformedStrings
-      include ActionController::HttpAuthentication::Basic
-
       NULL_BYTE_REGEX = Regexp.new(Regexp.escape("\u0000")).freeze
 
       attr_reader :app
@@ -23,26 +21,16 @@ module Gitlab
 
       private
 
-      def request_contains_malformed_string?(env)
+      def request_contains_malformed_string?(request)
         return false if ENV['DISABLE_REQUEST_VALIDATION'] == '1'
 
-        # Duplicate the env, so it is not modified when accessing the parameters
-        # https://github.com/rails/rails/blob/34991a6ae2fc68347c01ea7382fa89004159e019/actionpack/lib/action_dispatch/http/parameters.rb#L59
-        # The modification causes problems with our multipart middleware
-        request = ActionDispatch::Request.new(env.dup)
+        request = Rack::Request.new(request)
 
         return true if malformed_path?(request.path)
-        return true if credentials_malformed?(request)
 
         request.params.values.any? do |value|
           param_has_null_byte?(value)
         end
-      rescue ActionController::BadRequest
-        # If we can't build an ActionDispatch::Request something's wrong
-        # This would also happen if `#params` contains invalid UTF-8
-        # in this case we'll return a 400
-        #
-        true
       end
 
       def malformed_path?(path)
@@ -52,13 +40,6 @@ module Gitlab
         true
       end
 
-      def credentials_malformed?(request)
-        credentials = decode_credentials(request).presence
-        return false unless credentials
-
-        string_malformed?(credentials)
-      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/features/file_uploads/multipart_invalid_uploads_spec.rb

@@ -22,13 +22,13 @@ RSpec.describe 'Invalid uploads that must be rejected', :api, :js do
       )
     end
 
-    RSpec.shared_examples 'rejecting invalid keys' do |key_name:, message: nil, status: 500|
+    RSpec.shared_examples 'rejecting invalid keys' do |key_name:, message: nil|
       context "with invalid key #{key_name}" do
         let(:body) { { key_name => file, 'package[test][name]' => 'test' } }
 
         it { expect { subject }.not_to change { Packages::Package.nuget.count } }
 
-        it { expect(subject.code).to eq(status) }
+        it { expect(subject.code).to eq(500) }
 
         it { expect(subject.body).to include(message.presence || "invalid field: \"#{key_name}\"") }
       end
@@ -45,7 +45,7 @@ RSpec.describe 'Invalid uploads that must be rejected', :api, :js do
     # These keys are rejected directly by rack itself.
     # The request will not be received by multipart.rb (can't use the 'handling file uploads' shared example)
     it_behaves_like 'rejecting invalid keys', key_name: 'x' * 11000, message: 'Puma caught this error: exceeded available parameter key space (RangeError)'
-    it_behaves_like 'rejecting invalid keys', key_name: 'package[]test', status: 400, message: 'Bad Request'
+    it_behaves_like 'rejecting invalid keys', key_name: 'package[]test', message: 'Puma caught this error: expected Hash (got Array)'
 
     it_behaves_like 'handling file uploads', 'by rejecting uploads with an invalid key'
   end

spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb

@@ -4,8 +4,6 @@ require 'spec_helper'
 require "rack/test"
 
 RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
-  include GitHttpHelpers
-
   let(:null_byte) { "\u0000" }
   let(:escaped_null_byte) { "%00" }
   let(:invalid_string) { "mal\xC0formed" }
@@ -59,22 +57,6 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
     end
   end
 
-  context 'in authorization headers' do
-    let(:problematic_input) { null_byte }
-
-    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))
-
-      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
@@ -104,24 +86,24 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
 
         expect(subject.call(env)).to eq error_400
       end
-    end
-
-    context 'with null byte' do
-      let(:problematic_input) { null_byte }
-
-      it_behaves_like 'checks params'
 
       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" }] }
-                        }
-                      ])
+          {
+            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 }
@@ -142,10 +124,4 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
       expect(subject.call(env)).not_to eq error_400
     end
   end
-
-  it 'does not modify the env' do
-    env = env_for
-
-    expect { subject.call(env) }.not_to change { env }
-  end
 end

spec/requests/user_sends_malformed_strings_spec.rb

@@ -2,9 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe 'User sends malformed strings' do
-  include GitHttpHelpers
-
+RSpec.describe 'User sends malformed strings as params' do
   let(:null_byte) { "\u0000" }
   let(:invalid_string) { "mal\xC0formed" }
 
@@ -19,10 +17,4 @@ RSpec.describe 'User sends malformed strings' do
 
     expect(response).to have_gitlab_http_status(:bad_request)
   end
-
-  it 'raises a 400 error with null bytes in the auth headers' do
-    clone_get("project/path", user: "hello#{null_byte}", password: "nothing to see")
-
-    expect(response).to have_gitlab_http_status(:bad_request)
-  end
 end