Merge branch 'security-22076-sanitize-url-in-names' into 'master'

[master] Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs

See merge request gitlab/gitlabhq!2793

app/helpers/emails_helper.rb

@@ -36,6 +36,14 @@ module EmailsHelper
     nil
   end
 
+  def sanitize_name(name)
+    if name =~ URI::DEFAULT_PARSER.regexp[:URI_REF]
+      name.tr('.', '_')
+    else
+      name
+    end
+  end
+
   def password_reset_token_valid_time
     valid_hours = Devise.reset_password_within / 60 / 60
     if valid_hours >= 24

app/views/devise/mailer/_confirmation_instructions_secondary.html.haml

 #content
-  = email_default_heading("#{@resource.user.name}, you've added an additional email!")
+  = email_default_heading("#{sanitize_name(@resource.user.name)}, you've added an additional email!")
   %p Click the link below to confirm your email address (#{@resource.email})
   #cta
     = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token)

app/views/notify/_note_email.text.erb

@@ -3,7 +3,7 @@
 
 <%  discussion = note.discussion if note.part_of_discussion? -%>
 <%  if discussion && !discussion.individual_note? -%>
-<%=   note.author_name -%>
+<%=   sanitize_name(note.author_name) -%>
 <%    if discussion.new_discussion? -%>
 <%=     " started a new discussion" -%>
 <%    else -%>
@@ -16,7 +16,7 @@
 
 
 <%  elsif Gitlab::CurrentSettings.email_author_in_body -%>
-<%=     "#{note.author_name} commented:" -%>
+<%=     "#{sanitize_name(note.author_name)} commented:" -%>
 
 
 <%  end -%>

app/views/notify/autodevops_disabled_email.text.erb

@@ -3,7 +3,7 @@ Auto DevOps pipeline was disabled for <%= @project.name %>
 The Auto DevOps pipeline failed for pipeline <%= @pipeline.iid %> (<%= pipeline_url(@pipeline) %>) and has been disabled for <%= @project.name %>. In order to use the Auto DevOps pipeline with your project, please review the currently supported languagues (https://docs.gitlab.com/ee/topics/autodevops/#currently-supported-languages), adjust your project accordingly, and turn on the Auto DevOps pipeline within your CI/CD project settings (<%= project_settings_ci_cd_url(@project) %>).
 
 <% if @pipeline.user -%>
-  Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= @pipeline.user.name %> ( <%= user_url(@pipeline.user) %> )
+  Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= sanitize_name(@pipeline.user.name) %> ( <%= user_url(@pipeline.user) %> )
 <% else -%>
   Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API
 <% end -%>

app/views/notify/closed_issue_email.html.haml

 %p
-  Issue was closed by #{@updated_by.name}
+  Issue was closed by #{sanitize_name(@updated_by.name)}

app/views/notify/closed_issue_email.text.haml

-Issue was closed by #{@updated_by.name}
+Issue was closed by #{sanitize_name(@updated_by.name)}
 
 Issue ##{@issue.iid}: #{project_issue_url(@issue.project, @issue)}

app/views/notify/closed_merge_request_email.html.haml

 %p
-  Merge Request #{@merge_request.to_reference} was closed by #{@updated_by.name}
+  Merge Request #{@merge_request.to_reference} was closed by #{sanitize_name(@updated_by.name)}

app/views/notify/closed_merge_request_email.text.haml

-Merge Request #{@merge_request.to_reference} was closed by #{@updated_by.name}
+Merge Request #{@merge_request.to_reference} was closed by #{sanitize_name(@updated_by.name)}
 
 Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 
-Author: #{@merge_request.author_name}
-Assignee: #{@merge_request.assignee_name}
+Author: #{sanitize_name(@merge_request.author_name)}
+Assignee: #{sanitize_name(@merge_request.assignee_name)}

app/views/notify/issue_status_changed_email.html.haml

 %p
-  Issue was #{@issue_status} by #{@updated_by.name}
+  Issue was #{@issue_status} by #{sanitize_name(@updated_by.name)}

app/views/notify/issue_status_changed_email.text.erb

-Issue was <%= @issue_status %> by <%= @updated_by.name %>
+Issue was <%= @issue_status %> by <%= sanitize_name(@updated_by.name) %>
 
 Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %>
 

app/views/notify/member_access_requested_email.text.erb

-<%= member.user.name %> (<%= user_url(member.user) %>) requested <%= member.human_access %> access to the <%= member_source.human_name %> <%= member_source.model_name.singular %>.
+<%= sanitize_name(member.user.name) %> (<%= user_url(member.user) %>) requested <%= member.human_access %> access to the <%= member_source.human_name %> <%= member_source.model_name.singular %>.
 
 <%= polymorphic_url([member_source, :members]) %>

app/views/notify/member_invite_accepted_email.text.erb

-<%= member.invite_email %>, now known as <%= member.user.name %>, has accepted your invitation to join the <%= member_source.human_name %> <%= member_source.model_name.singular %>.
+<%= member.invite_email %>, now known as <%= sanitize_name(member.user.name) %>, has accepted your invitation to join the <%= member_source.human_name %> <%= member_source.model_name.singular %>.
 
 <%= member_source.web_url %>

app/views/notify/member_invited_email.text.erb

-You have been invited <%= "by #{member.created_by.name} " if member.created_by %>to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> as <%= member.human_access %>.
+You have been invited <%= "by #{sanitize_name(member.created_by.name)} " if member.created_by %>to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> as <%= member.human_access %>.
 
 Accept invitation: <%= invite_url(@token) %>
 Decline invitation: <%= decline_invite_url(@token) %>

app/views/notify/merge_request_status_email.html.haml

 %p
-  Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{@updated_by.name}
+  Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{sanitize_name(@updated_by.name)}

app/views/notify/merge_request_status_email.text.haml

-Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{@updated_by.name}
+Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{sanitize_name(@updated_by.name)}
 
 Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 
-Author: #{@merge_request.author_name}
-Assignee: #{@merge_request.assignee_name}
+Author: #{sanitize_name(@merge_request.author_name)}
+Assignee: #{sanitize_name(@merge_request.assignee_name)}

app/views/notify/merge_request_unmergeable_email.text.haml

@@ -4,5 +4,5 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m
 
 = merge_path_description(@merge_request, 'to')
 
-Author: #{@merge_request.author_name}
-Assignee: #{@merge_request.assignee_name}
+Author: #{sanitize_name(@merge_request.author_name)}
+Assignee: #{sanitize_name(@merge_request.assignee_name)}

app/views/notify/merged_merge_request_email.text.haml

@@ -4,5 +4,5 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m
 
 = merge_path_description(@merge_request, 'to')
 
-Author: #{@merge_request.author_name}
-Assignee: #{@merge_request.assignee_name}
+Author: #{sanitize_name(@merge_request.author_name)}
+Assignee: #{sanitize_name(@merge_request.assignee_name)}

app/views/notify/new_gpg_key_email.html.haml

 %p
-  Hi #{@user.name}!
+  Hi #{sanitize_name(@user.name)}!
 %p
   A new GPG key was added to your account:
 %p

app/views/notify/new_gpg_key_email.text.erb

-Hi <%= @user.name %>!
+Hi <%= sanitize_name(@user.name) %>!
 
 A new GPG key was added to your account:
 

app/views/notify/new_issue_email.text.erb

 New Issue was created.
 
 Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %>
-Author:    <%= @issue.author_name %>
+Author:    <%= sanitize_name(@issue.author_name) %>
 Assignee:  <%= @issue.assignee_list %>
 
 <%= @issue.description %>

app/views/notify/new_mention_in_issue_email.text.erb

 You have been mentioned in an issue.
 
 Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %>
-Author:    <%= @issue.author_name %>
-Assignee:  <%= @issue.assignee_list %>
+Author:    <%= sanitize_name(@issue.author_name) %>
+Assignee:  <%= sanitize_name(@issue.assignee_list) %>
 
 <%= @issue.description %>

app/views/notify/new_mention_in_merge_request_email.text.erb

@@ -3,7 +3,7 @@ You have been mentioned in Merge Request <%= @merge_request.to_reference %>
 <%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %>
 
 <%= merge_path_description(@merge_request, 'to') %>
-Author:    <%= @merge_request.author_name %>
-Assignee:  <%= @merge_request.assignee_name %>
+Author:    <%= sanitize_name(@merge_request.author_name) %>
+Assignee:  <%= sanitize_name(@merge_request.assignee_name) %>
 
 <%= @merge_request.description %>

app/views/notify/new_merge_request_email.html.haml

@@ -7,7 +7,7 @@
 
 - if @merge_request.assignee_id.present?
   %p
-    Assignee: #{@merge_request.assignee_name}
+    Assignee: #{sanitize_name(@merge_request.assignee_name)}
 
 = render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter
 

app/views/notify/new_ssh_key_email.html.haml

 %p
-  Hi #{@user.name}!
+  Hi #{sanitize_name(@user.name)}!
 %p
   A new public key was added to your account:
 %p

app/views/notify/new_ssh_key_email.text.erb

-Hi <%= @user.name %>!
+Hi <%= sanitize_name(@user.name) %>!
 
 A new public key was added to your account:
 

app/views/notify/new_user_email.html.haml

 %p
-  Hi #{@user['name']}!
+  Hi #{sanitize_name(@user['name'])}!
 %p
   - if Gitlab::CurrentSettings.allow_signup?
     Your account has been created successfully.

app/views/notify/new_user_email.text.erb

-Hi <%= @user.name %>!
+Hi <%= sanitize_name(@user.name) %>!
 
 The Administrator created an account for you. Now you are a member of the company GitLab application.
 

app/views/notify/pipeline_failed_email.text.erb

@@ -10,20 +10,20 @@ Commit: <%= @pipeline.short_sha %> ( <%= commit_url(@pipeline) %> )
 Commit Message: <%= @pipeline.git_commit_message.truncate(50) %>
 <% commit = @pipeline.commit -%>
 <% if commit.author -%>
-Commit Author: <%= commit.author.name %> ( <%= user_url(commit.author) %> )
+Commit Author: <%= sanitize_name(commit.author.name) %> ( <%= user_url(commit.author) %> )
 <% else -%>
 Commit Author: <%= commit.author_name %>
 <% end -%>
 <% if commit.different_committer? -%>
 <% if commit.committer -%>
-Committed by: <%= commit.committer.name %> ( <%= user_url(commit.committer) %> )
+Committed by: <%= sanitize_name(commit.committer.name) %> ( <%= user_url(commit.committer) %> )
 <% else -%>
 Committed by: <%= commit.committer_name %>
 <% end -%>
 <% end -%>
 
 <% if @pipeline.user -%>
-Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= @pipeline.user.name %> ( <%= user_url(@pipeline.user) %> )
+Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= sanitize_name(@pipeline.user.name) %> ( <%= user_url(@pipeline.user) %> )
 <% else -%>
 Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API
 <% end -%>

app/views/notify/pipeline_success_email.text.erb

@@ -10,13 +10,13 @@ Commit: <%= @pipeline.short_sha %> ( <%= commit_url(@pipeline) %> )
 Commit Message: <%= @pipeline.git_commit_message.truncate(50) %>
 <% commit = @pipeline.commit -%>
 <% if commit.author -%>
-Commit Author: <%= commit.author.name %> ( <%= user_url(commit.author) %> )
+Commit Author: <%= sanitize_name(commit.author.name) %> ( <%= user_url(commit.author) %> )
 <% else -%>
 Commit Author: <%= commit.author_name %>
 <% end -%>
 <% if commit.different_committer? -%>
 <% if commit.committer -%>
-Committed by: <%= commit.committer.name %> ( <%= user_url(commit.committer) %> )
+Committed by: <%= sanitize_name(commit.committer.name) %> ( <%= user_url(commit.committer) %> )
 <% else -%>
 Committed by: <%= commit.committer_name %>
 <% end -%>
@@ -25,7 +25,7 @@ Committed by: <%= commit.committer_name %>
 <% job_count = @pipeline.total_size -%>
 <% stage_count = @pipeline.stages_count -%>
 <% if @pipeline.user -%>
-Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= @pipeline.user.name %> ( <%= user_url(@pipeline.user) %> )
+Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= sanitize_name(@pipeline.user.name) %> ( <%= user_url(@pipeline.user) %> )
 <% else -%>
 Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API
 <% end -%>

app/views/notify/push_to_merge_request_email.html.haml

 %h3
-  = @updated_by_user.name
+  = sanitize_name(@updated_by_user.name)
   pushed new commits to merge request
   = link_to(@merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request))
 

app/views/notify/push_to_merge_request_email.text.haml

-#{@updated_by_user.name} pushed new commits to merge request #{@merge_request.to_reference}
+#{sanitize_name(@updated_by_user.name)} pushed new commits to merge request #{@merge_request.to_reference}
 \
 #{url_for(project_merge_request_url(@merge_request.target_project, @merge_request))}
 \

app/views/notify/reassigned_issue_email.html.haml

@@ -2,7 +2,7 @@
   Assignee changed
   - if @previous_assignees.any?
     from
-    %strong= @previous_assignees.map(&:name).to_sentence
+    %strong= sanitize_name(@previous_assignees.map(&:name).to_sentence)
   to
   - if @issue.assignees.any?
     %strong= @issue.assignee_list

app/views/notify/reassigned_issue_email.text.erb

@@ -2,5 +2,5 @@ Reassigned Issue <%= @issue.iid %>
 
 <%= url_for([@issue.project.namespace.becomes(Namespace), @issue.project, @issue, { only_path: false }]) %>
 
-Assignee changed <%= "from #{@previous_assignees.map(&:name).to_sentence}" if @previous_assignees.any? -%>
+Assignee changed <%= "from #{sanitize_name(@previous_assignees.map(&:name).to_sentence)}" if @previous_assignees.any? -%>
  to <%= "#{@issue.assignees.any? ? @issue.assignee_list : 'Unassigned'}" %>

app/views/notify/reassigned_merge_request_email.html.haml

@@ -2,9 +2,9 @@
   Assignee changed
   - if @previous_assignee
     from
-    %strong= @previous_assignee.name
+    %strong= sanitize_name(@previous_assignee.name)
   to
   - if @merge_request.assignee_id
-    %strong= @merge_request.assignee_name
+    %strong= sanitize_name(@merge_request.assignee_name)
   - else
     %strong Unassigned

app/views/notify/reassigned_merge_request_email.text.erb

@@ -2,5 +2,5 @@ Reassigned Merge Request <%= @merge_request.iid %>
 
 <%= url_for([@merge_request.project.namespace.becomes(Namespace), @merge_request.project, @merge_request, { only_path: false }]) %>
 
-Assignee changed <%= "from #{@previous_assignee.name}" if @previous_assignee -%>
- to <%= "#{@merge_request.assignee_id ? @merge_request.assignee_name : 'Unassigned'}" %>
+Assignee changed <%= "from #{sanitize_name(@previous_assignee.name)}" if @previous_assignee -%>
+ to <%= "#{@merge_request.assignee_id ? sanitize_name(@merge_request.assignee_name) : 'Unassigned'}" %>

app/views/notify/resolved_all_discussions_email.html.haml

 %p
-  All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{@resolved_by.name}
+  All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{sanitize_name(@resolved_by.name)}

app/views/notify/resolved_all_discussions_email.text.erb

-All discussions on Merge Request <%= @merge_request.to_reference %> were resolved by <%= @resolved_by.name %>
+All discussions on Merge Request <%= @merge_request.to_reference %> were resolved by <%= sanitize_name(@resolved_by.name) %>
 
 <%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %>

changelogs/unreleased/security-22076-sanitize-url-in-names.yml

+---
+title: Sanitize user full name to clean up any URL to prevent mail clients from auto-linking
+  URLs
+merge_request: 2793
+author:
+type: security

spec/helpers/emails_helper_spec.rb

 require 'spec_helper'
 
 describe EmailsHelper do
+  describe 'sanitize_name' do
+    context 'when name contains a valid URL string' do
+      it 'returns name with `.` replaced with `_` to prevent mail clients from auto-linking URLs' do
+        expect(sanitize_name('https://about.gitlab.com')).to eq('https://about_gitlab_com')
+        expect(sanitize_name('www.gitlab.com')).to eq('www_gitlab_com')
+        expect(sanitize_name('//about.gitlab.com/handbook/security/#best-practices')).to eq('//about_gitlab_com/handbook/security/#best-practices')
+      end
+
+      it 'returns name as it is when it does not contain a URL' do
+        expect(sanitize_name('Foo Bar')).to eq('Foo Bar')
+      end
+    end
+  end
+
   describe 'password_reset_token_valid_time' do
     def validate_time_string(time_limit, expected_string)
       Devise.reset_password_within = time_limit

spec/mailers/notify_spec.rb

@@ -9,8 +9,10 @@ describe Notify do
 
   include_context 'gitlab email notification'
 
+  let(:current_user_sanitized) { 'www_example_com' }
+
   set(:user) { create(:user) }
-  set(:current_user) { create(:user, email: "current@email.com") }
+  set(:current_user) { create(:user, email: "current@email.com", name: 'www.example.com') }
   set(:assignee) { create(:user, email: 'assignee@example.com', name: 'John Doe') }
 
   set(:merge_request) do
@@ -182,7 +184,7 @@ describe Notify do
           aggregate_failures do
             is_expected.to have_referable_subject(issue, reply: true)
             is_expected.to have_body_text(status)
-            is_expected.to have_body_text(current_user.name)
+            is_expected.to have_body_text(current_user_sanitized)
             is_expected.to have_body_text(project_issue_path project, issue)
           end
         end
@@ -361,7 +363,7 @@ describe Notify do
           aggregate_failures do
             is_expected.to have_referable_subject(merge_request, reply: true)
             is_expected.to have_body_text(status)
-            is_expected.to have_body_text(current_user.name)
+            is_expected.to have_body_text(current_user_sanitized)
             is_expected.to have_body_text(project_merge_request_path(project, merge_request))
           end
         end