diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2019-09-02 15:05:33 +0300 |
---|---|---|
committer | John Jarvis <jarv@gitlab.com> | 2019-09-02 15:21:38 +0300 |
commit | 2e0a846e20c872fad6d47216662620866081253a (patch) | |
tree | 2f56d22eab0a8d8a75f5d753767be1b7a1edb15e | |
parent | c838459f836972f6e181db50032e57eb843cff98 (diff) |
Merge branch '66803-fix-uploads-relative-link-filter' into 'master'
Fix permissions check in `RelativeLinkFilter`
See merge request gitlab-org/gitlab-ce!32448
-rw-r--r-- | changelogs/unreleased/66803-fix-uploads-relative-link-filter.yml | 5 | ||||
-rw-r--r-- | lib/banzai/filter/relative_link_filter.rb | 13 | ||||
-rw-r--r-- | spec/helpers/markup_helper_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/banzai/filter/relative_link_filter_spec.rb | 121 |
4 files changed, 44 insertions, 101 deletions
diff --git a/changelogs/unreleased/66803-fix-uploads-relative-link-filter.yml b/changelogs/unreleased/66803-fix-uploads-relative-link-filter.yml new file mode 100644 index 00000000000..523e5c8c545 --- /dev/null +++ b/changelogs/unreleased/66803-fix-uploads-relative-link-filter.yml @@ -0,0 +1,5 @@ +--- +title: Fix upload URLs in Markdown for users without access to project repository +merge_request: 32448 +author: +type: fixed diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index 846a7d46aad..2b734db5cfb 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -19,7 +19,6 @@ module Banzai def call return doc if context[:system_note] - return doc unless visible_to_user? @uri_types = {} clear_memoization(:linkable_files) @@ -50,7 +49,7 @@ module Banzai if html_attr.value.start_with?('/uploads/') process_link_to_upload_attr(html_attr) - elsif linkable_files? + elsif linkable_files? && repo_visible_to_user? process_link_to_repository_attr(html_attr) end end @@ -168,14 +167,8 @@ 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 + def repo_visible_to_user? + project && Ability.allowed?(current_user, :download_code, project) end def ref diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index 1757ec8fa4d..f6e1720e113 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -65,9 +65,6 @@ 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 @@ -81,9 +78,6 @@ 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 diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index 789530fbc56..f8b3748c663 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -289,121 +289,72 @@ 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 - + shared_examples 'rewrite project uploads' do 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 + it 'rewrites the link correctly' do doc = filter(link(upload_path)) - expect(doc.at_css('a')['href']).to eq(upload_path) + expect(doc.at_css('a')['href']).to eq(absolute_path) end end - end - context 'with an absolute URL' do - let(:absolute_path) { Gitlab.config.gitlab.url + relative_path } - let(:only_path) { false } - - it 'rewrites the link correctly' do + it 'rebuilds relative URL for a link' do doc = filter(link(upload_path)) + expect(doc.at_css('a')['href']).to eq(relative_path) - expect(doc.at_css('a')['href']).to eq(absolute_path) + doc = filter(nested(link(upload_path))) + expect(doc.at_css('a')['href']).to eq(relative_path) end - end - it 'rebuilds relative URL for a link' do - doc = filter(link(upload_path)) - expect(doc.at_css('a')['href']).to eq(relative_path) + it 'rebuilds relative URL for an image' do + doc = filter(image(upload_path)) + expect(doc.at_css('img')['src']).to eq(relative_path) - doc = filter(nested(link(upload_path))) - expect(doc.at_css('a')['href']).to eq(relative_path) - end + doc = filter(nested(image(upload_path))) + expect(doc.at_css('img')['src']).to eq(relative_path) + end - it 'rebuilds relative URL for an image' do - doc = filter(image(upload_path)) - expect(doc.at_css('img')['src']).to eq(relative_path) + it 'does not modify absolute URL' do + doc = filter(link('http://example.com')) + expect(doc.at_css('a')['href']).to eq 'http://example.com' + end - doc = filter(nested(image(upload_path))) - expect(doc.at_css('img')['src']).to eq(relative_path) - end + it 'supports unescaped Unicode filenames' do + path = '/uploads/한글.png' + doc = filter(link(path)) - it 'does not modify absolute URL' do - doc = filter(link('http://example.com')) - expect(doc.at_css('a')['href']).to eq 'http://example.com' - end + expect(doc.at_css('a')['href']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png") + end - it 'supports unescaped Unicode filenames' do - path = '/uploads/한글.png' - doc = filter(link(path)) + it 'supports escaped Unicode filenames' do + path = '/uploads/한글.png' + escaped = Addressable::URI.escape(path) + doc = filter(image(escaped)) - expect(doc.at_css('a')['href']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png") + expect(doc.at_css('img')['src']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png") + end end - it 'supports escaped Unicode filenames' do - path = '/uploads/한글.png' - escaped = Addressable::URI.escape(path) - doc = filter(image(escaped)) + context 'without project repository access' do + let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) } + + it_behaves_like 'rewrite project uploads' + end - expect(doc.at_css('img')['src']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png") + context 'with project repository access' do + it_behaves_like 'rewrite project uploads' end end context 'to a group upload' do - let(:upload_path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' } - let(:upload_link) { link(upload_path) } + let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') } 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 } |