Commit 4b5489e5 authored by Douwe Maan's avatar Douwe Maan Committed by Rémy Coutable

Merge branch 'issue_23548_dev' into 'master'

disable markdown in comments when referencing disabled features

fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/23548

This MR prevents the following references when tool is disabled:

- issues
- snippets
- commits - when repo is disabled
- commit range - when repo is disabled
- milestones

This MR does not prevent references to repository files, since they are just markdown links and don't leak
information.

See merge request !2011
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 2a51557b
......@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
## 8.13.4
- Honour issue and merge request visibility in their respective finders.
- Disable reference Markdown for unavailable features.
## 8.13.3 (2016-11-02)
......
......@@ -245,10 +245,39 @@ class Issue < ActiveRecord::Base
# Returns `true` if the current issue can be viewed by either a logged in User
# or an anonymous user.
def visible_to_user?(user = nil)
return false unless project.feature_available?(:issues, user)
user ? readable_by?(user) : publicly_visible?
end
def overdue?
due_date.try(:past?) || false
end
# Only issues on public projects should be checked for spam
def check_for_spam?
project.public?
end
def as_json(options = {})
super(options).tap do |json|
if options.has_key?(:labels)
json[:labels] = labels.as_json(
project: project,
only: [:id, :title, :description, :color],
methods: [:text_color]
)
end
end
end
private
# Returns `true` if the given User can read the current Issue.
#
# This method duplicates the same check of issue_policy.rb
# for performance reasons, check commit: 002ad215818450d2cbbc5fa065850a953dc7ada8
# Make sure to sync this method with issue_policy.rb
def readable_by?(user)
if user.admin?
true
......@@ -269,25 +298,4 @@ class Issue < ActiveRecord::Base
def publicly_visible?
project.public? && !confidential?
end
def overdue?
due_date.try(:past?) || false
end
# Only issues on public projects should be checked for spam
def check_for_spam?
project.public?
end
def as_json(options = {})
super(options).tap do |json|
if options.has_key?(:labels)
json[:labels] = labels.as_json(
project: project,
only: [:id, :title, :description, :color],
methods: [:text_color]
)
end
end
end
end
class IssuePolicy < IssuablePolicy
# This class duplicates the same check of Issue#readable_by? for performance reasons
# Make sure to sync this class checks with issue.rb to avoid security problems.
# Check commit 002ad215818450d2cbbc5fa065850a953dc7ada8 for more information.
def issue
@subject
end
......
......@@ -63,12 +63,7 @@ module Banzai
nodes.select do |node|
if node.has_attribute?(project_attr)
node_id = node.attr(project_attr).to_i
if project && project.id == node_id
true
else
can?(user, :read_project, projects[node_id])
end
can_read_reference?(user, projects[node_id])
else
true
end
......@@ -226,6 +221,15 @@ module Banzai
attr_reader :current_user, :project
# When a feature is disabled or visible only for
# team members we should not allow team members
# see reference comments.
# Override this method on subclasses
# to check if user can read resource
def can_read_reference?(user, ref_project)
raise NotImplementedError
end
def lazy(&block)
Gitlab::Lazy.new(&block)
end
......
......@@ -29,6 +29,12 @@ module Banzai
commits
end
private
def can_read_reference?(user, ref_project)
can?(user, :download_code, ref_project)
end
end
end
end
......@@ -33,6 +33,12 @@ module Banzai
range.valid_commits? ? range : nil
end
private
def can_read_reference?(user, ref_project)
can?(user, :download_code, ref_project)
end
end
end
end
......@@ -20,6 +20,12 @@ module Banzai
def issue_ids_per_project(nodes)
gather_attributes_per_project(nodes, self.class.data_attribute)
end
private
def can_read_reference?(user, ref_project)
can?(user, :read_issue, ref_project)
end
end
end
end
......@@ -6,6 +6,12 @@ module Banzai
def references_relation
Label
end
private
def can_read_reference?(user, ref_project)
can?(user, :read_label, ref_project)
end
end
end
end
......@@ -6,6 +6,12 @@ module Banzai
def references_relation
MergeRequest.includes(:author, :assignee, :target_project)
end
private
def can_read_reference?(user, ref_project)
can?(user, :read_merge_request, ref_project)
end
end
end
end
......@@ -6,6 +6,12 @@ module Banzai
def references_relation
Milestone
end
private
def can_read_reference?(user, ref_project)
can?(user, :read_milestone, ref_project)
end
end
end
end
......@@ -6,6 +6,12 @@ module Banzai
def references_relation
Snippet
end
private
def can_read_reference?(user, ref_project)
can?(user, :read_project_snippet, ref_project)
end
end
end
end
......@@ -30,22 +30,36 @@ module Banzai
nodes.each do |node|
if node.has_attribute?(group_attr)
node_group = groups[node.attr(group_attr).to_i]
if node_group &&
can?(user, :read_group, node_group)
visible << node
end
# Remaining nodes will be processed by the parent class'
# implementation of this method.
next unless can_read_group_reference?(node, user, groups)
visible << node
elsif can_read_project_reference?(node)
visible << node
else
remaining << node
end
end
# If project does not belong to a group
# and does not have the same project id as the current project
# base class will check if user can read the project that contains
# the user reference.
visible + super(current_user, remaining)
end
# Check if project belongs to a group which
# user can read.
def can_read_group_reference?(node, user, groups)
node_group = groups[node.attr('data-group').to_i]
node_group && can?(user, :read_group, node_group)
end
def can_read_project_reference?(node)
node_id = node.attr('data-project').to_i
project && project.id == node_id
end
def nodes_user_can_reference(current_user, nodes)
project_attr = 'data-project'
author_attr = 'data-author'
......@@ -88,6 +102,10 @@ module Banzai
collection_objects_for_ids(Project, ids).
flat_map { |p| p.team.members.to_a }
end
def can_read_reference?(user, ref_project)
can?(user, :read_project, ref_project)
end
end
end
end
......@@ -22,6 +22,7 @@ feature 'Start new branch from an issue', feature: true do
create(:note, :on_issue, :system, project: project,
note: "Mentioned in !#{referenced_mr.iid}")
end
let(:referenced_mr) do
create(:merge_request, :simple, source_project: project, target_project: project,
description: "Fixes ##{issue.iid}", author: user)
......
......@@ -28,31 +28,39 @@ describe Banzai::Filter::RedactorFilter, lib: true do
and_return(parser_class)
end
it 'removes unpermitted Project references' do
user = create(:user)
project = create(:empty_project)
context 'valid projects' do
before { allow_any_instance_of(Banzai::ReferenceParser::BaseParser).to receive(:can_read_reference?).and_return(true) }
link = reference_link(project: project.id, reference_type: 'test')
doc = filter(link, current_user: user)
it 'allows permitted Project references' do
user = create(:user)
project = create(:empty_project)
project.team << [user, :master]
link = reference_link(project: project.id, reference_type: 'test')
doc = filter(link, current_user: user)
expect(doc.css('a').length).to eq 0
expect(doc.css('a').length).to eq 1
end
end
it 'allows permitted Project references' do
user = create(:user)
project = create(:empty_project)
project.team << [user, :master]
context 'invalid projects' do
before { allow_any_instance_of(Banzai::ReferenceParser::BaseParser).to receive(:can_read_reference?).and_return(false) }
link = reference_link(project: project.id, reference_type: 'test')
doc = filter(link, current_user: user)
it 'removes unpermitted references' do
user = create(:user)
project = create(:empty_project)
expect(doc.css('a').length).to eq 1
end
link = reference_link(project: project.id, reference_type: 'test')
doc = filter(link, current_user: user)
it 'handles invalid Project references' do
link = reference_link(project: 12345, reference_type: 'test')
expect(doc.css('a').length).to eq 0
end
it 'handles invalid references' do
link = reference_link(project: 12345, reference_type: 'test')
expect { filter(link) }.not_to raise_error
expect { filter(link) }.not_to raise_error
end
end
end
......
......@@ -27,41 +27,12 @@ describe Banzai::ReferenceParser::BaseParser, lib: true do
let(:link) { empty_html_link }
context 'when the link has a data-project attribute' do
it 'returns the nodes if the attribute value equals the current project ID' do
it 'checks if user can read the resource' do
link['data-project'] = project.id.to_s
expect(Ability).not_to receive(:allowed?)
expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
end
it 'returns the nodes if the user can read the project' do
other_project = create(:empty_project, :public)
link['data-project'] = other_project.id.to_s
expect(Ability).to receive(:allowed?).
with(user, :read_project, other_project).
and_return(true)
expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
end
it 'returns an empty Array when the attribute value is empty' do
link['data-project'] = ''
expect(subject.nodes_visible_to_user(user, [link])).to eq([])
end
it 'returns an empty Array when the user can not read the project' do
other_project = create(:empty_project, :public)
link['data-project'] = other_project.id.to_s
expect(Ability).to receive(:allowed?).
with(user, :read_project, other_project).
and_return(false)
expect(subject).to receive(:can_read_reference?).with(user, project)
expect(subject.nodes_visible_to_user(user, [link])).to eq([])
subject.nodes_visible_to_user(user, [link])
end
end
......
......@@ -8,6 +8,14 @@ describe Banzai::ReferenceParser::CommitParser, lib: true do
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
before { link['data-commit'] = 123 }
it_behaves_like "referenced feature visibility", "repository"
end
end
describe '#referenced_by' do
context 'when the link has a data-project attribute' do
before do
......
......@@ -8,6 +8,14 @@ describe Banzai::ReferenceParser::CommitRangeParser, lib: true do
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
before { link['data-commit-range'] = '123..456' }
it_behaves_like "referenced feature visibility", "repository"
end
end
describe '#referenced_by' do
context 'when the link has a data-project attribute' do
before do
......
......@@ -8,6 +8,14 @@ describe Banzai::ReferenceParser::ExternalIssueParser, lib: true do
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
before { link['data-external-issue'] = 123 }
it_behaves_like "referenced feature visibility", "issues"
end
end
describe '#referenced_by' do
context 'when the link has a data-project attribute' do
before do
......
......@@ -4,10 +4,10 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do
include ReferenceParserHelpers
let(:project) { create(:empty_project, :public) }
let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) }
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) }
let(:link) { empty_html_link }
subject { described_class.new(project, user) }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
......@@ -15,6 +15,8 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do
link['data-issue'] = issue.id.to_s
end
it_behaves_like "referenced feature visibility", "issues"
it 'returns the nodes when the user can read the issue' do
expect(Ability).to receive(:issues_readable_by_user).
with([issue], user).
......
......@@ -9,6 +9,14 @@ describe Banzai::ReferenceParser::LabelParser, lib: true do
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
before { link['data-label'] = label.id.to_s }
it_behaves_like "referenced feature visibility", "issues", "merge_requests"
end
end
describe '#referenced_by' do
describe 'when the link has a data-label attribute' do
context 'using an existing label ID' do
......
......@@ -8,6 +8,19 @@ describe Banzai::ReferenceParser::MergeRequestParser, lib: true do
subject { described_class.new(merge_request.target_project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
let(:project) { merge_request.target_project }
before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
link['data-merge-request'] = merge_request.id.to_s
end
it_behaves_like "referenced feature visibility", "merge_requests"
end
end
describe '#referenced_by' do
describe 'when the link has a data-merge-request attribute' do
context 'using an existing merge request ID' do
......
......@@ -9,6 +9,14 @@ describe Banzai::ReferenceParser::MilestoneParser, lib: true do
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
before { link['data-milestone'] = milestone.id.to_s }
it_behaves_like "referenced feature visibility", "issues", "merge_requests"
end
end
describe '#referenced_by' do
describe 'when the link has a data-milestone attribute' do
context 'using an existing milestone ID' do
......
......@@ -9,6 +9,14 @@ describe Banzai::ReferenceParser::SnippetParser, lib: true do
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
before { link['data-snippet'] = snippet.id.to_s }
it_behaves_like "referenced feature visibility", "snippets"
end
end
describe '#referenced_by' do
describe 'when the link has a data-snippet attribute' do
context 'using an existing snippet ID' do
......
......@@ -103,6 +103,8 @@ describe Banzai::ReferenceParser::UserParser, lib: true do
it 'returns the nodes if the attribute value equals the current project ID' do
link['data-project'] = project.id.to_s
# Ensure that we dont call for Ability.allowed?
# When project_id in the node is equal to current project ID
expect(Ability).not_to receive(:allowed?)
expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
......
......@@ -257,8 +257,9 @@ describe Gitlab::ClosingIssueExtractor, lib: true do
context 'with an external issue tracker reference' do
it 'extracts the referenced issue' do
jira_project = create(:jira_project, name: 'JIRA_EXT1')
jira_project.team << [jira_project.creator, :master]
jira_issue = ExternalIssue.new("#{jira_project.name}-1", project: jira_project)
closing_issue_extractor = described_class.new jira_project
closing_issue_extractor = described_class.new(jira_project, jira_project.creator)
message = "Resolve #{jira_issue.to_reference}"
expect(closing_issue_extractor.closed_by_message(message)).to eq([jira_issue])
......
......@@ -6,7 +6,7 @@ describe Gitlab::Gfm::ReferenceRewriter do
let(:new_project) { create(:project, name: 'new') }
let(:user) { create(:user) }
before { old_project.team << [user, :guest] }
before { old_project.team << [user, :reporter] }
describe '#rewrite' do
subject do
......
......@@ -3,6 +3,8 @@ require 'spec_helper'
describe Gitlab::ReferenceExtractor, lib: true do
let(:project) { create(:project) }
before { project.team << [project.creator, :developer] }
subject { Gitlab::ReferenceExtractor.new(project, project.creator) }
it 'accesses valid user objects' do
......@@ -42,7 +44,6 @@ describe Gitlab::ReferenceExtractor, lib: true do
end
it 'accesses valid issue objects' do
project.team << [project.creator, :developer]
@i0 = create(:issue, project: project)
@i1 = create(:issue, project: project)
......
......@@ -22,7 +22,7 @@ describe Issue, models: true do
it { is_expected.to have_db_index(:deleted_at) }
end
describe 'visible_to_user' do
describe '.visible_to_user' do
let(:user) { create(:user) }
let(:authorized_user) { create(:user) }
let(:project) { create(:project, namespace: authorized_user.namespace) }
......@@ -102,11 +102,17 @@ describe Issue, models: true do
it 'returns the merge request to close this issue' do
allow(mr).to receive(:closes_issue?).with(issue).and_return(true)
expect(issue.closed_by_merge_requests).to eq([mr])
expect(issue.closed_by_merge_requests(mr.author)).to eq([mr])
end
it "returns an empty array when the merge request is closed already" do
closed_mr
expect(issue.closed_by_merge_requests(closed_mr.author)).to eq([])
end
it "returns an empty array when the current issue is closed already" do
expect(closed_issue.closed_by_merge_requests).to eq([])
expect(closed_issue.closed_by_merge_requests(closed_issue.author)).to eq([])
end
end
......@@ -212,7 +218,7 @@ describe Issue, models: true do
source_project: subject.project,
source_branch: "#{subject.iid}-branch" })
merge_request.create_cross_references!(user)
expect(subject.referenced_merge_requests).not_to be_empty
expect(subject.referenced_merge_requests(user)).not_to be_empty
expect(subject.related_branches(user)).to eq([subject.to_branch_name])
end
......@@ -308,6 +314,22 @@ describe Issue, models: true do
end
describe '#visible_to_user?' do
context 'without a user' do
let(:issue) { build(:issue) }
it 'returns true when the issue is publicly visible' do
expect(issue).to receive(:publicly_visible?).and_return(true)
expect(issue.visible_to_user?).to eq(true)
end
it 'returns false when the issue is not publicly visible' do
expect(issue).to receive(:publicly_visible?).and_return(false)
expect(issue.visible_to_user?).to eq(false)
end
end
context 'with a user' do
let(:user) { build(:user) }
let(:issue) { build(:issue) }
......@@ -323,26 +345,24 @@ describe Issue, models: true do
expect(issue.visible_to_user?(user)).to eq(false)
end
end
context 'without a user' do
let(:issue) { build(:issue) }
it 'returns false when feature is disabled' do
expect(issue).not_to receive(:readable_by?)
it 'returns true when the issue is publicly visible' do
expect(issue).to receive(:publicly_visible?).and_return(true)
issue.project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
expect(issue.visible_to_user?).to eq(true)
expect(issue.visible_to_user?(user)).to eq(false)
end
it 'returns false when the issue is not publicly visible' do
expect(issue).to receive(:publicly_visible?).and_return(false)
it 'returns false when restricted for members' do
expect(issue).not_to receive(:readable_by?)
expect(issue.visible_to_user?).to eq(false)
issue.project.project_feature.update_attribute(:issues_access_level, ProjectFeature::PRIVATE)
expect(issue.visible_to_user?(user)).to eq(false)
end
end
end
describe '#readable_by?' do
describe 'with a regular user that is not a team member' do
let(:user) { create(:user) }
......@@ -352,13 +372,13 @@ describe Issue, models: true do
it 'returns true for a regular issue' do
issue = build(:issue, project: project)
expect(issue).to be_readable_by(user)
expect(issue.visible_to_user?(user)).to eq(true)
end
it 'returns false for a confidential issue' do
issue = build(:issue, project: project, confidential: true)
expect(issue).not_to be_readable_by(user)
expect(issue.visible_to_user?(user)).to eq(false)
end
end
......@@ -369,13 +389,13 @@ describe Issue, models: true do
it 'returns true for a regular issue' do
issue = build(:issue, project: project)
expect(issue).to be_readable_by(user)
expect(issue.visible_to_user?(user)).to eq(true)
end
it 'returns false for a confidential issue' do
issue = build(:issue, :confidential, project: project)
expect(issue).not_to be_readable_by(user)
expect(issue.visible_to_user?(user)).to eq(false)
end
end
......@@ -387,13 +407,13 @@ describe Issue, models: true do
it 'returns false for a regular issue' do