Merge branch 'security-remove-leaky-401-responses-master' into 'master'

Private/internal repository enumeration via bruteforce on a vulnerable URL

See merge request gitlab/gitlab-ee!1345

app/controllers/application_controller.rb

@@ -17,7 +17,7 @@ class ApplicationController < ActionController::Base
   include Gitlab::Tracking::ControllerConcern
   include Gitlab::Experimentation::ControllerConcern
 
-  before_action :authenticate_user!
+  before_action :authenticate_user!, except: [:route_not_found]
   before_action :enforce_terms!, if: :should_enforce_terms?
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration
@@ -98,7 +98,9 @@ class ApplicationController < ActionController::Base
     if current_user
       not_found
     else
-      authenticate_user!
+      store_location_for(:user, request.fullpath) unless request.xhr?
+
+      redirect_to new_user_session_path, alert: I18n.t('devise.failure.unauthenticated')
     end
   end
 

changelogs/unreleased/29986-remove-leaky-401-responses.yml

+---
+title: Standardize error response when route is missing
+merge_request:
+author:
+type: security

ee/spec/controllers/groups/boards_controller_spec.rb

@@ -56,6 +56,16 @@ describe Groups::BoardsController do
     let(:parent) { group }
 
     it_behaves_like 'returns recently visited boards'
+
+    context 'unauthenticated' do
+      it 'returns a 401' do
+        sign_out(user)
+
+        list_boards(recent: true)
+
+        expect(response).to have_gitlab_http_status(401)
+      end
+    end
   end
 
   describe 'GET show' do

ee/spec/controllers/projects/boards_controller_spec.rb

@@ -31,6 +31,16 @@ describe Projects::BoardsController do
     let(:parent) { project }
 
     it_behaves_like 'returns recently visited boards'
+
+    context 'unauthenticated' do
+      it 'returns a 302' do
+        sign_out(user)
+
+        list_boards(recent: true)
+
+        expect(response).to have_gitlab_http_status(302)
+      end
+    end
   end
 
   describe 'GET show' do

ee/spec/controllers/projects/managed_licenses_controller_spec.rb

@@ -72,10 +72,10 @@ describe Projects::ManagedLicensesController do
     context 'with no logged in user' do
       let(:user) { unlogged_user }
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         subject
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -122,10 +122,10 @@ describe Projects::ManagedLicensesController do
     context 'with no logged in user' do
       let(:user) { unlogged_user }
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         subject
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -235,10 +235,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -347,10 +347,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -452,10 +452,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 

ee/spec/controllers/projects/settings/operations_controller_spec.rb

@@ -506,10 +506,10 @@ describe Projects::Settings::OperationsController do
         sign_out(user)
       end
 
-      it 'returns unauthorized status' do
+      it 'returns a redirect' do
         reset_alerting_token
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 

ee/spec/support/shared_examples/controllers/recent_boards.rb

@@ -5,16 +5,6 @@ require 'spec_helper'
 shared_examples 'returns recently visited boards' do
   let(:boards) { create_list(:board, 8, resource_parent: parent) }
 
-  context 'unauthenticated' do
-    it 'returns a 401' do
-      sign_out(user)
-
-      list_boards(recent: true)
-
-      expect(response).to have_gitlab_http_status(401)
-    end
-  end
-
   it 'returns last 4 visited boards' do
     [0, 2, 5, 3, 7, 1].each_with_index do |board_index, i|
       visit_board(boards[board_index], Time.now + i.minutes)

spec/controllers/application_controller_spec.rb

@@ -186,7 +186,7 @@ describe ApplicationController do
       expect(response).to have_gitlab_http_status(404)
     end
 
-    it 'redirects to login page via authenticate_user! if not authenticated' do
+    it 'redirects to login page if not authenticated' do
       get :index
 
       expect(response).to redirect_to new_user_session_path

spec/controllers/projects/commits_controller_spec.rb

@@ -142,7 +142,7 @@ describe Projects::CommitsController do
 
   context 'token authentication' do
     context 'public project' do
-      it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do
+      it_behaves_like 'authenticates sessionless user', :show, :atom, { public: true, ignore_incrementing: true } do
         before do
           public_project = create(:project, :repository, :public)
 
@@ -152,7 +152,7 @@ describe Projects::CommitsController do
     end
 
     context 'private project' do
-      it_behaves_like 'authenticates sessionless user', :show, :atom, public: false do
+      it_behaves_like 'authenticates sessionless user', :show, :atom, { public: false, ignore_incrementing: true } do
         before do
           private_project = create(:project, :repository, :private)
           private_project.add_maintainer(user)

spec/controllers/projects/error_tracking_controller_spec.rb

@@ -146,7 +146,7 @@ describe Projects::ErrorTrackingController do
       it 'redirects to sign-in page' do
         post :list_projects, params: list_projects_params
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 

spec/controllers/projects/issues_controller_spec.rb

@@ -1441,7 +1441,7 @@ describe Projects::IssuesController do
   context 'private project with token authentication' do
     let(:private_project) { create(:project, :private) }
 
-    it_behaves_like 'authenticates sessionless user', :index, :atom do
+    it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true do
       before do
         default_params.merge!(project_id: private_project, namespace_id: private_project.namespace)
 
@@ -1449,7 +1449,7 @@ describe Projects::IssuesController do
       end
     end
 
-    it_behaves_like 'authenticates sessionless user', :calendar, :ics do
+    it_behaves_like 'authenticates sessionless user', :calendar, :ics, ignore_incrementing: true do
       before do
         default_params.merge!(project_id: private_project, namespace_id: private_project.namespace)
 

spec/controllers/projects/tags_controller_spec.rb

@@ -41,7 +41,7 @@ describe Projects::TagsController do
   context 'private project with token authentication' do
     let(:private_project) { create(:project, :repository, :private) }
 
-    it_behaves_like 'authenticates sessionless user', :index, :atom do
+    it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true do
       before do
         default_params.merge!(project_id: private_project, namespace_id: private_project.namespace)
 

spec/controllers/projects_controller_spec.rb

@@ -1149,7 +1149,7 @@ describe ProjectsController do
   context 'private project with token authentication' do
     let(:private_project) { create(:project, :private) }
 
-    it_behaves_like 'authenticates sessionless user', :show, :atom do
+    it_behaves_like 'authenticates sessionless user', :show, :atom, ignore_incrementing: true do
       before do
         default_params.merge!(id: private_project, namespace_id: private_project.namespace)
 

spec/features/projects/pipelines/pipelines_spec.rb

@@ -819,7 +819,10 @@ describe 'Pipelines', :js do
     context 'when project is private' do
       let(:project) { create(:project, :private, :repository) }
 
-      it { expect(page).to have_content 'You need to sign in' }
+      it 'redirects the user to sign_in and displays the flash alert' do
+        expect(page).to have_content 'You need to sign in'
+        expect(page.current_path).to eq("/users/sign_in")
+      end
     end
   end
 

spec/features/projects/tags/user_views_tags_spec.rb

@@ -15,7 +15,7 @@ describe 'User views tags', :feature do
       it do
         visit project_tags_path(project, format: :atom)
 
-        expect(page).to have_gitlab_http_status(401)
+        expect(page.current_path).to eq("/users/sign_in")
       end
     end
 

spec/support/controllers/sessionless_auth_controller_shared_examples.rb

@@ -34,8 +34,15 @@ shared_examples 'authenticates sessionless user' do |path, format, params|
 
   context 'when the personal access token has no api scope', unless: params[:public] do
     it 'does not log the user in' do
-      expect(authentication_metrics)
-        .to increment(:user_unauthenticated_counter)
+      # Several instances of where these specs are shared route the request
+      #   through ApplicationController#route_not_found which does not involve
+      #   the usual auth code from Devise, so does not increment the
+      #   :user_unauthenticated_counter
+      #
+      unless params[:ignore_incrementing]
+        expect(authentication_metrics)
+          .to increment(:user_unauthenticated_counter)
+      end
 
       personal_access_token.update(scopes: [:read_user])
 
@@ -84,8 +91,15 @@ shared_examples 'authenticates sessionless user' do |path, format, params|
   end
 
   it "doesn't log the user in otherwise", unless: params[:public] do
-    expect(authentication_metrics)
-      .to increment(:user_unauthenticated_counter)
+    # Several instances of where these specs are shared route the request
+    #   through ApplicationController#route_not_found which does not involve
+    #   the usual auth code from Devise, so does not increment the
+    #   :user_unauthenticated_counter
+    #
+    unless params[:ignore_incrementing]
+      expect(authentication_metrics)
+        .to increment(:user_unauthenticated_counter)
+    end
 
     get path, params: default_params.merge(private_token: 'token')
 

spec/support/shared_examples/controllers/todos_shared_examples.rb

@@ -39,7 +39,7 @@ shared_examples 'todos actions' do
         post_create
       end.to change { user.todos.count }.by(0)
 
-      expect(response).to have_gitlab_http_status(parent.is_a?(Group) ? 401 : 302)
+      expect(response).to have_gitlab_http_status(302)
     end
   end
 end