Skip to content

Do not attribute unverified commit e-mails to GitLab users

Stan Hu requested to merge sh-issue-28493 into master

Previously we would identify any user with a matching e-mail address as a commit author or committer, but this caused confusion when commits contained invalid, unconfirmable e-mail addresses (e.g. my.local).

Now, we only assign a commit author or committer is a specific user if the e-mail has been confirmed.

Closes #28493 (closed)

What is using Commit#committer?

We can disregard the init_from_gitaly calls because those are initializing raw commits, rather than trying to load the User model. Mostly it looks like pipeline failed e-mails, and the EE push check still did the right thing even before adjusting the test (denied user access but with wrong error message).

$ git grep commit.committer | egrep -v "app/assets/|spec/|locale/|doc/" | pbcopy
app/helpers/commits_helper.rb:  def commit_committer_link(commit, options = {})
app/helpers/users_helper.rb:            class: 'has-tooltip commit-committer-link')
app/models/project_services/chat_message/pipeline_message.rb:      @committer = commit.committer
app/views/notify/pipeline_failed_email.html.haml:                      %img.avatar{ height: "24", src: avatar_icon_for(commit.committer, commit.committer_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
app/views/notify/pipeline_failed_email.html.haml:                      - if commit.committer
app/views/notify/pipeline_failed_email.html.haml:                        %a.muted{ href: user_url(commit.committer), style: "color:#333333;text-decoration:none;" }
app/views/notify/pipeline_failed_email.html.haml:                          = commit.committer.name
app/views/notify/pipeline_failed_email.html.haml:                          = commit.committer_name
app/views/notify/pipeline_failed_email.text.erb:<% if commit.committer -%>
app/views/notify/pipeline_failed_email.text.erb:Committed by: <%= sanitize_name(commit.committer.name) %> ( <%= user_url(commit.committer) %> )
app/views/notify/pipeline_failed_email.text.erb:Committed by: <%= commit.committer_name %>
app/views/notify/pipeline_success_email.html.haml:                      %img.avatar{ height: "24", src: avatar_icon_for(commit.committer, commit.committer_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
app/views/notify/pipeline_success_email.html.haml:                      - if commit.committer
app/views/notify/pipeline_success_email.html.haml:                        %a.muted{ href: user_url(commit.committer), style: "color:#333333;text-decoration:none;" }
app/views/notify/pipeline_success_email.html.haml:                          = commit.committer.name
app/views/notify/pipeline_success_email.html.haml:                          = commit.committer_name
app/views/notify/pipeline_success_email.text.erb:<% if commit.committer -%>
app/views/notify/pipeline_success_email.text.erb:Committed by: <%= sanitize_name(commit.committer.name) %> ( <%= user_url(commit.committer) %> )
app/views/notify/pipeline_success_email.text.erb:Committed by: <%= commit.committer_name %>
app/views/projects/commit/_commit_box.html.haml:        = commit_committer_link(@commit, avatar: true, size: 24)
db/migrate/20190402150158_backport_enterprise_schema.rb:      t.boolean "commit_committer_check"
db/schema.rb:    t.boolean "commit_committer_check"
ee/app/controllers/admin/push_rules_controller.rb:    if @push_rule.available?(:commit_committer_check)
ee/app/controllers/admin/push_rules_controller.rb:      allowed_fields << :commit_committer_check
ee/app/controllers/projects/push_rules_controller.rb:    if can?(current_user, :change_commit_committer_check, project)
ee/app/controllers/projects/push_rules_controller.rb:      allowed_fields << :commit_committer_check
ee/app/helpers/push_rules_helper.rb:  def commit_committer_check_description(push_rule)
ee/app/helpers/push_rules_helper.rb:    push_rule_update_description(message, push_rule, :commit_committer_check)
ee/app/models/license.rb:    commit_committer_check
ee/app/models/push_rule.rb:    commit_committer_check
ee/app/models/push_rule.rb:      commit_committer_check ||
ee/app/models/push_rule.rb:    return true unless available?(:commit_committer_check)
ee/app/models/push_rule.rb:    return true unless commit_committer_check
ee/app/models/push_rule.rb:  def commit_committer_check
ee/app/models/push_rule.rb:    read_setting_with_global_default(:commit_committer_check)
ee/app/models/push_rule.rb:  alias_method :commit_committer_check?, :commit_committer_check
ee/app/models/push_rule.rb:  def commit_committer_check=(value)
ee/app/models/push_rule.rb:    write_setting_with_global_default(:commit_committer_check, value)
ee/app/policies/ee/project_policy.rb:      condition(:commit_committer_check_disabled_globally) do
ee/app/policies/ee/project_policy.rb:        !PushRule.global&.commit_committer_check
ee/app/policies/ee/project_policy.rb:      condition(:commit_committer_check_available) do
ee/app/policies/ee/project_policy.rb:        @subject.feature_available?(:commit_committer_check)
ee/app/policies/ee/project_policy.rb:      rule { admin | (commit_committer_check_disabled_globally & can?(:maintainer_access)) }.policy do
ee/app/policies/ee/project_policy.rb:        enable :change_commit_committer_check
ee/app/policies/ee/project_policy.rb:      rule { commit_committer_check_available }.policy do
ee/app/policies/ee/project_policy.rb:        enable :read_commit_committer_check
ee/app/policies/ee/project_policy.rb:      rule { ~commit_committer_check_available }.policy do
ee/app/policies/ee/project_policy.rb:        prevent :change_commit_committer_check
ee/app/views/shared/push_rules/_commit_committer_check_setting.html.haml:- return unless push_rule.available?(:commit_committer_check)
ee/app/views/shared/push_rules/_commit_committer_check_setting.html.haml:  = form.check_box :commit_committer_check, class: "form-check-input", disabled: !can_change_push_rule?(form.object, :commit_committer_check), data: { qa_selector: 'committer_restriction_checkbox' }
ee/app/views/shared/push_rules/_commit_committer_check_setting.html.haml:  = form.label :commit_committer_check, class: "label-bold form-check-label" do
ee/app/views/shared/push_rules/_commit_committer_check_setting.html.haml:    = commit_committer_check_description(push_rule)
ee/app/views/shared/push_rules/_form.html.haml:= render 'shared/push_rules/commit_committer_check_setting', form: f, push_rule: f.object
ee/lib/api/github/entities.rb:            email: commit.committer_email
ee/lib/api/github/entities.rb:              name: commit.committer_name,
ee/lib/api/github/entities.rb:              email: commit.committer_email,
ee/lib/api/project_push_rule.rb:    before { authorize_change_param(user_project, :commit_committer_check, :reject_unsigned_commits) }
ee/lib/api/project_push_rule.rb:          optional :commit_committer_check, type: Boolean, desc: 'Users may only push their own commits'
ee/lib/api/project_push_rule.rb:                          :commit_committer_check,
ee/lib/ee/api/entities.rb:        expose_restricted :commit_committer_check, &:project
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:            unless push_rule.author_email_allowed?(commit.committer_email)
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              return "Committer's email '#{commit.committer_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              if commit.author_email.casecmp(commit.committer_email) == -1
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:                unless ::User.find_by_any_email(commit.committer_email).present?
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:                  return "Committer '#{commit.committer_email}' is not a member of team"
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:            unless push_rule.committer_allowed?(commit.committer_email, user_access.user)
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              committer = commit.committer(confirmed: false)
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              if committer_is_current_user && !committer.verified_email?(commit.committer_email)
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:                ERROR_MESSAGES[:committer_not_verified] % { committer_email: commit.committer_email }
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:                ERROR_MESSAGES[:committer_not_allowed] % { committer_email: commit.committer_email }
lib/gitlab/git/commit.rb:          committer: gitaly_commit_author_from_raw(raw_commit.committer)
lib/gitlab/git/commit.rb:        @committed_date = init_date_from_gitaly(commit.committer)
lib/gitlab/git/commit.rb:        @committer_name = commit.committer.name.dup
lib/gitlab/git/commit.rb:        @committer_email = commit.committer.email.dup
lib/gitlab/git/rugged_impl/commit.rb:          committer = commit.committer
lib/gitlab/gitaly_client/ref_service.rb:          committed_date: Time.at(response.commit_committer.date.seconds),
lib/gitlab/gitaly_client/ref_service.rb:          committer_name: response.commit_committer.name.dup,
lib/gitlab/gitaly_client/ref_service.rb:          committer_email: response.commit_committer.email.dup
lib/gitlab/gpg/commit.rb:        if gpg_key.verified_and_belongs_to_email?(@commit.committer_email)
lib/gitlab/gpg/commit.rb:        elsif gpg_key.user.all_emails.include?(@commit.committer_email)
qa/qa/ee/page/project/settings/push_rules.rb:            view 'ee/app/views/shared/push_rules/_commit_committer_check_setting.html.haml' do

What is using Commit#author?

git grep commit.author | egrep -v "app/assets/|spec/|locale/|doc/"
CHANGELOG-EE.md:- Prevent commit authors from self approvaling merge requests. !9007
CHANGELOG-EE.md:  - Added an ability to restrict commit authors to existing GitLab users
CHANGELOG.md:- Remove tooltips from commit author avatar and name in commit lists. !20674
CHANGELOG.md:- [SECURITY] Prevent a persistent XSS in the commit author block.
CHANGELOG.md:- [SECURITY] Prevent a persistent XSS in the commit author block.
CHANGELOG.md:- [SECURITY] Prevent a persistent XSS in the commit author block.
CHANGELOG.md:- Link to commit author user page from pipelines. !11100
app/controllers/projects/graphs_controller.rb:        author_name: commit.author_name,
app/controllers/projects/graphs_controller.rb:        author_email: commit.author_email,
app/controllers/projects/refs_controller.rb:    # Preload commit authors as they are used in rendering
app/helpers/commits_helper.rb:  # Returns a link to the commit author. If the author has a matching user and
app/helpers/commits_helper.rb:  def commit_author_link(commit, options = {})
app/models/repository.rb:          contributor.name = commit.author_name
app/serializers/commit_entity.rb:    GravatarService.new.execute(commit.author_email) # rubocop: disable CodeReuse/ServiceClass
app/views/notify/pipeline_failed_email.html.haml:                    %img.avatar{ height: "24", src: avatar_icon_for(commit.author, commit.author_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
app/views/notify/pipeline_failed_email.html.haml:                    - if commit.author
app/views/notify/pipeline_failed_email.html.haml:                      %a.muted{ href: user_url(commit.author), style: "color:#333333;text-decoration:none;" }
app/views/notify/pipeline_failed_email.html.haml:                        = commit.author.name
app/views/notify/pipeline_failed_email.html.haml:                        = commit.author_name
app/views/notify/pipeline_failed_email.text.erb:<% if commit.author -%>
app/views/notify/pipeline_failed_email.text.erb:Commit Author: <%= sanitize_name(commit.author.name) %> ( <%= user_url(commit.author) %> )
app/views/notify/pipeline_failed_email.text.erb:Commit Author: <%= commit.author_name %>
app/views/notify/pipeline_success_email.html.haml:                    %img.avatar{ height: "24", src: avatar_icon_for(commit.author, commit.author_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
app/views/notify/pipeline_success_email.html.haml:                    - if commit.author
app/views/notify/pipeline_success_email.html.haml:                      %a.muted{ href: user_url(commit.author), style: "color:#333333;text-decoration:none;" }
app/views/notify/pipeline_success_email.html.haml:                        = commit.author.name
app/views/notify/pipeline_success_email.html.haml:                        = commit.author_name
app/views/notify/pipeline_success_email.text.erb:<% if commit.author -%>
app/views/notify/pipeline_success_email.text.erb:Commit Author: <%= sanitize_name(commit.author.name) %> ( <%= user_url(commit.author) %> )
app/views/notify/pipeline_success_email.text.erb:Commit Author: <%= commit.author_name %>
app/views/notify/repository_push_email.html.haml:          %span by #{commit.author_name}
app/views/notify/repository_push_email.text.haml:    #{commit.short_id} by #{commit.author_name} at #{commit.committed_date.to_s(:iso8601)}
app/views/projects/blame/show.html.haml:                  = commit_author_link(commit, avatar: false)
app/views/projects/commit/_commit_box.html.haml:    #{time_ago_with_tooltip(@commit.authored_date)}
app/views/projects/commit/_commit_box.html.haml:      = commit_author_link(@commit, avatar: true, size: 24)
app/views/projects/commit/_commit_box.html.haml:    = user_status(@commit.author)
app/views/projects/commits/_commit.atom.builder:  xml.media   :thumbnail, width: "40", height: "40", url: image_url(avatar_icon_for_email(commit.author_email))
app/views/projects/commits/_commit.atom.builder:    xml.name commit.author_name
app/views/projects/commits/_commit.atom.builder:    xml.email commit.author_email
app/views/projects/commits/_commit.html.haml:        - commit_author_link = commit_author_link(commit, avatar: false, size: 24)
app/views/projects/commits/_commit.html.haml:        - commit_timeago = time_ago_with_tooltip(commit.authored_date, placement: 'bottom')
app/views/projects/commits/_commit.html.haml:        - commit_text =  _('%{commit_author_link} authored %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago }
app/views/projects/repositories/_feed.html.haml:      = image_tag avatar_icon_for_email(commit.author_email), class: "", width: 16, alt: ''
app/views/projects/repositories/_feed.html.haml:      = markdown(truncate(commit.title, length: 40), pipeline: :single_line, author: commit.author)
app/views/projects/tags/_tag.atom.builder:    xml.media   :thumbnail, width: '40', height: '40', url: image_url(avatar_icon_for_email(commit.author_email))
app/views/projects/tags/_tag.atom.builder:      xml.name  commit.author_name
app/views/projects/tags/_tag.atom.builder:      xml.email commit.author_email
app/views/projects/wikis/history.html.haml:            = commit.author_name
app/workers/process_commit_worker.rb:    author = commit.author || user
changelogs/archive.md:  - Cache the commit author in RequestStore to avoid extra lookups in PostReceive
changelogs/archive.md:- Link to commit authors everywhere
ee/app/views/shared/push_rules/_form.html.haml:    All commit author's email must match this
ee/lib/api/github/entities.rb:            login: commit.author&.username,
ee/lib/api/github/entities.rb:            email: commit.author_email
ee/lib/api/github/entities.rb:            login: commit.author&.username,
ee/lib/api/github/entities.rb:              name: commit.author_name,
ee/lib/api/github/entities.rb:              email: commit.author_email,
ee/lib/api/github/entities.rb:              date: commit.authored_date.iso8601,
ee/lib/api/project_push_rule.rb:          optional :author_email_regex, type: String, desc: 'All commit author emails must match this'
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:            unless push_rule.author_email_allowed?(commit.author_email)
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              return "Author's email '#{commit.author_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              unless ::User.find_by_any_email(commit.author_email).present?
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:                return "Author '#{commit.author_email}' is not a member of team"
ee/lib/ee/gitlab/checks/push_rules/commit_check.rb:              if commit.author_email.casecmp(commit.committer_email) == -1
ee/lib/gitlab/authority_analyzer.rb:        if commit.author && commit.author != @skip_user
ee/lib/gitlab/authority_analyzer.rb:          @users[commit.author] += 1
lib/api/branches.rb:        destroy_conditionally!(commit, last_updated: commit.authored_date) do
lib/api/tags.rb:        destroy_conditionally!(commit, last_updated: commit.authored_date) do
lib/flowdock/git/builder.rb:              name: commit.author_name,
lib/flowdock/git/builder.rb:              email: commit.author_email
lib/gitlab/ci/pipeline/preloader.rb:              preloader.preload_commit_authors
lib/gitlab/ci/pipeline/preloader.rb:        def preload_commit_authors
lib/gitlab/git/commit.rb:          author: gitaly_commit_author_from_raw(raw_commit.author),
lib/gitlab/git/commit.rb:          committer: gitaly_commit_author_from_raw(raw_commit.committer)
lib/gitlab/git/commit.rb:        @authored_date = init_date_from_gitaly(commit.author)
lib/gitlab/git/commit.rb:        @author_name = commit.author.name.dup
lib/gitlab/git/commit.rb:        @author_email = commit.author.email.dup
lib/gitlab/git/commit.rb:      def gitaly_commit_author_from_raw(author_or_committer)
lib/gitlab/git/rugged_impl/commit.rb:          author = commit.author
lib/gitlab/gitaly_client/operation_service.rb:          commit_author_name: encode_binary(author_name),
lib/gitlab/gitaly_client/operation_service.rb:          commit_author_email: encode_binary(author_email),
lib/gitlab/gitaly_client/ref_service.rb:          authored_date: Time.at(response.commit_author.date.seconds),
lib/gitlab/gitaly_client/ref_service.rb:          author_name: response.commit_author.name.dup,
lib/gitlab/gitaly_client/ref_service.rb:          author_email: response.commit_author.email.dup,
Edited by Stan Hu

Merge request reports