diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-24 18:53:53 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-24 18:53:53 +0000 |
commit | c483d1e2984b13c6f9ff1fdfd761be750d84e25c (patch) | |
tree | d65a4d64f205adb99d39e7b00fbfc61c6249ed70 | |
parent | a94d26a7b2bc20df348a8826b066e62151fa14d1 (diff) | |
parent | 81eba2203902d85e3c43912e62fa69244a296b2f (diff) | |
download | gitlab-ce-c483d1e2984b13c6f9ff1fdfd761be750d84e25c.tar.gz |
Merge branch 'security-remove-leaky-401-responses-12.2' into '12-2-stable'
Private/internal repository enumeration via bruteforce on a vulnerable URL
See merge request gitlab/gitlabhq!3456
12 files changed, 40 insertions, 22 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index af6644b8fcc..a246ec15535 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,7 +14,7 @@ class ApplicationController < ActionController::Base include SessionlessAuthentication include ConfirmEmailWarning - 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 @@ -92,7 +92,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 0b3833e6515..622122a7551 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -176,12 +176,6 @@ describe ApplicationController do expect(controller).to receive(:not_found) controller.send(:route_not_found) end - - it 'does redirect to login page via authenticate_user! if not authenticated' do - allow(controller).to receive(:current_user).and_return(nil) - expect(controller).to receive(:authenticate_user!) - controller.send(:route_not_found) - end end describe '#set_page_title_header' do diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb index 9db1ac2a46c..cb1a43fbbe4 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 d11ef24ef96..92b63ec96d5 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 fab47aa4701..e67f7a7c1c1 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1349,7 +1349,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) @@ -1357,7 +1357,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 083a1c1383a..59da77dd01d 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -1053,7 +1053,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 |