Add latest changes from gitlab-org/gitlab@master

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

@@ -1,5 +0,0 @@
----
-title: Handle nullbytes in auth headers
-merge_request: 46985
-author:
-type: fixed

doc/integration/saml.md

@@ -636,7 +636,9 @@ Group SAML on a self-managed instance is limited when compared to the recommende
 
 ## Troubleshooting
 
-You can find the base64-encoded SAML Response in the [`production_json.log`](../administration/logs.md#production_jsonlog).
+### SAML Response
+
+You can find the base64-encoded SAML Response in the [`production_json.log`](../administration/logs.md#production_jsonlog). This response is sent from the IdP, and contains user information that is consumed by GitLab. Many errors in the SAML integration can be solved by decoding this response and comparing it to the SAML settings in the GitLab configuration file.
 
 ### GitLab+SAML Testing Environments
 
@@ -682,7 +684,7 @@ This error means that the IdP doesn't recognize GitLab as a valid sender and
 receiver of SAML requests. Make sure to add the GitLab callback URL to the approved
 audiences of the IdP server.
 
-### Missing claims
+### Missing claims, or `Email can't be blank` errors
 
 The IdP server needs to pass certain information in order for GitLab to either
 create an account, or match the login information to an existing account. `email`
@@ -710,3 +712,10 @@ For this you need take the following into account:
 
 Make sure that one of the above described scenarios is valid, or the requests will
 fail with one of the mentioned errors.
+
+### User is blocked when signing in through SAML
+
+The following are the most likely reasons that a user is blocked when signing in through SAML:
+
+- In the configuration, `gitlab_rails['omniauth_block_auto_created_users'] = true` is set and this is the user's first time signing in.
+- There are [`required_groups`](#required-groups) configured, but the user is not a member of one.

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