Check that display_filename matches URL part number or fallback

If the display_filename of the attachment found from the
URL part number doesn't match the passed in display_filename
then the email may have been reparsed, causing a reordering.
In that case, look to see if there is another attachment that
uniquely matches that filename, and, if so, return that other
attachment.  If no matching uniquely matching filename is
found, redirect to the incoming message, rather than
returning a 404.

3 files changed, 64 insertions, 20 deletions

diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 5b64fe9ae..96f62319e 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -736,6 +736,7 @@ class RequestController < ApplicationController
 
     def get_attachment
         get_attachment_internal(false)
+        return unless @attachment
 
         # Prevent spam to magic request address. Note that the binary
         # subsitution method used depends on the content type
@@ -755,6 +756,7 @@ class RequestController < ApplicationController
             raise ActiveRecord::RecordNotFound.new("Attachment HTML not found.")
         end
         get_attachment_internal(true)
+        return unless @attachment
 
         # images made during conversion (e.g. images in PDF files) are put in the cache directory, so
         # the same cache code in cache_attachments above will display them.
@@ -801,8 +803,11 @@ class RequestController < ApplicationController
 
         # check permissions
         raise "internal error, pre-auth filter should have caught this" if !@info_request.user_can_view?(authenticated_user)
-        @attachment = IncomingMessage.get_attachment_by_url_part_number(@incoming_message.get_attachments_for_display, @part_number)
-        raise ActiveRecord::RecordNotFound.new("attachment not found part number " + @part_number.to_s + " incoming_message " + @incoming_message.id.to_s) if @attachment.nil?
+        @attachment = IncomingMessage.get_attachment_by_url_part_number_and_filename(@incoming_message.get_attachments_for_display, @part_number, @original_filename)
+        # If we can't find the right attachment, redirect to the incoming message:
+        unless @attachment
+            return redirect_to incoming_message_url(@incoming_message), :status => 303
+        end
 
         # check filename in URL matches that in database (use a censor rule if you want to change a filename)
         raise ActiveRecord::RecordNotFound.new("please use same filename as original file has, display: '" + @attachment.display_filename + "' old_display: '" + @attachment.old_display_filename + "' original: '" + @original_filename + "'") if @attachment.display_filename != @original_filename && @attachment.old_display_filename != @original_filename
diff --git a/app/models/incoming_message.rb b/app/models/incoming_message.rb
index afe3c425f..252f81bb7 100644
--- a/app/models/incoming_message.rb
+++ b/app/models/incoming_message.rb
@@ -171,10 +171,29 @@ class IncomingMessage < ActiveRecord::Base
         super
     end
 
-    # And look up by URL part number to get an attachment
+    # And look up by URL part number and display filename to get an attachment
     # XXX relies on extract_attachments calling MailHandler.ensure_parts_counted
-    def self.get_attachment_by_url_part_number(attachments, found_url_part_number)
-        attachments.detect { |a| a.url_part_number == found_url_part_number }
+    # The filename here is passed from the URL parameter, so it's the
+    # display_filename rather than the real filename.
+    def self.get_attachment_by_url_part_number_and_filename(attachments, found_url_part_number, display_filename)
+        attachment_by_part_number = attachments.detect { |a| a.url_part_number == found_url_part_number }
+        if attachment_by_part_number && attachment_by_part_number.display_filename == display_filename
+            # Then the filename matches, which is fine:
+            attachment_by_part_number
+        else
+            # Otherwise if the URL part number and filename don't
+            # match - this is probably due to a reparsing of the
+            # email.  In that case, try to find a unique matching
+            # filename from any attachment.
+            attachments_by_filename = attachments.select { |a|
+                a.display_filename == display_filename
+            }
+            if attachments_by_filename.length == 1
+                attachments_by_filename[0]
+            else
+                nil
+            end
+        end
     end
 
     # Converts email addresses we know about into textual descriptions of them
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 657837c72..67fed572a 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -578,23 +578,21 @@ describe RequestController, "when showing one request" do
             response.should contain "Walberswick Parish Council"
         end
 
-        it "should not cause a reparsing of the raw email, even when the result would be a 404" do
+        it "should not cause a reparsing of the raw email, even when the attachment can't be found" do
             ir = info_requests(:fancy_dog_request)
             receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
             ir.reload
-            attachment = IncomingMessage.get_attachment_by_url_part_number(ir.incoming_messages[1].get_attachments_for_display, 2)
+            attachment = IncomingMessage.get_attachment_by_url_part_number_and_filename(ir.incoming_messages[1].get_attachments_for_display, 2, 'hello.txt')
             attachment.body.should contain "Second hello"
 
             # change the raw_email associated with the message; this only be reparsed when explicitly asked for
             ir.incoming_messages[1].raw_email.data = ir.incoming_messages[1].raw_email.data.sub("Second", "Third")
-            # asking for an attachment by the wrong filename results
-            # in a 404 for browsing users.  This shouldn't cause a
-            # re-parse...
-            lambda {
-                get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2, :file_name => 'hello.txt.baz.html', :skip_cache => 1
-            }.should raise_error(ActiveRecord::RecordNotFound)
+            # asking for an attachment by the wrong filename should result in redirecting
+            # back to the incoming message, but shouldn't cause a reparse:
+            get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2, :file_name => 'hello.txt.baz.html', :skip_cache => 1
+            response.status.should == 303
 
-            attachment = IncomingMessage.get_attachment_by_url_part_number(ir.incoming_messages[1].get_attachments_for_display, 2)
+            attachment = IncomingMessage.get_attachment_by_url_part_number_and_filename(ir.incoming_messages[1].get_attachments_for_display, 2, 'hello.txt')
             attachment.body.should contain "Second hello"
 
             # ...nor should asking for it by its correct filename...
@@ -605,12 +603,36 @@ describe RequestController, "when showing one request" do
             force = true
             ir.incoming_messages[1].parse_raw_email!(force)
             ir.reload
-            attachment = IncomingMessage.get_attachment_by_url_part_number(ir.incoming_messages[1].get_attachments_for_display, 2)
+            attachment = IncomingMessage.get_attachment_by_url_part_number_and_filename(ir.incoming_messages[1].get_attachments_for_display, 2, 'hello.txt')
             attachment.body.should contain "Third hello"
             get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2, :file_name => 'hello.txt.html', :skip_cache => 1
             response.should contain "Third hello"
         end
 
+        it "should redirect to the incoming message if there's a wrong part number and an ambiguous filename" do
+            ir = info_requests(:fancy_dog_request)
+            receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
+            ir.reload
+
+            im = ir.incoming_messages[1]
+
+            attachment = IncomingMessage.get_attachment_by_url_part_number_and_filename(im.get_attachments_for_display, 5, 'hello world.txt')
+            attachment.should be_nil
+
+            get :get_attachment_as_html, :incoming_message_id => im.id, :id => ir.id, :part => 5, :file_name => 'hello world.txt', :skip_cache => 1
+            response.status.should == 303
+            new_location = response.header['Location']
+            new_location.should match(/request\/#{ir.url_title}#incoming-#{im.id}/)
+        end
+
+        it "should find a uniquely named filename even if the URL part number was wrong" do
+            ir = info_requests(:fancy_dog_request)
+            receive_incoming_mail('incoming-request-pdf-attachment.email', ir.incoming_email)
+            ir.reload
+            get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 5, :file_name => 'fs 50379341.pdf', :skip_cache => 1
+            response.content_type.should == "application/pdf"
+        end
+
         it "should treat attachments with unknown extensions as binary" do
             ir = info_requests(:fancy_dog_request)
             receive_incoming_mail('incoming-request-attachment-unknown-extension.email', ir.incoming_email)
@@ -625,10 +647,8 @@ describe RequestController, "when showing one request" do
             ir = info_requests(:fancy_dog_request)
             receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
 
-            lambda {
-                get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2,
-                    :file_name => 'http://trying.to.hack'
-            }.should raise_error(ActiveRecord::RecordNotFound)
+            get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2, :file_name => 'http://trying.to.hack'
+            response.status.should == 303
         end
 
         it "should censor attachments downloaded as binary" do
@@ -2394,7 +2414,7 @@ describe RequestController, "when caching fragments" do
         attachment = mock(FoiAttachment, :display_filename => long_name,
                                          :body_as_html => ['some text', 'wrapper'])
         IncomingMessage.stub!(:find).with("44").and_return(incoming_message)
-        IncomingMessage.stub!(:get_attachment_by_url_part_number).and_return(attachment)
+        IncomingMessage.stub!(:get_attachment_by_url_part_number_and_filename).and_return(attachment)
         InfoRequest.stub!(:find).with("132").and_return(info_request)
         params = { :file_name => long_name,
                    :controller => "request",