From 4d1036dc9d813ab861649b73fe7a83b3b73d1340 Mon Sep 17 00:00:00 2001 From: brianlball Date: Fri, 3 Jul 2026 08:44:20 -0400 Subject: [PATCH 1/4] Fix #841: fail fast on corrupt seed.zip, validate at upload, mark analysis failed - Analysis.seed_zip_error: read-only zip validation (structure + per-entry CRC); used by AnalysesController#upload/#create to reject corrupt seed zips with 422 - Analysis#run_initialization: Zip/Zlib errors are deterministic - fail on first attempt (no 3x retry) and remove partial extraction; transient errors still retry - ResqueJobs::InitializeAnalysis.perform: on failure set job status 'failed' + status_message so the analysis reaches a terminal API-visible state, then re-raise so Resque records the failed job - specs: analysis_init_spec.rb (model/job), analyses_upload_spec.rb (422 contract) Co-Authored-By: Claude Fable 5 --- server/app/controllers/analyses_controller.rb | 22 +++ .../jobs/resque_jobs/initialize_analysis.rb | 12 +- server/app/models/analysis.rb | 44 +++++ server/spec/models/analysis_init_spec.rb | 153 ++++++++++++++++++ server/spec/requests/analyses_upload_spec.rb | 69 ++++++++ 5 files changed, 298 insertions(+), 2 deletions(-) create mode 100644 server/spec/models/analysis_init_spec.rb create mode 100644 server/spec/requests/analyses_upload_spec.rb diff --git a/server/app/controllers/analyses_controller.rb b/server/app/controllers/analyses_controller.rb index b29249c76..c1d30b776 100644 --- a/server/app/controllers/analyses_controller.rb +++ b/server/app/controllers/analyses_controller.rb @@ -140,6 +140,19 @@ 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| + format.html { render action: 'new', 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 +404,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..e9f5badab 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 e 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..2e96352fe 100644 --- a/server/app/models/analysis.rb +++ b/server/app/models/analysis.rb @@ -93,6 +93,45 @@ def self.status_states ['na', 'init', 'queued', 'started', 'post-processing', 'completed'] end + # 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. + zf = ::Zip::File.new(file_path) + zf.each do |entry| + next unless entry.file? + + crc = ::Zlib.crc32 + entry.get_input_stream do |io| + while (chunk = io.read(1_048_576)) + 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 +534,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..3457c8f44 --- /dev/null +++ b/server/spec/models/analysis_init_spec.rb @@ -0,0 +1,153 @@ +# ******************************************************************************* +# 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 + 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 + 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..7f334af73 --- /dev/null +++ b/server/spec/requests/analyses_upload_spec.rb @@ -0,0 +1,69 @@ +# ******************************************************************************* +# 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 + 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 From bec9681ca7fd43b7833c21c9363fcebe18e6b971 Mon Sep 17 00:00:00 2001 From: brianlball Date: Mon, 6 Jul 2026 11:01:26 -0400 Subject: [PATCH 2/4] CI: run the issue #841 server specs in the docker (resque) job The linux/macos CI jobs already run the full server spec suite via 'openstudio_meta run_rspec', which reproduces the PAT local deployment (delayed_job, local mongo) and writes rspec output to spec/unit-test/logs/rspec.log rather than the CI console. The docker job has not run server model/request specs since Sept 2020 (49580920) - it runs only the docker_stack feature specs. Issue #841 is specific to the resque code path (InitializeAnalysis only runs under resque), so also run the new #841 spec files in the docker job, where RAILS_ENV=docker, resque/redis, and authenticated mongo are in play. They need only rails+mongo, finish in about a second, and leave the database empty, so they run before the feature specs. Verified in a local replica of the CI docker environment (nrel/openstudio-server:develop image + mongo as 'db' + redis as 'queue'): 12 examples, 0 failures. Co-Authored-By: Claude Fable 5 --- docker/server/run-server-tests.sh | 3 +++ 1 file changed, 3 insertions(+) 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 || $? )) From 3721a72af81210634d51a6247a703d39a0e28a23 Mon Sep 17 00:00:00 2001 From: brianlball Date: Mon, 6 Jul 2026 12:36:44 -0400 Subject: [PATCH 3/4] Clean up spec factory data so the docker CI job's live stack keeps working The docker job runs rspec as root inside the web container while the live app serves as an unprivileged user (nobody). The new #841 specs wrote seed zips through paperclip into /mnt/openstudio/server/assets/analyses, leaving that directory root-owned (755). Every later upload from the live app then failed with Errno::EACCES -> HTTP 500, which broke the docker_stack feature specs (custom_gems, 9/10 algo). Leftover factory projects also broke the empty-projects assertion in docker_stack_test_apis_spec. Fix: after(:all) Project.destroy_all in both spec files - paperclip deletes the attachments and prunes the emptied root-owned directory, so the live app recreates it under its own user. Verified in a fresh local replica of the CI docker environment, in CI order (specs first as root, then app-user traffic): 12 examples 0 failures, assets/analyses pruned, projects list empty, valid upload 201 (dir recreated by nobody), corrupt upload 422 through the live nginx/Passenger stack. Co-Authored-By: Claude Fable 5 --- server/spec/models/analysis_init_spec.rb | 6 ++++++ server/spec/requests/analyses_upload_spec.rb | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/server/spec/models/analysis_init_spec.rb b/server/spec/models/analysis_init_spec.rb index 3457c8f44..e6f7ec3bb 100644 --- a/server/spec/models/analysis_init_spec.rb +++ b/server/spec/models/analysis_init_spec.rb @@ -18,6 +18,12 @@ 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 diff --git a/server/spec/requests/analyses_upload_spec.rb b/server/spec/requests/analyses_upload_spec.rb index 7f334af73..59055a0e3 100644 --- a/server/spec/requests/analyses_upload_spec.rb +++ b/server/spec/requests/analyses_upload_spec.rb @@ -20,6 +20,12 @@ 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 From a8c88553ca30d9d6392f6a93c3df3b1220818dc3 Mon Sep 17 00:00:00 2001 From: brianlball Date: Mon, 6 Jul 2026 14:58:06 -0400 Subject: [PATCH 4/4] Address PR #844 Copilot review: zip-bomb caps, HTML 422 path, bare raise - Analysis.seed_zip_error: cap validation work at SEED_ZIP_MAX_INFLATED_BYTES (10 GB) and SEED_ZIP_MAX_ENTRIES (100k), counted while streaming, so a zip bomb cannot pin a web worker in the request path; 2 new specs via stub_const - AnalysesController#create corrupt-zip path: render plain-text 422 for HTML - 'render action: new' would raise MissingTemplate (no new.html.erb exists) - InitializeAnalysis: bare 'raise' instead of 'raise e' (style only - both preserve the original backtrace when re-raising the same exception object) Verified: 11 model examples locally, 14 total in the CI docker-env replica. Co-Authored-By: Claude Fable 5 --- server/app/controllers/analyses_controller.rb | 4 +++- .../jobs/resque_jobs/initialize_analysis.rb | 2 +- server/app/models/analysis.rb | 14 +++++++++++++ server/spec/models/analysis_init_spec.rb | 21 +++++++++++++++++++ 4 files changed, 39 insertions(+), 2 deletions(-) diff --git a/server/app/controllers/analyses_controller.rb b/server/app/controllers/analyses_controller.rb index c1d30b776..e70cc5f7a 100644 --- a/server/app/controllers/analyses_controller.rb +++ b/server/app/controllers/analyses_controller.rb @@ -146,7 +146,9 @@ def create zip_error = Analysis.seed_zip_error(params[:seed_zip].path) if zip_error respond_to do |format| - format.html { render action: 'new', status: :unprocessable_entity } + # 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 diff --git a/server/app/jobs/resque_jobs/initialize_analysis.rb b/server/app/jobs/resque_jobs/initialize_analysis.rb index e9f5badab..155afd7cf 100644 --- a/server/app/jobs/resque_jobs/initialize_analysis.rb +++ b/server/app/jobs/resque_jobs/initialize_analysis.rb @@ -23,7 +23,7 @@ def self.perform(analysis_type, analysis_id, job_id, options = {}) rescue StandardError => mark_error Rails.logger.error "Could not mark analysis #{analysis_id} as failed: #{mark_error.message}" end - raise e + 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 2e96352fe..b587f6105 100644 --- a/server/app/models/analysis.rb +++ b/server/app/models/analysis.rb @@ -93,6 +93,12 @@ 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 @@ -101,13 +107,21 @@ 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 diff --git a/server/spec/models/analysis_init_spec.rb b/server/spec/models/analysis_init_spec.rb index e6f7ec3bb..268d032fa 100644 --- a/server/spec/models/analysis_init_spec.rb +++ b/server/spec/models/analysis_init_spec.rb @@ -95,6 +95,27 @@ def attach_truncated_seed_zip(analysis) 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