Merge branch '207869-lfs-enabled-checks' into 'master'

Block LFS requests on snippets

See merge request gitlab-org/gitlab!45874

app/controllers/concerns/lfs_request.rb

 # frozen_string_literal: true
 
 # This concern assumes:
+# - a `#container` accessor
 # - a `#project` accessor
 # - a `#user` accessor
 # - a `#authentication_result` accessor
@@ -11,6 +12,7 @@
 # - a `#has_authentication_ability?(ability)` method
 module LfsRequest
   extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
 
   CONTENT_TYPE = 'application/vnd.git-lfs+json'
 
@@ -29,16 +31,19 @@ module LfsRequest
         message: _('Git LFS is not enabled on this GitLab server, contact your admin.'),
         documentation_url: help_url
       },
+      content_type: CONTENT_TYPE,
       status: :not_implemented
     )
   end
 
   def lfs_check_access!
-    return render_lfs_not_found unless project
+    return render_lfs_not_found unless container&.lfs_enabled?
     return if download_request? && lfs_download_access?
     return if upload_request? && lfs_upload_access?
 
-    if project.public? || can?(user, :read_project, project)
+    # Only return a 403 response if the user has download access permission,
+    # otherwise return a 404 to avoid exposing the existence of the container.
+    if lfs_download_access?
       lfs_forbidden!
     else
       render_lfs_not_found
@@ -72,9 +77,9 @@ module LfsRequest
   end
 
   def lfs_download_access?
-    return false unless project.lfs_enabled?
-
-    ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code? || deploy_token_can_download_code?
+    strong_memoize(:lfs_download_access) do
+      ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code? || deploy_token_can_download_code?
+    end
   end
 
   def deploy_token_can_download_code?
@@ -93,11 +98,12 @@ module LfsRequest
   end
 
   def lfs_upload_access?
-    return false unless project.lfs_enabled?
-    return false unless has_authentication_ability?(:push_code)
-    return false if limit_exceeded?
+    strong_memoize(:lfs_upload_access) do
+      next false unless has_authentication_ability?(:push_code)
+      next false if limit_exceeded?
 
-    lfs_deploy_token? || can?(user, :push_code, project)
+      lfs_deploy_token? || can?(user, :push_code, project)
+    end
   end
 
   def lfs_deploy_token?

app/controllers/repositories/git_http_client_controller.rb

@@ -6,7 +6,7 @@ module Repositories
     include KerberosSpnegoHelper
     include Gitlab::Utils::StrongMemoize
 
-    attr_reader :authentication_result, :redirected_path, :container
+    attr_reader :authentication_result, :redirected_path
 
     delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true
     delegate :type, to: :authentication_result, allow_nil: true, prefix: :auth_result
@@ -75,6 +75,12 @@ module Repositories
       headers['Www-Authenticate'] = challenges.join("\n") if challenges.any?
     end
 
+    def container
+      parse_repo_path unless defined?(@container)
+
+      @container
+    end
+
     def project
       parse_repo_path unless defined?(@project)
 

app/controllers/repositories/lfs_api_controller.rb

@@ -17,9 +17,9 @@ module Repositories
       end
 
       if download_request?
-        render json: { objects: download_objects! }
+        render json: { objects: download_objects! }, content_type: LfsRequest::CONTENT_TYPE
       elsif upload_request?
-        render json: { objects: upload_objects! }
+        render json: { objects: upload_objects! }, content_type: LfsRequest::CONTENT_TYPE
       else
         raise "Never reached"
       end
@@ -31,6 +31,7 @@ module Repositories
           message: _('Server supports batch API only, please update your Git LFS client to version 1.0.1 and up.'),
           documentation_url: "#{Gitlab.config.gitlab.url}/help"
         },
+        content_type: LfsRequest::CONTENT_TYPE,
         status: :not_implemented
       )
     end

app/controllers/repositories/lfs_storage_controller.rb

@@ -29,7 +29,7 @@ module Repositories
 
     def upload_finalize
       if store_file!(oid, size)
-        head 200
+        head 200, content_type: LfsRequest::CONTENT_TYPE
       else
         render plain: 'Unprocessable entity', status: :unprocessable_entity
       end

changelogs/unreleased/207869-lfs-enabled-checks.yml

+---
+title: Block LFS requests on snippets
+merge_request: 45874
+author:
+type: fixed

spec/controllers/concerns/lfs_request_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe LfsRequest do
-  include ProjectForksHelper
-
-  controller(Repositories::GitHttpClientController) do
-    # `described_class` is not available in this context
-    include LfsRequest
-
-    def show
-      head :ok
-    end
-
-    def project
-      @project ||= Project.find_by(id: params[:id])
-    end
-
-    def download_request?
-      true
-    end
-
-    def upload_request?
-      false
-    end
-
-    def ci?
-      false
-    end
-  end
-
-  let(:project) { create(:project, :public) }
-
-  before do
-    stub_lfs_setting(enabled: true)
-  end
-
-  context 'user is authenticated without access to lfs' do
-    before do
-      allow(controller).to receive(:authenticate_user)
-      allow(controller).to receive(:authentication_result) do
-        Gitlab::Auth::Result.new
-      end
-    end
-
-    context 'with access to the project' do
-      it 'returns 403' do
-        get :show, params: { id: project.id }
-
-        expect(response).to have_gitlab_http_status(:forbidden)
-      end
-    end
-
-    context 'without access to the project' do
-      context 'project does not exist' do
-        it 'returns 404' do
-          get :show, params: { id: 'does not exist' }
-
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
-      end
-
-      context 'project is private' do
-        let(:project) { create(:project, :private) }
-
-        it 'returns 404' do
-          get :show, params: { id: project.id }
-
-          expect(response).to have_gitlab_http_status(:not_found)
-        end
-      end
-    end
-  end
-end

spec/factories/wikis.rb

@@ -17,5 +17,9 @@ FactoryBot.define do
 
       container { project }
     end
+
+    trait :empty_repo do
+      after(:create, &:create_wiki_repository)
+    end
   end
 end

spec/requests/lfs_locks_api_spec.rb

@@ -3,24 +3,38 @@
 require 'spec_helper'
 
 RSpec.describe 'Git LFS File Locking API' do
+  include LfsHttpHelpers
   include WorkhorseHelpers
 
-  let(:project) { create(:project) }
-  let(:maintainer) { create(:user) }
-  let(:developer) { create(:user) }
-  let(:guest)     { create(:user) }
-  let(:path)      { 'README.md' }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:maintainer) { create(:user) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:reporter) { create(:user) }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:path) { 'README.md' }
+
+  let(:user) { developer }
   let(:headers) do
     {
-      'Authorization' => authorization
+      'Authorization' => authorize_user
     }.compact
   end
 
   shared_examples 'unauthorized request' do
-    context 'when user is not authorized' do
-      let(:authorization) { authorize_user(guest) }
+    context 'when user does not have download permission' do
+      let(:user) { guest }
 
-      it 'returns a forbidden 403 response' do
+      it 'returns a 404 response' do
+        post_lfs_json url, body, headers
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when user does not have upload permission' do
+      let(:user) { reporter }
+
+      it 'returns a 403 response' do
         post_lfs_json url, body, headers
 
         expect(response).to have_gitlab_http_status(:forbidden)
@@ -31,15 +45,15 @@ RSpec.describe 'Git LFS File Locking API' do
   before do
     allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
 
-    project.add_developer(maintainer)
+    project.add_maintainer(maintainer)
     project.add_developer(developer)
+    project.add_reporter(reporter)
     project.add_guest(guest)
   end
 
   describe 'Create File Lock endpoint' do
-    let(:url)           { "#{project.http_url_to_repo}/info/lfs/locks" }
-    let(:authorization) { authorize_user(developer) }
-    let(:body)          { { path: path } }
+    let(:url) { "#{project.http_url_to_repo}/info/lfs/locks" }
+    let(:body) { { path: path } }
 
     include_examples 'unauthorized request'
 
@@ -76,8 +90,7 @@ RSpec.describe 'Git LFS File Locking API' do
   end
 
   describe 'Listing File Locks endpoint' do
-    let(:url)           { "#{project.http_url_to_repo}/info/lfs/locks" }
-    let(:authorization) { authorize_user(developer) }
+    let(:url) { "#{project.http_url_to_repo}/info/lfs/locks" }
 
     include_examples 'unauthorized request'
 
@@ -95,8 +108,7 @@ RSpec.describe 'Git LFS File Locking API' do
   end
 
   describe 'List File Locks for verification endpoint' do
-    let(:url)           { "#{project.http_url_to_repo}/info/lfs/locks/verify" }
-    let(:authorization) { authorize_user(developer) }
+    let(:url) { "#{project.http_url_to_repo}/info/lfs/locks/verify" }
 
     include_examples 'unauthorized request'
 
@@ -116,9 +128,8 @@ RSpec.describe 'Git LFS File Locking API' do
   end
 
   describe 'Delete File Lock endpoint' do
-    let!(:lock)         { lock_file('README.md', developer) }
-    let(:url)           { "#{project.http_url_to_repo}/info/lfs/locks/#{lock[:id]}/unlock" }
-    let(:authorization) { authorize_user(developer) }
+    let!(:lock) { lock_file('README.md', developer) }
+    let(:url) { "#{project.http_url_to_repo}/info/lfs/locks/#{lock[:id]}/unlock" }
 
     include_examples 'unauthorized request'
 
@@ -136,7 +147,7 @@ RSpec.describe 'Git LFS File Locking API' do
       end
 
       context 'when a maintainer uses force' do
-        let(:authorization) { authorize_user(maintainer) }
+        let(:user) { maintainer }
 
         it 'deletes the lock' do
           project.add_maintainer(maintainer)
@@ -154,14 +165,6 @@ RSpec.describe 'Git LFS File Locking API' do
     result[:lock]
   end
 
-  def authorize_user(user)
-    ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password)
-  end
-
-  def post_lfs_json(url, body = nil, headers = nil)
-    post(url, params: body.try(:to_json), headers: (headers || {}).merge('Content-Type' => LfsRequest::CONTENT_TYPE))
-  end
-
   def do_get(url, params = nil, headers = nil)
     get(url, params: (params || {}), headers: (headers || {}).merge('Content-Type' => LfsRequest::CONTENT_TYPE))
   end

spec/support/helpers/lfs_http_helpers.rb

@@ -31,16 +31,16 @@ module LfsHttpHelpers
     post(url, params: params, headers: headers)
   end
 
-  def batch_url(project)
-    "#{project.http_url_to_repo}/info/lfs/objects/batch"
+  def batch_url(container)
+    "#{container.http_url_to_repo}/info/lfs/objects/batch"
   end
 
-  def objects_url(project, oid = nil, size = nil)
-    File.join(["#{project.http_url_to_repo}/gitlab-lfs/objects", oid, size].compact.map(&:to_s))
+  def objects_url(container, oid = nil, size = nil)
+    File.join(["#{container.http_url_to_repo}/gitlab-lfs/objects", oid, size].compact.map(&:to_s))
   end
 
-  def authorize_url(project, oid, size)
-    File.join(objects_url(project, oid, size), 'authorize')
+  def authorize_url(container, oid, size)
+    File.join(objects_url(container, oid, size), 'authorize')
   end
 
   def download_body(objects)

spec/support/shared_examples/requests/lfs_http_shared_examples.rb

@@ -2,42 +2,252 @@
 
 RSpec.shared_examples 'LFS http 200 response' do
   it_behaves_like 'LFS http expected response code and message' do
-    let(:response_code) { 200 }
+    let(:response_code) { :ok }
+  end
+end
+
+RSpec.shared_examples 'LFS http 200 blob response' do
+  it_behaves_like 'LFS http expected response code and message' do
+    let(:response_code) { :ok }
+    let(:content_type) { Repositories::LfsApiController::LFS_TRANSFER_CONTENT_TYPE }
+    let(:response_headers) { { 'X-Sendfile' => lfs_object.file.path } }
+  end
+end
+
+RSpec.shared_examples 'LFS http 200 workhorse response' do
+  it_behaves_like 'LFS http expected response code and message' do
+    let(:response_code) { :ok }
+    let(:content_type) { Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE }
   end
 end
 
 RSpec.shared_examples 'LFS http 401 response' do
   it_behaves_like 'LFS http expected response code and message' do
-    let(:response_code) { 401 }
+    let(:response_code) { :unauthorized }
+    let(:content_type) { 'text/plain' }
   end
 end
 
 RSpec.shared_examples 'LFS http 403 response' do
   it_behaves_like 'LFS http expected response code and message' do
-    let(:response_code) { 403 }
+    let(:response_code) { :forbidden }
     let(:message) { 'Access forbidden. Check your access level.' }
   end
 end
 
 RSpec.shared_examples 'LFS http 501 response' do
   it_behaves_like 'LFS http expected response code and message' do
-    let(:response_code) { 501 }
+    let(:response_code) { :not_implemented }
     let(:message) { 'Git LFS is not enabled on this GitLab server, contact your admin.' }
   end
 end
 
 RSpec.shared_examples 'LFS http 404 response' do
   it_behaves_like 'LFS http expected response code and message' do
-    let(:response_code) { 404 }
+    let(:response_code) { :not_found }
   end
 end
 
 RSpec.shared_examples 'LFS http expected response code and message' do
   let(:response_code) { }
-  let(:message) { }
+  let(:response_headers) { {} }
+  let(:content_type) { LfsRequest::CONTENT_TYPE }
+  let(:message) {}
 
-  it 'responds with the expected response code and message' do
+  specify do
     expect(response).to have_gitlab_http_status(response_code)
+    expect(response.headers.to_hash).to include(response_headers)
+    expect(response.media_type).to match(content_type)
     expect(json_response['message']).to eq(message) if message
   end
 end
+
+RSpec.shared_examples 'LFS http requests' do
+  include LfsHttpHelpers
+
+  let(:authorize_guest) {}
+  let(:authorize_download) {}
+  let(:authorize_upload) {}
+
+  let(:lfs_object) { create(:lfs_object, :with_file) }
+  let(:sample_oid) { lfs_object.oid }
+
+  let(:authorization) { authorize_user }
+  let(:headers) do
+    {
+      'Authorization' => authorization,
+      'X-Sendfile-Type' => 'X-Sendfile'
+    }
+  end
+
+  let(:request_download) do
+    get objects_url(container, sample_oid), params: {}, headers: headers
+  end
+
+  let(:request_upload) do
+    post_lfs_json batch_url(container), upload_body(multiple_objects), headers
+  end
+
+  before do
+    stub_lfs_setting(enabled: true)
+  end
+
+  context 'when LFS is disabled globally' do
+    before do
+      stub_lfs_setting(enabled: false)
+    end
+
+    describe 'download request' do
+      before do
+        request_download
+      end
+
+      it_behaves_like 'LFS http 501 response'
+    end
+
+    describe 'upload request' do
+      before do
+        request_upload
+      end
+
+      it_behaves_like 'LFS http 501 response'
+    end
+  end
+
+  context 'unauthenticated' do
+    let(:headers) { {} }
+
+    describe 'download request' do
+      before do
+        request_download
+      end
+
+      it_behaves_like 'LFS http 401 response'
+    end
+
+    describe 'upload request' do
+      before do
+        request_upload
+      end
+
+      it_behaves_like 'LFS http 401 response'
+    end
+  end
+
+  context 'without access' do
+    describe 'download request' do
+      before do
+        request_download
+      end
+
+      it_behaves_like 'LFS http 404 response'
+    end
+
+    describe 'upload request' do
+      before do
+        request_upload
+      end
+
+      it_behaves_like 'LFS http 404 response'
+    end
+  end
+
+  context 'with guest access' do
+    before do
+      authorize_guest
+    end
+
+    describe 'download request' do
+      before do
+        request_download
+      end
+
+      it_behaves_like 'LFS http 404 response'
+    end
+
+    describe 'upload request' do
+      before do
+        request_upload
+      end
+
+      it_behaves_like 'LFS http 404 response'
+    end
+  end
+
+  context 'with download permission' do
+    before do
+      authorize_download
+    end
+
+    describe 'download request' do
+      before do
+        request_download
+      end
+
+      it_behaves_like 'LFS http 200 blob response'
+
+      context 'when container does not exist' do
+        def objects_url(*args)
+          super.sub(container.full_path, 'missing/path')
+        end
+
+        it_behaves_like 'LFS http 404 response'
+      end
+    end
+
+    describe 'upload request' do
+      before do
+        request_upload
+      end
+
+      it_behaves_like 'LFS http 403 response'
+    end
+  end
+
+  context 'with upload permission' do
+    before do
+      authorize_upload
+    end
+
+    describe 'upload request' do
+      before do
+        request_upload
+      end
+
+      it_behaves_like 'LFS http 200 response'
+    end
+  end
+
+  describe 'deprecated API' do
+    shared_examples 'deprecated request' do
+      before do
+        request
+      end
+
+      it_behaves_like 'LFS http expected response code and message' do
+        let(:response_code) { 501 }
+        let(:message) { 'Server supports batch API only, please update your Git LFS client to version 1.0.1 and up.' }
+      end
+    end
+
+    context 'when fetching LFS object using deprecated API' do
+      subject(:request) do
+        get deprecated_objects_url(container, sample_oid), params: {}, headers: headers
+      end
+
+      it_behaves_like 'deprecated request'
+    end
+
+    context 'when handling LFS request using deprecated API' do
+      subject(:request) do
+        post_lfs_json deprecated_objects_url(container), nil, headers
+      end
+
+      it_behaves_like 'deprecated request'
+    end
+
+    def deprecated_objects_url(container, oid = nil)
+      File.join(["#{container.http_url_to_repo}/info/lfs/objects/", oid].compact)
+    end
+  end
+end