Escaped html characters

app/views/projects/diffs/_line.html.haml

@@ -32,9 +32,9 @@
         %a{ href: "##{line_code}", data: { linenumber: link_text } }
     %td.line_content.noteable_line{ class: type }<
       - if email
-        %pre= line.text
+        %pre= line.rich_text
       - else
-        = diff_line_content(line.text)
+        = diff_line_content(line.rich_text)
 
 - if line_discussions&.any?
   - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))

app/views/projects/diffs/_parallel_view.html.haml

@@ -24,7 +24,7 @@
               - discussion_left = discussions_left.try(:first)
               - if discussion_left && discussion_left.resolvable?
                 %diff-note-avatars{ "discussion-id" => discussion_left.id }
-            %td.line_content.parallel.noteable_line.left-side{ id: left_line_code, class: left.type }= diff_line_content(left.text)
+            %td.line_content.parallel.noteable_line.left-side{ id: left_line_code, class: left.type }= diff_line_content(left.rich_text)
         - else
           %td.old_line.diff-line-num.empty-cell
           %td.line_content.parallel.left-side
@@ -45,7 +45,7 @@
               - discussion_right = discussions_right.try(:first)
               - if discussion_right && discussion_right.resolvable?
                 %diff-note-avatars{ "discussion-id" => discussion_right.id }
-            %td.line_content.parallel.noteable_line.right-side{ id: right_line_code, class: right.type }= diff_line_content(right.text)
+            %td.line_content.parallel.noteable_line.right-side{ id: right_line_code, class: right.type }= diff_line_content(right.rich_text)
         - else
           %td.old_line.diff-line-num.empty-cell
           %td.line_content.parallel.right-side

changelogs/unreleased/security-49085-persistent-xss-rendering.yml

@@ -0,0 +1,5 @@
+---
+title: Fixed persistent XSS rendering/escaping of diff location lines
+merge_request:
+author:
+type: security

lib/gitlab/diff/highlight.rb

@@ -37,7 +37,7 @@ module Gitlab
             end
           end
 
-          diff_line.text = rich_line
+          diff_line.rich_text = rich_line
 
           diff_line
         end

lib/gitlab/diff/line.rb

@@ -85,7 +85,7 @@ module Gitlab
           old_line: old_line,
           new_line: new_line,
           text: text,
-          rich_text: rich_text || text,
+          rich_text: rich_text || CGI.escapeHTML(text),
           meta_data: meta_positions
         }
       end

spec/features/merge_request/user_sees_diff_spec.rb

@@ -2,6 +2,7 @@ require 'rails_helper'
 
 describe 'Merge request > User sees diff', :js do
   include ProjectForksHelper
+  include RepoHelpers
 
   let(:project) { create(:project, :public, :repository) }
   let(:merge_request) { create(:merge_request, source_project: project) }
@@ -81,5 +82,58 @@ describe 'Merge request > User sees diff', :js do
         expect(page).to have_selector('.js-cancel-fork-suggestion-button', count: 1)
       end
     end
+
+    context 'when file contains html' do
+      let(:current_user) { project.owner }
+      let(:branch_name) {"test_branch"}
+
+      def create_file(branch_name, file_name, content)
+        Files::CreateService.new(
+          project,
+          current_user,
+          start_branch: branch_name,
+          branch_name: branch_name,
+          commit_message: "Create file",
+          file_path: file_name,
+          file_content: content
+        ).execute
+
+        project.commit(branch_name)
+      end
+
+      it 'escapes any HTML special characters in the diff chunk header' do
+        file_content =
+          <<~CONTENT
+          function foo<input> {
+            let a = 1;
+            let b = 2;
+            let c = 3;
+            let d = 3;
+          }
+        CONTENT
+
+        new_file_content =
+          <<~CONTENT
+          function foo<input> {
+            let a = 1;
+            let b = 2;
+            let c = 3;
+            let x = 3;
+          }
+        CONTENT
+
+        file_name = 'xss_file.txt'
+
+        create_file('master', file_name, file_content)
+        merge_request = create(:merge_request, source_project: project)
+        create_file(merge_request.source_branch, file_name, new_file_content)
+
+        project.commit(merge_request.source_branch)
+
+        visit diffs_project_merge_request_path(project, merge_request)
+
+        expect(page).to have_text("function foo<input> {")
+      end
+    end
   end
 end

spec/lib/gitlab/diff/highlight_spec.rb

@@ -24,19 +24,19 @@ describe Gitlab::Diff::Highlight do
       it 'highlights and marks unchanged lines' do
         code = %Q{ <span id="LC7" class="line" lang="ruby">  <span class="k">def</span> <span class="nf">popen</span><span class="p">(</span><span class="n">cmd</span><span class="p">,</span> <span class="n">path</span><span class="o">=</span><span class="kp">nil</span><span class="p">)</span></span>\n}
 
-        expect(subject[2].text).to eq(code)
+        expect(subject[2].rich_text).to eq(code)
       end
 
       it 'highlights and marks removed lines' do
         code = %Q{-<span id="LC9" class="line" lang="ruby">      <span class="k">raise</span> <span class="s2">"System commands must be given as an array of strings"</span></span>\n}
 
-        expect(subject[4].text).to eq(code)
+        expect(subject[4].rich_text).to eq(code)
       end
 
       it 'highlights and marks added lines' do
         code = %Q{+<span id="LC9" class="line" lang="ruby">      <span class="k">raise</span> <span class="no"><span class="idiff left">RuntimeError</span></span><span class="p"><span class="idiff">,</span></span><span class="idiff right"> </span><span class="s2">"System commands must be given as an array of strings"</span></span>\n}
 
-        expect(subject[5].text).to eq(code)
+        expect(subject[5].rich_text).to eq(code)
       end
     end
 
@@ -69,8 +69,8 @@ describe Gitlab::Diff::Highlight do
       it 'marks added lines' do
         code = %q{+      raise <span class="idiff left right">RuntimeError, </span>&quot;System commands must be given as an array of strings&quot;}
 
-        expect(subject[5].text).to eq(code)
+        expect(subject[5].rich_text).to eq(code)
-        expect(subject[5].text).to be_html_safe
+        expect(subject[5].rich_text).to be_html_safe
       end
 
       context 'when the inline diff marker has an invalid range' do

spec/lib/gitlab/diff/line_spec.rb

@@ -0,0 +1,10 @@
+describe Gitlab::Diff::Line do
+  context "when setting rich text" do
+    it 'escapes any HTML special characters in the diff chunk header' do
+      subject = described_class.new("<input>", "", 0, 0, 0)
+      line = subject.as_json
+
+      expect(line[:rich_text]).to eq("&lt;input&gt;")
+    end
+  end
+end