From a92fa7d658ff64ac0b8e12bb6462f64b2a75c4a6 Mon Sep 17 00:00:00 2001 From: Paul Sandhu Date: Wed, 8 Jul 2026 10:15:07 -0700 Subject: [PATCH 1/2] Guard against a missing area page in result attachment generation The issue here is result-attachment generation 500s with `undefined method '<' for nil` whenever a template field area has no `page`. It's a permitted-but-not-required area param, so an imported or API-built area can omit it, and `pdf.pages[nil]` walks straight into HexaPDF's `nil < 0` at page_tree_node.rb:96. The existing `next if page.nil?` guard runs one line too late: the exception fires inside the `pages[...]` call itself. This adds a nil-guard before the index and skips the area, mirroring the surrounding `next if pdf.nil?` style. It logs a warning on the way out so a genuinely malformed template doesn't silently drop a field from a signed document without a trace. Adds a spec for the missing / nil / out-of-range page cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../generate_result_attachments.rb | 10 +++ .../generate_result_attachments_spec.rb | 81 +++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 spec/lib/submissions/generate_result_attachments_spec.rb diff --git a/lib/submissions/generate_result_attachments.rb b/lib/submissions/generate_result_attachments.rb index 5a60c69d4b..18d6ae5c84 100644 --- a/lib/submissions/generate_result_attachments.rb +++ b/lib/submissions/generate_result_attachments.rb @@ -194,6 +194,16 @@ def fill_submitter_fields(submitter, account, pdfs_index, with_signature_id:, is next if pdf.nil? + # HexaPDF's pages[nil] does `nil < 0` and raises. `page` is a + # permitted-but-not-required area param, so a malformed/imported area + # can omit it. Skip it, but log so silently-incomplete signed docs stay visible. + if area['page'].nil? + Rails.logger.warn( + "Skipping field area with no page (submitter=#{submitter.id}, attachment=#{area['attachment_uuid']})" + ) + next + end + page = pdf.pages[area['page']] next if page.nil? diff --git a/spec/lib/submissions/generate_result_attachments_spec.rb b/spec/lib/submissions/generate_result_attachments_spec.rb new file mode 100644 index 0000000000..544f2fd94b --- /dev/null +++ b/spec/lib/submissions/generate_result_attachments_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Submissions::GenerateResultAttachments do + let(:account) { create(:account) } + let(:user) { create(:user, account:) } + let(:template) { create(:template, account:, author: user) } + let(:submission) { create(:submission, template:, created_by_user: user) } + let(:submitter) { create(:submitter, submission:, uuid: SecureRandom.uuid) } + let(:attachment_uuid) { SecureRandom.uuid } + + # A single-page in-memory PDF keyed by the area's attachment_uuid, so the + # `pdf.nil?` guard passes and we reach the `pdf.pages[area['page']]` index. + let(:pdfs_index) do + doc = HexaPDF::Document.new + doc.pages.add + { attachment_uuid => doc } + end + + # Point the submitter's submission at a single text field whose one area is + # `area`, so `fill_submitter_fields` reaches the page lookup for that area. + def set_field_area(area) + submitter.submission.update!( + template_fields: [ + { 'uuid' => SecureRandom.uuid, 'submitter_uuid' => submitter.uuid, + 'type' => 'text', 'areas' => [area] } + ] + ) + end + + def fill + described_class.send( + :fill_submitter_fields, submitter, account, pdfs_index, + with_signature_id: false, is_flatten: false, with_headings: false + ) + end + + before { allow(Rails.logger).to receive(:warn) } + + describe '.fill_submitter_fields with a missing area page' do + context 'when the area omits the page key' do + before { set_field_area('attachment_uuid' => attachment_uuid) } + + it 'skips the area instead of raising (regression: HexaPDF pages[nil])' do + expect { fill }.not_to raise_error + end + + it 'logs that the area was skipped' do + fill + expect(Rails.logger).to have_received(:warn).with(/Skipping field area with no page/) + end + end + + context 'when the area page is explicitly nil' do + before { set_field_area('attachment_uuid' => attachment_uuid, 'page' => nil) } + + it 'skips the area instead of raising' do + expect { fill }.not_to raise_error + end + + it 'logs that the area was skipped' do + fill + expect(Rails.logger).to have_received(:warn).with(/Skipping field area with no page/) + end + end + + context 'when the area page is out of range' do + before { set_field_area('attachment_uuid' => attachment_uuid, 'page' => 5) } + + it 'does not raise (existing next-if-page-nil handling still applies)' do + expect { fill }.not_to raise_error + end + + it 'does not log a skipped-no-page warning (the guard only catches nil)' do + fill + expect(Rails.logger).not_to have_received(:warn).with(/Skipping field area with no page/) + end + end + end +end From 0bda0e26393a9e1932abe63823effa589e5c00f5 Mon Sep 17 00:00:00 2001 From: Paul Sandhu Date: Wed, 8 Jul 2026 10:19:57 -0700 Subject: [PATCH 2/2] Rename the spec helper to satisfy Naming/AccessorMethodName rubocop flags the set_ prefix as a writer-method name; assign_field_area reads fine and isn't mistaken for an accessor. Co-Authored-By: Claude Opus 4.8 (1M context) --- spec/lib/submissions/generate_result_attachments_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/lib/submissions/generate_result_attachments_spec.rb b/spec/lib/submissions/generate_result_attachments_spec.rb index 544f2fd94b..8b6019e053 100644 --- a/spec/lib/submissions/generate_result_attachments_spec.rb +++ b/spec/lib/submissions/generate_result_attachments_spec.rb @@ -20,7 +20,7 @@ # Point the submitter's submission at a single text field whose one area is # `area`, so `fill_submitter_fields` reaches the page lookup for that area. - def set_field_area(area) + def assign_field_area(area) submitter.submission.update!( template_fields: [ { 'uuid' => SecureRandom.uuid, 'submitter_uuid' => submitter.uuid, @@ -40,7 +40,7 @@ def fill describe '.fill_submitter_fields with a missing area page' do context 'when the area omits the page key' do - before { set_field_area('attachment_uuid' => attachment_uuid) } + before { assign_field_area('attachment_uuid' => attachment_uuid) } it 'skips the area instead of raising (regression: HexaPDF pages[nil])' do expect { fill }.not_to raise_error @@ -53,7 +53,7 @@ def fill end context 'when the area page is explicitly nil' do - before { set_field_area('attachment_uuid' => attachment_uuid, 'page' => nil) } + before { assign_field_area('attachment_uuid' => attachment_uuid, 'page' => nil) } it 'skips the area instead of raising' do expect { fill }.not_to raise_error @@ -66,7 +66,7 @@ def fill end context 'when the area page is out of range' do - before { set_field_area('attachment_uuid' => attachment_uuid, 'page' => 5) } + before { assign_field_area('attachment_uuid' => attachment_uuid, 'page' => 5) } it 'does not raise (existing next-if-page-nil handling still applies)' do expect { fill }.not_to raise_error