Merge branch '353997-use-versioned-schemas-when-validating-security-reports' into 'master'

Use versioned schemas when validating security reports

See merge request gitlab-org/gitlab!81907

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

@@ -19,6 +19,8 @@ module Gitlab
           end
 
           def parse!
+            set_report_version
+
             return report_data unless valid?
 
             raise SecurityReportParserError, "Invalid report format" unless report_data.is_a?(Hash)
@@ -26,7 +28,6 @@ module Gitlab
             create_scanner
             create_scan
             create_analyzer
-            set_report_version
 
             create_findings
 
@@ -66,7 +67,7 @@ module Gitlab
           end
 
           def schema_validator
-            @schema_validator ||= ::Gitlab::Ci::Parsers::Security::Validators::SchemaValidator.new(report.type, report_data)
+            @schema_validator ||= ::Gitlab::Ci::Parsers::Security::Validators::SchemaValidator.new(report.type, report_data, report.version)
           end
 
           def report_data

lib/gitlab/ci/parsers/security/validators/schema_validator.rb

@@ -46,15 +46,16 @@ module Gitlab
                 File.join(__dir__, 'schemas')
               end
 
-              def initialize(report_type)
+              def initialize(report_type, report_version)
                 @report_type = report_type.to_sym
+                @report_version = report_version.to_s
               end
 
               delegate :validate, to: :schemer
 
               private
 
-              attr_reader :report_type
+              attr_reader :report_type, :report_version
 
               def schemer
                 JSONSchemer.schema(pathname)
@@ -65,7 +66,19 @@ module Gitlab
               end
 
               def schema_path
-                File.join(root_path, file_name)
+                # We can't exactly error out here pre-15.0.
+                # If the report itself doesn't specify the schema version,
+                # it will be considered invalid post-15.0 but for now we will
+                # validate against earliest supported version.
+                # https://gitlab.com/gitlab-org/gitlab/-/issues/335789#note_801479803
+                # describes the indended behavior in detail
+                # TODO: After 15.0 - pass report_type and report_data here and
+                # error out if no version.
+                report_declared_version = File.join(root_path, report_version, file_name)
+                return report_declared_version if File.file?(report_declared_version)
+
+                earliest_supported_version = SUPPORTED_VERSIONS[report_type].min
+                File.join(root_path, earliest_supported_version, file_name)
               end
 
               def file_name
@@ -73,9 +86,10 @@ module Gitlab
               end
             end
 
-            def initialize(report_type, report_data)
+            def initialize(report_type, report_data, report_version = nil)
               @report_type = report_type
               @report_data = report_data
+              @report_version = report_version
             end
 
             def valid?
@@ -88,10 +102,10 @@ module Gitlab
 
             private
 
-            attr_reader :report_type, :report_data
+            attr_reader :report_type, :report_data, :report_version
 
             def schema
-              Schema.new(report_type)
+              Schema.new(report_type, report_version)
             end
           end
         end

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

@@ -60,7 +60,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
             it 'instantiates the validator with correct params' do
               parse_report
 
-              expect(validator_class).to have_received(:new).with(report.type, {})
+              expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
             end
 
             context 'when the report data is not valid according to the schema' do
@@ -110,7 +110,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
             it 'instantiates the validator with correct params' do
               parse_report
 
-              expect(validator_class).to have_received(:new).with(report.type, {})
+              expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
             end
 
             context 'when the report data is not valid according to the schema' do
@@ -175,7 +175,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
             it 'instantiates the validator with correct params' do
               parse_report
 
-              expect(validator_class).to have_received(:new).with(report.type, {})
+              expect(validator_class).to have_received(:new).with(report.type, {}, report.version)
             end
 
             context 'when the report data is not valid according to the schema' do

spec/lib/gitlab/ci/parsers/security/validators/schema_validator_spec.rb

@@ -49,14 +49,14 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Validators::SchemaValidator do
 
   using RSpec::Parameterized::TableSyntax
 
-  where(:report_type, :expected_errors, :valid_data) do
-    'sast' | ['root is missing required keys: vulnerabilities'] | { 'version' => '10.0.0', 'vulnerabilities' => [] }
-    :sast  | ['root is missing required keys: vulnerabilities'] | { 'version' => '10.0.0', 'vulnerabilities' => [] }
-    :secret_detection | ['root is missing required keys: vulnerabilities'] | { 'version' => '10.0.0', 'vulnerabilities' => [] }
+  where(:report_type, :report_version, :expected_errors, :valid_data) do
+    'sast' | '10.0.0' | ['root is missing required keys: vulnerabilities'] | { 'version' => '10.0.0', 'vulnerabilities' => [] }
+    :sast  | '10.0.0' | ['root is missing required keys: vulnerabilities'] | { 'version' => '10.0.0', 'vulnerabilities' => [] }
+    :secret_detection | '10.0.0' | ['root is missing required keys: vulnerabilities'] | { 'version' => '10.0.0', 'vulnerabilities' => [] }
   end
 
   with_them do
-    let(:validator) { described_class.new(report_type, report_data) }
+    let(:validator) { described_class.new(report_type, report_data, report_version) }
 
     describe '#valid?' do
       subject { validator.valid? }
@@ -72,6 +72,15 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Validators::SchemaValidator do
 
         it { is_expected.to be_truthy }
       end
+
+      context 'when no report_version is provided' do
+        let(:report_version) { nil }
+        let(:report_data) { valid_data }
+
+        it 'does not fail' do
+          expect { subject }.not_to raise_error
+        end
+      end
     end
 
     describe '#errors' do