diff options
| -rw-r--r-- | app/controllers/application_controller.rb | 59 | ||||
| -rw-r--r-- | app/controllers/general_controller.rb | 6 | ||||
| -rw-r--r-- | config/application.rb | 5 | ||||
| -rw-r--r-- | spec/integration/errors_spec.rb | 131 |
4 files changed, 134 insertions, 67 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 029b536ec..2615b61f2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -12,6 +12,11 @@ require 'open-uri' class ApplicationController < ActionController::Base class PermissionDenied < StandardError end + class RouteNotFound < StandardError + end + # assign our own handler method for non-local exceptions + rescue_from Exception, :with => :render_exception + # Standard headers, footers and navigation for whole site layout "default" include FastGettext::Translation # make functions like _, n_, N_ etc available) @@ -111,55 +116,35 @@ class ApplicationController < ActionController::Base end end - # Override default error handler, for production sites. - def rescue_action_in_public(exception) - # Looks for before_filters called something like `set_view_paths_{themename}`. These - # are set by the themes. - # Normally, this is called by the theme itself in a - # :before_filter, but when there's an error, this doesn't - # happen. By calling it here, we can ensure error pages are - # still styled according to the theme. - ActionController::Base.before_filters.select{|f| f.to_s =~ /set_view_paths/}.each do |f| - self.send(f) - end - # Make sure expiry time for session is set (before_filters are - # otherwise missed by this override) - session_remember_me + def render_exception(exception) - # Make sure the locale is set correctly too - set_gettext_locale + # In development, or the admin interface, or for a local request, let Rails handle the exception + # with its stack trace templates. Local requests in testing are a special case so that we can + # test this method - there we use consider_all_requests_local to control behaviour. + if Rails.application.config.consider_all_requests_local || local_request? || + (request.local? && !Rails.env.test?) + raise exception + end + @exception_backtrace = exception.backtrace.join("\n") + @exception_class = exception.class.to_s + @exception_message = exception.message case exception - when ActiveRecord::RecordNotFound, ActionController::UnknownAction, ActionController::RoutingError + when ActiveRecord::RecordNotFound, RouteNotFound @status = 404 when PermissionDenied @status = 403 else + message = "\n#{@exception_class} (#{@exception_message}):\n" + backtrace = Rails.backtrace_cleaner.clean(exception.backtrace, :silent) + message << " " << backtrace.join("\n ") + Rails.logger.fatal("#{message}\n\n") + ExceptionNotifier::Notifier.exception_notification(request.env, exception).deliver @status = 500 - notify_about_exception exception end - # Display user appropriate error message - @exception_backtrace = exception.backtrace.join("\n") - @exception_class = exception.class.to_s - @exception_message = exception.message render :template => "general/exception_caught", :status => @status end - # FIXME: This was disabled during the Rails 3 upgrade as this is now handled by Rack - # # For development sites. - # alias original_rescue_action_locally rescue_action_locally - # def rescue_action_locally(exception) - # # Make sure expiry time for session is set (before_filters are - # # otherwise missed by this override) - # session_remember_me - - # # Make sure the locale is set correctly too - # set_gettext_locale - - # # Display default, detailed error for developers - # original_rescue_action_locally(exception) - # end - def local_request? false end diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index 0df685829..9d0f91dda 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -222,5 +222,11 @@ class GeneralController < ApplicationController @locale = self.locale_from_params() render(:layout => false, :content_type => 'text/css') end + + # Handle requests for non-existent URLs - will be handled by ApplicationController::render_exception + def not_found + raise RouteNotFound + end + end diff --git a/config/application.rb b/config/application.rb index f5b525a36..92fd30685 100644 --- a/config/application.rb +++ b/config/application.rb @@ -55,8 +55,11 @@ module Alaveteli # will be in this time zone config.time_zone = ::AlaveteliConfiguration::time_zone - config.after_initialize do + config.after_initialize do |app| require 'routing_filters.rb' + # Add a catch-all route to force routing errors to be handled by the application, + # rather than by middleware. + app.routes.append{ match '*path', :to => 'general#not_found' } end config.autoload_paths << "#{Rails.root.to_s}/lib/mail_handler" diff --git a/spec/integration/errors_spec.rb b/spec/integration/errors_spec.rb index edf570182..ed0d7bfec 100644 --- a/spec/integration/errors_spec.rb +++ b/spec/integration/errors_spec.rb @@ -1,40 +1,113 @@ +# -*- coding: utf-8 -*- require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') -describe "When rendering errors" do +describe "When errors occur" do - before(:each) do - load_raw_emails_data + def set_consider_all_requests_local(value) + @requests_local = Rails.application.config.consider_all_requests_local + Rails.application.config.consider_all_requests_local = value end - it "should render a 404 for users that don't exist" do - get("/user/wobsnasm") - response.code.should == "404" + def restore_consider_all_requests_local + Rails.application.config.consider_all_requests_local = @requests_local end - it "should render a 404 for bodies that don't exist" do - get("/body/wobsnasm") - response.code.should == "404" + + before(:each) do + # This should happen automatically before each test but doesn't with these integration + # tests for some reason. + ActionMailer::Base.deliveries = [] end - it "should render a 500 for general errors" do - ir = info_requests(:naughty_chicken_request) - # Set an invalid state for the request. Note that update_attribute doesn't run the validations - ir.update_attribute(:described_state, "crotchety") - get("/request/#{ir.url_title}") - response.code.should == "500" + + after(:each) do + restore_consider_all_requests_local end - it "should render a 403 for attempts at directory listing for attachments" do - # make a fake cache - foi_cache_path = File.expand_path(File.join(File.dirname(__FILE__), '../../cache')) - FileUtils.mkdir_p(File.join(foi_cache_path, "views/en/request/101/101/response/1/attach/html/1")) - get("/request/101/response/1/attach/html/1/" ) - response.body.should include("Directory listing not allowed") - response.code.should == "403" - get("/request/101/response/1/attach/html" ) - response.body.should include("Directory listing not allowed") - response.code.should == "403" + + context 'when considering all requests local (by default all in development)' do + + before(:each) { set_consider_all_requests_local(true) } + + it 'should show a full trace for general errors' do + InfoRequest.stub!(:find_by_url_title!).and_raise("An example error") + get("/request/example") + response.body.should have_selector('div[id=traces]') + response.body.should match('An example error') + end + end - it "should render a 404 for non-existent 'details' pages for requests" do - get("/details/request/wobble" ) - response.code.should == "404" + + context 'when not considering all requests local' do + + before(:each) { set_consider_all_requests_local(false) } + + it "should render a 404 for unrouteable URLs using the general/exception_caught template" do + get("/frobsnasm") + response.should render_template('general/exception_caught') + response.code.should == "404" + end + + it "should render a 404 for users or bodies that don't exist using the general/exception_caught + template" do + ['/user/wobsnasm', '/body/wobsnasm'].each do |non_existent_url| + get(non_existent_url) + response.should render_template('general/exception_caught') + response.code.should == "404" + end + end + + it "should render a 500 for general errors using the general/exception_caught template" do + InfoRequest.stub!(:find_by_url_title!).and_raise("An example error") + get("/request/example") + response.should render_template('general/exception_caught') + response.body.should match('An example error') + response.code.should == "500" + end + + it 'should notify of a general error' do + InfoRequest.stub!(:find_by_url_title!).and_raise("An example error") + get("/request/example") + deliveries = ActionMailer::Base.deliveries + deliveries.size.should == 1 + mail = deliveries[0] + mail.body.should =~ /An example error/ + end + + it 'should log a general error' do + Rails.logger.should_receive(:fatal) + InfoRequest.stub!(:find_by_url_title!).and_raise("An example error") + get("/request/example") + end + + it 'should assign the locale for the general/exception_caught template' do + InfoRequest.stub!(:find_by_url_title!).and_raise("An example error") + get("/es/request/example") + response.should render_template('general/exception_caught') + response.body.should match('Lo sentimos, hubo un problema procesando esta página') + response.body.should match('An example error') + end + + it "should render a 403 with text body for attempts at directory listing for attachments" do + # make a fake cache + foi_cache_path = File.expand_path(File.join(File.dirname(__FILE__), '../../cache')) + FileUtils.mkdir_p(File.join(foi_cache_path, "views/en/request/101/101/response/1/attach/html/1")) + get("/request/101/response/1/attach/html/1/" ) + response.body.should include("Directory listing not allowed") + response.code.should == "403" + get("/request/101/response/1/attach/html" ) + response.body.should include("Directory listing not allowed") + response.code.should == "403" + end + + context "in the admin interface" do + + it 'should show a full trace for general errors' do + InfoRequest.stub!(:find).and_raise("An example error") + get("/admin/request/show/333") + response.body.should have_selector('div[id=traces]') + response.body.should match('An example error') + end + + end + end -end +end |