summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/serializers/diff_line_entity.rb2
-rw-r--r--changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml5
-rw-r--r--lib/gitlab/diff/highlight.rb2
-rw-r--r--spec/lib/gitlab/diff/highlight_spec.rb28
-rw-r--r--spec/serializers/diff_line_entity_spec.rb45
5 files changed, 80 insertions, 2 deletions
diff --git a/app/serializers/diff_line_entity.rb b/app/serializers/diff_line_entity.rb
index 2119a1017d3..942714b7787 100644
--- a/app/serializers/diff_line_entity.rb
+++ b/app/serializers/diff_line_entity.rb
@@ -9,6 +9,6 @@ class DiffLineEntity < Grape::Entity
expose :meta_positions, as: :meta_data
expose :rich_text do |line|
- line.rich_text || CGI.escapeHTML(line.text)
+ ERB::Util.html_escape(line.rich_text || line.text)
end
end
diff --git a/changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml b/changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml
new file mode 100644
index 00000000000..7520aa624c7
--- /dev/null
+++ b/changelogs/unreleased/security-fj-stored-xss-in-repository-imports.yml
@@ -0,0 +1,5 @@
+---
+title: Fix stored XSS in merge requests from imported repository
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb
index 1f012043e56..a605ddb5c33 100644
--- a/lib/gitlab/diff/highlight.rb
+++ b/lib/gitlab/diff/highlight.rb
@@ -24,7 +24,7 @@ module Gitlab
# ignore highlighting for "match" lines
next diff_line if diff_line.meta?
- rich_line = highlight_line(diff_line) || diff_line.text
+ rich_line = highlight_line(diff_line) || ERB::Util.html_escape(diff_line.text)
if line_inline_diffs = inline_diffs[i]
begin
diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb
index 3c8cf9c56cc..5d0a603d11d 100644
--- a/spec/lib/gitlab/diff/highlight_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_spec.rb
@@ -8,6 +8,20 @@ describe Gitlab::Diff::Highlight do
let(:diff) { commit.raw_diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
+ shared_examples 'without inline diffs' do
+ let(:code) { '<h2 onmouseover="alert(2)">Test</h2>' }
+
+ before do
+ allow(Gitlab::Diff::InlineDiff).to receive(:for_lines).and_return([])
+ allow_any_instance_of(Gitlab::Diff::Line).to receive(:text).and_return(code)
+ end
+
+ it 'returns html escaped diff text' do
+ expect(subject[1].rich_text).to eq html_escape(code)
+ expect(subject[1].rich_text).to be_html_safe
+ end
+ end
+
describe '#highlight' do
context "with a diff file" do
let(:subject) { described_class.new(diff_file, repository: project.repository).highlight }
@@ -38,6 +52,16 @@ describe Gitlab::Diff::Highlight do
expect(subject[5].rich_text).to eq(code)
end
+
+ context 'when no diff_refs' do
+ before do
+ allow(diff_file).to receive(:diff_refs).and_return(nil)
+ end
+
+ context 'when no inline diffs' do
+ it_behaves_like 'without inline diffs'
+ end
+ end
end
context "with diff lines" do
@@ -93,6 +117,10 @@ describe Gitlab::Diff::Highlight do
expect { subject }. to raise_exception(RangeError)
end
end
+
+ context 'when no inline diffs' do
+ it_behaves_like 'without inline diffs'
+ end
end
end
end
diff --git a/spec/serializers/diff_line_entity_spec.rb b/spec/serializers/diff_line_entity_spec.rb
new file mode 100644
index 00000000000..2549f64bcd3
--- /dev/null
+++ b/spec/serializers/diff_line_entity_spec.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DiffLineEntity do
+ include RepoHelpers
+
+ let(:code) { 'hello world' }
+ let(:line) { Gitlab::Diff::Line.new(code, 'new', 1, nil, 1) }
+ let(:entity) { described_class.new(line, request: {}) }
+
+ subject { entity.as_json }
+
+ it 'exposes correct attributes' do
+ expect(subject).to include(
+ :line_code, :type, :old_line, :new_line, :text, :meta_data, :rich_text
+ )
+ end
+
+ describe '#rich_text' do
+ let(:code) { '<h2 onmouseover="alert(2)">Test</h2>' }
+ let(:rich_text_value) { nil }
+
+ before do
+ line.instance_variable_set(:@rich_text, rich_text_value)
+ end
+
+ shared_examples 'escapes html tags' do
+ it do
+ expect(subject[:rich_text]).to eq html_escape(code)
+ expect(subject[:rich_text]).to be_html_safe
+ end
+ end
+
+ context 'when rich_line is present' do
+ let(:rich_text_value) { code }
+
+ it_behaves_like 'escapes html tags'
+ end
+
+ context 'when rich_line is not present' do
+ it_behaves_like 'escapes html tags'
+ end
+ end
+end