Merge branch 'rf-move-sast-reports-to-mr-widget-backend' into 'master'

Move SAST reports logic for MR widget to backend

See merge request gitlab-org/gitlab-ee!15114

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -50,6 +50,10 @@ module EE
         reports_response(merge_request.compare_dependency_scanning_reports)
       end
 
+      def sast_reports
+        reports_response(merge_request.compare_sast_reports)
+      end
+
       def metrics_reports
         reports_response(merge_request.compare_metrics_reports)
       end

ee/app/models/ee/ci/job_artifact.rb

@@ -16,6 +16,7 @@ module EE
       DEPENDENCY_LIST_REPORT_FILE_TYPES = %w[dependency_scanning].freeze
       METRICS_REPORT_FILE_TYPES = %w[metrics].freeze
       CONTAINER_SCANNING_REPORT_TYPES = %w[container_scanning].freeze
+      SAST_REPORT_TYPES = %w[sast].freeze
 
       scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
       scope :project_id_in, ->(ids) { joins(:project).merge(::Project.id_in(ids)) }
@@ -38,6 +39,10 @@ module EE
         with_file_types(CONTAINER_SCANNING_REPORT_TYPES)
       end
 
+      scope :sast_reports, -> do
+        with_file_types(SAST_REPORT_TYPES)
+      end
+
       scope :metrics_reports, -> do
         with_file_types(METRICS_REPORT_FILE_TYPES)
       end

ee/app/models/ee/merge_request.rb

@@ -140,6 +140,18 @@ module EE
       compare_reports(::Ci::CompareContainerScanningReportsService)
     end
 
+    def has_sast_reports?
+      actual_head_pipeline&.has_reports?(::Ci::JobArtifact.sast_reports)
+    end
+
+    def compare_sast_reports
+      unless has_sast_reports?
+        return { status: :error, status_reason: 'This merge request does not have SAST reports' }
+      end
+
+      compare_reports(::Ci::CompareSastReportsService)
+    end
+
     def compare_license_management_reports
       unless has_license_management_reports?
         return { status: :error, status_reason: 'This merge request does not have license management reports' }

ee/app/services/ci/compare_sast_reports_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class CompareSastReportsService < ::Ci::CompareReportsBaseService
+    def comparer_class
+      Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer
+    end
+
+    def serializer_class
+      Vulnerabilities::OccurrenceDiffSerializer
+    end
+
+    def get_report(pipeline)
+      report = pipeline&.security_reports&.get_report('sast')
+
+      raise report.error if report&.errored? # propagate error to base class's execute method
+
+      report
+    end
+  end
+end

ee/app/views/projects/merge_requests/show.html.haml

@@ -16,6 +16,7 @@
     window.gl.mrWidgetData.vulnerability_feedback_help_path = '#{help_page_path("user/application_security/index")}';
     window.gl.mrWidgetData.approvals_help_path = '#{help_page_path("user/project/merge_requests/merge_request_approvals")}';
     window.gl.mrWidgetData.visual_review_app_available = '#{@project.feature_available?(:visual_review_app)}' === 'true';
-    window.gl.mrWidgetData.license_management_comparsion_path = '#{license_management_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:license_management)}'
-    window.gl.mrWidgetData.container_scanning_comparsion_path = '#{container_scanning_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:container_scanning)}'
-    window.gl.mrWidgetData.dependency_scanning_comparsion_path = '#{dependency_scanning_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:dependency_scanning)}'
+    window.gl.mrWidgetData.license_management_comparison_path = '#{license_management_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:license_management)}'
+    window.gl.mrWidgetData.container_scanning_comparison_path = '#{container_scanning_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:container_scanning)}'
+    window.gl.mrWidgetData.dependency_scanning_comparison_path = '#{dependency_scanning_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:dependency_scanning)}'
+    window.gl.mrWidgetData.sast_comparison_path = '#{sast_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:sast)}'

ee/changelogs/unreleased/rf-move-sast-reports-to-mr-widget-backend.yml

+---
+title: Present SAST report comparison logic to backend
+merge_request: 15114
+author:
+type: changed

ee/config/routes/project.rb

@@ -71,6 +71,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           get :license_management_reports
           get :container_scanning_reports
           get :dependency_scanning_reports
+          get :sast_reports
         end
       end
 

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -563,6 +563,91 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  describe 'GET #sast_reports' do
+    let(:merge_request) { create(:ee_merge_request, :with_sast_reports, source_project: project, author: create(:user)) }
+    let(:params) do
+      {
+        namespace_id: project.namespace.to_param,
+        project_id: project,
+        id: merge_request.iid
+      }
+    end
+
+    subject { get :sast_reports, params: params, format: :json }
+
+    before do
+      allow_any_instance_of(::MergeRequest).to receive(:compare_reports)
+        .with(::Ci::CompareSastReportsService).and_return(comparison_status)
+    end
+
+    context 'when comparison is being processed' do
+      let(:comparison_status) { { status: :parsing } }
+
+      it 'sends polling interval' do
+        expect(::Gitlab::PollingInterval).to receive(:set_header)
+
+        subject
+      end
+
+      it 'returns 204 HTTP status' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:no_content)
+      end
+    end
+
+    context 'when comparison is done' do
+      let(:comparison_status) { { status: :parsed, data: { added: [], fixed: [], existing: [] } } }
+
+      it 'does not send polling interval' do
+        expect(::Gitlab::PollingInterval).not_to receive(:set_header)
+
+        subject
+      end
+
+      it 'returns 200 HTTP status' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to eq({ "added" => [], "fixed" => [], "existing" => [] })
+      end
+    end
+
+    context 'when user created corrupted vulnerability reports' do
+      let(:comparison_status) { { status: :error, status_reason: 'Failed to parse sast reports' } }
+
+      it 'does not send polling interval' do
+        expect(::Gitlab::PollingInterval).not_to receive(:set_header)
+
+        subject
+      end
+
+      it 'returns 400 HTTP status' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response).to eq({ 'status_reason' => 'Failed to parse sast reports' })
+      end
+    end
+
+    context 'when something went wrong on our system' do
+      let(:comparison_status) { {} }
+
+      it 'does not send polling interval' do
+        expect(::Gitlab::PollingInterval).not_to receive(:set_header)
+
+        subject
+      end
+
+      it 'returns 500 HTTP status' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:internal_server_error)
+        expect(json_response).to eq({ 'status_reason' => 'Unknown error' })
+      end
+    end
+  end
+
   describe 'GET #license_management_reports' do
     let(:merge_request) { create(:ee_merge_request, :with_license_management_reports, source_project: project, author: create(:user)) }
     let(:params) do

ee/spec/factories/ci/builds.rb

@@ -48,6 +48,12 @@ FactoryBot.define do
       end
     end
 
+    trait :sast_feature_branch do
+      after(:build) do |build|
+        build.job_artifacts << create(:ee_ci_job_artifact, :sast_feature_branch, job: build)
+      end
+    end
+
     trait :container_scanning_feature_branch do
       after(:build) do |build|
         build.job_artifacts << create(:ee_ci_job_artifact, :container_scanning_feature_branch, job: build)

ee/spec/factories/ci/job_artifacts.rb

@@ -12,6 +12,16 @@ FactoryBot.define do
       end
     end
 
+    trait :sast_feature_branch do
+      file_format :raw
+      file_type :sast
+
+      after(:build) do |artifact, _|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('ee/spec/fixtures/security_reports/feature-branch/gl-sast-report.json'), 'application/json')
+      end
+    end
+
     trait :sast_deprecated do
       file_type :sast
       file_format :raw

ee/spec/factories/ci/pipelines.rb

@@ -49,6 +49,14 @@ FactoryBot.define do
       end
     end
 
+    trait :with_sast_feature_branch do
+      status :success
+
+      after(:build) do |pipeline, evaluator|
+        pipeline.builds << build(:ee_ci_build, :sast_feature_branch, pipeline: pipeline, project: pipeline.project)
+      end
+    end
+
     trait :with_license_management_feature_branch do
       status :success
 

ee/spec/factories/merge_requests.rb

@@ -85,6 +85,18 @@ FactoryBot.define do
       end
     end
 
+    trait :with_sast_reports do
+      after(:build) do |merge_request|
+        merge_request.head_pipeline = build(
+          :ee_ci_pipeline,
+          :success,
+          :with_sast_report,
+          project: merge_request.source_project,
+          ref: merge_request.source_branch,
+          sha: merge_request.diff_head_sha)
+      end
+    end
+
     trait :with_metrics_reports do
       after(:build) do |merge_request|
         merge_request.head_pipeline = build(

ee/spec/fixtures/security_reports/feature-branch/gl-sast-report.json

@@ -856,92 +856,6 @@
       "line": 4,
       "url": "https://cwe.mitre.org/data/definitions/119.html",
       "tool": "flawfinder"
-    },
-    {
-      "category": "sast",
-      "message": "Check when opening files - can an attacker redirect it (via symlinks), force the opening of special file type (e.g., device files), move things around to create a race condition, control its ancestors, or change its contents? (CWE-362)",
-      "cve": "c/subdir/utils.c:bab681140fcc8fc3085b6bba74081b44ea145c1c98b5e70cf19ace2417d30770:CWE-362",
-      "confidence": "Low",
-      "scanner": {
-        "id": "flawfinder",
-        "name": "Flawfinder"
-      },
-      "location": {
-        "file": "c/subdir/utils.c",
-        "start_line": 8
-      },
-      "identifiers": [
-        {
-          "type": "cwe",
-          "name": "CWE-362",
-          "value": "362",
-          "url": "https://cwe.mitre.org/data/definitions/362.html"
-        }
-      ],
-      "file": "c/subdir/utils.c",
-      "line": 8,
-      "url": "https://cwe.mitre.org/data/definitions/362.html",
-      "tool": "flawfinder"
-    },
-    {
-      "category": "sast",
-      "message": "Statically-sized arrays can be improperly restricted, leading to potential overflows or other issues (CWE-119!/CWE-120)",
-      "cve": "cplusplus/src/hello.cpp:c8c6dd0afdae6814194cf0930b719f757ab7b379cf8f261e7f4f9f2f323a818a:CWE-119!/CWE-120",
-      "confidence": "Low",
-      "solution": "Perform bounds checking, use functions that limit length, or ensure that the size is larger than the maximum possible length",
-      "scanner": {
-        "id": "flawfinder",
-        "name": "Flawfinder"
-      },
-      "location": {
-        "file": "cplusplus/src/hello.cpp",
-        "start_line": 6
-      },
-      "identifiers": [
-        {
-          "type": "cwe",
-          "name": "CWE-119",
-          "value": "119",
-          "url": "https://cwe.mitre.org/data/definitions/119.html"
-        },
-        {
-          "type": "cwe",
-          "name": "CWE-120",
-          "value": "120",
-          "url": "https://cwe.mitre.org/data/definitions/120.html"
-        }
-      ],
-      "file": "cplusplus/src/hello.cpp",
-      "line": 6,
-      "url": "https://cwe.mitre.org/data/definitions/119.html",
-      "tool": "flawfinder"
-    },
-    {
-      "category": "sast",
-      "message": "Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120)",
-      "cve": "cplusplus/src/hello.cpp:331c04062c4fe0c7c486f66f59e82ad146ab33cdd76ae757ca41f392d568cbd0:CWE-120",
-      "confidence": "Low",
-      "solution": "Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused)",
-      "scanner": {
-        "id": "flawfinder",
-        "name": "Flawfinder"
-      },
-      "location": {
-        "file": "cplusplus/src/hello.cpp",
-        "start_line": 7
-      },
-      "identifiers": [
-        {
-          "type": "cwe",
-          "name": "CWE-120",
-          "value": "120",
-          "url": "https://cwe.mitre.org/data/definitions/120.html"
-        }
-      ],
-      "file": "cplusplus/src/hello.cpp",
-      "line": 7,
-      "url": "https://cwe.mitre.org/data/definitions/120.html",
-      "tool": "flawfinder"
     }
   ]
 }

ee/spec/models/merge_request_spec.rb

@@ -168,7 +168,7 @@ describe MergeRequest do
       stub_licensed_features(container_scanning: true)
     end
 
-    context 'when head pipeline has container scannning reports' do
+    context 'when head pipeline has container scanning reports' do
       let(:merge_request) { create(:ee_merge_request, :with_container_scanning_reports, source_project: project) }
 
       it { is_expected.to be_truthy }
@@ -181,6 +181,27 @@ describe MergeRequest do
     end
   end
 
+  describe '#has_sast_reports?' do
+    subject { merge_request.has_sast_reports? }
+    let(:project) { create(:project, :repository) }
+
+    before do
+      stub_licensed_features(sast: true)
+    end
+
+    context 'when head pipeline has sast reports' do
+      let(:merge_request) { create(:ee_merge_request, :with_sast_reports, source_project: project) }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when head pipeline does not have sast reports' do
+      let(:merge_request) { create(:ee_merge_request, source_project: project) }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
   describe '#has_metrics_reports?' do
     subject { merge_request.has_metrics_reports? }
     let(:project) { create(:project, :repository) }
@@ -261,6 +282,65 @@ describe MergeRequest do
     end
   end
 
+  describe '#compare_sast_reports' do
+    subject { merge_request.compare_sast_reports }
+
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
+    let!(:base_pipeline) do
+      create(:ee_ci_pipeline,
+             :with_sast_report,
+             project: project,
+             ref: merge_request.target_branch,
+             sha: merge_request.diff_base_sha)
+    end
+
+    before do
+      merge_request.update!(head_pipeline_id: head_pipeline.id)
+    end
+
+    context 'when head pipeline has sast reports' do
+      let!(:head_pipeline) do
+        create(:ee_ci_pipeline,
+               :with_sast_report,
+               project: project,
+               ref: merge_request.source_branch,
+               sha: merge_request.diff_head_sha)
+      end
+
+      context 'when reactive cache worker is parsing asynchronously' do
+        it 'returns status' do
+          expect(subject[:status]).to eq(:parsing)
+        end
+      end
+
+      context 'when reactive cache worker is inline' do
+        before do
+          synchronous_reactive_cache(merge_request)
+        end
+
+        it 'returns status and data' do
+          expect_any_instance_of(Ci::CompareSastReportsService)
+              .to receive(:execute).with(base_pipeline, head_pipeline).and_call_original
+
+          subject
+        end
+
+        context 'when cached results is not latest' do
+          before do
+            allow_any_instance_of(Ci::CompareSastReportsService)
+                .to receive(:latest?).and_return(false)
+          end
+
+          it 'raises and InvalidateReactiveCache error' do
+            expect { subject }.to raise_error(ReactiveCaching::InvalidateReactiveCache)
+          end
+        end
+      end
+    end
+  end
+
   describe '#compare_license_management_reports' do
     subject { merge_request.compare_license_management_reports }
 

ee/spec/serializers/vulnerabilities/occurrence_reports_comparer_entity_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 describe Vulnerabilities::OccurrenceReportsComparerEntity do
+  describe 'container scanning report comparison' do
     let!(:base_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report) }
     let!(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_feature_branch) }
     let(:base_report) { base_pipeline.security_reports.get_report('container_scanning')}
@@ -21,4 +22,22 @@ describe Vulnerabilities::OccurrenceReportsComparerEntity do
         expect(subject.keys).to match_array([:added, :existing, :fixed])
       end
     end
+  end
+
+  describe 'sast report comparison' do
+    let!(:base_pipeline) { create(:ee_ci_pipeline, :with_sast_report) }
+    let!(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_feature_branch) }
+    let(:base_report) { base_pipeline.security_reports.get_report('sast')}
+    let(:head_report) { head_pipeline.security_reports.get_report('sast')}
+    let(:comparer) { Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer.new(base_report, head_report) }
+    let(:entity) { described_class.new(comparer) }
+
+    describe '#as_json' do
+      subject { entity.as_json }
+
+      it 'contains the added existing and fixed vulnerabilities for sast' do
+        expect(subject.keys).to match_array([:added, :existing, :fixed])
+      end
+    end
+  end
 end

ee/spec/services/ci/compare_container_scanning_reports_service_spec.rb

@@ -36,7 +36,7 @@ describe Ci::CompareContainerScanningReportsService do
         expect(subject[:data]['added']).to include(a_hash_including('compare_key' => 'CVE-2017-15650'))
       end
 
-      it 'reports existing container vulenerabilities' do
+      it 'reports existing container vulnerabilities' do
         expect(subject[:data]['existing'].count).to eq(0)
       end
 

ee/spec/services/ci/compare_sast_reports_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::CompareSastReportsService do
+  let(:service) { described_class.new(project) }
+  let(:project) { create(:project, :repository) }
+
+  before do
+    stub_licensed_features(container_scanning: true)
+    stub_licensed_features(sast: true)
+  end
+
+  describe '#execute' do
+    subject { service.execute(base_pipeline, head_pipeline) }
+
+    context 'when head pipeline has sast reports' do
+      let!(:base_pipeline) { create(:ee_ci_pipeline) }
+      let!(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_report, project: project) }
+
+      it 'reports new vulnerabilities' do
+        expect(subject[:status]).to eq(:parsed)
+        expect(subject[:data]['added'].count).to eq(33)
+        expect(subject[:data]['existing'].count).to eq(0)
+        expect(subject[:data]['fixed'].count).to eq(0)
+      end
+    end
+
+    context 'when base and head pipelines have sast reports' do
+      let!(:base_pipeline) { create(:ee_ci_pipeline, :with_sast_report, project: project) }
+      let!(:head_pipeline) { create(:ee_ci_pipeline, :with_sast_feature_branch, project: project) }
+
+      it 'reports status as parsed' do
+        expect(subject[:status]).to eq(:parsed)
+      end
+
+      it 'reports new vulnerability' do
+        expect(subject[:data]['added'].count).to eq(1)
+        expect(subject[:data]['added']).to include(a_hash_including('compare_key' => 'c/subdir/utils.c:b466873101951fe96e1332f6728eb7010acbbd5dfc3b65d7d53571d091a06d9e:CWE-119!/CWE-120'))
+      end
+
+      it 'reports existing sast vulnerabilities' do
+        expect(subject[:data]['existing'].count).to eq(29)
+      end
+
+      it 'reports fixed sast vulnerabilities' do
+        expect(subject[:data]['fixed'].count).to eq(4)
+        compare_keys = subject[:data]['fixed'].map { |t| t['compare_key'] }
+        expected_keys = ['c/subdir/utils.c:b466873101951fe96e1332f6728eb7010acbbd5dfc3b65d7d53571d091a06d9e:CWE-119!/CWE-120', 'c/subdir/utils.c:bab681140fcc8fc3085b6bba74081b44ea145c1c98b5e70cf19ace2417d30770:CWE-362', 'cplusplus/src/hello.cpp:c8c6dd0afdae6814194cf0930b719f757ab7b379cf8f261e7f4f9f2f323a818a:CWE-119!/CWE-120', 'cplusplus/src/hello.cpp:331c04062c4fe0c7c486f66f59e82ad146ab33cdd76ae757ca41f392d568cbd0:CWE-120']
+        expect(compare_keys - expected_keys).to eq([])
+      end
+    end
+  end
+end