diff options
| -rw-r--r-- | app/controllers/request_controller.rb | 9 | ||||
| -rw-r--r-- | app/models/incoming_message.rb | 25 | ||||
| -rw-r--r-- | spec/controllers/request_controller_spec.rb | 50 |
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", |