Better route matching for read-only detection

1 files changed, 24 insertions, 9 deletions

diff --git a/lib/gitlab/middleware/read_only/controller.rb b/lib/gitlab/middleware/read_only/controller.rb
index 45b644e6510..4a99b7cca5c 100644
--- a/lib/gitlab/middleware/read_only/controller.rb
+++ b/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