diff --git a/docker/server/run-server-tests.sh b/docker/server/run-server-tests.sh index a857bbea7..e02bd5ac1 100755 --- a/docker/server/run-server-tests.sh +++ b/docker/server/run-server-tests.sh @@ -19,6 +19,9 @@ do done #cd /opt/openstudio/server && bundle exec rspec; (( exit_status = exit_status || $? )) +# Model/request specs for seed.zip upload validation + InitializeAnalysis failure handling (issue #841). +# These need only rails+mongo, so run them first - they are fast and leave the db empty. +cd /opt/openstudio/server && bundle exec rspec spec/models/analysis_init_spec.rb spec/requests/analyses_upload_spec.rb; (( exit_status = exit_status || $? )) # Run only the algorithm specs. The other features/*_spec files should probably disappear and capybara/gecko # can be removed. cd /opt/openstudio/server && bundle exec rspec spec/features/docker_stack_custom_gems.rb; (( exit_status = exit_status || $? )) diff --git a/server/app/controllers/analyses_controller.rb b/server/app/controllers/analyses_controller.rb index b29249c76..e70cc5f7a 100644 --- a/server/app/controllers/analyses_controller.rb +++ b/server/app/controllers/analyses_controller.rb @@ -140,6 +140,21 @@ def create params = analysis_params params[:project_id] = project_id + # Reject corrupt seed zips at creation time with a 422 instead of letting them fail + # later in ResqueJobs::InitializeAnalysis and strand the analysis (issue #841) + if params[:seed_zip].respond_to?(:path) + zip_error = Analysis.seed_zip_error(params[:seed_zip].path) + if zip_error + respond_to do |format| + # no new.html.erb view exists (analyses are created via the JSON API), so + # render plain text rather than a missing template + format.html { render plain: zip_error, status: :unprocessable_entity } + format.json { render json: { status: 'error', error_message: zip_error }, status: :unprocessable_entity } + end + return + end + end + @analysis = Analysis.new(params) @analysis.save! # Make sure to save it before processing it further. Rails 5 upgrade issue. @@ -391,6 +406,15 @@ def upload @analysis = Analysis.find(params[:id]) if @analysis + # Reject corrupt seed zips at upload time with a 422 instead of letting them fail + # later in ResqueJobs::InitializeAnalysis and strand the analysis (issue #841) + if params[:file].respond_to?(:path) + zip_error = Analysis.seed_zip_error(params[:file].path) + if zip_error + render json: { status: 'error', error_message: zip_error }, status: :unprocessable_entity + return + end + end @analysis.seed_zip = params[:file] end diff --git a/server/app/jobs/resque_jobs/initialize_analysis.rb b/server/app/jobs/resque_jobs/initialize_analysis.rb index 737f7d881..155afd7cf 100644 --- a/server/app/jobs/resque_jobs/initialize_analysis.rb +++ b/server/app/jobs/resque_jobs/initialize_analysis.rb @@ -9,13 +9,21 @@ class InitializeAnalysis # Perform set up before running an analysis # this is enqueued in Analysis#start. - # todo error handling - # todo handle cleanup if this fails def self.perform(analysis_type, analysis_id, job_id, options = {}) # TODO: error handling and logging around looking up analysis and detecting start/complete analysis = Analysis.find(analysis_id) # this will handle unzipping to osdata volume and running any initialization scripts analysis.run_initialization + rescue StandardError => e + # Mark the analysis failed so it reaches a terminal state visible to clients instead + # of sitting in 'queued' forever, then re-raise so Resque records the failed job (issue #841). + Rails.logger.error "InitializeAnalysis failed for analysis #{analysis_id}: #{e.message}" + begin + analysis&.fail_job!("Analysis initialization failed: #{e.message}") + rescue StandardError => mark_error + Rails.logger.error "Could not mark analysis #{analysis_id} as failed: #{mark_error.message}" + end + raise end # after_perform hooks only called if job completes successfully diff --git a/server/app/models/analysis.rb b/server/app/models/analysis.rb index 390af9b10..b587f6105 100644 --- a/server/app/models/analysis.rb +++ b/server/app/models/analysis.rb @@ -93,6 +93,59 @@ def self.status_states ['na', 'init', 'queued', 'started', 'post-processing', 'completed'] end + # Caps for upload-time zip validation. Far above any real seed zip, but fatal to a + # zip bomb that would otherwise pin a web worker inflating unbounded data in the + # request path. + SEED_ZIP_MAX_INFLATED_BYTES = 10 * 1024 * 1024 * 1024 # 10 GB + SEED_ZIP_MAX_ENTRIES = 100_000 + + # Validate that a file is a structurally sound ZIP whose entries inflate cleanly with + # matching CRCs. Returns nil if valid, otherwise a String describing the problem. + # Called at upload time so corrupt seed zips are rejected with a 422 instead of + # failing later in ResqueJobs::InitializeAnalysis and stranding the analysis (issue #841). + def self.seed_zip_error(file_path) + # Zip::File.new instead of the Zip::File.open block form: open's implicit close + # calls commit, which can REWRITE the archive being validated. new reads the + # central directory and releases the file handle, keeping validation read-only. + entries = 0 + inflated_bytes = 0 + zf = ::Zip::File.new(file_path) + zf.each do |entry| + next unless entry.file? + + entries += 1 + return "seed zip contains more than #{SEED_ZIP_MAX_ENTRIES} file entries" if entries > SEED_ZIP_MAX_ENTRIES + + crc = ::Zlib.crc32 + entry.get_input_stream do |io| + while (chunk = io.read(1_048_576)) + inflated_bytes += chunk.bytesize + return "seed zip inflates to more than #{SEED_ZIP_MAX_INFLATED_BYTES} bytes" if inflated_bytes > SEED_ZIP_MAX_INFLATED_BYTES + + crc = ::Zlib.crc32(chunk, crc) + end + end + return "seed zip entry '#{entry.name}' is corrupt (CRC mismatch)" unless crc == entry.crc + end + nil + rescue ::Zip::Error, ::Zlib::Error => e + "seed zip is not a valid ZIP archive: #{e.message}" + end + + # Mark the analysis's most recent job as failed so the analysis reaches a terminal, + # API-visible state instead of sitting in 'queued'/'started' forever when + # initialization fails (issue #841). + def fail_job!(message) + self.status_message = message + save! + job = jobs.order_by(:index.asc).last + return unless job + + job.status = 'failed' + job.status_message = message + job.save! + end + # FIXME: analysis_type is somewhat ambiguous here, as it's argument to this method and also a class method name def start(no_delay, analysis_type = 'batch_run', options = {}) Rails.logger.debug "analysis.start enter" @@ -495,6 +548,11 @@ def run_initialization # OpenStudio::Workflow.extract_archive(download_file, analysis_dir) extract_archive(seed_zip.path, shared_directory_path) end + rescue ::Zip::Error, ::Zlib::Error => e + # A corrupt zip is a deterministic failure - retrying cannot succeed (issue #841). + # Remove any partially extracted files so a later re-run does not skip-and-reuse them. + FileUtils.rm_rf(shared_directory_path) + raise "Seed zip for analysis #{id} is corrupt and cannot be extracted: #{e.message}" rescue StandardError => e retry if extract_count < extract_max_count raise "Extraction of the seed.zip file failed #{extract_max_count} times with error #{e.message}" diff --git a/server/spec/models/analysis_init_spec.rb b/server/spec/models/analysis_init_spec.rb new file mode 100644 index 000000000..268d032fa --- /dev/null +++ b/server/spec/models/analysis_init_spec.rb @@ -0,0 +1,180 @@ +# ******************************************************************************* +# OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC. +# See also https://openstudio.net/license +# ******************************************************************************* + +require 'rails_helper' +require 'tmpdir' + +# Regression specs for https://github.com/NatLabRockies/OpenStudio-server/issues/841 - +# a corrupt seed.zip must fail fast (no pointless retries of a deterministic failure), +# leave the analysis in a terminal 'failed' state visible to API clients, and be +# detectable before the InitializeAnalysis job ever runs. +RSpec.describe Analysis, type: :model do + before :all do + Project.destroy_all + @analysis = FactoryBot.create(:analysis) + @tmp_dir = Dir.mktmpdir('analysis-init-spec') + end + + after :all do + # Destroy factory data so paperclip deletes the seed zip files and prunes the + # emptied assets/analyses directory. In the docker CI job this spec runs as root + # inside the web container while the live app runs as an unprivileged user - a + # leftover root-owned assets/analyses dir makes every later upload fail with + # EACCES, and leftover projects break docker_stack_test_apis_spec assertions. + Project.destroy_all + FileUtils.rm_rf(@tmp_dir) + end + + def build_valid_zip(path) + Zip::OutputStream.open(path) do |zos| + zos.put_next_entry('data/seed.txt') + zos.write 'seed zip payload for crc check' + end + path + end + + # STORED (uncompressed) entry so the byte flip below corrupts the payload without + # breaking the zip structure - the archive still opens and extracts, only a CRC + # comparison can catch the corruption + def build_crc_corrupt_zip(path) + Zip::OutputStream.open(path) do |zos| + zos.put_next_entry('data/seed.txt', nil, nil, Zip::Entry::STORED) + zos.write 'seed zip payload for crc check' + end + content = File.binread(path) + File.binwrite(path, content.sub('seed zip payload', 'SEED ZIP PAYLOAD')) + path + end + + # Cut the archive in half: the 'PK' magic bytes survive (so content-type checks pass) + # but the central directory is gone, which is what a partial/interrupted upload looks like + def build_truncated_zip(path) + build_valid_zip(path) + content = File.binread(path) + File.binwrite(path, content[0, content.length / 2]) + path + end + + def attach_truncated_seed_zip(analysis) + truncated = build_truncated_zip(File.join(@tmp_dir, 'truncated_seed.zip')) + analysis.seed_zip = File.new(truncated) + analysis.save! + analysis + end + + describe '.seed_zip_error' do + it 'returns nil for a valid zip' do + # Validates: upload validation must not reject healthy seed zips + path = build_valid_zip(File.join(@tmp_dir, 'valid.zip')) + expect(Analysis.seed_zip_error(path)).to be_nil + end + + it 'returns nil for the reference seed zip fixture' do + # Validates: the real fixture used across the suite passes validation + expect(Analysis.seed_zip_error("#{Rails.root}/spec/files/batch_datapoints/example_csv.zip")).to be_nil + end + + it 'reports a file that is not a zip archive' do + # Regression: issue #841 - garbage uploads were accepted and only failed in InitializeAnalysis + path = File.join(@tmp_dir, 'garbage.zip') + File.binwrite(path, 'this is not a zip archive at all') + expect(Analysis.seed_zip_error(path)).to match(/not a valid ZIP archive/) + end + + it 'reports a truncated zip archive' do + # Regression: issue #841 - partially uploaded seed zips were accepted and only failed in InitializeAnalysis + path = build_truncated_zip(File.join(@tmp_dir, 'truncated.zip')) + expect(Analysis.seed_zip_error(path)).to match(/not a valid ZIP archive/) + end + + it 'reports an entry whose payload does not match its CRC' do + # Validates: bit-rot that keeps the zip structure intact is still caught (rubyzip + # 2.x does not verify CRCs on extract, so this is the only line of defense) + path = build_crc_corrupt_zip(File.join(@tmp_dir, 'crc_corrupt.zip')) + expect(Analysis.seed_zip_error(path)).to match(/entry 'data\/seed\.txt' is corrupt \(CRC mismatch\)/) + end + + it 'rejects an archive that inflates past the size cap' do + # Validates: validation must not inflate unbounded data in the request path - a + # zip bomb would otherwise pin a web worker (PR #844 review) + stub_const('Analysis::SEED_ZIP_MAX_INFLATED_BYTES', 10) + path = build_valid_zip(File.join(@tmp_dir, 'inflates_past_cap.zip')) + expect(Analysis.seed_zip_error(path)).to eq 'seed zip inflates to more than 10 bytes' + end + + it 'rejects an archive with more file entries than the cap' do + # Validates: entry-count cap bounds validation work in the request path (PR #844 review) + stub_const('Analysis::SEED_ZIP_MAX_ENTRIES', 1) + path = File.join(@tmp_dir, 'too_many_entries.zip') + Zip::OutputStream.open(path) do |zos| + zos.put_next_entry('a.txt') + zos.write 'a' + zos.put_next_entry('b.txt') + zos.write 'b' + end + expect(Analysis.seed_zip_error(path)).to eq 'seed zip contains more than 1 file entries' + end + end + + describe '#run_initialization with a corrupt seed zip' do + it 'fails on the first attempt without retrying and cleans up partial extraction' do + # Regression: issue #841 - corrupt zips were retried 3 times ("Extraction of the + # seed.zip file failed 3 times with error zlib error while inflating") even though + # the failure is deterministic + attach_truncated_seed_zip(@analysis) + FileUtils.mkdir_p(@analysis.shared_directory_path) + File.write(File.join(@analysis.shared_directory_path, 'partial_file.txt'), 'stale partial extraction') + + expect(@analysis).to receive(:extract_archive).once.and_call_original + expect { @analysis.run_initialization }.to raise_error(/corrupt and cannot be extracted/) + expect(Dir.exist?(@analysis.shared_directory_path)).to be(false), 'partial extraction dir must be removed so a re-run does not skip-and-reuse stale files' + end + + it 'still retries transient extraction errors up to 3 attempts' do + # Validates: fail-fast on corrupt zips must not remove the retry that papers over + # transient IO failures (e.g. NFS hiccups on the osdata volume) + attempts = 0 + allow(@analysis).to receive(:extract_archive) do + attempts += 1 + raise Errno::EIO, 'transient io failure' if attempts < 3 + end + + expect { @analysis.run_initialization }.not_to raise_error + expect(attempts).to eq(3), 'transient errors should be retried until the 3-attempt cap' + end + end + + describe '#fail_job!' do + it 'marks the newest job failed so the analysis reaches a terminal state' do + # Regression: issue #841 - analyses with failed initialization sat in 'queued' + # forever with no API-visible failure state + job = Job.new_job(@analysis.id, 'batch_run', 0, {}) + + @analysis.fail_job!('seed zip is corrupt') + + expect(job.reload.status).to eq 'failed' + expect(job.status_message).to eq 'seed zip is corrupt' + expect(@analysis.reload.status_message).to eq 'seed zip is corrupt' + expect(@analysis.status).to eq 'failed' + end + end + + describe 'ResqueJobs::InitializeAnalysis.perform with a corrupt seed zip' do + it 'marks the analysis failed and re-raises so Resque records the failure' do + # Regression: issue #841 - InitializeAnalysis had no error handling; failures left + # the analysis stuck and RunAnalysis was silently never enqueued + attach_truncated_seed_zip(@analysis) + job = Job.new_job(@analysis.id, 'batch_run', 0, {}) + + expect do + ResqueJobs::InitializeAnalysis.perform('batch_run', @analysis.id, job.id) + end.to raise_error(/corrupt and cannot be extracted/) + + expect(job.reload.status).to eq 'failed' + expect(job.status_message).to match(/Analysis initialization failed: Seed zip for analysis #{@analysis.id} is corrupt/) + expect(@analysis.reload.status).to eq 'failed' + end + end +end diff --git a/server/spec/requests/analyses_upload_spec.rb b/server/spec/requests/analyses_upload_spec.rb new file mode 100644 index 000000000..59055a0e3 --- /dev/null +++ b/server/spec/requests/analyses_upload_spec.rb @@ -0,0 +1,75 @@ +# ******************************************************************************* +# OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC. +# See also https://openstudio.net/license +# ******************************************************************************* + +require 'rails_helper' +require 'tmpdir' + +# Regression specs for https://github.com/NatLabRockies/OpenStudio-server/issues/841 - +# corrupt seed.zip uploads must be rejected with a 422 and a descriptive error instead +# of being accepted and later stranding the analysis in a failed InitializeAnalysis job. +RSpec.describe 'Analyses seed zip upload', type: :request do + before :all do + Project.destroy_all + FactoryBot.create(:project_with_analyses, analyses_count: 1) + + @project = Project.first + @analysis = @project.analyses.first + @tmp_dir = Dir.mktmpdir('analyses-upload-spec') + end + + after :all do + # Destroy factory data so paperclip deletes the uploaded seed zips and prunes the + # emptied assets/analyses directory. In the docker CI job this spec runs as root + # inside the web container while the live app runs as an unprivileged user - a + # leftover root-owned assets/analyses dir makes every later upload fail with + # EACCES, and leftover projects break docker_stack_test_apis_spec assertions. + Project.destroy_all + FileUtils.rm_rf(@tmp_dir) + end + + describe 'POST /analyses/:id/upload' do + it 'rejects a file that is not a zip archive with 422 and a descriptive error' do + # Regression: issue #841 - corrupt uploads were accepted and only failed later in + # ResqueJobs::InitializeAnalysis, permanently stranding the analysis + corrupt_path = File.join(@tmp_dir, 'corrupt_seed.zip') + File.binwrite(corrupt_path, 'this is not a zip archive at all') + + post "/analyses/#{@analysis.id}/upload.json", + params: { file: Rack::Test::UploadedFile.new(corrupt_path, 'application/zip') } + + expect(response).to have_http_status(422) + expect(json['error_message']).to match(/not a valid ZIP archive/) + end + + it 'rejects a truncated zip archive with 422 and a descriptive error' do + # Regression: issue #841 - partial/interrupted uploads keep the 'PK' magic bytes, + # so a content-type check alone does not catch them + valid_bytes = File.binread("#{Rails.root}/spec/files/batch_datapoints/example_csv.zip") + truncated_path = File.join(@tmp_dir, 'truncated_seed.zip') + File.binwrite(truncated_path, valid_bytes[0, valid_bytes.length / 2]) + + post "/analyses/#{@analysis.id}/upload.json", + params: { file: Rack::Test::UploadedFile.new(truncated_path, 'application/zip') } + + expect(response).to have_http_status(422) + expect(json['error_message']).to match(/not a valid ZIP archive/) + end + + it 'accepts a valid seed zip' do + # Validates: the new upload validation must not reject healthy seed zips + # NOTE: on Windows dev machines this example fails because rack-test 2.2.0 builds + # the multipart body with a text-mode read that truncates at the first 0x1A byte; + # it passes on Linux (CI). Analysis.seed_zip_error accepting this exact fixture is + # also covered directly in spec/models/analysis_init_spec.rb. + post "/analyses/#{@analysis.id}/upload.json", + params: { file: Rack::Test::UploadedFile.new( + "#{Rails.root}/spec/files/batch_datapoints/example_csv.zip", 'application/zip' + ) } + + expect(response).to have_http_status(:created) + expect(@analysis.reload.seed_zip.original_filename).to eq 'example_csv.zip' + end + end +end