Merge branch...

Merge branch 'reports_scanned_resources_should_return_a_scanned_resource_object_227703' into 'master'

Replace scanned_resources_for_csv with ScannedResource object

See merge request gitlab-org/gitlab!37540

ee/app/controllers/projects/security/scanned_resources_controller.rb

@@ -30,14 +30,14 @@ module Projects
 
       def render_csv
         CsvBuilders::SingleBatch.new(
-          ::Gitlab::Ci::Parsers::Security::ScannedResources.new.scanned_resources_for_csv(@scanned_resources),
+          @scanned_resources,
           {
             'Method': 'request_method',
-            'Scheme': 'scheme',
-            'Host': 'host',
-            'Port': 'port',
-            'Path': 'path',
-            'Query String': 'query_string'
+            'Scheme': 'url_scheme',
+            'Host': 'url_host',
+            'Port': 'url_port',
+            'Path': 'url_path',
+            'Query String': 'url_query'
           }
         ).render
       end

ee/app/services/security/scanned_resources_service.rb

@@ -21,8 +21,8 @@ module Security
         scanned_resources = scanned_resources.first(@limit) if @limit
         acc[type] = scanned_resources.map do |resource|
           {
-            'request_method' => resource['method'],
-            'url' => resource['url']
+            'request_method' => resource.request_method,
+            'url' => resource.request_uri.to_s
           }
         end
       end

ee/lib/gitlab/ci/parsers/security/common.rb

@@ -11,7 +11,8 @@ module Gitlab
             report_data = parse_report(json_data)
             raise SecurityReportParserError, "Invalid report format" unless report_data.is_a?(Hash)
 
-            report.scanned_resources = report_data.dig('scan', 'scanned_resources') || []
+            report.scanned_resources = create_scanned_resources(report_data.dig('scan', 'scanned_resources'))
+
             create_scanner(report, report_data.dig('scan', 'scanner'))
 
             collate_remediations(report_data).each do |vulnerability|
@@ -26,6 +27,17 @@ module Gitlab
 
           protected
 
+          def create_scanned_resources(scanned_resources)
+            return [] unless scanned_resources
+
+            scanned_resources.map do |sr|
+              uri = URI.parse(sr['url'])
+              ::Gitlab::Ci::Reports::Security::ScannedResource.new(uri, sr['method'])
+            rescue URI::InvalidURIError
+              nil
+            end.compact
+          end
+
           def parse_report(json_data)
             Gitlab::Json.parse!(json_data)
           end

ee/lib/gitlab/ci/parsers/security/scanned_resources.rb

@@ -13,21 +13,6 @@ module Gitlab
             scanned_resources_sum
           end
 
-          def scanned_resources_for_csv(scanned_resources)
-            scanned_resources.map do |sr|
-              uri = URI.parse(sr['url'] || '')
-              OpenStruct.new({
-                request_method: sr['method'],
-                scheme: uri.scheme,
-                host: uri.host,
-                port: uri.port,
-                path: uri.path,
-                query_string: uri.query,
-                raw_url: sr['url']
-              })
-            end
-          end
-
           private
 
           def parse_report_json(blob)

ee/lib/gitlab/ci/reports/security/scanned_resource.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Reports
+      module Security
+        class ScannedResource
+          include Gitlab::Utils::StrongMemoize
+
+          attr_reader :request_method
+          attr_reader :request_uri
+
+          delegate :scheme, :host, :port, :path, :query, to: :request_uri, prefix: :url
+
+          def initialize(uri, request_method)
+            raise ArgumentError unless uri.is_a?(URI)
+
+            @request_method = request_method
+            @request_uri = uri
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/projects/security/scanned_resources_controller_spec.rb

@@ -30,13 +30,6 @@ RSpec.describe Projects::Security::ScannedResourcesController do
         end
       end
 
-      it 'returns a CSV representation of the scanned resources' do
-        expect_next_instance_of(::Gitlab::Ci::Parsers::Security::ScannedResources) do |instance|
-          expect(instance).to receive(:scanned_resources_for_csv).and_return([])
-        end
-        expect(subject).to have_gitlab_http_status(:ok)
-      end
-
       context 'when the pipeline id is missing' do
         let_it_be(:action_params) { { project_id: project, namespace_id: project.namespace } }
 

ee/spec/lib/gitlab/ci/parsers/security/common_spec.rb

@@ -24,67 +24,135 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       expect(report.findings.map(&:confidence)).not_to include("undefined")
     end
 
+    context 'parsing scanned resources' do
+      describe 'when the URL is invalid' do
+        let(:raw_json) do
+          {
+          "vulnerabilities": [],
+          "remediations": [],
+          "dependency_files": [],
+          "scan": {
+            "scanned_resources": [
+              {
+                "method": "GET",
+                "type": "url",
+                "url": "not a URL"
+              }
+            ]
+          }
+        }
+        end
+
+        it 'skips invalid URLs' do
+          parser.parse!(raw_json.to_json, report)
+
+          expect(report.scanned_resources).to be_empty
+        end
+      end
+
+      describe 'when the URLs are valid' do
+        let(:raw_json) do
+          {
+          "vulnerabilities": [],
+          "remediations": [],
+          "dependency_files": [],
+          "scan": {
+            "scanned_resources": [
+              {
+                "method": "GET",
+                "type": "url",
+                "url": "http://example.com/1"
+              },
+              {
+                "method": "POST",
+                "type": "url",
+                "url": "http://example.com/2"
+              },
+              {
+                "method": "GET",
+                "type": "url",
+                "url": "http://example.com/3"
+              }
+            ]
+          }
+        }
+        end
+
+        it 'creates a scanned resource for each URL' do
+          parser.parse!(raw_json.to_json, report)
+
+          expect(report.scanned_resources.length).to eq(3)
+        end
+
+        it 'converts the JSON to Scanned Resource objects' do
+          parser.parse!(raw_json.to_json, report)
+
+          expect(report.scanned_resources.first).to be_a(::Gitlab::Ci::Reports::Security::ScannedResource)
+        end
+      end
+    end
+
     context 'parsing remediations' do
       let(:raw_json) do
         {
-        "vulnerabilities": [
-          {
-            "category": "dependency_scanning",
-            "name": "Vulnerabilities in libxml2",
-            "message": "Vulnerabilities in libxml2 in nokogiri",
-            "description": "",
-            "cve": "CVE-1020",
-            "severity": "High",
-            "solution": "Upgrade to latest version.",
-            "scanner": { "id": "gemnasium", "name": "Gemnasium" },
-            "location": {},
-            "identifiers": [],
-            "links": [{ "url": "" }]
-          },
-          {
-            "id": "bb2fbeb1b71ea360ce3f86f001d4e84823c3ffe1a1f7d41ba7466b14cfa953d3",
-            "category": "dependency_scanning",
-            "name": "Regular Expression Denial of Service",
-            "message": "Regular Expression Denial of Service in debug",
-            "description": "",
-            "cve": "CVE-1030",
-            "severity": "Unknown",
-            "solution": "Upgrade to latest versions.",
-            "scanner": {
-              "id": "gemnasium",
-              "name": "Gemnasium"
+          "vulnerabilities": [
+            {
+              "category": "dependency_scanning",
+              "name": "Vulnerabilities in libxml2",
+              "message": "Vulnerabilities in libxml2 in nokogiri",
+              "description": "",
+              "cve": "CVE-1020",
+              "severity": "High",
+              "solution": "Upgrade to latest version.",
+              "scanner": { "id": "gemnasium", "name": "Gemnasium" },
+              "location": {},
+              "identifiers": [],
+              "links": [{ "url": "" }]
             },
-            "location": {},
-            "identifiers": [],
-            "links": [{ "url": "" }]
-          },
-          {
-            "category": "dependency_scanning",
-            "name": "Authentication bypass via incorrect DOM traversal and canonicalization",
-            "message": "Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js",
-            "description": "",
-            "cve": "yarn/yarn.lock:saml2-js:gemnasium:9952e574-7b5b-46fa-a270-aeb694198a98",
-            "severity": "Unknown",
-            "solution": "Upgrade to fixed version.\r\n",
+            {
+              "id": "bb2fbeb1b71ea360ce3f86f001d4e84823c3ffe1a1f7d41ba7466b14cfa953d3",
+              "category": "dependency_scanning",
+              "name": "Regular Expression Denial of Service",
+              "message": "Regular Expression Denial of Service in debug",
+              "description": "",
+              "cve": "CVE-1030",
+              "severity": "Unknown",
+              "solution": "Upgrade to latest versions.",
+              "scanner": {
+                "id": "gemnasium",
+                "name": "Gemnasium"
+              },
+              "location": {},
+              "identifiers": [],
+              "links": [{ "url": "" }]
+            },
+            {
+              "category": "dependency_scanning",
+              "name": "Authentication bypass via incorrect DOM traversal and canonicalization",
+              "message": "Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js",
+              "description": "",
+              "cve": "yarn/yarn.lock:saml2-js:gemnasium:9952e574-7b5b-46fa-a270-aeb694198a98",
+              "severity": "Unknown",
+              "solution": "Upgrade to fixed version.\r\n",
+              "scanner": {
+                "id": "gemnasium",
+                "name": "Gemnasium"
+              },
+              "location": {},
+              "identifiers": [],
+              "links": [{ "url": "" }, { "url": "" }]
+            }
+          ],
+          "remediations": [],
+          "dependency_files": [],
+          "scan": {
             "scanner": {
               "id": "gemnasium",
-              "name": "Gemnasium"
-            },
-            "location": {},
-            "identifiers": [],
-            "links": [{ "url": "" }, { "url": "" }]
-          }
-        ],
-        "remediations": [],
-        "dependency_files": [],
-        "scan": {
-          "scanner": {
-            "id": "gemnasium",
-            "name": "Gemnasium",
-            "vendor": { "name": "GitLab" }
+              "name": "Gemnasium",
+              "vendor": { "name": "GitLab" }
+            }
           }
         }
-      }
       end
 
       it 'finds remediation with same cve' do

ee/spec/lib/gitlab/ci/parsers/security/scanned_resources_spec.rb

@@ -32,61 +32,4 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ScannedResources do
       it { is_expected.to be(0) }
     end
   end
-
-  describe '#scanned_resources_for_csv' do
-    subject { parser.scanned_resources_for_csv(scanned_resources) }
-
-    context 'when there are scanned resources' do
-      let(:scanned_resources) do
-        [
-          { "method" => "GET", "type" => "url", "url" => "http://railsgoat:3001" },
-          { "method" => "GET", "type" => "url", "url" => "http://railsgoat:3001/" },
-          { "method" => "GET", "type" => "url", "url" => "http://railsgoat:3001/login?foo=bar" },
-          { "method" => "POST", "type" => "url", "url" => "http://railsgoat:3001/users" }
-        ]
-      end
-
-      it 'converts the hash to OpenStructs', :aggregate_failures do
-        expect(subject.length).to eq(4)
-        resource = subject[2]
-        expect(resource.request_method).to eq('GET')
-        expect(resource.scheme).to eq('http')
-        expect(resource.host).to eq('railsgoat')
-        expect(resource.port).to eq(3001)
-        expect(resource.path).to eq('/login')
-        expect(resource.query_string).to eq('foo=bar')
-      end
-    end
-
-    context 'when there is an invalid URL' do
-      let(:scanned_resources) do
-        [
-          { "method" => "GET", "type" => "url", "url" => "http://railsgoat:3001" },
-          { "method" => "GET", "type" => "url", "url" => "notaURL" },
-          { "method" => "GET", "type" => "url", "url" => "" },
-          { "method" => "GET", "type" => "url", "url" => nil },
-          { "method" => "GET", "type" => "url", "url" => "http://railsgoat:3001/login?foo=bar" }
-        ]
-      end
-
-      it 'returns a blank object with full URL string', :aggregate_failures do
-        expect(subject.length).to eq(5)
-
-        invalid_url = subject[1]
-        expect(invalid_url.request_method).to eq('GET')
-        expect(invalid_url.scheme).to be_nil
-        expect(invalid_url.raw_url).to eq('notaURL')
-
-        blank_url = subject[2]
-        expect(blank_url.request_method).to eq('GET')
-        expect(blank_url.scheme).to be_nil
-        expect(blank_url.raw_url).to eq('')
-
-        nil_url = subject[3]
-        expect(nil_url.request_method).to eq('GET')
-        expect(nil_url.scheme).to be_nil
-        expect(nil_url.raw_url).to be_nil
-      end
-    end
-  end
 end

ee/spec/lib/gitlab/ci/reports/security/scanned_resource_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Reports::Security::ScannedResource do
+  let(:url) { 'http://example.com:3001/1?foo=bar' }
+  let(:request_method) { 'GET' }
+
+  context 'when the URI is not a URI' do
+    subject { ::Gitlab::Ci::Reports::Security::ScannedResource.new(url, request_method) }
+
+    it 'raises an error' do
+      expect { subject }.to raise_error(ArgumentError)
+    end
+  end
+
+  context 'when the URL is valid' do
+    subject { ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse(url), request_method) }
+
+    it 'sets the URL attributes' do
+      expect(subject.request_method).to eq(request_method)
+      expect(subject.request_uri.to_s).to eq(url)
+      expect(subject.url_scheme).to eq('http')
+      expect(subject.url_host).to eq('example.com')
+      expect(subject.url_port).to eq(3001)
+      expect(subject.url_path).to eq('/1')
+      expect(subject.url_query).to eq('foo=bar')
+    end
+  end
+end

ee/spec/services/security/merge_reports_service_spec.rb

@@ -82,13 +82,29 @@ RSpec.describe Security::MergeReportsService, '#execute' do
 
   let(:report_1_findings) { [finding_id_1, finding_id_2_loc_1, finding_cwe_2, finding_wasc_1] }
 
+  let(:scanned_resource) do
+    ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com'), 'GET')
+  end
+
+  let(:scanned_resource_1) do
+    ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com'), 'POST')
+  end
+
+  let(:scanned_resource_2) do
+    ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com/2'), 'GET')
+  end
+
+  let(:scanned_resource_3) do
+    ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com/3'), 'GET')
+  end
+
   let(:report_1) do
     build(
       :ci_reports_security_report,
       scanners: [scanner_1, scanner_2],
       findings: report_1_findings,
       identifiers: report_1_findings.flat_map(&:identifiers),
-      scanned_resources: ['example.com', 'example.com/1', 'example.com/2']
+      scanned_resources: [scanned_resource, scanned_resource_1, scanned_resource_2]
     )
   end
 
@@ -100,7 +116,7 @@ RSpec.describe Security::MergeReportsService, '#execute' do
       scanners: [scanner_2],
       findings: report_2_findings,
       identifiers: finding_id_2_loc_2.identifiers,
-      scanned_resources: ['example.com', 'example.com/3']
+      scanned_resources: [scanned_resource, scanned_resource_1, scanned_resource_3]
     )
   end
 
@@ -153,10 +169,10 @@ RSpec.describe Security::MergeReportsService, '#execute' do
   it 'deduplicates scanned resources' do
     expect(subject.scanned_resources).to(
       eq([
-        'example.com',
-        'example.com/1',
-        'example.com/2',
-        'example.com/3'
+        scanned_resource,
+        scanned_resource_1,
+        scanned_resource_2,
+        scanned_resource_3
       ])
     )
   end