From ce5fc4c88dc46a9ebc1e6bb43de1d262ea56b886 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Sat, 7 Nov 2020 03:09:23 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../unreleased/bvl-handle-invalid-headers.yml | 5 --- doc/integration/saml.md | 13 +++++- .../middleware/handle_malformed_strings.rb | 23 +--------- .../multipart_invalid_uploads_spec.rb | 6 +-- .../handle_malformed_strings_spec.rb | 44 +++++-------------- .../user_sends_malformed_strings_spec.rb | 10 +---- 6 files changed, 27 insertions(+), 74 deletions(-) delete mode 100644 changelogs/unreleased/bvl-handle-invalid-headers.yml diff --git a/changelogs/unreleased/bvl-handle-invalid-headers.yml b/changelogs/unreleased/bvl-handle-invalid-headers.yml deleted file mode 100644 index 74f6b5700f1..00000000000 --- a/changelogs/unreleased/bvl-handle-invalid-headers.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Handle nullbytes in auth headers -merge_request: 46985 -author: -type: fixed diff --git a/doc/integration/saml.md b/doc/integration/saml.md index ee08a0026cd..ac649e48c1a 100644 --- a/doc/integration/saml.md +++ b/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. diff --git a/lib/gitlab/middleware/handle_malformed_strings.rb b/lib/gitlab/middleware/handle_malformed_strings.rb index 9baa639caea..bb2a8ead525 100644 --- a/lib/gitlab/middleware/handle_malformed_strings.rb +++ b/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. diff --git a/spec/features/file_uploads/multipart_invalid_uploads_spec.rb b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb index b3ace2e30ff..e9e24c12af1 100644 --- a/spec/features/file_uploads/multipart_invalid_uploads_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb b/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb index fec273ecafd..5ed1580fa8d 100644 --- a/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb +++ b/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 diff --git a/spec/requests/user_sends_malformed_strings_spec.rb b/spec/requests/user_sends_malformed_strings_spec.rb index da533606be5..b6eda9159bc 100644 --- a/spec/requests/user_sends_malformed_strings_spec.rb +++ b/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