Avoid #authenticate_user! in #route_not_found
This method, #route_not_found, is executed as the final fallback for unrecognized routes (as the name might imply.) We want to avoid `#authenticate_user!` when calling `#route_not_found`; `#authenticate_user!` can, depending on the request format, return a 401 instead of redirecting to a login page. This opens a subtle security exploit where anonymous users will receive a 401 response when attempting to access a private repo, while a recognized user will receive a 404, exposing the existence of the private, hidden repo.
This commit is contained in:
parent
7e2b100854
commit
8395032721
|
@ -16,7 +16,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
|
||||
|
@ -95,7 +95,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
|
||||
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Standardize error response when route is missing
|
||||
merge_request:
|
||||
author:
|
||||
type: security
|
|
@ -202,7 +202,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
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -1440,7 +1440,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)
|
||||
|
||||
|
@ -1448,7 +1448,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)
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
||||
|
|
|
@ -827,7 +827,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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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')
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue