diff options
12 files changed, 41 insertions, 17 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1443a71f6b1..27e88ae569e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -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 @@ -97,7 +97,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 diff --git a/changelogs/unreleased/29986-remove-leaky-401-responses.yml b/changelogs/unreleased/29986-remove-leaky-401-responses.yml new file mode 100644 index 00000000000..3d60011b63f --- /dev/null +++ b/changelogs/unreleased/29986-remove-leaky-401-responses.yml @@ -0,0 +1,5 @@ +--- +title: Standardize error response when route is missing +merge_request: +author: +type: security diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index ed91b5973b8..993e4020a75 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -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 diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb index 9c4d6fdcb2a..1977e92e42b 100644 --- a/spec/controllers/projects/commits_controller_spec.rb +++ b/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) diff --git a/spec/controllers/projects/error_tracking_controller_spec.rb b/spec/controllers/projects/error_tracking_controller_spec.rb index 4c224e960a6..31868f5f717 100644 --- a/spec/controllers/projects/error_tracking_controller_spec.rb +++ b/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 diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index c9558abab33..d36336a9f67 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/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) diff --git a/spec/controllers/projects/tags_controller_spec.rb b/spec/controllers/projects/tags_controller_spec.rb index b99b5d611fc..f077b4c99fc 100644 --- a/spec/controllers/projects/tags_controller_spec.rb +++ b/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) diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index ea7dd78329a..e0df9556eb8 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/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) diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 4fb72eb8737..76d8ad1638b 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -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 diff --git a/spec/features/projects/tags/user_views_tags_spec.rb b/spec/features/projects/tags/user_views_tags_spec.rb index f344b682715..bc570f502bf 100644 --- a/spec/features/projects/tags/user_views_tags_spec.rb +++ b/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 diff --git a/spec/support/controllers/sessionless_auth_controller_shared_examples.rb b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb index b5149a0fcb1..bc95fcd6b88 100644 --- a/spec/support/controllers/sessionless_auth_controller_shared_examples.rb +++ b/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') diff --git a/spec/support/shared_examples/controllers/todos_shared_examples.rb b/spec/support/shared_examples/controllers/todos_shared_examples.rb index f3f9abb7da2..914bf506320 100644 --- a/spec/support/shared_examples/controllers/todos_shared_examples.rb +++ b/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 |