Security bot says policy violate but not accurate
Summary
Customer reported all merge requests received a comment from the GitLab Security Bot stating a policy violation. However, the policies currently do not block or require approval right now. While it doesn't seem to be blocking anyone, its behavior is confusing.
Steps to reproduce
What is the current bug behavior?
Security bot stating that there's policy violation when there isn't any
What is the expected correct behavior?
Security bot should only prompted when there's an actual policy violation
Relevant logs and/or screenshots
Output of checks
Results of GitLab environment info
Expand for output related to GitLab environment info
(For installations with omnibus-gitlab package run and paste the output of: \\\`sudo gitlab-rake gitlab:env:info\\\`) (For installations from source run and paste the output of: \\\`sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production\\\`)
Results of GitLab application Check
Expand for output related to the GitLab application check
(For installations with omnibus-gitlab package run and paste the output of: \`sudo gitlab-rake gitlab:check SANITIZE=true\`) (For installations from source run and paste the output of: \`sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true\`) (we will only investigate if the tests are passing)
Possible fixes
We should show a different message when the approvals are optional.
diff --git a/ee/app/services/security/scan_result_policies/generate_policy_violation_comment_service.rb b/ee/app/services/security/scan_result_policies/generate_policy_violation_comment_service.rb
index 7d82c2e51825..9614e16b0a40 100644
--- a/ee/app/services/security/scan_result_policies/generate_policy_violation_comment_service.rb
+++ b/ee/app/services/security/scan_result_policies/generate_policy_violation_comment_service.rb
@@ -7,13 +7,14 @@ class GeneratePolicyViolationCommentService
LOCK_SLEEP_SEC = 0.5.seconds
- attr_reader :merge_request, :project, :report_type, :violated_policy
+ attr_reader :merge_request, :project, :report_type, :violated_policy, :requires_approval
- def initialize(merge_request, report_type, violated_policy)
+ def initialize(merge_request, params = {})
@merge_request = merge_request
@project = merge_request.project
- @report_type = report_type
- @violated_policy = violated_policy
+ @report_type = params['report_type']
+ @violated_policy = params['violated_policy']
+ @requires_approval = params['requires_approval']
end
def execute
@@ -45,7 +46,7 @@ def exclusive_lock_key
def comment
@comment ||= PolicyViolationComment.new(existing_comment).tap do |violation_comment|
if violated_policy
- violation_comment.add_report_type(report_type)
+ violation_comment.add_report_type(report_type, requires_approval)
else
violation_comment.remove_report_type(report_type)
end
diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb
index 3df6bb977155..0f697a4ece94 100644
--- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb
+++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb
@@ -26,7 +26,7 @@ def execute
violated_rules, unviolated_rules = partition_rules(approval_rules)
ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any?
- generate_policy_bot_comment(violated_rules.any?)
+ generate_policy_bot_comment(violated_rules)
end
private
@@ -94,16 +94,21 @@ def target_pipeline_security_scan_types
end
strong_memoize_attr :target_pipeline_security_scan_types
- def generate_policy_bot_comment(violated_policy)
+ def generate_policy_bot_comment(violated_rules)
return if Feature.disabled?(:security_policy_approval_notification, pipeline.project)
Security::GeneratePolicyViolationCommentWorker.perform_async(
merge_request.id,
{ 'report_type' => Security::ScanResultPolicies::PolicyViolationComment::REPORT_TYPES[:scan_finding],
- 'violated_policy' => violated_policy }
+ 'violated_policy' => violated_rules.any?,
+ 'requires_approval' => rules_requiring_approval?(violated_rules) }
)
end
+ def rules_requiring_approval?(approval_rules)
+ approval_rules.any? { |rule| rule.approvals_required > 0 }
+ end
+
def target_pipeline
merge_request.latest_finished_target_branch_pipeline_for_scan_result_policy
end
diff --git a/ee/app/services/security/sync_license_scanning_rules_service.rb b/ee/app/services/security/sync_license_scanning_rules_service.rb
index f2b8bb2fe8ea..7a00222c8e4c 100644
--- a/ee/app/services/security/sync_license_scanning_rules_service.rb
+++ b/ee/app/services/security/sync_license_scanning_rules_service.rb
@@ -47,7 +47,7 @@ def remove_required_license_finding_approval(merge_request)
violates_policy?(merge_request, rule)
end
ApprovalMergeRequestRule.remove_required_approved(unviolated_rules)
- generate_policy_bot_comment(merge_request, violated_rules.any?)
+ generate_policy_bot_comment(merge_request, violated_rules)
end
## Checks if a policy rule violates the following conditions:
@@ -139,14 +139,19 @@ def license_names_from_report
end
strong_memoize_attr :license_names_from_report
- def generate_policy_bot_comment(merge_request, violated_policy)
+ def generate_policy_bot_comment(merge_request, violated_rules)
return if Feature.disabled?(:security_policy_approval_notification, pipeline.project)
Security::GeneratePolicyViolationCommentWorker.perform_async(
merge_request.id,
{ 'report_type' => Security::ScanResultPolicies::PolicyViolationComment::REPORT_TYPES[:license_scanning],
- 'violated_policy' => violated_policy }
+ 'violated_policy' => violated_rules.any?,
+ 'requires_approval' => rules_requiring_approval?(violated_rules) }
)
end
+
+ def rules_requiring_approval?(approval_rules)
+ approval_rules.any? { |rule| rule.approvals_required > 0 }
+ end
end
end
diff --git a/ee/app/workers/security/generate_policy_violation_comment_worker.rb b/ee/app/workers/security/generate_policy_violation_comment_worker.rb
index 1dcaea0c58c3..06a562356833 100644
--- a/ee/app/workers/security/generate_policy_violation_comment_worker.rb
+++ b/ee/app/workers/security/generate_policy_violation_comment_worker.rb
@@ -20,8 +20,7 @@ def perform(merge_request_id, params = {})
result = Security::ScanResultPolicies::GeneratePolicyViolationCommentService.new(
merge_request,
- params['report_type'],
- params['violated_policy']
+ params
).execute
return unless result.error?
@@ -37,6 +36,7 @@ def log_message(errors, merge_request_id, params)
merge_request_id: merge_request_id,
violated_policy: params['violated_policy'],
report_type: params['report_type'],
+ requires_approval: params['requires_approval'],
message: errors
))
end
diff --git a/ee/lib/security/scan_result_policies/policy_violation_comment.rb b/ee/lib/security/scan_result_policies/policy_violation_comment.rb
index 8523ba5e0814..0624717ea987 100644
--- a/ee/lib/security/scan_result_policies/policy_violation_comment.rb
+++ b/ee/lib/security/scan_result_policies/policy_violation_comment.rb
@@ -5,29 +5,49 @@ module ScanResultPolicies
class PolicyViolationComment
MESSAGE_HEADER = '<!-- policy_violation_comment -->'
VIOLATED_REPORTS_HEADER_PATTERN = /<!-- violated_reports: ([a-z_,]+)/
+ APPROVALS_HEADER_PATTERN = /<!-- optional_approvals: ([a-z_,]+)/
REPORT_TYPES = {
license_scanning: 'license_scanning',
scan_finding: 'scan_finding'
}.freeze
- attr_reader :reports, :existing_comment
+ MESSAGE_REQUIRES_APPROVAL = <<~TEXT.squish
+ Security and compliance scanners enforced by your organization have completed and identified that approvals
+ are required due to one or more policy violations.
+ Review the policy's rules in the MR widget and assign reviewers to proceed.
+ TEXT
+
+ MESSAGE_REQUIRES_NO_APPROVAL = <<~TEXT.squish
+ Security and compliance scanners enforced by your organization have completed and identified one or more
+ policy violations.
+ Consider including optional reviewers based on the policy rules in the MR widget.
+ TEXT
+
+ attr_reader :reports, :optional_approval_reports, :existing_comment
def initialize(existing_comment)
@existing_comment = existing_comment
@reports = Set.new
+ @optional_approval_reports = Set.new
return unless existing_comment
- match = existing_comment.note.match(VIOLATED_REPORTS_HEADER_PATTERN)
- match[1].split(',').each { |report_type| add_report_type(report_type) } if match
+ parse_reports
end
- def add_report_type(report_type)
+ def add_report_type(report_type, requires_approval)
@reports = (reports + [report_type]) & REPORT_TYPES.values
+
+ add_optional_approval_report(report_type) unless requires_approval
+ end
+
+ def add_optional_approval_report(report_type)
+ @optional_approval_reports = (optional_approval_reports + [report_type]) & REPORT_TYPES.values
end
def remove_report_type(report_type)
@reports -= [report_type]
+ @optional_approval_reports -= [report_type]
end
def body
@@ -38,6 +58,18 @@ def body
private
+ def parse_reports
+ match = existing_comment.note.match(VIOLATED_REPORTS_HEADER_PATTERN)
+ match[1].split(',').each { |report_type| add_report_type(report_type, true) } if match
+
+ parse_optional_approval_reports
+ end
+
+ def parse_optional_approval_reports
+ match = existing_comment.note.match(APPROVALS_HEADER_PATTERN)
+ match[1].split(',').each { |report_type| add_optional_approval_report(report_type) } if match
+ end
+
def fixed_note_body
'Security policy violations have been resolved.'
end
@@ -45,13 +77,10 @@ def fixed_note_body
def body_message
return fixed_note_body if reports.empty?
- message = <<~TEXT.squish
- Security and compliance scanners enforced by your organization have completed and identified that approvals
- are required due to one or more policy violations.
- Review the policy's rules in the MR widget and assign reviewers to proceed.
- TEXT
+ message = reports == optional_approval_reports ? MESSAGE_REQUIRES_NO_APPROVAL : MESSAGE_REQUIRES_APPROVAL
<<~MARKDOWN
<!-- violated_reports: #{reports.join(',')} -->
+ #{"<!-- optional_approvals: #{optional_approval_reports.join(',')} -->" if optional_approval_reports.any?}
| :warning: **Policy violation(s) detected**|
| ----------------------------------------- |
| #{message} |Edited by Martin Čavoj