Merge branch 'security-id-fix-disclosure-of-private-repo-names' into 'master'

Return 404 on LFS request if project doesn't exist

See merge request gitlab/gitlabhq!3505

3 files changed, 48 insertions, 1 deletions

diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb
index 733265f4099..417bb169f39 100644
--- a/app/controllers/concerns/lfs_request.rb
+++ b/app/controllers/concerns/lfs_request.rb
@@ -34,6 +34,7 @@ module LfsRequest
   end
 
   def lfs_check_access!
+    return render_lfs_not_found unless project
     return if download_request? && lfs_download_access?
     return if upload_request? && lfs_upload_access?
 
diff --git a/changelogs/unreleased/security-id-fix-disclosure-of-private-repo-names.yml b/changelogs/unreleased/security-id-fix-disclosure-of-private-repo-names.yml
new file mode 100644
index 00000000000..dfd7a2d11f9
--- /dev/null
+++ b/changelogs/unreleased/security-id-fix-disclosure-of-private-repo-names.yml
@@ -0,0 +1,5 @@
+---
+title: Return 404 on LFS request if project doesn't exist
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/concerns/lfs_request_spec.rb b/spec/controllers/concerns/lfs_request_spec.rb
index cb8c0b8f71c..823b9a50434 100644
--- a/spec/controllers/concerns/lfs_request_spec.rb
+++ b/spec/controllers/concerns/lfs_request_spec.rb
@@ -16,13 +16,17 @@ describe LfsRequest do
     end
 
     def project
-      @project ||= Project.find(params[:id])
+      @project ||= Project.find_by(id: params[:id])
     end
 
     def download_request?
       true
     end
 
+    def upload_request?
+      false
+    end
+
     def ci?
       false
     end
@@ -49,4 +53,41 @@ describe LfsRequest do
       expect(assigns(:storage_project)).to eq(project)
     end
   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.status).to eq(403)
+      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.status).to eq(404)
+        end
+      end
+
+      context 'project is private' do
+        let(:project) { create(:project, :private) }
+
+        it 'returns 404' do
+          get :show, params: { id: project.id }
+
+          expect(response.status).to eq(404)
+        end
+      end
+    end
+  end
 end