diff options
-rw-r--r-- | app/helpers/wiki_page_version_helper.rb | 20 | ||||
-rw-r--r-- | app/views/shared/wikis/show.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/security-fj-fix-xss-wiki-email.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/git/wiki_page_version.rb | 9 | ||||
-rw-r--r-- | spec/helpers/wiki_page_version_helper_spec.rb | 80 | ||||
-rw-r--r-- | spec/lib/gitlab/git/wiki_page_version_spec.rb | 14 |
6 files changed, 119 insertions, 11 deletions
diff --git a/app/helpers/wiki_page_version_helper.rb b/app/helpers/wiki_page_version_helper.rb new file mode 100644 index 00000000000..ae20717ad99 --- /dev/null +++ b/app/helpers/wiki_page_version_helper.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module WikiPageVersionHelper + def wiki_page_version_author_url(wiki_page_version) + user = wiki_page_version.author + user.nil? ? "mailto:#{wiki_page_version.author_email}" : Gitlab::UrlBuilder.build(user) + end + + def wiki_page_version_author_avatar(wiki_page_version) + image_tag(avatar_icon_for_email(wiki_page_version.author_email, 24), class: "avatar s24 float-none gl-mr-0!") + end + + def wiki_page_version_author_header(wiki_page_version) + avatar = wiki_page_version_author_avatar(wiki_page_version) + name = "<strong>".html_safe + wiki_page_version.author_name + "</strong>".html_safe + link_start = "<a href='".html_safe + wiki_page_version_author_url(wiki_page_version) + "'>".html_safe + + html_escape(_("Last edited by %{link_start}%{avatar} %{name}%{link_end}")) % { avatar: avatar, name: name, link_start: link_start, link_end: '</a>'.html_safe } + end +end diff --git a/app/views/shared/wikis/show.html.haml b/app/views/shared/wikis/show.html.haml index 6d14ba8fe7b..8a5cd94bde9 100644 --- a/app/views/shared/wikis/show.html.haml +++ b/app/views/shared/wikis/show.html.haml @@ -7,7 +7,7 @@ .nav-text.flex-fill %span.wiki-last-edit-by - if @page.last_version - = html_escape(_("Last edited by %{link_start}%{avatar} %{name}%{link_end}")) % { avatar: image_tag(avatar_icon_for_email(@page.last_version.author_email, 24), class: "avatar s24 float-none gl-mr-0!"), name: "<strong>#{@page.last_version.author_name}</strong>".html_safe, link_start: "<a href='#{@page.last_version.author_url}'>".html_safe, link_end: '</a>'.html_safe } + = wiki_page_version_author_header(@page.last_version) = time_ago_with_tooltip(@page.last_version.authored_date) .nav-controls.pb-md-3.pb-lg-0 diff --git a/changelogs/unreleased/security-fj-fix-xss-wiki-email.yml b/changelogs/unreleased/security-fj-fix-xss-wiki-email.yml new file mode 100644 index 00000000000..9faabdc9750 --- /dev/null +++ b/changelogs/unreleased/security-fj-fix-xss-wiki-email.yml @@ -0,0 +1,5 @@ +--- +title: Fix XSS in wiki author email and name +merge_request: +author: +type: security diff --git a/lib/gitlab/git/wiki_page_version.rb b/lib/gitlab/git/wiki_page_version.rb index efe39fa852c..1de0a0204cf 100644 --- a/lib/gitlab/git/wiki_page_version.rb +++ b/lib/gitlab/git/wiki_page_version.rb @@ -3,6 +3,8 @@ module Gitlab module Git class WikiPageVersion + include Gitlab::Utils::StrongMemoize + attr_reader :commit, :format def initialize(commit, format) @@ -12,9 +14,10 @@ module Gitlab delegate :message, :sha, :id, :author_name, :author_email, :authored_date, to: :commit - def author_url - user = ::User.find_by_any_email(author_email) - user.nil? ? "mailto:#{author_email}" : Gitlab::UrlBuilder.build(user) + def author + strong_memoize(:author) do + ::User.find_by_any_email(author_email) + end end end end diff --git a/spec/helpers/wiki_page_version_helper_spec.rb b/spec/helpers/wiki_page_version_helper_spec.rb new file mode 100644 index 00000000000..bc500c28c5a --- /dev/null +++ b/spec/helpers/wiki_page_version_helper_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WikiPageVersionHelper do + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:user) { create(:user, username: 'foo') } + + let(:commit_with_user) { create(:commit, project: project, author: user)} + let(:commit_without_user) { create(:commit, project: project, author_name: 'Foo', author_email: 'foo@example.com')} + let(:wiki_page_version) { Gitlab::Git::WikiPageVersion.new(commit, nil) } + + describe '#wiki_page_version_author_url' do + subject { helper.wiki_page_version_author_url(wiki_page_version) } + + context 'when user exists' do + let(:commit) { commit_with_user } + + it 'returns the link to the user profile' do + expect(subject).to eq('http://localhost/foo') + end + end + + context 'when user does not exist' do + let(:commit) { commit_without_user } + + it 'returns the mailto link' do + expect(subject).to eq "mailto:#{commit_without_user.author_email}" + end + end + end + + describe '#wiki_page_version_author_avatar' do + let(:commit) { commit_with_user } + + subject { helper.wiki_page_version_author_avatar(wiki_page_version) } + + it 'returns the user avatar', :aggregate_failures do + avatar = Nokogiri::HTML.parse(subject) + + expect(avatar.css('img')[0].attr('class')).to eq('avatar s24 float-none gl-mr-0! lazy') + expect(avatar.css('img')[0].attr('data-src')).not_to be_empty + expect(avatar.css('img')[0].attr('src')).not_to be_empty + end + end + + describe '#wiki_page_version_author_header', :aggregate_failures do + let(:commit_with_xss) { create(:commit, project: project, author_email: "#' style=animation-name:blinking-dot onanimationstart=alert(document.domain) other", author_name: "<i>foo</i>") } + let(:header) { Nokogiri::HTML.parse(subject) } + + subject { helper.wiki_page_version_author_header(wiki_page_version) } + + context 'when user exists' do + let(:commit) { commit_with_user } + + it 'renders commit header with user info' do + expect(header.css('a')[0].attr('href')).to eq("http://localhost/foo") + expect(header.css('a')[0].children[2].to_s).to eq("<strong>#{user.name}</strong>") + end + end + + context 'when user does not exist' do + let(:commit) { commit_without_user } + + it 'renders commit header with info from commit' do + expect(header.css('a')[0].attr('href')).to eq("mailto:#{commit.author_email}") + expect(header.css('a')[0].children[2].to_s).to eq("<strong>#{wiki_page_version.author_name}</strong>") + end + end + + context 'when user info has XSS' do + let(:commit) { commit_with_xss } + + it 'sets the right href and escapes HTML chars' do + expect(header.css('a')[0].attr('href')).to eq("mailto:#{commit.author_email}") + expect(header.css('a')[0].children[2].to_s).to eq("<strong><i>foo</i></strong>") + end + end + end +end diff --git a/spec/lib/gitlab/git/wiki_page_version_spec.rb b/spec/lib/gitlab/git/wiki_page_version_spec.rb index 836fa2449ec..b117e757f6e 100644 --- a/spec/lib/gitlab/git/wiki_page_version_spec.rb +++ b/spec/lib/gitlab/git/wiki_page_version_spec.rb @@ -4,24 +4,24 @@ require 'spec_helper' RSpec.describe Gitlab::Git::WikiPageVersion do let_it_be(:project) { create(:project, :public, :repository) } - let(:user) { create(:user, username: 'someone') } + let_it_be(:user) { create(:user, username: 'someone') } - describe '#author_url' do - subject(:author_url) { described_class.new(commit, nil).author_url } + describe '#author' do + subject(:author) { described_class.new(commit, nil).author } context 'user exists in gitlab' do let(:commit) { create(:commit, project: project, author: user) } - it 'returns the profile link of the user' do - expect(author_url).to eq('http://localhost/someone') + it 'returns the user' do + expect(author).to eq user end end context 'user does not exist in gitlab' do let(:commit) { create(:commit, project: project, author_email: "someone@somewebsite.com") } - it 'returns a mailto: url' do - expect(author_url).to eq('mailto:someone@somewebsite.com') + it 'returns nil' do + expect(author).to be_nil end end end |