From d552e364fc6edd6e6655400dfe6962a6d37000a7 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 6 Apr 2021 14:59:12 +0200 Subject: [PATCH 1/3] retry 5 times on integrity error and then block processing --- app/jobs/virus_scanner_job.rb | 11 ++++++++++- app/lib/active_storage/virus_scanner.rb | 5 +++++ spec/jobs/virus_scanner_job_spec.rb | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/jobs/virus_scanner_job.rb b/app/jobs/virus_scanner_job.rb index 71b859dc0..3f612184f 100644 --- a/app/jobs/virus_scanner_job.rb +++ b/app/jobs/virus_scanner_job.rb @@ -11,7 +11,16 @@ class VirusScannerJob < ApplicationJob # If the file is not analyzed yet, retry later (to avoid clobbering metadata) retry_on FileNotAnalyzedYetError, wait: :exponentially_longer, attempts: 10 # If for some reason the file appears invalid, retry for a while - retry_on ActiveStorage::IntegrityError, attempts: 10, wait: 5.seconds + retry_on(ActiveStorage::IntegrityError, attempts: 5, wait: 5.seconds) do |job, _error| + blob = job.arguments.first + + metadata = { + virus_scan_result: ActiveStorage::VirusScanner::INTEGRITY_ERROR, + scanned_at: Time.zone.now + } + + blob.update!(metadata: blob.metadata.merge(metadata)) + end def perform(blob) if !blob.analyzed? then raise FileNotAnalyzedYetError end diff --git a/app/lib/active_storage/virus_scanner.rb b/app/lib/active_storage/virus_scanner.rb index b51b05855..90cd0a528 100644 --- a/app/lib/active_storage/virus_scanner.rb +++ b/app/lib/active_storage/virus_scanner.rb @@ -8,6 +8,7 @@ class ActiveStorage::VirusScanner PENDING = 'pending' INFECTED = 'infected' SAFE = 'safe' + INTEGRITY_ERROR = 'integrity_error' def pending? blob.metadata[:virus_scan_result] == PENDING @@ -21,6 +22,10 @@ class ActiveStorage::VirusScanner blob.metadata[:virus_scan_result] == SAFE end + def corrupt? + blob.metadata[:virus_scan_result] == INTEGRITY_ERROR + end + def done? started? && blob.metadata[:virus_scan_result] != PENDING end diff --git a/spec/jobs/virus_scanner_job_spec.rb b/spec/jobs/virus_scanner_job_spec.rb index 887fd3260..ce663cdac 100644 --- a/spec/jobs/virus_scanner_job_spec.rb +++ b/spec/jobs/virus_scanner_job_spec.rb @@ -18,6 +18,20 @@ describe VirusScannerJob, type: :job do blob.analyze end + context "when there is an integrity error" do + before do + blob.update_column('checksum', 'integrity error') + + assert_performed_jobs(5) do + VirusScannerJob.perform_later(blob) + end + end + + it do + expect(blob.reload.virus_scanner.corrupt?).to be_truthy + end + end + context "when no virus is found" do before do allow(ClamavService).to receive(:safe_file?).and_return(true) From e636e3a752f43cc7148fbf15d5575dc4cb1a3693 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 6 Apr 2021 18:00:54 +0200 Subject: [PATCH 2/3] add merge_and_update_metadata method --- app/jobs/virus_scanner_job.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/jobs/virus_scanner_job.rb b/app/jobs/virus_scanner_job.rb index 3f612184f..a76ee8979 100644 --- a/app/jobs/virus_scanner_job.rb +++ b/app/jobs/virus_scanner_job.rb @@ -19,7 +19,7 @@ class VirusScannerJob < ApplicationJob scanned_at: Time.zone.now } - blob.update!(metadata: blob.metadata.merge(metadata)) + merge_and_update_metadata(blob, metadata) end def perform(blob) @@ -27,10 +27,16 @@ class VirusScannerJob < ApplicationJob if blob.virus_scanner.done? then return end metadata = extract_metadata_via_virus_scanner(blob) - blob.update!(metadata: blob.metadata.merge(metadata)) + VirusScannerJob.merge_and_update_metadata(blob, metadata) end def extract_metadata_via_virus_scanner(blob) ActiveStorage::VirusScanner.new(blob).metadata end + + private + + def self.merge_and_update_metadata(blob, metadata) + blob.update!(metadata: blob.metadata.merge(metadata)) + end end From 7567e51a3f39ce3936e9d7960c15915612465010 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 6 Apr 2021 16:32:45 +0200 Subject: [PATCH 3/3] add ui for integrity error --- app/views/shared/attachment/_show.html.haml | 6 ++++++ spec/views/shared/attachment/_show.html.haml_spec.rb | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/app/views/shared/attachment/_show.html.haml b/app/views/shared/attachment/_show.html.haml index 34b8c62e7..4437724a7 100644 --- a/app/views/shared/attachment/_show.html.haml +++ b/app/views/shared/attachment/_show.html.haml @@ -28,3 +28,9 @@ (virus détecté, merci d’envoyer un autre fichier) - else (virus détecté, le téléchargement de ce fichier est bloqué) + - elsif attachment.virus_scanner.corrupt? + - if user_can_upload + (le fichier est corrompu, merci d’envoyer un autre fichier) + - else + (le fichier est corrompu, le téléchargement est bloqué) + diff --git a/spec/views/shared/attachment/_show.html.haml_spec.rb b/spec/views/shared/attachment/_show.html.haml_spec.rb index faa00daf4..b9737c8c4 100644 --- a/spec/views/shared/attachment/_show.html.haml_spec.rb +++ b/spec/views/shared/attachment/_show.html.haml_spec.rb @@ -55,4 +55,14 @@ describe 'shared/attachment/_show.html.haml', type: :view do expect(subject).to have_text('virus détecté') end end + + context 'when the file is corrupted' do + let(:virus_scan_result) { ActiveStorage::VirusScanner::INTEGRITY_ERROR } + + it 'displays the filename, but doesn’t allow to download the file' do + expect(subject).to have_text(champ.piece_justificative_file.filename.to_s) + expect(subject).not_to have_link(champ.piece_justificative_file.filename.to_s) + expect(subject).to have_text('corrompu') + end + end end