When extracting attachments for an incoming message and getting the body of the main part in order to look for uuencoded text, make sure that we're getting that main part from the reparsed attachments, and not getting an obsolete attachment. Fixes #958.

2 files changed, 59 insertions, 7 deletions

diff --git a/app/models/incoming_message.rb b/app/models/incoming_message.rb
index 252f81bb7..f959a8799 100644
--- a/app/models/incoming_message.rb
+++ b/app/models/incoming_message.rb
@@ -568,9 +568,11 @@ class IncomingMessage < ActiveRecord::Base
         text
     end
 
-    # Returns part which contains main body text, or nil if there isn't one
-    def get_main_body_text_part
-        leaves = self.foi_attachments
+    # Returns part which contains main body text, or nil if there isn't one,
+    # from a set of foi_attachments. If the leaves parameter is empty or not
+    # supplied, uses its own foi_attachments.
+    def get_main_body_text_part(leaves=[])
+        leaves = self.foi_attachments if leaves.empty?
 
         # Find first part which is text/plain or text/html
         # (We have to include HTML, as increasingly there are mail clients that
@@ -604,6 +606,7 @@ class IncomingMessage < ActiveRecord::Base
         # nil in this case)
         return p
     end
+
     # Returns attachments that are uuencoded in main body part
     def _uudecode_and_save_attachments(text)
         # Find any uudecoded things buried in it, yeuchly
@@ -657,12 +660,16 @@ class IncomingMessage < ActiveRecord::Base
             attachment = self.foi_attachments.find_or_create_by_hexdigest(attrs[:hexdigest])
             attachment.update_attributes(attrs)
             attachment.save!
-            attachments << attachment.id
+            attachments << attachment
         end
+
         # Reload to refresh newly created foi_attachments
         self.reload
 
-        main_part = get_main_body_text_part
+        # get the main body part from the set of attachments we just created,
+        # not from the self.foi_attachments association - some of the total set of
+        # self.foi_attachments may now be obsolete
+        main_part = get_main_body_text_part(attachments)
         # we don't use get_main_body_text_internal, as we want to avoid charset
         # conversions, since /usr/bin/uudecode needs to deal with those.
         # e.g. for https://secure.mysociety.org/admin/foi/request/show_raw_email/24550
@@ -673,12 +680,14 @@ class IncomingMessage < ActiveRecord::Base
                 c += 1
                 uudecode_attachment.url_part_number = c
                 uudecode_attachment.save!
-                attachments << uudecode_attachment.id
+                attachments << uudecode_attachment
             end
         end
 
+        attachment_ids = attachments.map{ |attachment| attachment.id }
         # now get rid of any attachments we no longer have
-        FoiAttachment.destroy_all("id NOT IN (#{attachments.join(',')}) AND incoming_message_id = #{self.id}")
+        FoiAttachment.destroy_all(["id NOT IN (?) AND incoming_message_id = ?",
+                                    attachment_ids, self.id])
    end
 
     # Returns body text as HTML with quotes flattened, and emails removed.
diff --git a/spec/models/incoming_message_spec.rb b/spec/models/incoming_message_spec.rb
index 1d86c26ad..03152f5ff 100644
--- a/spec/models/incoming_message_spec.rb
+++ b/spec/models/incoming_message_spec.rb
@@ -542,3 +542,46 @@ describe IncomingMessage, "when TNEF attachments are attached to messages" do
     end
 end
 
+describe IncomingMessage, "when extracting attachments" do
+
+    it 'handles the case where reparsing changes the body of the main part
+        and the cached attachment has been deleted' do
+        # original set of attachment attributes
+        attachment_attributes = { :url_part_number => 1,
+                                  :within_rfc822_subject => nil,
+                                  :content_type => "text/plain",
+                                  :charset => nil,
+                                  :body => "No way!\n",
+                                  :hexdigest => "0c8b1b0f5cb9c94ed15a180e73b5c7d1",
+                                  :filename => nil }
+
+        # Make a small change in the body returned for the attachment
+        new_attachment_attributes = attachment_attributes.merge(:body => "No way!",
+                                                                :hexdigest => "74d2c0a41e074f9cebe49324d5b47414")
+
+
+        # Simulate parsing with the original attachments
+        MailHandler.stub!(:get_attachment_attributes).and_return([attachment_attributes])
+        incoming_message = incoming_messages(:useless_incoming_message)
+
+        # Extract the attachments
+        incoming_message.extract_attachments!
+
+        # delete the cached file for the main body part
+        main = incoming_message.get_main_body_text_part
+        main.delete_cached_file!
+
+        # Simulate reparsing with the slightly changed body
+        MailHandler.stub!(:get_attachment_attributes).and_return([new_attachment_attributes])
+
+        # Re-extract the attachments
+        incoming_message.extract_attachments!
+
+        attachments = incoming_message.foi_attachments
+        attachments.size.should == 1
+        attachments.first.hexdigest.should == "74d2c0a41e074f9cebe49324d5b47414"
+        attachments.first.body.should == 'No way!'
+    end
+
+end
+