issue #351 redux

Corrected diagnosis, test & fix for issue #351. Fixes #351.

2 files changed, 45 insertions, 5 deletions

diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index ccd9636d1..2295d6718 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -716,7 +716,10 @@ class RequestController < ApplicationController
         @incoming_message.parse_raw_email!
         @info_request = @incoming_message.info_request
         if @incoming_message.info_request_id != params[:id].to_i
-            message = "Incoming message %d does not belong to request %d" % [@incoming_message.info_request_id, params[:id]]
+            # Note that params[:id] might not be an integer, though
+            # if we’ve got this far then it must begin with an integer
+            # and that integer must be the id number of an actual request.
+            message = "Incoming message %d does not belong to request '%s'" % [@incoming_message.info_request_id, params[:id]]
             raise ActiveRecord::RecordNotFound.new(message)
         end
         @part_number = params[:part].to_i
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 6161bf3ac..25dce3f22 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -235,7 +235,7 @@ describe RequestController, "when showing one request" do
             response.should have_text(/tënde/u)
         end
 
-        it "should generate valid HTML verson of plain text attachments " do
+        it "should generate valid HTML verson of plain text attachments" do
             ir = info_requests(:fancy_dog_request) 
             receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
             ir.reload
@@ -244,16 +244,53 @@ describe RequestController, "when showing one request" do
             response.should have_text(/Second hello/)
         end
 
-        it "should return 404 for ugly URLs contain a request id that isn't an integer " do
+        # This is a regression test for a bug where URLs of this form were causing 500 errors
+        # instead of 404s.
+        #
+        # (Note that in fact only the integer-prefix of the URL part is used, so there are
+        # *some* “ugly URLs containing a request id that isn't an integer” that actually return
+        # a 200 response. The point is that IDs of this sort were triggering an error in the
+        # error-handling path, causing the wrong sort of error response to be returned in the
+        # case where the integer prefix referred to the wrong request.)
+        #
+        # https://github.com/sebbacon/alaveteli/issues/351
+        it "should return 404 for ugly URLs containing a request id that isn't an integer" do
             ir = info_requests(:fancy_dog_request) 
             receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
             ir.reload
             ugly_id = "55195"
             lambda {
-                get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => ugly_id, :part => 2, :file_name => ['hello.txt.html'], :skip_cache => 1
+                get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ugly_id, :part => 2, :file_name => ['hello.txt.html'], :skip_cache => 1
+            }.should raise_error(ActiveRecord::RecordNotFound)
+
+            lambda {
+                get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => ugly_id, :part => 2, :file_name => ['hello.txt'], :skip_cache => 1
+            }.should raise_error(ActiveRecord::RecordNotFound)
+        end
+        it "should return 404 when incoming message and request ids don't match" do
+            ir = info_requests(:fancy_dog_request)
+            wrong_id = info_requests(:naughty_chicken_request).id
+            receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
+            ir.reload
+            lambda {
+                get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => wrong_id, :part => 2, :file_name => ['hello.txt.html'], :skip_cache => 1
+            }.should raise_error(ActiveRecord::RecordNotFound)
+        end
+        it "should return 404 for ugly URLs contain a request id that isn't an integer, even if the integer prefix refers to an actual request" do
+            ir = info_requests(:fancy_dog_request)
+            receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
+            ir.reload
+            ugly_id = "%d95" % [info_requests(:naughty_chicken_request).id]
+            
+            lambda {
+                get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ugly_id, :part => 2, :file_name => ['hello.txt.html'], :skip_cache => 1
+            }.should raise_error(ActiveRecord::RecordNotFound)
+
+            lambda {
+                get :get_attachment_as_html, :incoming_message_id => ir.incoming_messages[1].id, :id => ugly_id, :part => 2, :file_name => ['hello.txt'], :skip_cache => 1
             }.should raise_error(ActiveRecord::RecordNotFound)
         end
-        it "should return 404 when incoming message and request ids don't match " do
+        it "should return 404 when incoming message and request ids don't match" do
             ir = info_requests(:fancy_dog_request)
             wrong_id = info_requests(:naughty_chicken_request).id
             receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)