From b76601b5ce22808f2d73fa791e372d596e6b4d16 Mon Sep 17 00:00:00 2001 From: celdem Date: Tue, 6 Aug 2019 16:57:23 +0100 Subject: [PATCH] Move dependency scannning reports tp mr widget backend Expose new end point Small change in front end --- .../ee/projects/merge_requests_controller.rb | 4 + ee/app/models/ee/merge_request.rb | 12 ++ ...are_dependency_scanning_reports_service.rb | 21 +++ .../projects/merge_requests/show.html.haml | 1 + ...5-move-ds-reports-to-mr-widget-backend.yml | 5 + ee/config/routes/project.rb | 1 + .../merge_requests_controller_spec.rb | 85 +++++++++ ee/spec/factories/ci/builds.rb | 18 ++ ee/spec/factories/ci/job_artifacts.rb | 20 +++ ee/spec/factories/ci/pipelines.rb | 16 ++ ee/spec/factories/merge_requests.rb | 12 ++ .../gl-dependency-scanning-report.json | 170 ++++++++++++++++++ ee/spec/models/merge_request_spec.rb | 21 +++ ...ependency_scanning_reports_service_spec.rb | 63 +++++++ 14 files changed, 449 insertions(+) create mode 100644 ee/app/services/ci/compare_dependency_scanning_reports_service.rb create mode 100644 ee/changelogs/unreleased/12005-move-ds-reports-to-mr-widget-backend.yml create mode 100644 ee/spec/fixtures/security-reports/feature-branch/gl-dependency-scanning-report.json create mode 100644 ee/spec/services/ci/compare_dependency_scanning_reports_service_spec.rb diff --git a/ee/app/controllers/ee/projects/merge_requests_controller.rb b/ee/app/controllers/ee/projects/merge_requests_controller.rb index 58fb98f9515f3d..ca6ba7f7101633 100644 --- a/ee/app/controllers/ee/projects/merge_requests_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests_controller.rb @@ -46,6 +46,10 @@ def container_scanning_reports reports_response(merge_request.compare_container_scanning_reports) end + def dependency_scanning_reports + reports_response(merge_request.compare_dependency_scanning_reports) + end + def metrics_reports reports_response(merge_request.compare_metrics_reports) end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 6814fabc72f61c..16ffc4635547d3 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -112,6 +112,18 @@ def participant_approvers end end + def has_dependency_scanning_reports? + actual_head_pipeline&.has_reports?(::Ci::JobArtifact.dependency_list_reports) + end + + def compare_dependency_scanning_reports + unless has_dependency_scanning_reports? + return { status: :error, status_reason: 'This merge request does not have dependency scanning reports' } + end + + compare_reports(::Ci::CompareDependencyScanningReportsService) + end + def has_license_management_reports? actual_head_pipeline&.has_reports?(::Ci::JobArtifact.license_management_reports) end diff --git a/ee/app/services/ci/compare_dependency_scanning_reports_service.rb b/ee/app/services/ci/compare_dependency_scanning_reports_service.rb new file mode 100644 index 00000000000000..93d6fd2b3ed689 --- /dev/null +++ b/ee/app/services/ci/compare_dependency_scanning_reports_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Ci + class CompareDependencyScanningReportsService < ::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('dependency_scanning') + + raise report.error if report&.errored? # propagate error to base class's execute method + + report + end + end +end diff --git a/ee/app/views/projects/merge_requests/show.html.haml b/ee/app/views/projects/merge_requests/show.html.haml index 9eea23476cb5c7..e799ca40d6f6a3 100644 --- a/ee/app/views/projects/merge_requests/show.html.haml +++ b/ee/app/views/projects/merge_requests/show.html.haml @@ -18,3 +18,4 @@ 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)}' diff --git a/ee/changelogs/unreleased/12005-move-ds-reports-to-mr-widget-backend.yml b/ee/changelogs/unreleased/12005-move-ds-reports-to-mr-widget-backend.yml new file mode 100644 index 00000000000000..a68b778982ba94 --- /dev/null +++ b/ee/changelogs/unreleased/12005-move-ds-reports-to-mr-widget-backend.yml @@ -0,0 +1,5 @@ +--- +title: Move dependency scanning comparision logic to backend +merge_request: 15023 +author: +type: changed diff --git a/ee/config/routes/project.rb b/ee/config/routes/project.rb index 01ce3f3c8eb0fb..ba9c7329f43a5f 100644 --- a/ee/config/routes/project.rb +++ b/ee/config/routes/project.rb @@ -70,6 +70,7 @@ get :metrics_reports get :license_management_reports get :container_scanning_reports + get :dependency_scanning_reports end end diff --git a/ee/spec/controllers/projects/merge_requests_controller_spec.rb b/ee/spec/controllers/projects/merge_requests_controller_spec.rb index cdbdb540dfcc1f..e5976ec4cb9a92 100644 --- a/ee/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/ee/spec/controllers/projects/merge_requests_controller_spec.rb @@ -393,6 +393,91 @@ def expect_rebase_worker_for(user) end end + describe 'GET #dependency_scanning_reports' do + let(:merge_request) { create(:ee_merge_request, :with_dependency_scanning_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 :dependency_scanning_reports, params: params, format: :json } + + before do + allow_any_instance_of(::MergeRequest).to receive(:compare_reports) + .with(::Ci::CompareDependencyScanningReportsService).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 container scanning 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 container scanning 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 #container_scanning_reports' do let(:merge_request) { create(:ee_merge_request, :with_container_scanning_reports, source_project: project, author: create(:user)) } let(:params) do diff --git a/ee/spec/factories/ci/builds.rb b/ee/spec/factories/ci/builds.rb index 144b3829e04881..3b9eec924fdbdd 100644 --- a/ee/spec/factories/ci/builds.rb +++ b/ee/spec/factories/ci/builds.rb @@ -60,6 +60,24 @@ end end + trait :dependency_scanning_feature_branch do + after(:build) do |build| + build.job_artifacts << create(:ee_ci_job_artifact, :dependency_scanning_feature_branch, job: build) + end + end + + trait :dependency_scanning_report do + after(:build) do |build| + build.job_artifacts << create(:ee_ci_job_artifact, :dependency_scanning_report, job: build) + end + end + + trait :corrupted_dependency_scanning_report do + after(:build) do |build| + build.job_artifacts << create(:ee_ci_job_artifact, :corrupted_dependency_scanning_report, job: build) + end + end + trait :license_management_feature_branch do after(:build) do |build| build.job_artifacts << create(:ee_ci_job_artifact, :license_management_feature_branch, job: build) diff --git a/ee/spec/factories/ci/job_artifacts.rb b/ee/spec/factories/ci/job_artifacts.rb index 714d13c1b2ad41..6138a8caad07a3 100644 --- a/ee/spec/factories/ci/job_artifacts.rb +++ b/ee/spec/factories/ci/job_artifacts.rb @@ -112,6 +112,26 @@ end end + trait :dependency_scanning_feature_branch do + file_format :raw + file_type :dependency_scanning + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( + Rails.root.join('ee/spec/fixtures/security-reports/feature-branch/gl-dependency-scanning-report.json'), 'application/json') + end + end + + trait :corrupted_dependency_scanning_report do + file_format :raw + file_type :dependency_scanning + + after(:build) do |artifact, evaluator| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/trace/sample_trace'), 'application/json') + end + end + trait :container_scanning do file_format :raw file_type :container_scanning diff --git a/ee/spec/factories/ci/pipelines.rb b/ee/spec/factories/ci/pipelines.rb index f4515c4d837218..3b4bb9a3900157 100644 --- a/ee/spec/factories/ci/pipelines.rb +++ b/ee/spec/factories/ci/pipelines.rb @@ -33,6 +33,22 @@ end end + trait :with_dependency_scanning_feature_branch do + status :success + + after(:build) do |pipeline, evaluator| + pipeline.builds << build(:ee_ci_build, :dependency_scanning_feature_branch, pipeline: pipeline, project: pipeline.project) + end + end + + trait :with_corrupted_dependency_scanning_report do + status :success + + after(:build) do |pipeline, evaluator| + pipeline.builds << build(:ee_ci_build, :corrupted_dependency_scanning_report, pipeline: pipeline, project: pipeline.project) + end + end + trait :with_license_management_feature_branch do status :success diff --git a/ee/spec/factories/merge_requests.rb b/ee/spec/factories/merge_requests.rb index 58758277fea466..588e5643d92fd2 100644 --- a/ee/spec/factories/merge_requests.rb +++ b/ee/spec/factories/merge_requests.rb @@ -73,6 +73,18 @@ end end + trait :with_dependency_scanning_reports do + after(:build) do |merge_request| + merge_request.head_pipeline = build( + :ee_ci_pipeline, + :success, + :with_dependency_scanning_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( diff --git a/ee/spec/fixtures/security-reports/feature-branch/gl-dependency-scanning-report.json b/ee/spec/fixtures/security-reports/feature-branch/gl-dependency-scanning-report.json new file mode 100644 index 00000000000000..ff452b6b59222a --- /dev/null +++ b/ee/spec/fixtures/security-reports/feature-branch/gl-dependency-scanning-report.json @@ -0,0 +1,170 @@ +{ + "version": "1.3", + "vulnerabilities": [ + { + "category": "dependency_scanning", + "name": "io.netty/netty - CVE-2014-3488", + "message": "DoS by CPU exhaustion when using malicious SSL packets", + "cve": "app/pom.xml:io.netty/netty@3.9.1.Final:CVE-2014-3488", + "severity": "Unknown", + "solution": "Upgrade to the latest version", + "scanner": { + "id": "gemnasium", + "name": "Gemnasium" + }, + "location": { + "file": "app/pom.xml", + "dependency": { + "package": { + "name": "io.netty/netty" + }, + "version": "3.9.1.Final" + } + }, + "identifiers": [ + { + "type": "gemnasium", + "name": "Gemnasium-d1bf36d9-9f07-46cd-9cfc-8675338ada8f", + "value": "d1bf36d9-9f07-46cd-9cfc-8675338ada8f", + "url": "https://deps.sec.gitlab.com/packages/maven/io.netty/netty/versions/3.9.1.Final/advisories" + }, + { + "type": "cve", + "name": "CVE-2014-3488", + "value": "CVE-2014-3488", + "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3488" + } + ], + "links": [ + { + "url": "https://bugzilla.redhat.com/CVE-2014-3488" + }, + { + "url": "http://netty.io/news/2014/06/11/3.html" + }, + { + "url": "https://github.com/netty/netty/issues/2562" + } + ], + "priority": "Unknown", + "file": "app/pom.xml", + "url": "https://bugzilla.redhat.com/CVE-2014-3488", + "tool": "gemnasium" + }, + { + "category": "dependency_scanning", + "name": "Django - CVE-2017-12794", + "message": "Possible XSS in traceback section of technical 500 debug page", + "cve": "app/requirements.txt:Django@1.11.3:CVE-2017-12794", + "severity": "Unknown", + "solution": "Upgrade to latest version or apply patch.", + "scanner": { + "id": "gemnasium", + "name": "Gemnasium" + }, + "location": { + "file": "app/requirements.txt", + "dependency": { + "package": { + "name": "Django" + }, + "version": "1.11.3" + } + }, + "identifiers": [ + { + "type": "gemnasium", + "name": "Gemnasium-6162a015-8635-4a15-8d7c-dc9321db366f", + "value": "6162a015-8635-4a15-8d7c-dc9321db366f", + "url": "https://deps.sec.gitlab.com/packages/pypi/Django/versions/1.11.3/advisories" + }, + { + "type": "cve", + "name": "CVE-2017-12794", + "value": "CVE-2017-12794", + "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-12794" + } + ], + "links": [ + { + "url": "https://www.djangoproject.com/weblog/2017/sep/05/security-releases/" + } + ], + "priority": "Unknown", + "file": "app/requirements.txt", + "url": "https://www.djangoproject.com/weblog/2017/sep/05/security-releases/", + "tool": "gemnasium" + }, + { + "category": "dependency_scanning", + "message": "Directory traversal vulnerability in rubyzip", + "cve": "Gemfile.lock:rubyzip:cve:CVE-2017-5946", + "severity": "High", + "solution": "upgrade to \u003e= 1.2.1", + "scanner": { + "id": "bundler_audit", + "name": "bundler-audit" + }, + "location": { + "file": "Gemfile.lock", + "dependency": { + "package": { + "name": "rubyzip" + }, + "version": "1.2.0" + } + }, + "identifiers": [ + { + "type": "cve", + "name": "CVE-2017-5946", + "value": "CVE-2017-5946", + "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-5946" + } + ], + "links": [ + { + "url": "https://github.com/rubyzip/rubyzip/issues/315" + } + ] + }, + { + "category": "dependency_scanning", + "name": "ffi - CVE-2018-1000201", + "message": "ruby-ffi DDL loading issue on Windows OS", + "cve": "ffi:1.9.18:CVE-2018-1000201", + "severity": "High", + "solution": "upgrade to \u003e= 1.9.24", + "scanner": { + "id": "bundler_audit", + "name": "bundler-audit" + }, + "location": { + "file": "sast-sample-rails/Gemfile.lock", + "dependency": { + "package": { + "name": "ffi" + }, + "version": "1.9.18" + } + }, + "identifiers": [ + { + "type": "cve", + "name": "CVE-2018-1000201", + "value": "CVE-2018-1000201", + "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-1000201" + } + ], + "links": [ + { + "url": "https://github.com/ffi/ffi/releases/tag/1.9.24" + } + ], + "priority": "High", + "file": "sast-sample-rails/Gemfile.lock", + "url": "https://github.com/ffi/ffi/releases/tag/1.9.24", + "tool": "bundler_audit" + } + ] + } \ No newline at end of file diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index f897976a2ca064..5fd4edaeb75a85 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -139,6 +139,27 @@ end end + describe '#has_dependency_scanning_reports?' do + subject { merge_request.has_dependency_scanning_reports? } + let(:project) { create(:project, :repository) } + + before do + stub_licensed_features(container_scanning: true) + end + + context 'when head pipeline has dependency scannning reports' do + let(:merge_request) { create(:ee_merge_request, :with_dependency_scanning_reports, source_project: project) } + + it { is_expected.to be_truthy } + end + + context 'when head pipeline does not have dependency scanning reports' do + let(:merge_request) { create(:ee_merge_request, source_project: project) } + + it { is_expected.to be_falsey } + end + end + describe '#has_container_scanning_reports?' do subject { merge_request.has_container_scanning_reports? } let(:project) { create(:project, :repository) } diff --git a/ee/spec/services/ci/compare_dependency_scanning_reports_service_spec.rb b/ee/spec/services/ci/compare_dependency_scanning_reports_service_spec.rb new file mode 100644 index 00000000000000..78e3792d061def --- /dev/null +++ b/ee/spec/services/ci/compare_dependency_scanning_reports_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::CompareDependencyScanningReportsService do + let(:service) { described_class.new(project) } + let(:project) { create(:project, :repository) } + + before do + stub_licensed_features(dependency_scanning: true) + end + + describe '#execute' do + subject { service.execute(base_pipeline, head_pipeline) } + + context 'when head pipeline has dependency scanning reports' do + let!(:base_pipeline) { create(:ee_ci_pipeline) } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) } + + it 'reports new vulnerabilities' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(4) + 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 dependency scanning reports' do + let!(:base_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_report, project: project) } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_dependency_scanning_feature_branch, project: project) } + + it 'reports status as parsed' do + expect(subject[:status]).to eq(:parsed) + end + + it 'reports fixed vulnerability' do + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['added']).to include(a_hash_including('compare_key' => 'Gemfile.lock:rubyzip:cve:CVE-2017-5946')) + end + + it 'reports existing dependency vulenerabilities' do + expect(subject[:data]['existing'].count).to eq(3) + end + + it 'reports fixed dependency scanning vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(1) + compare_keys = subject[:data]['fixed'].map { |t| t['compare_key'] } + expected_keys = %w(rails/Gemfile.lock:nokogiri@1.8.0:USN-3424-1) + expect(compare_keys - expected_keys).to eq([]) + end + end + + context 'when head pipeline has corrupted dependency scanning vulnerability reports' do + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_corrupted_dependency_scanning_report, project: project) } + + it 'returns status and error message' do + expect(subject[:status]).to eq(:error) + expect(subject[:status_reason]).to include('JSON parsing failed') + end + end + end +end -- GitLab