kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Avoid exposing unaccessible repo data upon GFM processing

Browse source 
When post-processing relative links to absolute links
RelativeLinkFilter didn't take into consideration that
internal repository data could be exposed for users
that do not have repository access to the project.

This commit solves that by checking whether the user
can `download_code` at this repository, avoiding any
processing of this filter if the user can't.

Additionally, if we're processing for a group (
no project was given), we check if the user can
read it in order to expand the href as an extra.
That doesn't seem necessarily a breach now,
but an extra check doesn't hurt as after all
the user needs to be able to `read_group`.
This commit is contained in:
Oswaldo Ferreira 2019-08-20 17:36:57 -03:00
parent 50ff074e79
commit 4daf3dc0db
4 changed files with 97 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
changelogs/unreleased/security-exposed-default-branch.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Avoid exposing unaccessible repo data upon GFM post processing
merge_request:
author:
type: security

16
lib/banzai/filter/relative_link_filter.rb
View file

@ -9,6 +9,7 @@ module Banzai
# Context options:
# :commit
# :group
# :current_user
# :project
# :project_wiki
# :ref
@ -18,6 +19,7 @@ module Banzai
def call
return doc if context[:system_note]
return doc unless visible_to_user?
@uri_types = {}
clear_memoization(:linkable_files)
@ -166,6 +168,16 @@ module Banzai
Gitlab.config.gitlab.relative_url_root.presence || '/'
end
def visible_to_user?
if project
Ability.allowed?(current_user, :download_code, project)
elsif group
Ability.allowed?(current_user, :read_group, group)
else # Objects detached from projects or groups, e.g. Personal Snippets.
true
end
end
def ref
context[:ref] || project.default_branch
end
@ -178,6 +190,10 @@ module Banzai
context[:project]
end
def current_user
context[:current_user]
end
def repository
@repository ||= project&.repository
end

6
spec/helpers/markup_helper_spec.rb
View file

@ -65,6 +65,9 @@ describe MarkupHelper do
describe 'inside a group' do
before do
# Ensure the generated reference links aren't redacted
group.add_maintainer(user)
helper.instance_variable_set(:@group, group)
helper.instance_variable_set(:@project, nil)
end
@ -78,6 +81,9 @@ describe MarkupHelper do
let(:project_in_group) { create(:project, group: group) }
before do
# Ensure the generated reference links aren't redacted
project_in_group.add_maintainer(user)
helper.instance_variable_set(:@group, group)
helper.instance_variable_set(:@project, project_in_group)
end

72
spec/lib/banzai/filter/relative_link_filter_spec.rb
View file

@ -7,6 +7,7 @@ describe Banzai::Filter::RelativeLinkFilter do
contexts.reverse_merge!({
commit: commit,
project: project,
current_user: user,
group: group,
project_wiki: project_wiki,
ref: ref,
@ -33,7 +34,8 @@ describe Banzai::Filter::RelativeLinkFilter do
%(<div>#{element}</div>)
end
let(:project) { create(:project, :repository) }
let(:project) { create(:project, :repository, :public) }
let(:user) { create(:user) }
let(:group) { nil }
let(:project_path) { project.full_path }
let(:ref) { 'markdown' }
@ -75,6 +77,11 @@ describe Banzai::Filter::RelativeLinkFilter do
include_examples :preserve_unchanged
end
context 'without project repository access' do
let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) }
include_examples :preserve_unchanged
end
it 'does not raise an exception on invalid URIs' do
act = link("://foo")
expect { filter(act) }.not_to raise_error
@ -282,6 +289,37 @@ describe Banzai::Filter::RelativeLinkFilter do
let(:relative_path) { "/#{project.full_path}#{upload_path}" }
context 'to a project upload' do
context 'without project repository access' do
let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) }
it 'does not rebuild relative URL for a link' do
doc = filter(link(upload_path))
expect(doc.at_css('a')['href']).to eq(upload_path)
doc = filter(nested(link(upload_path)))
expect(doc.at_css('a')['href']).to eq(upload_path)
end
it 'does not rebuild relative URL for an image' do
doc = filter(image(upload_path))
expect(doc.at_css('img')['src']).to eq(upload_path)
doc = filter(nested(image(upload_path)))
expect(doc.at_css('img')['src']).to eq(upload_path)
end
context 'with an absolute URL' do
let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
let(:only_path) { false }
it 'does not rewrite the link' do
doc = filter(link(upload_path))
expect(doc.at_css('a')['href']).to eq(upload_path)
end
end
end
context 'with an absolute URL' do
let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
let(:only_path) { false }
@ -331,11 +369,41 @@ describe Banzai::Filter::RelativeLinkFilter do
end
context 'to a group upload' do
let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
let(:upload_path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' }
let(:upload_link) { link(upload_path) }
let(:group) { create(:group) }
let(:project) { nil }
let(:relative_path) { "/groups/#{group.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg" }
context 'without group read access' do
let(:group) { create(:group, :private) }
it 'does not rewrite the link' do
doc = filter(upload_link)
expect(doc.at_css('a')['href']).to eq(upload_path)
end
it 'does not rewrite the link for subgroup' do
group.update!(parent: create(:group))
doc = filter(upload_link)
expect(doc.at_css('a')['href']).to eq(upload_path)
end
context 'with an absolute URL' do
let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
let(:only_path) { false }
it 'does not rewrite the link' do
doc = filter(upload_link)
expect(doc.at_css('a')['href']).to eq(upload_path)
end
end
end
context 'with an absolute URL' do
let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
let(:only_path) { false }