Validate that sensitive emails are sent to a single recipient

Nick Malcolmrequested to merge
442831-mass-assignment-protection into master

What does this MR do and why?

Validate that sensitive emails are sent to a single recipient

This is a defense-in-depth measure. It introduces a "time of use" check to validate that, when appropriate, an email is only being sent to a single email address. This should already be the case but, in the event a bug is introduced further up the stack in the future, this should prevent potential security vulnerabilities1.

This check is performed for:

  • All Devise emails2:
    • confirmation_instructions
    • reset_password_instructions
    • unlock_instructions
    • email_changed
    • password_change
  • GitLab's extra emails in DeviseMailer:
    • password_change_by_admin
    • user_admin_approval
    • reset_password_instructions
    • email_changed
  • Identity Verification3 emails:
    • verification_instructions_email
    • confirmation_instructions_email

None of these emails should be sent to more than one person, or have other recipients in the CC or BCC fields. An exception will be raised if this occurs.

As many of these emails are sent in a delayed manner, the exceptions will be handled by ActiveJob.

References:

  1. Account Takeover via Password Reset without use... (#436084 - closed)
  2. GitLab's DeviseMailer is set as Devise's mailer in config/initializers/8_devise.rb:22
  3. https://docs.gitlab.com/ee/security/identity_verification.html
  4. https://gitlab.com/gitlab-org/gitlab/-/issues/442831+

Will this break things?

I'm pretty confident it wont:

  • None of the methods above involve cc or bccing anyone
  • All of the use of devise mailers pass either a user (which Devise automagically gets the user.email out of), or passes to: user.email.
Click to expand grep 
% rg "cc:" -g '!spec' -g '*.rb'
lib/gitlab/email/handler/service_desk_handler.rb
127:                cc: mail.cc

ee/app/models/concerns/identity_verifiable.rb
144:    # { email: true, phone: false, cc: false }, then prerequisite methods state

ee/app/mailers/ci_minutes_usage_mailer.rb
12:      bcc: recipients,
22:      bcc: recipients,

ee/app/mailers/emails/namespace_storage_usage_mailer.rb
23:        bcc: recipients,
40:        bcc: recipients,

ee/app/mailers/license_mailer.rb
14:      bcc: recipients,

app/controllers/projects/service_desk_controller.rb
52:      add_external_participants_from_cc: service_desk_settings&.add_external_participants_from_cc
% rg "DeviseMailer" -g '!spec' -g '*.rb'
config/initializers/8_devise.rb
22:  config.mailer = "DeviseMailer"

app/services/users/approve_service.rb
18:        DeviseMailer.user_admin_approval(user).deliver_later

app/mailers/previews/devise_mailer_preview.rb
3:class DeviseMailerPreview < ActionMailer::Preview
5:    DeviseMailer.confirmation_instructions(unsaved_user, 'faketoken', {})
12:    DeviseMailer.confirmation_instructions(user, 'faketoken', {})
16:    DeviseMailer.reset_password_instructions(unsaved_user, 'faketoken', {})
20:    DeviseMailer.unlock_instructions(unsaved_user, 'faketoken', {})
24:    DeviseMailer.password_change(unsaved_user, {})
28:    DeviseMailer.user_admin_approval(unsaved_user, {})
32:    DeviseMailer.email_changed(unsaved_user, {})

app/mailers/devise_mailer.rb
3:class DeviseMailer < Devise::Mailer
% rg 'confirmation_instructions|reset_password_instructions|unlock_instructions|email_changed|password_change|password_change_by_admin|user_admin_approval|reset_password_instructions|email_changed|verification_instructions_email|confirmation_instructions_email' -g '!spec' -g '*.rb'
vendor/gems/attr_encrypted/test/active_record_test.rb
188:    refute person.email_changed?
190:    refute person.email_changed?
192:    assert person.email_changed?
194:    assert person.email_changed?

app/services/users/approve_service.rb
15:        # Please see Devise's implementation of `resend_confirmation_instructions` for detail.
16:        user.resend_confirmation_instructions
18:        DeviseMailer.user_admin_approval(user).deliver_later

app/services/users/update_service.rb
64:      return unless @user.email_changed?
71:      return unless @user.email_changed?

app/services/emails/confirm_service.rb
6:      email.resend_confirmation_instructions

app/controllers/registrations_controller.rb
47:      send_custom_confirmation_instructions
320:  def send_custom_confirmation_instructions

app/controllers/concerns/authenticates_with_two_factor.rb
45:    return handle_changed_user(user) if user_password_changed?(user)
154:  def user_password_changed?(user)

app/controllers/concerns/verifies_with_email.rb
99:    send_verification_instructions_email(user, raw_token)
102:  def send_verification_instructions_email(user, token)
106:    Notify.verification_instructions_email(email, token: token).deliver_later

app/controllers/confirmations_controller.rb
20:  def after_resending_confirmation_instructions_path_for(resource)

app/controllers/profiles/emails_controller.rb
4:  before_action :find_email, only: [:destroy, :resend_confirmation_instructions]
8:    only: [:resend_confirmation_instructions]
36:  def resend_confirmation_instructions

app/controllers/user_settings/passwords_controller.rb
60:      current_user.send_reset_password_instructions

app/controllers/passwords_controller.rb
65:    redirect_to after_sending_reset_password_instructions_path_for(resource_name),

app/models/user.rb
326:  validate  :check_password_weakness, if: :encrypted_password_changed?
331:  validate :unique_email, if: :email_changed?
332:  validate :notification_email_verified, if: :notification_email_changed?
333:  validate :public_email_verified, if: :public_email_changed?
334:  validate :commit_email_verified, if: :commit_email_changed?
335:  validate :email_allowed_by_restrictions?, if: ->(user) { user.new_record? ? !user.created_by_id : user.email_changed? }
354:  before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
355:  before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? }
1589:    !email_changed? || new_record?

app/models/concerns/admin_changed_password_notifier.rb
40:    skip_password_change_notification! # skip sending the default Devise 'password changed' notification
47:    send_devise_notification(:password_change_by_admin)
55:    self.class.send_password_change_notification && saved_change_to_encrypted_password? &&

app/models/concerns/recoverable_by_any_email.rb
10:    def send_reset_password_instructions(attributes = {})
19:      recoverable.send_reset_password_instructions(to: email.email)
24:  def send_reset_password_instructions(opts = {})
27:    send_reset_password_instructions_notification(token, opts)
39:  def send_reset_password_instructions_notification(token, opts = {})
40:    send_devise_notification(:reset_password_instructions, token, opts)

app/models/notification_setting.rb
17:  validate :notification_email_verified, if: :notification_email_changed?

app/models/integrations/jira.rb
676:      api_url_changed? || url_changed? || username_changed? || password_changed?

app/models/email.rb
11:  validate :unique_email, if: ->(email) { email.email_changed? }

app/mailers/previews/notify_preview.rb
330:  def verification_instructions_email
331:    Notify.verification_instructions_email(user.email, token: '123456').message

app/mailers/previews/devise_mailer_preview.rb
4:  def confirmation_instructions_for_signup
5:    DeviseMailer.confirmation_instructions(unsaved_user, 'faketoken', {})
8:  def confirmation_instructions_for_new_email
12:    DeviseMailer.confirmation_instructions(user, 'faketoken', {})
15:  def reset_password_instructions
16:    DeviseMailer.reset_password_instructions(unsaved_user, 'faketoken', {})
19:  def unlock_instructions
20:    DeviseMailer.unlock_instructions(unsaved_user, 'faketoken', {})
23:  def password_change
24:    DeviseMailer.password_change(unsaved_user, {})
27:  def user_admin_approval
28:    DeviseMailer.user_admin_approval(unsaved_user, {})
31:  def email_changed
32:    DeviseMailer.email_changed(unsaved_user, {})

app/mailers/devise_mailer.rb
21:  def password_change_by_admin(record, opts = {})
22:    devise_mail(record, :password_change_by_admin, opts)
25:  def user_admin_approval(record, opts = {})
26:    devise_mail(record, :user_admin_approval, opts)
29:  def reset_password_instructions(record, token, opts = {})
34:  def email_changed(record, opts = {})
36:      devise_mail(record, :email_changed_gitlab_com, opts)
38:      devise_mail(record, :email_changed, opts)

config/initializers/8_devise.rb
128:  config.send_password_change_notification = true
131:  config.send_email_changed_notification = true

config/routes/profile.rb
61:        put :resend_confirmation_instructions

app/mailers/emails/identity_verification.rb
7:    def verification_instructions_email(email, token:)

ee/app/services/users/email_verification/send_custom_confirmation_instructions_service.rb
40:        ::Notify.confirmation_instructions_email(user.email, token: token).deliver_later

ee/app/controllers/users/identity_verification_controller.rb
298:      Notify.confirmation_instructions_email(@user.email, token: token).deliver_later

ee/app/controllers/ee/confirmations_controller.rb
18:      audit_changes(:email, as: 'email address', model: resource, event_type: 'user_email_changed_and_user_signed_in')

ee/app/controllers/ee/registrations_controller.rb
63:      custom_confirmation_instructions_service.set_token(save: false)
78:    override :send_custom_confirmation_instructions
79:    def send_custom_confirmation_instructions
82:      custom_confirmation_instructions_service.send_instructions
85:    def custom_confirmation_instructions_service
88:    strong_memoize_attr :custom_confirmation_instructions_service

ee/app/mailers/ee/preview/notify_preview.rb
83:        def confirmation_instructions_email
84:          ::Notify.confirmation_instructions_email(user.email, token: '123456').message

ee/app/mailers/ee/emails/identity_verification.rb
8:      def confirmation_instructions_email(email, token:)

ee/app/models/ee/user.rb
46:        user.email_changed? && user.enterprise_user? && !user.skip_enterprise_user_email_change_restrictions?
51:      after_update :email_changed_hook, if: :saved_change_to_email?
684:    def send_confirmation_instructions
823:    def email_changed_hook

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Related to #442831

Edited by Nick Malcolm

Merge request reports