Skip to content

Extend policy bot comment with violation data

Release notes

The security policy bot gives users context to understand when policies are enforced on their project, when evaluation is completed, and if there are any violations blocking an MR, with guidance to resolve them. We have now extended support in the bot comment to supply additional insight into why an MR may be blocked by a policy, with more granular feedback on how to resolve. Details provided by the comment include:

  • Security findings that are specifically blocking the MR
  • Out-of-policy licenses
  • Policy errors that may default in a "fail closed" and blocking behavior
  • Details regarding the pipelines that are being considered in the evaluation

With these extra details, you can now more quickly understand the state of your MR and self-serve to troubleshoot any issues.

Why are we doing this work

In Add violation_data to scan_result_policy_violat... (#433390 - closed) we're adding a new column violation_data to be able to save details about what caused policy violations.

In this issue, we want add a service that parses the violation_data detected and saved from various rules and returns it in a normalized form. The data should be extended with additional information for the findings, so that we can link to the file & line where the violations happened, display severity and the scanner (see the designs).

Relevant links

Non-functional requirements

  • Documentation:
  • Feature flag: we should do these changes behind feature flag
  • Performance:
  • Testing:

Implementation plan

  1. Create a service to extract violations for a merge_request and return them in a normalized form for all kinds of rules. A rough idea:
diff --git a/ee/app/services/security/security_orchestration_policies/scan_result_policy_violations_presenter.rb b/ee/app/services/security/security_orchestration_policies/scan_result_policy_violations_presenter.rb
new file mode 100644
index 0000000000000000000000000000000000000000..99cf65d86759db7f536bf7585bf112a472364b70
--- /dev/null
+++ b/ee/app/services/security/security_orchestration_policies/scan_result_policy_violations_presenter.rb
@@ -0,0 +1,134 @@
+# frozen_string_literal: true
+
+module Security
+  module SecurityOrchestrationPolicies
+    class ScanResultPolicyViolationsPresenter
+      include ::Gitlab::Utils::StrongMemoize
+
+      attr_reader :merge_request
+
+      def initialize(merge_request)
+        @merge_request = merge_request
+      end
+
+      def violations
+        merge_request.scan_result_policy_violations.map do |violation|
+          rule = scan_result_policy_rules[violation.scan_result_policy_id]
+          {
+            report_type: rule.report_type,
+            name: rule.name,
+            scan_result_policy_id: rule.scan_result_policy_id,
+            violations: violations_for_rule(rule, violation)
+          }
+        end
+      end
+
+      def errors
+        merge_request.scan_result_policy_violations.filter_map do |violation|
+          next unless violation.violation_data&.fetch('error', nil).present?
+
+          rule = scan_result_policy_rules[violation.scan_result_policy_id]
+          {
+            report_type: rule.report_type,
+            name: rule.name,
+            scan_result_policy_id: rule.scan_result_policy_id,
+            **error_data(violation)
+          }
+        end
+      end
+
+      private
+
+      def scan_result_policy_rules
+        merge_request.approval_rules.with_scan_result_policy_read.including_scan_result_policy_read
+                     .index_by(&:scan_result_policy_id)
+      end
+
+      strong_memoize_attr :scan_result_policy_rules
+
+      def pipeline
+        merge_request.actual_head_pipeline
+      end
+
+      strong_memoize_attr :pipeline
+
+      def violations_for_rule(rule, violation)
+        case rule.report_type
+        when Security::ScanResultPolicy::SCAN_FINDING
+          violations_for_scan_finding(violation)
+        when Security::ScanResultPolicy::LICENSE_SCANNING
+          denied_licenses.map do |license|
+            normalize_rule_violation(
+              description: "#{license.name} detected (#{license.dependencies.map(&:name).uniq.join(', ')})",
+              path: license.dependencies.map(&:path).uniq.join(', '),
+              status: 'NEWLY_DETECTED', # TODO
+              scan_type: 'dependency_scanning'
+            )
+          end
+        when Security::ScanResultPolicy::ANY_MERGE_REQUEST
+          commits_violation = violation.violation_data&.dig('violations', 'commits')
+          return [] if commits_violation.nil?
+
+          [
+            normalize_rule_violation(
+              description:
+                "Merge request requires #{rule.approvals_required} approvals for #{commits_violation} commits"
+            )
+          ]
+        end
+      end
+
+      def violations_for_scan_finding(violation)
+        uuids = violation.violation_data&.dig('violations', 'uuids')
+
+        # Extend using UnenforceablePolicyRulesService to cover when pipeline is empty
+        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/133627
+        return [] if uuids.nil? || pipeline.nil?
+
+        findings = pipeline.security_findings.by_uuid(uuids.values.flatten)
+        findings.map do |finding|
+          normalize_rule_violation(
+            scan_type: finding.scan.type,
+            severity: finding.severity,
+            path: finding.present.blob_path,
+            status: uuids['previously_existing']&.include?(finding.uuid) ? 'PREVIOUSLY_EXISTING' : 'NEWLY_DETECTED',
+            description: [
+              ("line #{finding.location[:start_line]} in" if finding.location[:start_line].present?),
+              "file #{finding.location[:file]}"
+            ].compact.join(' ').capitalize
+          )
+        end
+      end
+
+      def denied_licenses
+        # This may be sub-optimal performance-wise. Alternatively, extract the license names using data persisted in https://gitlab.com/gitlab-org/gitlab/-/issues/433401
+        merge_request.project.license_compliance(pipeline).find_policies(
+          detected_only: true, classification: %w[denied]
+        )
+      end
+
+      def normalize_rule_violation(description:, path: nil, status: nil, scan_type: nil, severity: nil)
+        {
+          scan_type: scan_type,
+          severity: severity,
+          path: path,
+          status: status,
+          description: description
+        }
+      end
+
+      def error_data(violation)
+        error = violation.violation_data['error']
+        suggestion = case error
+                     when 'SCAN_REMOVED'
+                       'Resolve the error and re-run the pipeline.'
+                     else
+                       ''
+                     end
+        {
+          code: error,
+          suggestion: suggestion
+        }
+      end
+    end
+  end
+end
  1. Update Security::ScanResultPolicies::PolicyViolationComment to use the service above and create the message based on the designs.

Verification steps

  1. Create a policy with all kinds of rules (scan finding, license scanning, any merge request)
  2. Create MR causing violations
  3. Verify that a bot comment message is displayed, linking to the specific violations based on the findings
  4. Verify that error messages are shown in the comment if there is no CI configuration in the project
  5. Verify that error is presented if a scanner is removed in the source branch
Edited by Martin Čavoj