Better route matching for read-only detection

lib/gitlab/middleware/read_only/controller.rb

@@ -4,8 +4,18 @@ module Gitlab
       class Controller
         DISALLOWED_METHODS = %w(POST PATCH PUT DELETE).freeze
         APPLICATION_JSON = 'application/json'.freeze
+        APPLICATION_JSON_TYPES = %W{#{APPLICATION_JSON} application/vnd.git-lfs+json}.freeze
         ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance'.freeze
 
+        WHITELISTED_GIT_ROUTES = {
+          'projects/git_http' => %w{git_upload_pack git_receive_pack}
+        }.freeze
+
+        WHITELISTED_GIT_LFS_ROUTES = {
+          'projects/lfs_api' => %w{batch},
+          'projects/lfs_locks_api' => %w{verify create unlock}
+        }.freeze
+
         def initialize(app, env)
           @app = app
           @env = env
@@ -36,7 +46,7 @@ module Gitlab
         end
 
         def json_request?
-          request.media_type == APPLICATION_JSON
+          APPLICATION_JSON_TYPES.include?(request.media_type)
         end
 
         def rack_flash
@@ -63,22 +73,27 @@ module Gitlab
           grack_route || ReadOnly.internal_routes.any? { |path| request.path.include?(path) } || lfs_route || sidekiq_route
         end
 
-        def sidekiq_route
-          request.path.start_with?('/admin/sidekiq')
-        end
-
         def grack_route
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          return false unless request.path.end_with?('.git/git-upload-pack')
+          return false unless
+            request.path.end_with?('.git/git-upload-pack', '.git/git-receive-pack')
 
-          route_hash[:controller] == 'projects/git_http' && route_hash[:action] == 'git_upload_pack'
+          WHITELISTED_GIT_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
 
         def lfs_route
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          return false unless request.path.end_with?('/info/lfs/objects/batch')
+          unless request.path.end_with?('/info/lfs/objects/batch',
+            '/info/lfs/locks', '/info/lfs/locks/verify') ||
+              %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path)
+            return false
+          end
+
+          WHITELISTED_GIT_LFS_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
+        end
 
-          route_hash[:controller] == 'projects/lfs_api' && route_hash[:action] == 'batch'
+        def sidekiq_route
+          request.path.start_with?('/admin/sidekiq')
         end
       end
     end

spec/lib/gitlab/middleware/read_only_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::Middleware::ReadOnly do
   include Rack::Test::Methods
+  using RSpec::Parameterized::TableSyntax
 
   RSpec::Matchers.define :be_a_redirect do
     match do |response|
@@ -117,39 +118,41 @@ describe Gitlab::Middleware::ReadOnly do
     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_a_redirect
         expect(subject).not_to disallow_request
       end
 
-      it 'expects a POST LFS request to batch URL to be allowed' do
-        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-        response = request.post('/root/rouge.git/info/lfs/objects/batch')
+      it 'expects requests to sidekiq admin to be allowed' do
+        response = request.post('/admin/sidekiq')
 
         expect(response).not_to be_a_redirect
         expect(subject).not_to disallow_request
-      end
 
-      it 'expects a POST request to git-upload-pack URL to be allowed' do
-        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-        response = request.post('/root/rouge.git/git-upload-pack')
+        response = request.get('/admin/sidekiq')
 
         expect(response).not_to be_a_redirect
         expect(subject).not_to disallow_request
       end
 
-      it 'expects requests to sidekiq admin to be allowed' do
-        response = request.post('/admin/sidekiq')
-
-        expect(response).not_to be_a_redirect
-        expect(subject).not_to disallow_request
+      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
 
-        response = request.get('/admin/sidekiq')
+      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_a_redirect
-        expect(subject).not_to disallow_request
+          expect(response).not_to be_a_redirect
+          expect(subject).not_to disallow_request
+        end
       end
     end
   end