diff options
30 files changed, 421 insertions, 97 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 95156209715..993cd0013f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,20 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 15.7.2 (2023-01-09) + +### Security (9 changes) + +- [Avoid regex with potential for poorly performing backtracking](gitlab-org/security/gitlab@1cb3b4904b25f1e47a40ddd48f3fdcb16bf02401) ([merge request](gitlab-org/security/gitlab!2987)) +- [Protect web-hook url variables after changing URL](gitlab-org/security/gitlab@58015aa49e63456094fcbf06a8fa739ac2a27f21) ([merge request](gitlab-org/security/gitlab!2976)) +- [Limit the size of user agent to reduce ReDos attack](gitlab-org/security/gitlab@ac3eb7cbf4a1701a499d0cbbbae568c55914c8c3) ([merge request](gitlab-org/security/gitlab!2985)) +- [Protect Sentry auth-token after changing URL](gitlab-org/security/gitlab@eba316d255caaa497e3a137aba5f262fd6272939) ([merge request](gitlab-org/security/gitlab!2983)) +- [Delete project specific licenses when license policy is deleted](gitlab-org/security/gitlab@a6bef9aee6175401408a12fe1439e775b84bc8cb) ([merge request](gitlab-org/security/gitlab!2969)) +- [Restrict user avatar availability based on visibility restrictions](gitlab-org/security/gitlab@9620a1bcae911c84112cc14da22711a344b89acf) ([merge request](gitlab-org/security/gitlab!2971)) +- [Policy change to read and destroy token without license for .com](gitlab-org/security/gitlab@5fcf1350fafe9a30f17fa19a3567620f10df1ccd) ([merge request](gitlab-org/security/gitlab!2968)) +- [Restrict Grafana API access on public projects](gitlab-org/security/gitlab@3274a7fbeabc04f9db69ffd052e0e77a6b71a7f8) ([merge request](gitlab-org/security/gitlab!2960)) +- [Fix "Race condition enables verified email forgery"](gitlab-org/security/gitlab@c3e6fede4230a3ce0fc1d0e4c82f5f3ede41f663) ([merge request](gitlab-org/security/gitlab!2966)) + ## 15.7.1 (2023-01-05) ### Fixed (2 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index d1a0eb1c34a..0b363a89973 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -15.7.1
\ No newline at end of file +15.7.2
\ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index d1a0eb1c34a..0b363a89973 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -15.7.1
\ No newline at end of file +15.7.2
\ No newline at end of file @@ -1 +1 @@ -15.7.1
\ No newline at end of file +15.7.2
\ No newline at end of file diff --git a/app/controllers/concerns/page_limiter.rb b/app/controllers/concerns/page_limiter.rb index 1d044a41899..97df540d31b 100644 --- a/app/controllers/concerns/page_limiter.rb +++ b/app/controllers/concerns/page_limiter.rb @@ -58,7 +58,7 @@ module PageLimiter # Record the page limit being hit in Prometheus def record_page_limit_interception - dd = DeviceDetector.new(request.user_agent) + dd = Gitlab::SafeDeviceDetector.new(request.user_agent) Gitlab::Metrics.counter(:gitlab_page_out_of_bounds, controller: params[:controller], diff --git a/app/controllers/projects/grafana_api_controller.rb b/app/controllers/projects/grafana_api_controller.rb index d5099367873..9cd511f6a11 100644 --- a/app/controllers/projects/grafana_api_controller.rb +++ b/app/controllers/projects/grafana_api_controller.rb @@ -4,6 +4,8 @@ class Projects::GrafanaApiController < Projects::ApplicationController include RenderServiceResults include MetricsDashboard + before_action :authorize_read_grafana!, only: :proxy + feature_category :metrics urgency :low diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 09419a4589d..66f715f32af 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -52,6 +52,8 @@ class UploadsController < ApplicationController # access to itself when a secret is given. # For instance, user avatars are readable by anyone, # while temporary, user snippet uploads are not. + return false if !current_user && public_visibility_restricted? + !secret? || can?(current_user, :update_user, model) when Appearance true diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb index e3e2f423da3..c38d69df8e4 100644 --- a/app/helpers/submodule_helper.rb +++ b/app/helpers/submodule_helper.rb @@ -17,40 +17,26 @@ module SubmoduleHelper url = File.join(Gitlab.config.gitlab.url, repository.project.full_path) end - if url =~ %r{([^/:]+)/([^/]+(?:\.git)?)\Z} - namespace = Regexp.last_match(1) - project = Regexp.last_match(2) - gitlab_hosts = [Gitlab.config.gitlab.url, - Gitlab.config.gitlab_shell.ssh_path_prefix] - - gitlab_hosts.each do |host| - if url.start_with?(host) - namespace, _, project = url.sub(host, '').rpartition('/') - break - end - end - - namespace.delete_prefix!('/') - project.rstrip! - project.delete_suffix!('.git') - - if self_url?(url, namespace, project) - [ - url_helpers.namespace_project_path(namespace, project), - url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id), - (url_helpers.namespace_project_compare_path(namespace, project, to: submodule_item_id, from: old_submodule_item_id) if old_submodule_item_id) - ] - elsif relative_self_url?(url) - relative_self_links(url, submodule_item_id, old_submodule_item_id, repository.project) - elsif gist_github_dot_com_url?(url) - gist_github_com_tree_links(namespace, project, submodule_item_id) - elsif github_dot_com_url?(url) - github_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) - elsif gitlab_dot_com_url?(url) - gitlab_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) - else - [sanitize_submodule_url(url), nil, nil] - end + namespace, project = extract_namespace_project(url) + + if namespace.blank? || project.blank? + return [sanitize_submodule_url(url), nil, nil] + end + + if self_url?(url, namespace, project) + [ + url_helpers.namespace_project_path(namespace, project), + url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id), + (url_helpers.namespace_project_compare_path(namespace, project, to: submodule_item_id, from: old_submodule_item_id) if old_submodule_item_id) + ] + elsif relative_self_url?(url) + relative_self_links(url, submodule_item_id, old_submodule_item_id, repository.project) + elsif gist_github_dot_com_url?(url) + gist_github_com_tree_links(namespace, project, submodule_item_id) + elsif github_dot_com_url?(url) + github_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) + elsif gitlab_dot_com_url?(url) + gitlab_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id) else [sanitize_submodule_url(url), nil, nil] end @@ -58,6 +44,30 @@ module SubmoduleHelper protected + def extract_namespace_project(url) + namespace_fragment, _, project = url.rpartition('/') + namespace = namespace_fragment.rpartition(%r{[:/]}).last + + return [nil, nil] unless project.present? && namespace.present? + + gitlab_hosts = [Gitlab.config.gitlab.url, + Gitlab.config.gitlab_shell.ssh_path_prefix] + + matching_host = gitlab_hosts.find do |host| + url.start_with?(host) + end + + if matching_host + namespace, _, project = url.delete_prefix(matching_host).rpartition('/') + end + + namespace.delete_prefix!('/') + project.rstrip! + project.delete_suffix!('.git') + + [namespace, project] + end + def gist_github_dot_com_url?(url) url =~ %r{gist\.github\.com[/:][^/]+/[^/]+\Z} end diff --git a/app/models/active_session.rb b/app/models/active_session.rb index b16c4a2b353..2d1dec1977d 100644 --- a/app/models/active_session.rb +++ b/app/models/active_session.rb @@ -67,7 +67,7 @@ class ActiveSession def self.set(user, request) Gitlab::Redis::Sessions.with do |redis| session_private_id = request.session.id.private_id - client = DeviceDetector.new(request.user_agent) + client = Gitlab::SafeDeviceDetector.new(request.user_agent) timestamp = Time.current expiry = Settings.gitlab['session_expire_delay'] * 60 diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 189291a38ec..49418cda3ac 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -41,6 +41,7 @@ class WebHook < ApplicationRecord after_initialize :initialize_url_variables before_validation :reset_token + before_validation :reset_url_variables, unless: ->(hook) { hook.is_a?(ServiceHook) } before_validation :set_branch_filter_nil, if: :branch_filter_strategy_all_branches? validates :push_events_branch_filter, untrusted_regexp: true, if: :branch_filter_strategy_regex? validates :push_events_branch_filter, "web_hooks/wildcard_branch_filter": true, if: :branch_filter_strategy_wildcard? @@ -213,6 +214,10 @@ class WebHook < ApplicationRecord self.token = nil if url_changed? && !encrypted_token_changed? end + def reset_url_variables + self.url_variables = {} if url_changed? && !encrypted_url_variables_changed? + end + def next_failure_count recent_failures.succ.clamp(1, MAX_FAILURES) end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 858c145de3f..8eea995529c 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -273,6 +273,9 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy rule { can?(:admin_group) & resource_access_token_feature_available }.policy do enable :read_resource_access_tokens enable :destroy_resource_access_tokens + end + + rule { can?(:admin_group) & resource_access_token_creation_allowed }.policy do enable :admin_setting_to_allow_project_access_token_creation end @@ -338,12 +341,16 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy true end + def resource_access_token_create_feature_available? + true + end + def can_read_group_member? !(@subject.private? && access_level == GroupMember::NO_ACCESS) end def resource_access_token_creation_allowed? - resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? + resource_access_token_create_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? end def valid_dependency_proxy_deploy_token diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 7f67e80e432..fd3dbb54d57 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -157,7 +157,9 @@ class ProjectPolicy < BasePolicy condition(:service_desk_enabled) { @subject.service_desk_enabled? } with_scope :subject - condition(:resource_access_token_feature_available) { resource_access_token_feature_available? } + condition(:resource_access_token_feature_available) do + resource_access_token_feature_available? + end condition(:resource_access_token_creation_allowed) { resource_access_token_creation_allowed? } # We aren't checking `:read_issue` or `:read_merge_request` in this case @@ -308,6 +310,8 @@ class ProjectPolicy < BasePolicy rule { guest & can?(:download_code) }.enable :build_download_code rule { guest & can?(:read_container_image) }.enable :build_read_container_image + rule { guest & ~public_project }.enable :read_grafana + rule { can?(:reporter_access) }.policy do enable :admin_issue_board enable :download_code @@ -340,6 +344,7 @@ class ProjectPolicy < BasePolicy enable :read_package enable :read_product_analytics enable :read_ci_cd_analytics + enable :read_grafana end # We define `:public_user_access` separately because there are cases in gitlab-ee @@ -919,12 +924,16 @@ class ProjectPolicy < BasePolicy true end + def resource_access_token_create_feature_available? + true + end + def resource_access_token_creation_allowed? group = project.group return true unless group # always enable for projects in personal namespaces - resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? + resource_access_token_create_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? end def project diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb index 2f23d47029c..d52306ef805 100644 --- a/app/services/error_tracking/list_projects_service.rb +++ b/app/services/error_tracking/list_projects_service.rb @@ -2,6 +2,8 @@ module ErrorTracking class ListProjectsService < ErrorTracking::BaseService + MASKED_TOKEN_REGEX = /\A\*+\z/.freeze + private def perform @@ -20,23 +22,31 @@ module ErrorTracking def project_error_tracking_setting (super || project.build_error_tracking_setting).tap do |setting| + url_changed = !setting.api_url&.start_with?(params[:api_host]) + setting.api_url = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from( api_host: params[:api_host], organization_slug: 'org', project_slug: 'proj' ) - setting.token = token(setting) + setting.token = token(setting, url_changed) setting.enabled = true end end strong_memoize_attr :project_error_tracking_setting - def token(setting) + def token(setting, url_changed) + return if url_changed && masked_token? + # Use param token if not masked, otherwise use database token - return params[:token] unless /\A\*+\z/.match?(params[:token]) + return params[:token] unless masked_token? setting.token end + + def masked_token? + MASKED_TOKEN_REGEX.match?(params[:token]) + end end end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index cb2711b6fee..96018db5974 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -31,6 +31,7 @@ module Users assign_identity build_canonical_email + reset_unconfirmed_email if @user.save(validate: validate) && update_status notify_success(user_exists) @@ -64,6 +65,13 @@ module Users Users::UpdateCanonicalEmailService.new(user: @user).execute end + def reset_unconfirmed_email + return unless @user.persisted? + return unless @user.email_changed? + + @user.update_column(:unconfirmed_email, nil) + end + def update_status return true unless @status_params diff --git a/app/views/devise/mailer/_confirmation_instructions_account.html.haml b/app/views/devise/mailer/_confirmation_instructions_account.html.haml index 9d469ff6e7b..c1655818770 100644 --- a/app/views/devise/mailer/_confirmation_instructions_account.html.haml +++ b/app/views/devise/mailer/_confirmation_instructions_account.html.haml @@ -1,7 +1,7 @@ - confirmation_link = confirmation_url(@resource, confirmation_token: @token) - if @resource.unconfirmed_email.present? || !@resource.created_recently? #content - = email_default_heading(@resource.unconfirmed_email || @resource.email) + = email_default_heading(@email) %p= _('Click the link below to confirm your email address.') #cta = link_to _('Confirm your email address'), confirmation_link diff --git a/app/views/devise/mailer/_confirmation_instructions_account.text.erb b/app/views/devise/mailer/_confirmation_instructions_account.text.erb index e6da78e3a3d..7e4f38885f6 100644 --- a/app/views/devise/mailer/_confirmation_instructions_account.text.erb +++ b/app/views/devise/mailer/_confirmation_instructions_account.text.erb @@ -1,5 +1,5 @@ <% if @resource.unconfirmed_email.present? || !@resource.created_recently? %> -<%= @resource.unconfirmed_email || @resource.email %>, +<%= @email %>, <%= _('Use the link below to confirm your email address.') %> <% else %> <% if Gitlab.com? %> diff --git a/doc/operations/metrics/embed_grafana.md b/doc/operations/metrics/embed_grafana.md index b307aa5fa32..43a7447a978 100644 --- a/doc/operations/metrics/embed_grafana.md +++ b/doc/operations/metrics/embed_grafana.md @@ -55,9 +55,10 @@ To set up the Grafana API in Grafana: 1. Select **Save Changes**. NOTE: -If the Grafana integration is enabled, any user with read access to the GitLab -project can query metrics from the Prometheus instance. All requests proxied -through GitLab are authenticated with the same Grafana Administrator API token. +If the Grafana integration is enabled, users with the Reporter role on public +projects and the Guest role on non-public projects can query metrics from the +Prometheus instance. All requests proxied through GitLab are authenticated with +the same Grafana Administrator API token. ### Generate a link to a panel diff --git a/lib/gitlab/safe_device_detector.rb b/lib/gitlab/safe_device_detector.rb new file mode 100644 index 00000000000..ba1010ae5e4 --- /dev/null +++ b/lib/gitlab/safe_device_detector.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +# rubocop:disable Gitlab/NamespacedClass +require 'device_detector' + +module Gitlab + class SafeDeviceDetector < ::DeviceDetector + USER_AGENT_MAX_SIZE = 1024 + + def initialize(user_agent) + super(user_agent) + @user_agent = user_agent && user_agent[0..USER_AGENT_MAX_SIZE] + end + end +end + +# rubocop:enable Gitlab/NamespacedClass diff --git a/spec/controllers/projects/grafana_api_controller_spec.rb b/spec/controllers/projects/grafana_api_controller_spec.rb index 2e25b0271ce..90ab49f9467 100644 --- a/spec/controllers/projects/grafana_api_controller_spec.rb +++ b/spec/controllers/projects/grafana_api_controller_spec.rb @@ -2,13 +2,20 @@ require 'spec_helper' -RSpec.describe Projects::GrafanaApiController do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } +RSpec.describe Projects::GrafanaApiController, feature_category: :metrics do + let_it_be(:project) { create(:project, :public) } + let_it_be(:reporter) { create(:user) } + let_it_be(:guest) { create(:user) } + let(:anonymous) { nil } + let(:user) { reporter } + + before_all do + project.add_reporter(reporter) + project.add_guest(guest) + end before do - project.add_reporter(user) - sign_in(user) + sign_in(user) if user end describe 'GET #proxy' do @@ -41,6 +48,39 @@ RSpec.describe Projects::GrafanaApiController do end end + shared_examples_for 'accessible' do + let(:service_result) { nil } + + it 'returns non erroneous response' do + get :proxy, params: params + + # We don't care about the specific code as long it's not an error. + expect(response).to have_gitlab_http_status(:no_content) + end + end + + shared_examples_for 'not accessible' do + let(:service_result) { nil } + + it 'returns 404 Not found' do + get :proxy, params: params + + expect(response).to have_gitlab_http_status(:not_found) + expect(Grafana::ProxyService).not_to have_received(:new) + end + end + + shared_examples_for 'login required' do + let(:service_result) { nil } + + it 'redirects to login page' do + get :proxy, params: params + + expect(response).to redirect_to(new_user_session_path) + expect(Grafana::ProxyService).not_to have_received(:new) + end + end + context 'with a successful result' do let(:service_result) { { status: :success, body: '{}' } } @@ -96,6 +136,38 @@ RSpec.describe Projects::GrafanaApiController do it_behaves_like 'error response', :bad_request end end + + context 'as guest' do + let(:user) { guest } + + it_behaves_like 'not accessible' + end + + context 'as anonymous' do + let(:user) { anonymous } + + it_behaves_like 'not accessible' + end + + context 'on a private project' do + let_it_be(:project) { create(:project, :private) } + + before_all do + project.add_guest(guest) + end + + context 'as anonymous' do + let(:user) { anonymous } + + it_behaves_like 'login required' + end + + context 'as guest' do + let(:user) { guest } + + it_behaves_like 'accessible' + end + end end describe 'GET #metrics_dashboard' do diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index e128db8d1c1..3e9c56d3274 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -268,17 +268,35 @@ RSpec.describe UploadsController do end context "when not signed in" do - it "responds with status 200" do - get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" } + context "when restricted visibility level is not set to public" do + before do + stub_application_setting(restricted_visibility_levels: []) + end - expect(response).to have_gitlab_http_status(:ok) + it "responds with status 200" do + get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" } + + expect(response).to have_gitlab_http_status(:ok) + end + + it_behaves_like 'content publicly cached' do + subject do + get :show, params: { model: 'user', mounted_as: 'avatar', id: user.id, filename: 'dk.png' } + + response + end + end end - it_behaves_like 'content publicly cached' do - subject do - get :show, params: { model: 'user', mounted_as: 'avatar', id: user.id, filename: 'dk.png' } + context "when restricted visibility level is set to public" do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end - response + it "responds with status 401" do + get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" } + + expect(response).to have_gitlab_http_status(:unauthorized) end end end diff --git a/spec/helpers/submodule_helper_spec.rb b/spec/helpers/submodule_helper_spec.rb index a419b6b9c84..2e8304e8b49 100644 --- a/spec/helpers/submodule_helper_spec.rb +++ b/spec/helpers/submodule_helper_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe SubmoduleHelper do +RSpec.describe SubmoduleHelper, feature_category: :source_code_management do include RepoHelpers let(:submodule_item) { double(id: 'hash', path: 'rack') } diff --git a/spec/lib/gitlab/safe_device_detector_spec.rb b/spec/lib/gitlab/safe_device_detector_spec.rb new file mode 100644 index 00000000000..c37dc1e1c7e --- /dev/null +++ b/spec/lib/gitlab/safe_device_detector_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'device_detector' +require_relative '../../../lib/gitlab/safe_device_detector' + +RSpec.describe Gitlab::SafeDeviceDetector, feature_category: :authentication_and_authorization do + it 'retains the behavior for normal user agents' do + chrome_user_agent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 \ + (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36" + + expect(described_class.new(chrome_user_agent).user_agent).to be_eql(chrome_user_agent) + expect(described_class.new(chrome_user_agent).name).to be_eql('Chrome') + end + + it 'truncates big user agents' do + big_user_agent = "chrome #{'abc' * 1024}" + expect(described_class.new(big_user_agent).user_agent).not_to be_eql(big_user_agent) + end +end diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb index 360eb827927..b8e768b7a5f 100644 --- a/spec/mailers/devise_mailer_spec.rb +++ b/spec/mailers/devise_mailer_spec.rb @@ -11,7 +11,7 @@ RSpec.describe DeviseMailer do subject { described_class.confirmation_instructions(user, 'faketoken', {}) } context "when confirming a new account" do - let(:user) { build(:user, created_at: 1.minute.ago, unconfirmed_email: nil) } + let(:user) { create(:user, created_at: 1.minute.ago) } it "shows the expected text" do expect(subject.body.encoded).to have_text "Welcome" @@ -20,7 +20,13 @@ RSpec.describe DeviseMailer do end context "when confirming the unconfirmed_email" do - let(:user) { build(:user, unconfirmed_email: 'jdoe@example.com') } + subject { described_class.confirmation_instructions(user, user.confirmation_token, { to: user.unconfirmed_email }) } + + let(:user) { create(:user) } + + before do + user.update!(email: 'unconfirmed-email@example.com') + end it "shows the expected text" do expect(subject.body.encoded).not_to have_text "Welcome" @@ -30,7 +36,7 @@ RSpec.describe DeviseMailer do end context "when re-confirming the primary email after a security issue" do - let(:user) { build(:user, created_at: 10.days.ago, unconfirmed_email: nil) } + let(:user) { create(:user, created_at: Devise.confirm_within.ago) } it "shows the expected text" do expect(subject.body.encoded).not_to have_text "Welcome" diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 994d5688808..75ff917c036 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe WebHook do +RSpec.describe WebHook, feature_category: :integrations do include AfterNextHelpers let_it_be(:project) { create(:project) } @@ -225,6 +225,32 @@ RSpec.describe WebHook do end end + describe 'before_validation :reset_url_variables' do + subject(:hook) { build_stubbed(:project_hook, :url_variables, project: project, url: 'http://example.com/{abc}') } + + it 'resets url variables if url changed' do + hook.url = 'http://example.com/new-hook' + + expect(hook).to be_valid + expect(hook.url_variables).to eq({}) + end + + it 'resets url variables if url is changed but url variables stayed the same' do + hook.url = 'http://test.example.com/{abc}' + + expect(hook).not_to be_valid + expect(hook.url_variables).to eq({}) + end + + it 'does not reset url variables if both url and url variables are changed' do + hook.url = 'http://example.com/{one}/{two}' + hook.url_variables = { 'one' => 'foo', 'two' => 'bar' } + + expect(hook).to be_valid + expect(hook.url_variables).to eq({ 'one' => 'foo', 'two' => 'bar' }) + end + end + it "only consider these branch filter strategies are valid" do expected_valid_types = %w[all_branches regex wildcard] expect(described_class.branch_filter_strategies.keys).to contain_exactly(*expected_valid_types) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4a66af4ddf1..4dbcc68af19 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -311,6 +311,34 @@ RSpec.describe User do end end end + + describe 'confirmation instructions for unconfirmed email' do + let(:unconfirmed_email) { 'first-unconfirmed-email@example.com' } + let(:another_unconfirmed_email) { 'another-unconfirmed-email@example.com' } + + context 'when email is changed to another before performing the job that sends confirmation instructions for previous email change request' do + it "mentions the recipient's email in the message body", :aggregate_failures do + same_user = User.find(user.id) + same_user.update!(email: unconfirmed_email) + + user.update!(email: another_unconfirmed_email) + + perform_enqueued_jobs + + confirmation_instructions_for_unconfirmed_email = ActionMailer::Base.deliveries.find do |message| + message.subject == 'Confirmation instructions' && message.to.include?(unconfirmed_email) + end + expect(confirmation_instructions_for_unconfirmed_email.html_part.body.encoded).to match same_user.unconfirmed_email + expect(confirmation_instructions_for_unconfirmed_email.text_part.body.encoded).to match same_user.unconfirmed_email + + confirmation_instructions_for_another_unconfirmed_email = ActionMailer::Base.deliveries.find do |message| + message.subject == 'Confirmation instructions' && message.to.include?(another_unconfirmed_email) + end + expect(confirmation_instructions_for_another_unconfirmed_email.html_part.body.encoded).to match user.unconfirmed_email + expect(confirmation_instructions_for_another_unconfirmed_email.text_part.body.encoded).to match user.unconfirmed_email + end + end + end end describe 'validations' do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 9b2d10283f1..e370f536519 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -668,6 +668,35 @@ RSpec.describe ProjectPolicy do end end + describe 'read_grafana', feature_category: :metrics do + using RSpec::Parameterized::TableSyntax + + let(:policy) { :read_grafana } + + where(:project_visibility, :role, :allowed) do + :public | :anonymous | false + :public | :guest | false + :public | :reporter | true + :internal | :anonymous | false + :internal | :guest | true + :internal | :reporter | true + :private | :anonymous | false + :private | :guest | true + :private | :reporter | true + end + + with_them do + let(:current_user) { public_send(role) } + let(:project) { public_send("#{project_visibility}_project") } + + if params[:allowed] + it { is_expected.to be_allowed(policy) } + else + it { is_expected.not_to be_allowed(policy) } + end + end + end + describe 'update_max_artifacts_size' do context 'when no user' do let(:current_user) { anonymous } diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index ce391bd1ca0..8408adcc21d 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::ListProjectsService do +RSpec.describe ErrorTracking::ListProjectsService, feature_category: :integrations do let_it_be(:user) { create(:user) } let_it_be(:project, reload: true) { create(:project) } @@ -51,15 +51,33 @@ RSpec.describe ErrorTracking::ListProjectsService do end context 'masked param token' do - let(:params) { ActionController::Parameters.new(token: "*********", api_host: new_api_host) } + let(:params) { ActionController::Parameters.new(token: "*********", api_host: api_host) } - before do - expect(error_tracking_setting).to receive(:list_sentry_projects) + context 'with the current api host' do + let(:api_host) { 'https://sentrytest.gitlab.com' } + + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) .and_return({ projects: [] }) + end + + it 'uses database token' do + expect { subject.execute }.not_to change { error_tracking_setting.token } + end end - it 'uses database token' do - expect { subject.execute }.not_to change { error_tracking_setting.token } + context 'with a new api host' do + let(:api_host) { new_api_host } + + it 'returns an error' do + expect(result[:message]).to start_with('Token is a required field') + expect(error_tracking_setting).not_to be_valid + expect(error_tracking_setting).not_to receive(:list_sentry_projects) + end + + it 'resets the token' do + expect { subject.execute }.to change { error_tracking_setting.token }.from(token).to(nil) + end end end diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index 411cd7316d8..f4ea757f81a 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -77,6 +77,34 @@ RSpec.describe Users::UpdateService do subject end + context 'when race condition' do + # See https://gitlab.com/gitlab-org/gitlab/-/issues/382957 + it 'updates email for stale user', :aggregate_failures do + unconfirmed_email = 'unconfirmed-email-user-has-access-to@example.com' + forgery_email = 'forgery@example.com' + + user.update!(email: unconfirmed_email) + + stale_user = User.find(user.id) + + service1 = described_class.new(stale_user, { email: unconfirmed_email }.merge(user: stale_user)) + + service2 = described_class.new(user, { email: forgery_email }.merge(user: user)) + + service2.execute + reloaded_user = User.find(user.id) + expect(reloaded_user.unconfirmed_email).to eq(forgery_email) + expect(stale_user.confirmation_token).not_to eq(user.confirmation_token) + expect(reloaded_user.confirmation_token).to eq(user.confirmation_token) + + service1.execute + reloaded_user = User.find(user.id) + expect(reloaded_user.unconfirmed_email).to eq(unconfirmed_email) + expect(stale_user.confirmation_token).not_to eq(user.confirmation_token) + expect(reloaded_user.confirmation_token).to eq(stale_user.confirmation_token) + end + end + context 'when check_password is true' do def update_user(user, opts) described_class.new(user, opts.merge(user: user)).execute(check_password: true) @@ -139,9 +167,24 @@ RSpec.describe Users::UpdateService do update_user(user, job_title: 'supreme leader of the universe') end.not_to change { user.user_canonical_email } end + + it 'does not reset unconfirmed email' do + unconfirmed_email = 'unconfirmed-email@example.com' + user.update!(email: unconfirmed_email) + + expect do + update_user(user, job_title: 'supreme leader of the universe') + end.not_to change { user.unconfirmed_email } + end end end + it 'does not try to reset unconfirmed email for a new user' do + expect do + update_user(build(:user), job_title: 'supreme leader of the universe') + end.not_to raise_error + end + def update_user(user, opts) described_class.new(user, opts.merge(user: user)).execute end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index c081b20d95f..4b925a058e7 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -129,7 +129,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state context 'there is userinfo' do before do - project_hook.update!(url: 'http://{one}:{two}@example.com') + project_hook.update!( + url: 'http://{one}:{two}@example.com', + url_variables: { 'one' => 'a', 'two' => 'b' } + ) stub_full_request('http://example.com', method: :post) end diff --git a/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb b/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb index 337ad024fc0..cc91b73449a 100644 --- a/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb +++ b/spec/support/shared_examples/policies/resource_access_token_shared_examples.rb @@ -71,26 +71,3 @@ RSpec.shared_examples 'Self-managed Core resource access tokens' do end end end - -RSpec.shared_examples 'GitLab.com Core resource access tokens' do - before do - allow(::Gitlab).to receive(:com?).and_return(true) - stub_ee_application_setting(should_check_namespace_plan: true) - end - - context 'with owner access' do - let(:current_user) { owner } - - context 'create resource access tokens' do - it { is_expected.not_to be_allowed(:create_resource_access_tokens) } - end - - context 'read resource access tokens' do - it { is_expected.not_to be_allowed(:read_resource_access_tokens) } - end - - context 'destroy resource access tokens' do - it { is_expected.not_to be_allowed(:destroy_resource_access_tokens) } - end - end -end |