Commit edf7dbfa authored by Robert Speicher's avatar Robert Speicher Committed by Alejandro Rodríguez

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 6e1b52b8
...@@ -55,7 +55,9 @@ module DiffHelper ...@@ -55,7 +55,9 @@ module DiffHelper
if line.blank? if line.blank?
" ".html_safe " ".html_safe
else 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
end end
......
---
title: Don't accidentally mark unsafe diff lines as HTML safe
merge_request:
author:
...@@ -60,15 +60,58 @@ describe DiffHelper do ...@@ -60,15 +60,58 @@ describe DiffHelper do
end end
describe '#diff_line_content' do describe '#diff_line_content' do
it 'returns non breaking space when line is empty' do context 'when the line is empty' do
expect(diff_line_content(nil)).to eq(' ') it 'returns a non breaking space' do
end expect(diff_line_content(nil)).to eq(' ')
end
it 'returns the line itself' do
expect(diff_line_content(diff_file.diff_lines.first.text)). it 'returns an HTML-safe string' do
to eq('@@ -6,12 +6,18 @@ module Popen') expect(diff_line_content(nil)).to be_html_safe
expect(diff_line_content(diff_file.diff_lines.first.type)).to eq('match') end
expect(diff_file.diff_lines.first.new_pos).to eq(6) 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
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