diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml
index cd0fb21f8a7..ffdca500abe 100644
--- a/app/views/projects/diffs/_line.html.haml
+++ b/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?))
diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml
index 1f0ca211074..e47361354f3 100644
--- a/app/views/projects/diffs/_parallel_view.html.haml
+++ b/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
diff --git a/changelogs/unreleased/security-49085-persistent-xss-rendering.yml b/changelogs/unreleased/security-49085-persistent-xss-rendering.yml
new file mode 100644
index 00000000000..dc15d356c1c
--- /dev/null
+++ b/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
diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb
index 5c1baa19b66..1f012043e56 100644
--- a/lib/gitlab/diff/highlight.rb
+++ b/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
diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb
index 1faf7770634..633985d5caa 100644
--- a/lib/gitlab/diff/line.rb
+++ b/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
diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb
index d6e7ff33d5d..0c15febe8df 100644
--- a/spec/features/merge_request/user_sees_diff_spec.rb
+++ b/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 {
+ let a = 1;
+ let b = 2;
+ let c = 3;
+ let d = 3;
+ }
+ CONTENT
+
+ new_file_content =
+ <<~CONTENT
+ function foo {
+ 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 {")
+ end
+ end
end
end
diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb
index 7c9e8c8d04e..3c8cf9c56cc 100644
--- a/spec/lib/gitlab/diff/highlight_spec.rb
+++ b/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{ def popen(cmd, path=nil)\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{- raise "System commands must be given as an array of strings"\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{+ raise RuntimeError, "System commands must be given as an array of strings"\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 RuntimeError, "System commands must be given as an array of strings"}
- expect(subject[5].text).to eq(code)
- expect(subject[5].text).to be_html_safe
+ expect(subject[5].rich_text).to eq(code)
+ expect(subject[5].rich_text).to be_html_safe
end
context 'when the inline diff marker has an invalid range' do
diff --git a/spec/lib/gitlab/diff/line_spec.rb b/spec/lib/gitlab/diff/line_spec.rb
new file mode 100644
index 00000000000..e9d382d59c6
--- /dev/null
+++ b/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("", "", 0, 0, 0)
+ line = subject.as_json
+
+ expect(line[:rich_text]).to eq("<input>")
+ end
+ end
+end