Block LFS requests on snippets

The repository routes for project repositories are ambiguous and also
match project snippet repositories, so LFS requests for project snippets
will work but snippets are not ready yet to properly support LFS.

We can work around this by checking `#lfs_enabled?` on the `container`
instead of the `project`, which for snippets will be the snippet itself,
and `Snippet#lfs_enabled?` is currently hard-coded to return `false`.

To simplify things, we also remove the project-specific access check and
use `lfs_download_access?` instead to determine wether to expose the
existence of the project (404 response) or not (403 response), when
sending an error response. When LFS is disabled on the container we now
also send a 404 instead of a 403.

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