Commit 51764c07 authored by Robert Speicher's avatar Robert Speicher Committed by Alejandro Rodriguez

Merge branch 'html-safe-diff-line-content' into 'security'

Don't accidentally mark unsafe diff lines as HTML safe

Fixes potential XSS issue when a legacy diff note is created on a merge
request whose diff contained HTML

See https://gitlab.com/gitlab-org/gitlab-ce/issues/25249

See merge request !2040
parent 07217fb1
......@@ -55,7 +55,9 @@ module DiffHelper
if line.blank?
" ".html_safe
else
line.sub(/^[\-+ ]/, '').html_safe
# We can't use `sub` because the HTML-safeness of `line` will not survive.
line[0] = '' if line.start_with?('+', '-', ' ')
line
end
end
......
---
title: Don't accidentally mark unsafe diff lines as HTML safe
merge_request:
author:
......@@ -60,15 +60,58 @@ describe DiffHelper do
end
describe '#diff_line_content' do
it 'returns non breaking space when line is empty' do
context 'when the line is empty' do
it 'returns a non breaking space' do
expect(diff_line_content(nil)).to eq(' ')
end
it 'returns the line itself' do
expect(diff_line_content(diff_file.diff_lines.first.text)).
to eq('@@ -6,12 +6,18 @@ module Popen')
expect(diff_line_content(diff_file.diff_lines.first.type)).to eq('match')
expect(diff_file.diff_lines.first.new_pos).to eq(6)
it 'returns an HTML-safe string' do
expect(diff_line_content(nil)).to be_html_safe
end
end
context 'when the line is not empty' do
context 'when the line starts with +, -, or a space' do
it 'strips the first character' do
expect(diff_line_content('+new line')).to eq('new line')
expect(diff_line_content('-new line')).to eq('new line')
expect(diff_line_content(' new line')).to eq('new line')
end
context 'when the line is HTML-safe' do
it 'returns an HTML-safe string' do
expect(diff_line_content('+new line'.html_safe)).to be_html_safe
expect(diff_line_content('-new line'.html_safe)).to be_html_safe
expect(diff_line_content(' new line'.html_safe)).to be_html_safe
end
end
context 'when the line is not HTML-safe' do
it 'returns a non-HTML-safe string' do
expect(diff_line_content('+new line')).not_to be_html_safe
expect(diff_line_content('-new line')).not_to be_html_safe
expect(diff_line_content(' new line')).not_to be_html_safe
end
end
end
context 'when the line does not start with a +, -, or a space' do
it 'returns the string' do
expect(diff_line_content('@@ -6,12 +6,18 @@ module Popen')).to eq('@@ -6,12 +6,18 @@ module Popen')
end
context 'when the line is HTML-safe' do
it 'returns an HTML-safe string' do
expect(diff_line_content('@@ -6,12 +6,18 @@ module Popen'.html_safe)).to be_html_safe
end
end
context 'when the line is not HTML-safe' do
it 'returns a non-HTML-safe string' do
expect(diff_line_content('@@ -6,12 +6,18 @@ module Popen')).not_to be_html_safe
end
end
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