Add latest changes from gitlab-org/gitlab@13-6-stable-eev13.6.0-rc42

3 files changed, 184 insertions, 288 deletions

diff --git a/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb b/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb
new file mode 100644
index 00000000000..e806f6478b7
--- /dev/null
+++ b/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb
@@ -0,0 +1,182 @@
+# frozen_string_literal: true
+
+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" }
+  let(:escaped_invalid_string) { "mal%c0formed" }
+  let(:error_400) { [400, { 'Content-Type' => 'text/plain' }, ['Bad Request']] }
+  let(:app) { double(:app) }
+
+  subject { described_class.new(app) }
+
+  before do
+    allow(app).to receive(:call) do |args|
+      args
+    end
+  end
+
+  def env_for(params = {})
+    Rack::MockRequest.env_for('/', { params: params })
+  end
+
+  context 'in the URL' do
+    it 'rejects null bytes' do
+      # We have to create the env separately or Rack::MockRequest complains about invalid URI
+      env = env_for
+      env['PATH_INFO'] = "/someplace/witha#{null_byte}nullbyte"
+
+      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
+      env['PATH_INFO'] = "/someplace/with_an/#{invalid_string}"
+
+      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 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 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.
+      special_token = 'GL-Geo ta8KakZWpu0AcledQ6n0:eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjoie1wic2NvcGVcIjpcImdlb19hcGlcIn0iLCJqdGkiOiIwYWFmNzVlYi1lNWRkLTRkZjEtODQzYi1lM2E5ODhhNDMwMzIiLCJpYXQiOjE2MDQ3MDI4NzUsIm5iZiI6MTYwNDcwMjg3MCwiZXhwIjoxNjA0NzAyOTM1fQ.NcgDipDyxSP5uSzxc01ylzH4GkTxJRflNNjT7U6fpg4'
+      expect(Base64.decode64(special_token)).to include(null_byte)
+
+      env = env_for.merge('HTTP_AUTHORIZATION' => special_token)
+
+      expect(subject.call(env)).not_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
+        env = env_for(name: "null#{problematic_input}byte")
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "rejects bad params for hashes with strings" do
+        env = env_for(name: { inner_key: "I am #{problematic_input} bad" })
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "rejects bad params for arrays with strings" do
+        env = env_for(name: ["I am #{problematic_input} bad"])
+
+        expect(subject.call(env)).to eq error_400
+      end
+
+      it "rejects bad params for arrays containing hashes with string values" do
+        env = env_for(name: [
+          {
+            inner_key: "I am #{problematic_input} bad"
+          }
+        ])
+
+        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" }] }
+                        }
+                      ])
+
+        expect(subject.call(env)).not_to eq error_400
+      end
+    end
+
+    context 'with malformed strings' do
+      it_behaves_like 'checks params' do
+        let(:problematic_input) { invalid_string }
+      end
+    end
+  end
+
+  context 'without problematic input' do
+    it "does not error for strings" do
+      env = env_for(name: "safe name")
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+
+    it "does not error with no params" do
+      env = env_for
+
+      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/lib/gitlab/middleware/handle_null_bytes_spec.rb b/spec/lib/gitlab/middleware/handle_null_bytes_spec.rb
deleted file mode 100644
index 76a5174817e..00000000000
--- a/spec/lib/gitlab/middleware/handle_null_bytes_spec.rb
+++ /dev/null
@@ -1,88 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-require "rack/test"
-
-RSpec.describe Gitlab::Middleware::HandleNullBytes do
-  let(:null_byte) { "\u0000" }
-  let(:error_400) { [400, {}, ["Bad Request"]] }
-  let(:app) { double(:app) }
-
-  subject { described_class.new(app) }
-
-  before do
-    allow(app).to receive(:call) do |args|
-      args
-    end
-  end
-
-  def env_for(params = {})
-    Rack::MockRequest.env_for('/', { params: params })
-  end
-
-  context 'with null bytes in params' do
-    it 'rejects null bytes in a top level param' do
-      env = env_for(name: "null#{null_byte}byte")
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "responds with 400 BadRequest for hashes with strings" do
-      env = env_for(name: { inner_key: "I am #{null_byte} bad" })
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "responds with 400 BadRequest for arrays with strings" do
-      env = env_for(name: ["I am #{null_byte} bad"])
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "responds with 400 BadRequest for arrays containing hashes with string values" do
-      env = env_for(name: [
-        {
-          inner_key: "I am #{null_byte} bad"
-        }
-      ])
-
-      expect(subject.call(env)).to eq error_400
-    end
-
-    it "gives up and does not 400 with too deeply nested params" do
-      env = env_for(name: [
-        {
-          inner_key: { deeper_key: [{ hash_inside_array_key: "I am #{null_byte} bad" }] }
-        }
-      ])
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-  end
-
-  context 'without null bytes in params' do
-    it "does not respond with a 400 for strings" do
-      env = env_for(name: "safe name")
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-
-    it "does not respond with a 400 with no params" do
-      env = env_for
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-  end
-
-  context 'when disabled via env flag' do
-    before do
-      stub_env('REJECT_NULL_BYTES', '1')
-    end
-
-    it 'does not respond with a 400 no matter what' do
-      env = env_for(name: "null#{null_byte}byte")
-
-      expect(subject.call(env)).not_to eq error_400
-    end
-  end
-end
diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb
index 50dd38278b9..642b47fe087 100644
--- a/spec/lib/gitlab/middleware/read_only_spec.rb
+++ b/spec/lib/gitlab/middleware/read_only_spec.rb
@@ -3,209 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Middleware::ReadOnly do
-  include Rack::Test::Methods
-  using RSpec::Parameterized::TableSyntax
-
-  let(:rack_stack) do
-    rack = Rack::Builder.new do
-      use ActionDispatch::Session::CacheStore
-      use ActionDispatch::Flash
-    end
-
-    rack.run(subject)
-    rack.to_app
-  end
-
-  let(:observe_env) do
-    Module.new do
-      attr_reader :env
-
-      def call(env)
-        @env = env
-        super
-      end
-    end
-  end
-
-  let(:request) { Rack::MockRequest.new(rack_stack) }
-
-  subject do
-    described_class.new(fake_app).tap do |app|
-      app.extend(observe_env)
-    end
-  end
-
-  context 'normal requests to a read-only GitLab instance' do
-    let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } }
-
+  context 'when database is read-only' do
     before do
       allow(Gitlab::Database).to receive(:read_only?) { true }
     end
 
-    it 'expects PATCH requests to be disallowed' do
-      response = request.patch('/test_request')
-
-      expect(response).to be_redirect
-      expect(subject).to disallow_request
-    end
-
-    it 'expects PUT requests to be disallowed' do
-      response = request.put('/test_request')
-
-      expect(response).to be_redirect
-      expect(subject).to disallow_request
-    end
-
-    it 'expects POST requests to be disallowed' do
-      response = request.post('/test_request')
-
-      expect(response).to be_redirect
-      expect(subject).to disallow_request
-    end
-
-    it 'expects a internal POST request to be allowed after a disallowed request' do
-      response = request.post('/test_request')
-
-      expect(response).to be_redirect
-
-      response = request.post("/api/#{API::API.version}/internal")
-
-      expect(response).not_to be_redirect
-    end
-
-    it 'expects DELETE requests to be disallowed' do
-      response = request.delete('/test_request')
-
-      expect(response).to be_redirect
-      expect(subject).to disallow_request
-    end
-
-    it 'expects POST of new file that looks like an LFS batch url to be disallowed' do
-      expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-      response = request.post('/root/gitlab-ce/new/master/app/info/lfs/objects/batch')
-
-      expect(response).to be_redirect
-      expect(subject).to disallow_request
-    end
-
-    it 'returns last_vistited_url for disallowed request' do
-      response = request.post('/test_request')
-
-      expect(response.location).to eq 'http://localhost/'
-    end
-
-    context 'whitelisted requests' do
-      it 'expects a POST internal request to be allowed' do
-        expect(Rails.application.routes).not_to receive(:recognize_path)
-        response = request.post("/api/#{API::API.version}/internal")
-
-        expect(response).not_to be_redirect
-        expect(subject).not_to disallow_request
-      end
-
-      it 'expects a graphql request to be allowed' do
-        response = request.post("/api/graphql")
-
-        expect(response).not_to be_redirect
-        expect(subject).not_to disallow_request
-      end
-
-      context 'relative URL is configured' do
-        before do
-          stub_config_setting(relative_url_root: '/gitlab')
-        end
-
-        it 'expects a graphql request to be allowed' do
-          response = request.post("/gitlab/api/graphql")
-
-          expect(response).not_to be_redirect
-          expect(subject).not_to disallow_request
-        end
-      end
-
-      context 'sidekiq admin requests' do
-        where(:mounted_at) do
-          [
-            '',
-            '/',
-            '/gitlab',
-            '/gitlab/',
-            '/gitlab/gitlab',
-            '/gitlab/gitlab/'
-          ]
-        end
-
-        with_them do
-          before do
-            stub_config_setting(relative_url_root: mounted_at)
-          end
-
-          it 'allows requests' do
-            path = File.join(mounted_at, 'admin/sidekiq')
-            response = request.post(path)
-
-            expect(response).not_to be_redirect
-            expect(subject).not_to disallow_request
-
-            response = request.get(path)
-
-            expect(response).not_to be_redirect
-            expect(subject).not_to disallow_request
-          end
-        end
-      end
-
-      where(:description, :path) do
-        'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
-        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
-        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
-        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
-        'request to git-upload-pack'  | '/root/rouge.git/git-upload-pack'
-        'request to git-receive-pack' | '/root/rouge.git/git-receive-pack'
-      end
-
-      with_them do
-        it "expects a POST #{description} URL to be allowed" do
-          expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-          response = request.post(path)
-
-          expect(response).not_to be_redirect
-          expect(subject).not_to disallow_request
-        end
-      end
-    end
-  end
-
-  context 'json requests to a read-only GitLab instance' do
-    let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'application/json' }, ['OK']] } }
-    let(:content_json) { { 'CONTENT_TYPE' => 'application/json' } }
-
-    before do
-      allow(Gitlab::Database).to receive(:read_only?) { true }
-    end
-
-    it 'expects PATCH requests to be disallowed' do
-      response = request.patch('/test_request', content_json)
-
-      expect(response).to disallow_request_in_json
-    end
-
-    it 'expects PUT requests to be disallowed' do
-      response = request.put('/test_request', content_json)
-
-      expect(response).to disallow_request_in_json
-    end
-
-    it 'expects POST requests to be disallowed' do
-      response = request.post('/test_request', content_json)
-
-      expect(response).to disallow_request_in_json
-    end
-
-    it 'expects DELETE requests to be disallowed' do
-      response = request.delete('/test_request', content_json)
-
-      expect(response).to disallow_request_in_json
-    end
+    it_behaves_like 'write access for a read-only GitLab instance'
   end
 end