Commit 195e3aad authored by mo khan's avatar mo khan

Return empty report instead of raising error

The backtrace and context of the original error is lost
when we rescue and raise a new error. This makes it difficult
to debug.

If a report has been tampered with and is invalid
it is safer to return an empty report than to
bubble up an error that cannot be dealt with
meaningfully by clients of the parser code.

There is no action that we can take
when a JSON parse error occurs due to
an invalid JSON document. Re-processing
the document will not yield a different result.

It is very easy to upload a `gl-license-scanning-report.json`
that is not a valid JSON document. This change stops recording
errors that we cannot take action on.
parent 5b6fafe9
---
title: Stop recording JSON parser errors due to third party license scanning integrations
merge_request: 26944
author:
type: fixed
......@@ -17,11 +17,17 @@ module Gitlab
parser = PARSERS.fetch(report.major_version)
parser.new(report).parse(json)
rescue JSON::ParserError
raise LicenseScanningParserError, 'JSON parsing failed'
rescue => e
Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
raise LicenseScanningParserError, 'License scanning report parsing failed'
rescue JSON::ParserError => error
Gitlab::ErrorTracking.track_exception(error, error_details_for(json_data))
end
private
def error_details_for(json)
return { message: 'artifact is blank' } if json.blank?
return { message: "artifact is too big (#{json.bytesize} bytes)" } if json.bytesize > 1.megabyte
{ message: 'artifact is not JSON', raw: json }
end
end
end
......
......@@ -173,11 +173,12 @@ RSpec.describe Gitlab::Ci::Parsers::LicenseCompliance::LicenseScanning do
end
context 'when the report is not a valid JSON document' do
it do
expect do
subject.parse!('blah', report)
end.to raise_error(Gitlab::Ci::Parsers::LicenseCompliance::LicenseScanning::LicenseScanningParserError)
before do
subject.parse!('invalid', report)
end
it { expect(report.version).to eql('1.0') }
it { expect(report.licenses).to be_empty }
end
end
end
......@@ -252,8 +252,9 @@ RSpec.describe Ci::Build do
create(:ee_ci_job_artifact, :license_scan, :with_corrupted_data, job: job, project: job.project)
end
it 'raises an error' do
expect { subject }.to raise_error(Gitlab::Ci::Parsers::LicenseCompliance::LicenseScanning::LicenseScanningParserError)
it 'returns an empty report' do
expect { subject }.not_to raise_error
expect(license_scanning_report).to be_empty
end
end
......
......@@ -52,16 +52,16 @@ RSpec.describe Ci::CompareLicenseScanningReportsService do
let!(:base_pipeline) { build(:ee_ci_pipeline, :with_corrupted_license_scanning_report, project: project) }
let!(:head_pipeline) { build(:ee_ci_pipeline, :with_corrupted_license_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')
it 'does not expose parser errors' do
expect(subject[:status]).to eq(:parsed)
end
it 'returns status and error message when pipeline is nil' do
result = service.execute(nil, head_pipeline)
context "when the base pipeline is nil" do
subject { service.execute(nil, head_pipeline) }
expect(result[:status]).to eq(:error)
expect(result[:status_reason]).to include('JSON parsing failed')
it 'does not expose parser errors' do
expect(subject[:status]).to eq(:parsed)
end
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment