From f27bd619b9ac7b9308fb3a96bb48b04cb0f23c2c Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Wed, 7 Dec 2016 17:34:29 -0500 Subject: [PATCH] change: Render an opaque internal error by default for non-Liquid::Error (#835) These errors may contain sensitive information, so is safer to render a more vague message by default. This is done by replacing non-Liquid::Error exceptions with a Liquid::InternalError exception with the non-Liquid::Error accessible on through the cause method. This also allows the template name and line number to be attached to the template errors. The exception_handler render option has been changed to exception_renderer since now it should raise an exception to re-raise on a liquid rendering error or return a string to be rendered where the error occurred. --- History.md | 1 + lib/liquid/context.rb | 42 +++++++++---------------- lib/liquid/errors.rb | 9 +----- lib/liquid/template.rb | 4 +-- test/integration/error_handling_test.rb | 32 ++++++++++--------- test/integration/template_test.rb | 12 ++++--- 6 files changed, 45 insertions(+), 55 deletions(-) diff --git a/History.md b/History.md index 0cbc692..f317aec 100644 --- a/History.md +++ b/History.md @@ -3,6 +3,7 @@ ## 4.0.0 / not yet released / branch "master" ### Changed +* Render an opaque internal error by default for non-Liquid::Error (#835) [Dylan Thacker-Smith] * Ruby 2.0 support dropped (#832) [Dylan Thacker-Smith] * Add to_number Drop method to allow custom drops to work with number filters (#731) * Add strict_variables and strict_filters options to detect undefined references (#691) diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 7692821..2b7544b 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -13,7 +13,7 @@ module Liquid # context['bob'] #=> nil class Context class Context attr_reader :scopes, :errors, :registers, :environments, :resource_limits - attr_accessor :exception_handler, :template_name, :partial, :global_filter, :strict_variables, :strict_filters + attr_accessor :exception_renderer, :template_name, :partial, :global_filter, :strict_variables, :strict_filters def initialize(environments = {}, outer_scope = {}, registers = {}, rethrow_errors = false, resource_limits = nil) @environments = [environments].flatten @@ -28,7 +28,7 @@ module Liquid @this_stack_used = false if rethrow_errors - self.exception_handler = ->(e) { raise } + self.exception_renderer = ->(e) { raise } end @interrupts = [] @@ -74,32 +74,13 @@ module Liquid end def handle_error(e, line_number = nil, raw_token = nil) - if e.is_a?(Liquid::Error) - e.template_name ||= template_name - e.line_number ||= line_number - end - - output = nil - - if exception_handler - args = [e] - args << { line_number: line_number, raw_token: raw_token } if exception_handler.arity == 2 - result = exception_handler.call(*args) - case result - when Exception - e = result - if e.is_a?(Liquid::Error) - e.template_name ||= template_name - e.line_number ||= line_number - end - when String - output = result - else - raise if result - end - end + e = internal_error unless e.is_a?(Liquid::Error) + e.template_name ||= template_name + e.line_number ||= line_number errors.push(e) - output || Liquid::Error.render(e) + + e = exception_renderer.call(e) if exception_renderer + e.to_s end def invoke(method, *args) @@ -223,6 +204,13 @@ module Liquid private + def internal_error + # raise and catch to set backtrace and cause on exception + raise Liquid::InternalError, 'internal' + rescue Liquid::InternalError => exc + exc + end + def squash_instance_assigns_with_environments @scopes.last.each_key do |k| @environments.each do |env| diff --git a/lib/liquid/errors.rb b/lib/liquid/errors.rb index 6dcd05e..defa5ea 100644 --- a/lib/liquid/errors.rb +++ b/lib/liquid/errors.rb @@ -17,14 +17,6 @@ module Liquid str end - def self.render(e) - if e.is_a?(Liquid::Error) - e.to_s - else - "Liquid error: #{e}" - end - end - private def message_prefix @@ -60,4 +52,5 @@ module Liquid UndefinedDropMethod = Class.new(Error) UndefinedFilter = Class.new(Error) MethodOverrideError = Class.new(Error) + InternalError = Class.new(Error) end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 40985ed..ebd8fc0 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -167,7 +167,7 @@ module Liquid c = args.shift if @rethrow_errors - c.exception_handler = ->(e) { raise } + c.exception_renderer = ->(e) { raise } end c @@ -241,7 +241,7 @@ module Liquid def apply_options_to_context(context, options) context.add_filters(options[:filters]) if options[:filters] context.global_filter = options[:global_filter] if options[:global_filter] - context.exception_handler = options[:exception_handler] if options[:exception_handler] + context.exception_renderer = options[:exception_renderer] if options[:exception_renderer] context.strict_variables = options[:strict_variables] if options[:strict_variables] context.strict_filters = options[:strict_filters] if options[:strict_filters] end diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index 95d65db..9d20be2 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -202,22 +202,26 @@ class ErrorHandlingTest < Minitest::Test end end - def test_exception_handler_with_string_result - template = Liquid::Template.parse('This is an argument error: {{ errors.argument_error }}') - output = template.render({ 'errors' => ErrorDrop.new }, exception_handler: ->(e) { '' }) - assert_equal 'This is an argument error: ', output - assert_equal [ArgumentError], template.errors.map(&:class) - end - - class InternalError < Liquid::Error - end - - def test_exception_handler_with_exception_result + def test_default_exception_renderer_with_internal_error template = Liquid::Template.parse('This is a runtime error: {{ errors.runtime_error }}', line_numbers: true) - handler = ->(e) { e.is_a?(Liquid::Error) ? e : InternalError.new('internal') } - output = template.render({ 'errors' => ErrorDrop.new }, exception_handler: handler) + + output = template.render({ 'errors' => ErrorDrop.new }) + assert_equal 'This is a runtime error: Liquid error (line 1): internal', output - assert_equal [InternalError], template.errors.map(&:class) + assert_equal [Liquid::InternalError], template.errors.map(&:class) + end + + def test_exception_renderer_exposing_non_liquid_error + template = Liquid::Template.parse('This is a runtime error: {{ errors.runtime_error }}', line_numbers: true) + exceptions = [] + handler = ->(e) { exceptions << e; e.cause } + + output = template.render({ 'errors' => ErrorDrop.new }, exception_renderer: handler) + + assert_equal 'This is a runtime error: runtime error', output + assert_equal [Liquid::InternalError], exceptions.map(&:class) + assert_equal exceptions, template.errors + assert_equal '#', exceptions.first.cause.inspect end class TestFileSystem diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index 8ad616d..253b976 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -215,16 +215,20 @@ class TemplateTest < Minitest::Test assert_equal 'ruby error in drop', e.message end - def test_exception_handler_doesnt_reraise_if_it_returns_false + def test_exception_renderer_that_returns_string exception = nil - Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_handler: ->(e) { exception = e; false }) + handler = ->(e) { exception = e; '' } + + output = Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_renderer: handler) + assert exception.is_a?(Liquid::ZeroDivisionError) + assert_equal '', output end - def test_exception_handler_does_reraise_if_it_returns_true + def test_exception_renderer_that_raises exception = nil assert_raises(Liquid::ZeroDivisionError) do - Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_handler: ->(e) { exception = e; true }) + Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_renderer: ->(e) { exception = e; raise }) end assert exception.is_a?(Liquid::ZeroDivisionError) end