diff options
author | José Iván Vargas López <jvargas@gitlab.com> | 2018-08-29 00:27:31 +0300 |
---|---|---|
committer | José Iván Vargas López <jvargas@gitlab.com> | 2018-08-29 00:27:31 +0300 |
commit | 365217eb9a3272133b6db53726d443f1eb126cde (patch) | |
tree | 3952a78821253ae526b11e7fc1843bd8fe4e2a2b | |
parent | 1df7360ff945fe0004e1a7f13fe77fc4b3018f98 (diff) | |
parent | aa73b3e19b740ec779780e9e7a0528fc9e20e8ee (diff) |
Merge branch 'security-49085-persistent-xss-rendering' into 'master'
[master] Fixed persistent XSS rendering/escaping of diff location lines
See merge request gitlab/gitlabhq!2463
-rw-r--r-- | app/views/projects/diffs/_line.html.haml | 4 | ||||
-rw-r--r-- | app/views/projects/diffs/_parallel_view.html.haml | 4 | ||||
-rw-r--r-- | changelogs/unreleased/security-49085-persistent-xss-rendering.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/diff/highlight.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/diff/line.rb | 9 | ||||
-rw-r--r-- | spec/features/merge_request/user_sees_diff_spec.rb | 54 | ||||
-rw-r--r-- | spec/lib/gitlab/conflict/file_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/highlight_spec.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/line_spec.rb | 22 |
9 files changed, 96 insertions, 18 deletions
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..1ab6df0b6ae 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -1,16 +1,17 @@ module Gitlab module Diff class Line - SERIALIZE_KEYS = %i(line_code text type index old_pos new_pos).freeze + SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze attr_reader :line_code, :type, :index, :old_pos, :new_pos attr_writer :rich_text attr_accessor :text - def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil) + def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil) @text, @type, @index = text, type, index @old_pos, @new_pos = old_pos, new_pos @parent_file = parent_file + @rich_text = rich_text # When line code is not provided from cache store we build it # using the parent_file(Diff::File or Conflict::File). @@ -18,7 +19,7 @@ module Gitlab end def self.init_from_hash(hash) - new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code]) + new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code], rich_text: hash[:rich_text]) end def to_hash @@ -85,7 +86,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<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 diff --git a/spec/lib/gitlab/conflict/file_spec.rb b/spec/lib/gitlab/conflict/file_spec.rb index 5b343920429..9095ffbfd52 100644 --- a/spec/lib/gitlab/conflict/file_spec.rb +++ b/spec/lib/gitlab/conflict/file_spec.rb @@ -69,10 +69,6 @@ describe Gitlab::Conflict::File do CGI.unescapeHTML(ActionView::Base.full_sanitizer.sanitize(html)).delete("\n") end - it 'modifies the existing lines' do - expect { conflict_file.highlight_lines! }.to change { conflict_file.lines.map(&:instance_variables) } - end - it 'is called implicitly when rich_text is accessed on a line' do expect(conflict_file).to receive(:highlight_lines!).once.and_call_original 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{ <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>"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..76d411973c7 --- /dev/null +++ b/spec/lib/gitlab/diff/line_spec.rb @@ -0,0 +1,22 @@ +describe Gitlab::Diff::Line do + describe '.init_from_hash' do + it 'round-trips correctly with to_hash' do + line = described_class.new('<input>', 'match', 0, 0, 1, + parent_file: double(:file), + line_code: double(:line_code), + rich_text: '<input>') + + expect(described_class.init_from_hash(line.to_hash).to_hash) + .to eq(line.to_hash) + end + end + + 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("<input>") + end + end +end |