Skip to content

Commit

Permalink
Update parsing of incoming emails
Browse files Browse the repository at this point in the history
Ensure we don't ever replace attachments which have been hidden using
prominence with newly parsed attachments.

This could in theory happen if the hexdigest which is based on the
attachment body changes. There have been occasions this has happened to
due to changes in the underlying mail gem and other parsing changes.

See: #7875
  • Loading branch information
gbp committed Sep 28, 2023
1 parent 60cde68 commit 2e470e2
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 2 deletions.
16 changes: 14 additions & 2 deletions app/models/incoming_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class IncomingMessage < ApplicationRecord
include CacheAttributesFromRawEmail
include Taggable

UnableToExtractAttachments = Class.new(StandardError)

MAX_ATTACHMENT_TEXT_CLIPPED = 1_000_000 # 1Mb ish

belongs_to :info_request,
Expand Down Expand Up @@ -470,8 +472,18 @@ def extract_attachments
attachments += _uudecode_attachments(main_part.body, c)
end

# Purge old attachments that have been rebuilt with a new hexdigest
(foi_attachments - attachments).each(&:mark_for_destruction)
# Purge old public attachments that will be rebuilt with a new hexdigest
old_attachments = (foi_attachments - attachments)
hidden_old_attachments = old_attachments.reject { _1.is_public? }

Check warning on line 477 in app/models/incoming_message.rb

View workflow job for this annotation

GitHub Actions / build

[rubocop] reported by reviewdog 🐶 [Correctable] Style/SymbolProc: Pass &:is_public? as an argument to reject instead of a block. Raw Output: app/models/incoming_message.rb:477:53: C: [Correctable] Style/SymbolProc: Pass &:is_public? as an argument to reject instead of a block. hidden_old_attachments = old_attachments.reject { _1.is_public? } ^^^^^^^^^^^^^^^^^

if hidden_old_attachments.any?
# if there are hidden attachments error as we don't want to re-build and
# lose the prominence as this will make them public
raise UnableToExtractAttachments, "due to prominence of attachments " \
"(ID=#{hidden_old_attachments.map(&:id).join(', ')})"
else
old_attachments.each(&:mark_for_destruction)
end
end

# Returns body text as HTML with quotes flattened, and emails removed.
Expand Down
17 changes: 17 additions & 0 deletions spec/models/incoming_message_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,23 @@ def populate_raw_email(fixture)
expect(im._get_attachment_text_internal.valid_encoding?).to be true
end

it 'does not raise error if existing hidden attachment will be retained' do
incoming_message = FactoryBot.create(:incoming_message)
foi_attachment = incoming_message.foi_attachments.first
foi_attachment.update(prominence: 'hidden')

expect { incoming_message.extract_attachments! }.to_not raise_error
end

it 'raises error if existing hidden attachment will be deleted' do
incoming_message = FactoryBot.create(:incoming_message)
foi_attachment = incoming_message.foi_attachments.first
foi_attachment.update(prominence: 'hidden', hexdigest: '123')

expect { incoming_message.extract_attachments! }.to raise_error(
IncomingMessage::UnableToExtractAttachments
)
end
end

RSpec.describe IncomingMessage, 'when getting the body of a message for html display' do
Expand Down

0 comments on commit 2e470e2

Please sign in to comment.