diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 12:58:26 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 12:58:46 +0000 |
commit | e87fbda1d91e97a958bdf97d781dd754a867ea7b (patch) | |
tree | 42f6817d889bc35964beae8f776f9f4583caad99 | |
parent | 3c992c78e972b8019ae49015d445524d654e4076 (diff) | |
download | gitlab-ce-e87fbda1d91e97a958bdf97d781dd754a867ea7b.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-1-stable-ee
-rw-r--r-- | app/controllers/admin/users_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/concerns/impersonation.rb | 6 | ||||
-rw-r--r-- | app/controllers/import/gitea_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/profiles/passwords_controller.rb | 8 | ||||
-rw-r--r-- | config/initializers/doorkeeper.rb | 5 | ||||
-rw-r--r-- | doc/user/project/settings/import_export.md | 1 | ||||
-rw-r--r-- | lib/gitlab/import_export/group/import_export.yml | 1 | ||||
-rw-r--r-- | lib/gitlab/import_export/project/import_export.yml | 1 | ||||
-rw-r--r-- | lib/gitlab/legacy_github_import/client.rb | 6 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/controllers/admin/users_controller_spec.rb | 15 | ||||
-rw-r--r-- | spec/controllers/import/gitea_controller_spec.rb | 42 | ||||
-rw-r--r-- | spec/controllers/oauth/applications_controller_spec.rb | 63 | ||||
-rw-r--r-- | spec/features/profiles/password_spec.rb | 78 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 | ||||
-rw-r--r-- | spec/lib/gitlab/legacy_github_import/client_spec.rb | 9 |
16 files changed, 196 insertions, 55 deletions
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 145b4d10b16..677e61dc9f0 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -44,7 +44,7 @@ class Admin::UsersController < Admin::ApplicationController end def impersonate - if can?(user, :log_in) + if can?(user, :log_in) && !impersonation_in_progress? session[:impersonator_id] = current_user.id warden.set_user(user, scope: :user) @@ -56,7 +56,9 @@ class Admin::UsersController < Admin::ApplicationController redirect_to root_path else flash[:alert] = - if user.blocked? + if impersonation_in_progress? + _("You are already impersonating another user") + elsif user.blocked? _("You cannot impersonate a blocked user") elsif user.internal? _("You cannot impersonate an internal user") diff --git a/app/controllers/concerns/impersonation.rb b/app/controllers/concerns/impersonation.rb index a4f2c263eb4..0764fbc8eb3 100644 --- a/app/controllers/concerns/impersonation.rb +++ b/app/controllers/concerns/impersonation.rb @@ -14,7 +14,7 @@ module Impersonation protected def check_impersonation_availability - return unless session[:impersonator_id] + return unless impersonation_in_progress? unless Gitlab.config.gitlab.impersonation_enabled stop_impersonation @@ -31,6 +31,10 @@ module Impersonation current_user end + def impersonation_in_progress? + session[:impersonator_id].present? + end + def log_impersonation_event Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{current_user.username}") end diff --git a/app/controllers/import/gitea_controller.rb b/app/controllers/import/gitea_controller.rb index 5a4eef352b8..32c9da67e90 100644 --- a/app/controllers/import/gitea_controller.rb +++ b/app/controllers/import/gitea_controller.rb @@ -66,11 +66,13 @@ class Import::GiteaController < Import::GithubController override :client_options def client_options - { host: provider_url, api_version: 'v1' } + verified_url, provider_hostname = verify_blocked_uri + + { host: verified_url.scheme == 'https' ? provider_url : verified_url.to_s, api_version: 'v1', hostname: provider_hostname } end def verify_blocked_uri - Gitlab::UrlBlocker.validate!( + @verified_url_and_hostname ||= Gitlab::UrlBlocker.validate!( provider_url, allow_localhost: allow_local_requests?, allow_local_network: allow_local_requests?, diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 85e901eb3eb..c8c2dd1c7d6 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -47,6 +47,8 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_attributes[:password_automatically_set] = false unless @user.password_automatically_set || @user.valid_password?(user_params[:current_password]) + handle_invalid_current_password_attempt! + redirect_to edit_profile_password_path, alert: _('You must provide a valid current password') return end @@ -85,6 +87,12 @@ class Profiles::PasswordsController < Profiles::ApplicationController render_404 unless @user.allow_password_authentication? end + def handle_invalid_current_password_attempt! + Gitlab::AppLogger.info(message: 'Invalid current password when attempting to update user password', username: @user.username, ip: request.remote_ip) + + @user.increment_failed_attempts! + end + def user_params params.require(:user).permit(:current_password, :password, :password_confirmation) end diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 4533779339a..d1666530501 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -48,6 +48,11 @@ Doorkeeper.configure do # Issue access tokens with refresh token (disabled by default) use_refresh_token + # Forbids creating/updating applications with arbitrary scopes that are + # not in configuration, i.e. `default_scopes` or `optional_scopes`. + # (disabled by default) + enforce_configured_scopes + # Forces the usage of the HTTPS protocol in non-native redirect uris (enabled # by default in non-development environments). OAuth2 delegates security in # communication to the HTTPS protocol so it is wise to keep this enabled. diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 22fb2da8038..240987a1df4 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -137,6 +137,7 @@ The following items are **not** exported: - Webhooks - Any encrypted tokens - Merge Request Approvers +- Repository size limits NOTE: For more details on the specific data persisted in a project export, see the diff --git a/lib/gitlab/import_export/group/import_export.yml b/lib/gitlab/import_export/group/import_export.yml index 630f918a78b..f7ab1677001 100644 --- a/lib/gitlab/import_export/group/import_export.yml +++ b/lib/gitlab/import_export/group/import_export.yml @@ -37,6 +37,7 @@ excluded_attributes: - :trial_ends_on - :shared_runners_minute_limit - :extra_shared_runners_minutes_limit + - :repository_size_limit epics: - :state_id diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index 2772e7ef02b..27c5aaefb89 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -173,6 +173,7 @@ excluded_attributes: - :show_default_award_emojis - :services - :exported_protected_branches + - :repository_size_limit namespaces: - :runners_token - :runners_token_encrypted diff --git a/lib/gitlab/legacy_github_import/client.rb b/lib/gitlab/legacy_github_import/client.rb index 4482610523e..48a8e0ce6d7 100644 --- a/lib/gitlab/legacy_github_import/client.rb +++ b/lib/gitlab/legacy_github_import/client.rb @@ -8,9 +8,10 @@ module Gitlab attr_reader :access_token, :host, :api_version, :wait_for_rate_limit_reset - def initialize(access_token, host: nil, api_version: 'v3', wait_for_rate_limit_reset: true) + def initialize(access_token, host: nil, api_version: 'v3', wait_for_rate_limit_reset: true, hostname: nil) @access_token = access_token @host = host.to_s.sub(%r{/+\z}, '') + @hostname = hostname @api_version = api_version @users = {} @wait_for_rate_limit_reset = wait_for_rate_limit_reset @@ -28,7 +29,8 @@ module Gitlab # If there is no config, we're connecting to github.com and we # should verify ssl. connection_options: { - ssl: { verify: config ? config['verify_ssl'] : true } + ssl: { verify: config ? config['verify_ssl'] : true }, + headers: { host: @hostname }.compact } ) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 30dc6fc2176..4150ae55565 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -37284,6 +37284,9 @@ msgstr "" msgid "You are already a member of this %{member_source}." msgstr "" +msgid "You are already impersonating another user" +msgstr "" + msgid "You are an admin, which means granting access to %{client_name} will allow them to interact with GitLab as an admin as well. Proceed with caution." msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 6dc5c38cb76..4e8eb552210 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -809,5 +809,20 @@ RSpec.describe Admin::UsersController do expect(response).to have_gitlab_http_status(:not_found) end end + + context 'when impersonating an admin and attempting to impersonate again' do + let(:admin2) { create(:admin) } + + before do + post :impersonate, params: { id: admin2.username } + end + + it 'does not allow double impersonation', :aggregate_failures do + post :impersonate, params: { id: user.username } + + expect(flash[:alert]).to eq(_('You are already impersonating another user')) + expect(warden.user).to eq(admin2) + end + end end end diff --git a/spec/controllers/import/gitea_controller_spec.rb b/spec/controllers/import/gitea_controller_spec.rb index 3e4b159271a..568712d29cb 100644 --- a/spec/controllers/import/gitea_controller_spec.rb +++ b/spec/controllers/import/gitea_controller_spec.rb @@ -54,6 +54,48 @@ RSpec.describe Import::GiteaController do end end end + + context 'when DNS Rebinding protection is enabled' do + let(:token) { 'gitea token' } + + let(:ip_uri) { 'http://167.99.148.217' } + let(:uri) { 'try.gitea.io' } + let(:https_uri) { "https://#{uri}" } + let(:http_uri) { "http://#{uri}" } + + before do + session[:gitea_access_token] = token + + allow(Gitlab::UrlBlocker).to receive(:validate!).with(https_uri, anything).and_return([Addressable::URI.parse(https_uri), uri]) + allow(Gitlab::UrlBlocker).to receive(:validate!).with(http_uri, anything).and_return([Addressable::URI.parse(ip_uri), uri]) + + allow(Gitlab::LegacyGithubImport::Client).to receive(:new).and_return(double('Gitlab::LegacyGithubImport::Client', repos: [], orgs: [])) + end + + context 'when provided host url is using https' do + let(:host_url) { https_uri } + + it 'uses unchanged host url to send request to Gitea' do + expect(Gitlab::LegacyGithubImport::Client).to receive(:new).with(token, host: https_uri, api_version: 'v1', hostname: 'try.gitea.io') + + get :status, format: :json + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when provided host url is using http' do + let(:host_url) { http_uri } + + it 'uses changed host url to send request to Gitea' do + expect(Gitlab::LegacyGithubImport::Client).to receive(:new).with(token, host: 'http://167.99.148.217', api_version: 'v1', hostname: 'try.gitea.io') + + get :status, format: :json + + expect(response).to have_gitlab_http_status(:ok) + end + end + end end end diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb index f21ef324884..5bf3b4c48bf 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -98,6 +98,19 @@ RSpec.describe Oauth::ApplicationsController do end describe 'POST #create' do + let(:oauth_params) do + { + doorkeeper_application: { + name: 'foo', + redirect_uri: redirect_uri, + scopes: scopes + } + } + end + + let(:redirect_uri) { 'http://example.org' } + let(:scopes) { ['api'] } + subject { post :create, params: oauth_params } it 'creates an application' do @@ -116,38 +129,42 @@ RSpec.describe Oauth::ApplicationsController do expect(response).to redirect_to(profile_path) end - context 'redirect_uri' do + context 'when redirect_uri is invalid' do + let(:redirect_uri) { 'javascript://alert()' } + render_views it 'shows an error for a forbidden URI' do - invalid_uri_params = { - doorkeeper_application: { - name: 'foo', - redirect_uri: 'javascript://alert()', - scopes: ['api'] - } - } - - post :create, params: invalid_uri_params + subject expect(response.body).to include 'Redirect URI is forbidden by the server' + expect(response).to render_template('doorkeeper/applications/index') end end context 'when scopes are not present' do + let(:scopes) { [] } + render_views it 'shows an error for blank scopes' do - invalid_uri_params = { - doorkeeper_application: { - name: 'foo', - redirect_uri: 'http://example.org' - } - } - - post :create, params: invalid_uri_params + subject expect(response.body).to include 'Scopes can't be blank' + expect(response).to render_template('doorkeeper/applications/index') + end + end + + context 'when scopes are invalid' do + let(:scopes) { %w(api foo) } + + render_views + + it 'shows an error for invalid scopes' do + subject + + expect(response.body).to include 'Scopes doesn't match configured on the server.' + expect(response).to render_template('doorkeeper/applications/index') end end @@ -185,14 +202,4 @@ RSpec.describe Oauth::ApplicationsController do def disable_user_oauth allow(Gitlab::CurrentSettings.current_application_settings).to receive(:user_oauth_applications?).and_return(false) end - - def oauth_params - { - doorkeeper_application: { - name: 'foo', - redirect_uri: 'http://example.org', - scopes: ['api'] - } - } - end end diff --git a/spec/features/profiles/password_spec.rb b/spec/features/profiles/password_spec.rb index c9059395377..893dd2c76e0 100644 --- a/spec/features/profiles/password_spec.rb +++ b/spec/features/profiles/password_spec.rb @@ -78,40 +78,80 @@ RSpec.describe 'Profile > Password' do end end - context 'Change passowrd' do + context 'Change password' do + let(:new_password) { '22233344' } + before do sign_in(user) visit(edit_profile_password_path) end - it 'does not change user passowrd without old one' do - page.within '.update-password' do - fill_passwords('22233344', '22233344') + shared_examples 'user enters an incorrect current password' do + subject do + page.within '.update-password' do + fill_in 'user_current_password', with: user_current_password + fill_passwords(new_password, new_password) + end end - page.within '.flash-container' do - expect(page).to have_content 'You must provide a valid current password' - end - end + it 'handles the invalid password attempt, and prompts the user to try again', :aggregate_failures do + expect(Gitlab::AppLogger).to receive(:info) + .with(message: 'Invalid current password when attempting to update user password', username: user.username, ip: user.current_sign_in_ip) + + subject + + user.reload - it 'does not change password with invalid old password' do - page.within '.update-password' do - fill_in 'user_current_password', with: 'invalid' - fill_passwords('password', 'confirmation') + expect(user.failed_attempts).to eq(1) + expect(user.valid_password?(new_password)).to eq(false) + expect(current_path).to eq(edit_profile_password_path) + + page.within '.flash-container' do + expect(page).to have_content('You must provide a valid current password') + end end - page.within '.flash-container' do - expect(page).to have_content 'You must provide a valid current password' + it 'locks the user account when user passes the maximum attempts threshold', :aggregate_failures do + user.update!(failed_attempts: User.maximum_attempts.pred) + + subject + + expect(current_path).to eq(new_user_session_path) + + page.within '.flash-container' do + expect(page).to have_content('Your account is locked.') + end end end - it 'changes user password' do - page.within '.update-password' do - fill_in "user_current_password", with: user.password - fill_passwords('22233344', '22233344') + context 'when current password is blank' do + let(:user_current_password) { nil } + + it_behaves_like 'user enters an incorrect current password' + end + + context 'when current password is incorrect' do + let(:user_current_password) {'invalid' } + + it_behaves_like 'user enters an incorrect current password' + end + + context 'when the password reset is successful' do + subject do + page.within '.update-password' do + fill_in "user_current_password", with: user.password + fill_passwords(new_password, new_password) + end end - expect(current_path).to eq new_user_session_path + it 'changes the password, logs the user out and prompts them to sign in again', :aggregate_failures do + expect { subject }.to change { user.reload.valid_password?(new_password) }.to(true) + expect(current_path).to eq new_user_session_path + + page.within '.flash-container' do + expect(page).to have_content('Password was successfully updated. Please sign in again.') + end + end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 0be0da86651..b238a4733e3 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -546,7 +546,6 @@ Project: - disable_overriding_approvers_per_merge_request - merge_requests_ff_only_enabled - issues_template -- repository_size_limit - sync_time - service_desk_enabled - last_repository_updated_at diff --git a/spec/lib/gitlab/legacy_github_import/client_spec.rb b/spec/lib/gitlab/legacy_github_import/client_spec.rb index 0929b90d1f4..83ba5858d81 100644 --- a/spec/lib/gitlab/legacy_github_import/client_spec.rb +++ b/spec/lib/gitlab/legacy_github_import/client_spec.rb @@ -86,6 +86,15 @@ RSpec.describe Gitlab::LegacyGithubImport::Client do it 'builds a endpoint with the given options' do expect(client.api.api_endpoint).to eq 'https://try.gitea.io/api/v3/' end + + context 'and hostname' do + subject(:client) { described_class.new(token, host: 'https://167.99.148.217/', api_version: 'v1', hostname: 'try.gitea.io') } + + it 'builds a endpoint with the given options' do + expect(client.api.connection_options.dig(:headers, :host)).to eq 'try.gitea.io' + expect(client.api.api_endpoint).to eq 'https://167.99.148.217/api/v1/' + end + end end end |