Commit 365217eb authored by José Iván Vargas López's avatar José Iván Vargas López

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
parents 1df7360f aa73b3e1
...@@ -32,9 +32,9 @@ ...@@ -32,9 +32,9 @@
%a{ href: "##{line_code}", data: { linenumber: link_text } } %a{ href: "##{line_code}", data: { linenumber: link_text } }
%td.line_content.noteable_line{ class: type }< %td.line_content.noteable_line{ class: type }<
- if email - if email
%pre= line.text %pre= line.rich_text
- else - else
= diff_line_content(line.text) = diff_line_content(line.rich_text)
- if line_discussions&.any? - if line_discussions&.any?
- discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?)) - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))
......
...@@ -24,7 +24,7 @@ ...@@ -24,7 +24,7 @@
- discussion_left = discussions_left.try(:first) - discussion_left = discussions_left.try(:first)
- if discussion_left && discussion_left.resolvable? - if discussion_left && discussion_left.resolvable?
%diff-note-avatars{ "discussion-id" => discussion_left.id } %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 - else
%td.old_line.diff-line-num.empty-cell %td.old_line.diff-line-num.empty-cell
%td.line_content.parallel.left-side %td.line_content.parallel.left-side
...@@ -45,7 +45,7 @@ ...@@ -45,7 +45,7 @@
- discussion_right = discussions_right.try(:first) - discussion_right = discussions_right.try(:first)
- if discussion_right && discussion_right.resolvable? - if discussion_right && discussion_right.resolvable?
%diff-note-avatars{ "discussion-id" => discussion_right.id } %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 - else
%td.old_line.diff-line-num.empty-cell %td.old_line.diff-line-num.empty-cell
%td.line_content.parallel.right-side %td.line_content.parallel.right-side
......
---
title: Fixed persistent XSS rendering/escaping of diff location lines
merge_request:
author:
type: security
...@@ -37,7 +37,7 @@ module Gitlab ...@@ -37,7 +37,7 @@ module Gitlab
end end
end end
diff_line.text = rich_line diff_line.rich_text = rich_line
diff_line diff_line
end end
......
module Gitlab module Gitlab
module Diff module Diff
class Line 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_reader :line_code, :type, :index, :old_pos, :new_pos
attr_writer :rich_text attr_writer :rich_text
attr_accessor :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 @text, @type, @index = text, type, index
@old_pos, @new_pos = old_pos, new_pos @old_pos, @new_pos = old_pos, new_pos
@parent_file = parent_file @parent_file = parent_file
@rich_text = rich_text
# When line code is not provided from cache store we build it # When line code is not provided from cache store we build it
# using the parent_file(Diff::File or Conflict::File). # using the parent_file(Diff::File or Conflict::File).
...@@ -18,7 +19,7 @@ module Gitlab ...@@ -18,7 +19,7 @@ module Gitlab
end end
def self.init_from_hash(hash) 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 end
def to_hash def to_hash
...@@ -85,7 +86,7 @@ module Gitlab ...@@ -85,7 +86,7 @@ module Gitlab
old_line: old_line, old_line: old_line,
new_line: new_line, new_line: new_line,
text: text, text: text,
rich_text: rich_text || text, rich_text: rich_text || CGI.escapeHTML(text),
meta_data: meta_positions meta_data: meta_positions
} }
end end
......
...@@ -2,6 +2,7 @@ require 'rails_helper' ...@@ -2,6 +2,7 @@ require 'rails_helper'
describe 'Merge request > User sees diff', :js do describe 'Merge request > User sees diff', :js do
include ProjectForksHelper include ProjectForksHelper
include RepoHelpers
let(:project) { create(:project, :public, :repository) } let(:project) { create(:project, :public, :repository) }
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
...@@ -81,5 +82,58 @@ describe 'Merge request > User sees diff', :js do ...@@ -81,5 +82,58 @@ describe 'Merge request > User sees diff', :js do
expect(page).to have_selector('.js-cancel-fork-suggestion-button', count: 1) expect(page).to have_selector('.js-cancel-fork-suggestion-button', count: 1)
end end
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
end end
...@@ -69,10 +69,6 @@ describe Gitlab::Conflict::File do ...@@ -69,10 +69,6 @@ describe Gitlab::Conflict::File do
CGI.unescapeHTML(ActionView::Base.full_sanitizer.sanitize(html)).delete("\n") CGI.unescapeHTML(ActionView::Base.full_sanitizer.sanitize(html)).delete("\n")
end 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 it 'is called implicitly when rich_text is accessed on a line' do
expect(conflict_file).to receive(:highlight_lines!).once.and_call_original expect(conflict_file).to receive(:highlight_lines!).once.and_call_original
......
...@@ -24,19 +24,19 @@ describe Gitlab::Diff::Highlight do ...@@ -24,19 +24,19 @@ describe Gitlab::Diff::Highlight do
it 'highlights and marks unchanged lines' 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} 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 end
it 'highlights and marks removed lines' do 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} 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 end
it 'highlights and marks added lines' do 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} 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
end end
...@@ -69,8 +69,8 @@ describe Gitlab::Diff::Highlight do ...@@ -69,8 +69,8 @@ describe Gitlab::Diff::Highlight do
it 'marks added lines' 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;} 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 end
context 'when the inline diff marker has an invalid range' do context 'when the inline diff marker has an invalid range' do
......
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: '&lt;input&gt;')
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("&lt;input&gt;")
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment