From 0d0da22a0f4047784ca2124f1555645bfab826a0 Mon Sep 17 00:00:00 2001
From: Ian Anderson <ianderson@gitlab.com>
Date: Wed, 12 Jul 2023 08:08:31 +0000
Subject: [PATCH] Check non-public issues for spam
---
app/models/concerns/spammable.rb | 6 +--
app/models/issue.rb | 20 ++------
app/services/spam/spam_verdict_service.rb | 2 +-
spec/models/concerns/spammable_spec.rb | 26 +++-------
spec/models/issue_spec.rb | 48 ++++++++++++++++---
.../spam/spam_verdict_service_spec.rb | 19 +++++++-
6 files changed, 76 insertions(+), 45 deletions(-)
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
index 6550c5a94a0a..5986f8f5b5fb 100644
--- a/app/models/concerns/spammable.rb
+++ b/app/models/concerns/spammable.rb
@@ -138,7 +138,7 @@ def spammable_text
result.reject(&:blank?).join("\n")
end
- # Override in Spammable if further checks are necessary
+ # Override in included class if further checks are necessary
def check_for_spam?(*)
spammable_attribute_changed?
end
@@ -153,8 +153,8 @@ def check_for_spam(user:, action:, extra_features: {})
end
end
- # Override in Spammable if differs
- def allow_possible_spam?
+ # Override in included class if you want to allow possible spam under specific circumstances
+ def allow_possible_spam?(*)
Gitlab::CurrentSettings.allow_possible_spam
end
end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 453c451cebf1..f94ebf30de3e 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -584,16 +584,12 @@ def visible_to_user?(user = nil)
user, project.external_authorization_classification_label)
end
- def check_for_spam?(user:)
- # content created via support bots is always checked for spam, EVEN if
- # the issue is not publicly visible and/or confidential
- return true if user.support_bot? && spammable_attribute_changed?
+ # Always enforce spam check for support bot but allow for other users when issue is not publicly visible
+ def allow_possible_spam?(user)
+ return true if Gitlab::CurrentSettings.allow_possible_spam
+ return false if user.support_bot?
- # Only check for spam on issues which are publicly visible (and thus indexed in search engines)
- return false unless publicly_visible?
-
- # Only check for spam if certain attributes have changed
- spammable_attribute_changed?
+ !publicly_visible?
end
def supports_recaptcha?
@@ -826,12 +822,6 @@ def persist_pg_full_text_search_vector(search_vector)
Issues::SearchData.upsert({ project_id: project_id, issue_id: id, search_vector: search_vector }, unique_by: %i(project_id issue_id))
end
- def spammable_attribute_changed?
- # NOTE: We need to check them for spam when issues are made non-confidential, because spam
- # may have been added while they were confidential and thus not being checked for spam.
- super || confidential_changed?(from: true, to: false)
- end
-
def ensure_metrics!
Issue::Metrics.record!(self)
end
diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb
index 160ebb81b397..e0a6d58b9040 100644
--- a/app/services/spam/spam_verdict_service.rb
+++ b/app/services/spam/spam_verdict_service.rb
@@ -85,7 +85,7 @@ def override_via_allow_possible_spam?(verdict:)
# than the override verdict's priority value), then we don't need to override it.
return false if SUPPORTED_VERDICTS[verdict][:priority] > SUPPORTED_VERDICTS[OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM][:priority]
- target.allow_possible_spam? || user.allow_possible_spam?
+ target.allow_possible_spam?(user) || user.allow_possible_spam?
end
def spamcheck_client
diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb
index 8a2fa6675e5d..7ef0473aea8b 100644
--- a/spec/models/concerns/spammable_spec.rb
+++ b/spec/models/concerns/spammable_spec.rb
@@ -71,8 +71,8 @@
expect(issue.check_for_spam?(user: issue.author)).to eq(true)
end
- it 'returns false for other visibility levels' do
- expect(issue.check_for_spam?(user: issue.author)).to eq(false)
+ it 'returns true for other visibility levels' do
+ expect(issue.check_for_spam?(user: issue.author)).to eq(true)
end
end
@@ -82,7 +82,7 @@
end
context 'when the model is spam' do
- where(model: [:issue, :merge_request, :snippet, :spammable_model])
+ where(model: [:issue, :merge_request, :note, :snippet, :spammable_model])
with_them do
subject do
@@ -94,21 +94,7 @@
it 'has an error related to spam on the model' do
expect(subject.errors.messages[:base])
- .to match_array /Your #{subject.class.model_name.human.downcase} has been recognized as spam./
- end
- end
-
- context 'when the spammable model is a Note' do
- subject do
- Note.new.tap do |m|
- m.spam!
- m.invalidate_if_spam
- end
- end
-
- it 'has an error related to spam on the model' do
- expect(subject.errors.messages[:base])
- .to match_array /Your comment has been recognized as spam./
+ .to match_array /Your #{subject.spammable_entity_type} has been recognized as spam./
end
end
end
@@ -293,7 +279,9 @@ def invalidate_if_spam(is_spam: false, needs_recaptcha: false)
end
describe '#allow_possible_spam?' do
- subject { issue.allow_possible_spam? }
+ let_it_be(:user) { build(:user) }
+
+ subject { spammable_model.allow_possible_spam?(user) }
context 'when the `allow_possible_spam` application setting is turned off' do
it { is_expected.to eq(false) }
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index ecf9e4480eb1..7cf3981969c6 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -1559,6 +1559,42 @@
end
end
+ describe '#allow_possible_spam?' do
+ let_it_be(:issue) { build(:issue) }
+
+ subject { issue.allow_possible_spam?(issue.author) }
+
+ context 'when the `allow_possible_spam` application setting is turned off' do
+ context 'when the issue is private' do
+ it { is_expected.to eq(true) }
+
+ context 'when the user is the support bot' do
+ before do
+ allow(issue.author).to receive(:support_bot?).and_return(true)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+ end
+
+ context 'when the issue is public' do
+ before do
+ allow(issue).to receive(:publicly_visible?).and_return(true)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+ end
+
+ context 'when the `allow_possible_spam` application setting is turned on' do
+ before do
+ stub_application_setting(allow_possible_spam: true)
+ end
+
+ it { is_expected.to eq(true) }
+ end
+ end
+
describe '#check_for_spam?' do
let_it_be(:support_bot) { ::User.support_bot }
@@ -1568,24 +1604,24 @@
false | Gitlab::VisibilityLevel::PUBLIC | false | { description: 'new' } | true
false | Gitlab::VisibilityLevel::PUBLIC | false | { title: 'new' } | true
# confidential to non-confidential
- false | Gitlab::VisibilityLevel::PUBLIC | true | { confidential: false } | true
+ false | Gitlab::VisibilityLevel::PUBLIC | true | { confidential: false } | false
# non-confidential to confidential
false | Gitlab::VisibilityLevel::PUBLIC | false | { confidential: true } | false
# spammable attributes changing on confidential
- false | Gitlab::VisibilityLevel::PUBLIC | true | { description: 'new' } | false
+ false | Gitlab::VisibilityLevel::PUBLIC | true | { description: 'new' } | true
# spammable attributes changing while changing to confidential
- false | Gitlab::VisibilityLevel::PUBLIC | false | { title: 'new', confidential: true } | false
+ false | Gitlab::VisibilityLevel::PUBLIC | false | { title: 'new', confidential: true } | true
# spammable attribute not changing
false | Gitlab::VisibilityLevel::PUBLIC | false | { description: 'original description' } | false
# non-spammable attribute changing
false | Gitlab::VisibilityLevel::PUBLIC | false | { weight: 3 } | false
# spammable attributes changing on non-public
- false | Gitlab::VisibilityLevel::INTERNAL | false | { description: 'new' } | false
- false | Gitlab::VisibilityLevel::PRIVATE | false | { description: 'new' } | false
+ false | Gitlab::VisibilityLevel::INTERNAL | false | { description: 'new' } | true
+ false | Gitlab::VisibilityLevel::PRIVATE | false | { description: 'new' } | true
### support-bot cases
# confidential to non-confidential
- true | Gitlab::VisibilityLevel::PUBLIC | true | { confidential: false } | true
+ true | Gitlab::VisibilityLevel::PUBLIC | true | { confidential: false } | false
# non-confidential to confidential
true | Gitlab::VisibilityLevel::PUBLIC | false | { confidential: true } | false
# spammable attributes changing on confidential
diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb
index bf6273048071..70f43d82ead0 100644
--- a/spec/services/spam/spam_verdict_service_spec.rb
+++ b/spec/services/spam/spam_verdict_service_spec.rb
@@ -403,7 +403,24 @@
describe 'issue' do
let(:target) { issue }
- it_behaves_like 'execute spam verdict service'
+ context 'when issue is publicly visible' do
+ before do
+ allow(issue).to receive(:publicly_visible?).and_return(true)
+ end
+
+ it_behaves_like 'execute spam verdict service'
+ end
+
+ context 'when issue is not publicly visible' do
+ before do
+ allow(issue).to receive(:publicly_visible?).and_return(false)
+ allow(service).to receive(:get_spamcheck_verdict).and_return(BLOCK_USER)
+ end
+
+ it 'overrides and renders the override verdict' do
+ expect(service.execute).to eq OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM
+ end
+ end
end
describe 'snippet' do
--
GitLab