From a9e8cf78fda696738f63e726796f6232c3751603 Mon Sep 17 00:00:00 2001 From: lest Date: Mon, 21 Nov 2011 20:13:54 +0300 Subject: [PATCH 1/4] add ActionController::Metal#show_detailed_exceptions? --- actionpack/lib/action_controller/metal.rb | 5 +++ .../middleware/show_exceptions.rb | 16 +++---- .../test/dispatch/show_exceptions_test.rb | 45 +++++++++++-------- railties/lib/rails/application.rb | 2 +- 4 files changed, 39 insertions(+), 29 deletions(-) diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 125dbf6bb5..d5f150e7c9 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -196,10 +196,15 @@ module ActionController @_request = request @_env = request.env @_env['action_controller.instance'] = self + @_env['action_dispatch.show_detailed_exceptions'] = show_detailed_exceptions? process(name) to_a end + def show_detailed_exceptions? + defined?(Rails.application) && Rails.application.config.consider_all_requests_local || request.local? + end + def to_a #:nodoc: response ? response.to_a : [status, headers, response_body] end diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb index 2fa68c64c5..569063f4db 100644 --- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb @@ -38,9 +38,8 @@ module ActionDispatch "application's log file and/or the web server's log file to find out what " << "went wrong."]] - def initialize(app, consider_all_requests_local = false) + def initialize(app) @app = app - @consider_all_requests_local = consider_all_requests_local end def call(env) @@ -65,11 +64,10 @@ module ActionDispatch log_error(exception) exception = original_exception(exception) - request = Request.new(env) - if @consider_all_requests_local || request.local? - rescue_action_locally(request, exception) + if env['action_dispatch.show_detailed_exceptions'] == true + rescue_action_diagnostics(env, exception) else - rescue_action_in_public(exception) + rescue_action_error_page(exception) end rescue Exception => failsafe_error $stderr.puts "Error during failsafe response: #{failsafe_error}\n #{failsafe_error.backtrace * "\n "}" @@ -78,9 +76,9 @@ module ActionDispatch # Render detailed diagnostics for unhandled exceptions rescued from # a controller action. - def rescue_action_locally(request, exception) + def rescue_action_diagnostics(env, exception) template = ActionView::Base.new([RESCUES_TEMPLATE_PATH], - :request => request, + :request => Request.new(env), :exception => exception, :application_trace => application_trace(exception), :framework_trace => framework_trace(exception), @@ -98,7 +96,7 @@ module ActionDispatch # it will first attempt to render the file at public/500.da.html # then attempt to render public/500.html. If none of them exist, # the body of the response will be left empty. - def rescue_action_in_public(exception) + def rescue_action_error_page(exception) status = status_code(exception) locale_path = "#{public_path}/#{status}.#{I18n.locale}.html" if I18n.locale path = "#{public_path}/#{status}.html" diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb index 42f6c7f79f..09eebb3ab5 100644 --- a/actionpack/test/dispatch/show_exceptions_test.rb +++ b/actionpack/test/dispatch/show_exceptions_test.rb @@ -2,28 +2,35 @@ require 'abstract_unit' class ShowExceptionsTest < ActionDispatch::IntegrationTest - Boomer = lambda do |env| - req = ActionDispatch::Request.new(env) - case req.path - when "/not_found" - raise ActionController::UnknownAction - when "/runtime_error" - raise RuntimeError - when "/method_not_allowed" - raise ActionController::MethodNotAllowed - when "/not_implemented" - raise ActionController::NotImplemented - when "/unprocessable_entity" - raise ActionController::InvalidAuthenticityToken - when "/not_found_original_exception" - raise ActionView::Template::Error.new('template', {}, AbstractController::ActionNotFound.new) - else - raise "puke!" + class Boomer + def initialize(show_exceptions = false) + @show_exceptions = show_exceptions + end + + def call(env) + env['action_dispatch.show_exceptions'] = @show_exceptions + req = ActionDispatch::Request.new(env) + case req.path + when "/not_found" + raise ActionController::UnknownAction + when "/runtime_error" + raise RuntimeError + when "/method_not_allowed" + raise ActionController::MethodNotAllowed + when "/not_implemented" + raise ActionController::NotImplemented + when "/unprocessable_entity" + raise ActionController::InvalidAuthenticityToken + when "/not_found_original_exception" + raise ActionView::Template::Error.new('template', {}, AbstractController::ActionNotFound.new) + else + raise "puke!" + end end end - ProductionApp = ActionDispatch::ShowExceptions.new(Boomer, false) - DevelopmentApp = ActionDispatch::ShowExceptions.new(Boomer, true) + ProductionApp = ActionDispatch::ShowExceptions.new(Boomer.new(false)) + DevelopmentApp = ActionDispatch::ShowExceptions.new(Boomer.new(true)) test "rescue in public from a remote ip" do @app = ProductionApp diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 82fffe86bb..817fa39cab 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -166,7 +166,7 @@ module Rails middleware.use ::Rack::MethodOverride middleware.use ::ActionDispatch::RequestId middleware.use ::Rails::Rack::Logger, config.log_tags # must come after Rack::MethodOverride to properly log overridden methods - middleware.use ::ActionDispatch::ShowExceptions, config.consider_all_requests_local + middleware.use ::ActionDispatch::ShowExceptions middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies if config.action_dispatch.x_sendfile_header.present? middleware.use ::Rack::Sendfile, config.action_dispatch.x_sendfile_header From c6d6b28bb4e105fd0ae7a0ef3c7df4bc416bd397 Mon Sep 17 00:00:00 2001 From: lest Date: Tue, 22 Nov 2011 11:24:05 +0300 Subject: [PATCH 2/4] refactor show exceptions tests --- .../test/controller/show_exceptions_test.rb | 59 ++++++++++++++ .../test/dispatch/show_exceptions_test.rb | 77 +++++++------------ 2 files changed, 86 insertions(+), 50 deletions(-) create mode 100644 actionpack/test/controller/show_exceptions_test.rb diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb new file mode 100644 index 0000000000..c328a42e89 --- /dev/null +++ b/actionpack/test/controller/show_exceptions_test.rb @@ -0,0 +1,59 @@ +require 'abstract_unit' + +module ShowExceptions + class ShowExceptionsController < ActionController::Metal + use ActionDispatch::ShowExceptions + + def boom + raise 'boom!' + end + end + + class ShowExceptionsTest < ActionDispatch::IntegrationTest + test 'show error page from a remote ip' do + @app = ShowExceptionsController.action(:boom) + self.remote_addr = '208.77.188.166' + get '/' + assert_equal "500 error fixture\n", body + end + + test 'show diagnostics from a local ip' do + @app = ShowExceptionsController.action(:boom) + ['127.0.0.1', '127.0.0.127', '::1', '0:0:0:0:0:0:0:1', '0:0:0:0:0:0:0:1%0'].each do |ip_address| + self.remote_addr = ip_address + get '/' + assert_match /boom/, body + end + end + + test 'show diagnostics from a remote ip when consider_all_requests_local is true' do + Rails.stubs(:application).returns stub(:config => stub(:consider_all_requests_local => true)) + @app = ShowExceptionsController.action(:boom) + self.remote_addr = '208.77.188.166' + get '/' + assert_match /boom/, body + end + end + + class ShowExceptionsOverridenController < ShowExceptionsController + private + + def show_detailed_exceptions? + params['detailed'] == '1' + end + end + + class ShowExceptionsOverridenTest < ActionDispatch::IntegrationTest + test 'show error page' do + @app = ShowExceptionsOverridenController.action(:boom) + get '/', {'detailed' => '0'} + assert_equal "500 error fixture\n", body + end + + test 'show diagnostics message' do + @app = ShowExceptionsOverridenController.action(:boom) + get '/', {'detailed' => '1'} + assert_match /boom/, body + end + end +end diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb index 09eebb3ab5..9f4d6c530f 100644 --- a/actionpack/test/dispatch/show_exceptions_test.rb +++ b/actionpack/test/dispatch/show_exceptions_test.rb @@ -3,12 +3,12 @@ require 'abstract_unit' class ShowExceptionsTest < ActionDispatch::IntegrationTest class Boomer - def initialize(show_exceptions = false) - @show_exceptions = show_exceptions + def initialize(detailed = false) + @detailed = detailed end def call(env) - env['action_dispatch.show_exceptions'] = @show_exceptions + env['action_dispatch.show_detailed_exceptions'] = @detailed req = ActionDispatch::Request.new(env) case req.path when "/not_found" @@ -32,9 +32,8 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest ProductionApp = ActionDispatch::ShowExceptions.new(Boomer.new(false)) DevelopmentApp = ActionDispatch::ShowExceptions.new(Boomer.new(true)) - test "rescue in public from a remote ip" do + test "rescue with error page when show_exceptions is false" do @app = ProductionApp - self.remote_addr = '208.77.188.166' get "/", {}, {'action_dispatch.show_exceptions' => true} assert_response 500 @@ -49,48 +48,8 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest assert_equal "", body end - test "rescue locally from a local request" do - @app = ProductionApp - ['127.0.0.1', '127.0.0.127', '::1', '0:0:0:0:0:0:0:1', '0:0:0:0:0:0:0:1%0'].each do |ip_address| - self.remote_addr = ip_address - - get "/", {}, {'action_dispatch.show_exceptions' => true} - assert_response 500 - assert_match(/puke/, body) - - get "/not_found", {}, {'action_dispatch.show_exceptions' => true} - assert_response 404 - assert_match(/#{ActionController::UnknownAction.name}/, body) - - get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true} - assert_response 405 - assert_match(/ActionController::MethodNotAllowed/, body) - end - end - - test "localize public rescue message" do - # Change locale - old_locale, I18n.locale = I18n.locale, :da - - begin - @app = ProductionApp - self.remote_addr = '208.77.188.166' - - get "/", {}, {'action_dispatch.show_exceptions' => true} - assert_response 500 - assert_equal "500 localized error fixture\n", body - - get "/not_found", {}, {'action_dispatch.show_exceptions' => true} - assert_response 404 - assert_equal "404 error fixture\n", body - ensure - I18n.locale = old_locale - end - end - - test "always rescue locally in development mode" do + test "rescue with diagnostics message when show_exceptions is true" do @app = DevelopmentApp - self.remote_addr = '208.77.188.166' get "/", {}, {'action_dispatch.show_exceptions' => true} assert_response 500 @@ -105,6 +64,25 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest assert_match(/ActionController::MethodNotAllowed/, body) end + test "localize rescue error page" do + # Change locale + old_locale, I18n.locale = I18n.locale, :da + + begin + @app = ProductionApp + + get "/", {}, {'action_dispatch.show_exceptions' => true} + assert_response 500 + assert_equal "500 localized error fixture\n", body + + get "/not_found", {}, {'action_dispatch.show_exceptions' => true} + assert_response 404 + assert_equal "404 error fixture\n", body + ensure + I18n.locale = old_locale + end + end + test "does not show filtered parameters" do @app = DevelopmentApp @@ -114,16 +92,15 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest assert_match(""foo"=>"[FILTERED]"", body) end - test "show registered original exception for wrapped exceptions when consider_all_requests_local is false" do + test "show registered original exception for wrapped exceptions when show_exceptions is false" do @app = ProductionApp - self.remote_addr = '208.77.188.166' get "/not_found_original_exception", {}, {'action_dispatch.show_exceptions' => true} assert_response 404 assert_match(/404 error/, body) end - test "show registered original exception for wrapped exceptions when consider_all_requests_local is true" do + test "show registered original exception for wrapped exceptions when show_exceptions is true" do @app = DevelopmentApp get "/not_found_original_exception", {}, {'action_dispatch.show_exceptions' => true} @@ -132,7 +109,7 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest end test "show the controller name in the diagnostics template when controller name is present" do - @app = ProductionApp + @app = DevelopmentApp get("/runtime_error", {}, { 'action_dispatch.show_exceptions' => true, 'action_dispatch.request.parameters' => { From 5bcd119b8d9bb6d88c949956de1ce13c2673b877 Mon Sep 17 00:00:00 2001 From: lest Date: Tue, 22 Nov 2011 13:34:13 +0300 Subject: [PATCH 3/4] move show_detailed_exceptions? to Rescue module --- actionpack/lib/action_controller/metal.rb | 5 ----- actionpack/lib/action_controller/metal/rescue.rb | 10 ++++++++++ actionpack/lib/action_controller/railtie.rb | 2 ++ actionpack/test/controller/show_exceptions_test.rb | 4 ++-- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index d5f150e7c9..125dbf6bb5 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -196,15 +196,10 @@ module ActionController @_request = request @_env = request.env @_env['action_controller.instance'] = self - @_env['action_dispatch.show_detailed_exceptions'] = show_detailed_exceptions? process(name) to_a end - def show_detailed_exceptions? - defined?(Rails.application) && Rails.application.config.consider_all_requests_local || request.local? - end - def to_a #:nodoc: response ? response.to_a : [status, headers, response_body] end diff --git a/actionpack/lib/action_controller/metal/rescue.rb b/actionpack/lib/action_controller/metal/rescue.rb index eb037aa1b0..736ff5b31c 100644 --- a/actionpack/lib/action_controller/metal/rescue.rb +++ b/actionpack/lib/action_controller/metal/rescue.rb @@ -3,6 +3,11 @@ module ActionController #:nodoc: extend ActiveSupport::Concern include ActiveSupport::Rescuable + included do + config_accessor :consider_all_requests_local + self.consider_all_requests_local = false if consider_all_requests_local.nil? + end + def rescue_with_handler(exception) if (exception.respond_to?(:original_exception) && (orig_exception = exception.original_exception) && @@ -12,10 +17,15 @@ module ActionController #:nodoc: super(exception) end + def show_detailed_exceptions? + consider_all_requests_local || request.local? + end + private def process_action(*args) super rescue Exception => exception + request.env['action_dispatch.show_detailed_exceptions'] = show_detailed_exceptions? rescue_with_handler(exception) || raise(exception) end end diff --git a/actionpack/lib/action_controller/railtie.rb b/actionpack/lib/action_controller/railtie.rb index f0c29825ba..de7b837ecc 100644 --- a/actionpack/lib/action_controller/railtie.rb +++ b/actionpack/lib/action_controller/railtie.rb @@ -21,6 +21,8 @@ module ActionController paths = app.config.paths options = app.config.action_controller + options.consider_all_requests_local ||= app.config.consider_all_requests_local + options.assets_dir ||= paths["public"].first options.javascripts_dir ||= paths["public/javascripts"].first options.stylesheets_dir ||= paths["public/stylesheets"].first diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb index c328a42e89..39245e9574 100644 --- a/actionpack/test/controller/show_exceptions_test.rb +++ b/actionpack/test/controller/show_exceptions_test.rb @@ -1,7 +1,7 @@ require 'abstract_unit' module ShowExceptions - class ShowExceptionsController < ActionController::Metal + class ShowExceptionsController < ActionController::Base use ActionDispatch::ShowExceptions def boom @@ -27,7 +27,7 @@ module ShowExceptions end test 'show diagnostics from a remote ip when consider_all_requests_local is true' do - Rails.stubs(:application).returns stub(:config => stub(:consider_all_requests_local => true)) + ShowExceptionsController.any_instance.stubs(:consider_all_requests_local).returns(true) @app = ShowExceptionsController.action(:boom) self.remote_addr = '208.77.188.166' get '/' From 3a1d51959bf569a7419fe8ab9416b338334b4800 Mon Sep 17 00:00:00 2001 From: lest Date: Tue, 22 Nov 2011 17:36:58 +0300 Subject: [PATCH 4/4] deprecation warning, changelog entry --- actionpack/CHANGELOG.md | 4 ++++ actionpack/lib/action_dispatch/middleware/show_exceptions.rb | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index dc52262503..fe422f71d5 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,5 +1,9 @@ ## Rails 3.2.0 (unreleased) ## +* Refactor ActionDispatch::ShowExceptions. Controller is responsible for choice to show exceptions. *Sergey Nartimov* + + It's possible to override +show_detailed_exceptions?+ in controllers to specify which requests should provide debugging information on errors. + * Responders now return 204 No Content for API requests without a response body (as in the new scaffold) *José Valim* * Added ActionDispatch::RequestId middleware that'll make a unique X-Request-Id header available to the response and enables the ActionDispatch::Request#uuid method. This makes it easy to trace requests from end-to-end in the stack and to identify individual requests in mixed logs like Syslog *DHH* diff --git a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb index 569063f4db..52dce4cc81 100644 --- a/actionpack/lib/action_dispatch/middleware/show_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/show_exceptions.rb @@ -2,6 +2,7 @@ require 'active_support/core_ext/exception' require 'action_controller/metal/exceptions' require 'active_support/notifications' require 'action_dispatch/http/request' +require 'active_support/deprecation' module ActionDispatch # This middleware rescues any exception returned by the application and renders @@ -38,7 +39,8 @@ module ActionDispatch "application's log file and/or the web server's log file to find out what " << "went wrong."]] - def initialize(app) + def initialize(app, consider_all_requests_local = nil) + ActiveSupport::Deprecation.warn "Passing consider_all_requests_local option to ActionDispatch::ShowExceptions middleware no longer works" unless consider_all_requests_local.nil? @app = app end