From e0792c80ddf3706a4a66c8d57e94b3c0bdffb702 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 11 Nov 2016 16:57:25 +0100 Subject: [PATCH] Fixed issue #385 - Invalid (html) content in notification templates breaks application. --- lib/notification_factory/mailer.rb | 8 +- lib/notification_factory/renderer.rb | 124 ++++++++++++++++++ lib/notification_factory/slack.rb | 8 +- lib/notification_factory/template.rb | 111 ++-------------- .../notification_factory_renderer_test.rb | 86 ++++++++++++ .../notification_factory_template_test.rb | 40 ++++++ 6 files changed, 270 insertions(+), 107 deletions(-) create mode 100644 lib/notification_factory/renderer.rb create mode 100644 test/unit/notification_factory_renderer_test.rb create mode 100644 test/unit/notification_factory_template_test.rb diff --git a/lib/notification_factory/mailer.rb b/lib/notification_factory/mailer.rb index de4a9b44a..ca70330c4 100644 --- a/lib/notification_factory/mailer.rb +++ b/lib/notification_factory/mailer.rb @@ -221,7 +221,7 @@ returns def self.template(data) if data[:templateInline] - return NotificationFactory::Template.new(data[:objects], data[:locale], data[:templateInline], false).render + return NotificationFactory::Renderer.new(data[:objects], data[:locale], data[:templateInline], false).render end template = NotificationFactory.template_read( @@ -231,8 +231,8 @@ returns type: 'mailer', ) - message_subject = NotificationFactory::Template.new(data[:objects], data[:locale], template[:subject], false).render - message_body = NotificationFactory::Template.new(data[:objects], data[:locale], template[:body]).render + message_subject = NotificationFactory::Renderer.new(data[:objects], data[:locale], template[:subject], false).render + message_body = NotificationFactory::Renderer.new(data[:objects], data[:locale], template[:body]).render if !data[:raw] application_template = NotificationFactory.application_template_read( @@ -241,7 +241,7 @@ returns ) data[:objects][:message] = message_body data[:objects][:standalone] = data[:standalone] - message_body = NotificationFactory::Template.new(data[:objects], data[:locale], application_template).render + message_body = NotificationFactory::Renderer.new(data[:objects], data[:locale], application_template).render end { subject: message_subject, diff --git a/lib/notification_factory/renderer.rb b/lib/notification_factory/renderer.rb new file mode 100644 index 000000000..628bf2df4 --- /dev/null +++ b/lib/notification_factory/renderer.rb @@ -0,0 +1,124 @@ +class NotificationFactory::Renderer + +=begin + +examples how to use + + message_subject = NotificationFactory::Renderer.new( + { + ticket: Ticket.first, + }, + 'de-de', + 'some template <%= d "ticket.title", false %> <%= c "fqdn", false %>', + false + ).render + + message_body = NotificationFactory::Renderer.new( + { + ticket: Ticket.first, + }, + 'de-de', + 'some template <%= d "ticket.title", true %> <%= c "fqdn", true %>', + ).render + +=end + + def initialize(objects, locale, template, escape = true) + @objects = objects + @locale = locale || 'en-us' + @template = NotificationFactory::Template.new(template) + @escape = escape + end + + def render + ERB.new(@template.to_s).result(binding) + end + + # d - data of object + # d('user.firstname', htmlEscape) + def d(key, escape = nil) + + # do validaton, ignore some methodes + if key =~ /(`|\.(|\s*)(save|destroy|delete|remove|drop|update\(|update_att|create\(|new|all|where|find))/i + return "#{key} (not allowed)" + end + + value = nil + object_methods = key.split('.') + object_name = object_methods.shift.to_sym + object_refs = @objects[object_name] + object_methods_s = '' + object_methods.each { |method_raw| + + method = method_raw.strip + + if object_methods_s != '' + object_methods_s += '.' + end + object_methods_s += method + + # if method exists + if !object_refs.respond_to?( method.to_sym ) + value = "\#{#{object_name}.#{object_methods_s} / no such method}" + break + end + object_refs = object_refs.send( method.to_sym ) + } + placeholder = if !value + object_refs + else + value + end + escaping(placeholder, escape) + end + + # c - config + # c('fqdn', htmlEscape) + def c(key, escape = nil) + config = Setting.get(key) + escaping(config, escape) + end + + # t - translation + # t('yes', htmlEscape) + def t(key, escape = nil) + translation = Translation.translate(@locale, key) + escaping(translation, escape) + end + + # a_html - article body in html + # a_html(article) + def a_html(article) + content_type = d "#{article}.content_type", false + if content_type =~ /html/ + return d "#{article}.body", false + end + d("#{article}.body", false).text2html + end + + # a_text - article body in text + # a_text(article) + def a_text(article) + content_type = d "#{article}.content_type", false + body = d "#{article}.body", false + if content_type =~ /html/ + body = body.html2text + end + (body.strip + "\n").gsub(/^(.*?)$/, '> \\1') + end + + # h - htmlEscape + # h('fqdn', htmlEscape) + def h(key) + return key if !key + CGI.escapeHTML(key.to_s) + end + + private + + def escaping(key, escape) + return key if escape == false + return key if escape.nil? && !@escape + h key + end +end diff --git a/lib/notification_factory/slack.rb b/lib/notification_factory/slack.rb index 895b2ff05..c0102e460 100644 --- a/lib/notification_factory/slack.rb +++ b/lib/notification_factory/slack.rb @@ -23,7 +23,7 @@ returns def self.template(data) if data[:templateInline] - return NotificationFactory::Template.new(data[:objects], data[:locale], data[:templateInline]).render + return NotificationFactory::Renderer.new(data[:objects], data[:locale], data[:templateInline]).render end template = NotificationFactory.template_read( @@ -33,8 +33,8 @@ returns type: 'slack', ) - message_subject = NotificationFactory::Template.new(data[:objects], data[:locale], template[:subject]).render - message_body = NotificationFactory::Template.new(data[:objects], data[:locale], template[:body]).render + message_subject = NotificationFactory::Renderer.new(data[:objects], data[:locale], template[:subject]).render + message_body = NotificationFactory::Renderer.new(data[:objects], data[:locale], template[:body]).render if !data[:raw] application_template = NotificationFactory.application_template_read( @@ -43,7 +43,7 @@ returns ) data[:objects][:message] = message_body data[:objects][:standalone] = data[:standalone] - message_body = NotificationFactory::Template.new(data[:objects], data[:locale], application_template).render + message_body = NotificationFactory::Renderer.new(data[:objects], data[:locale], application_template).render end { subject: message_subject.strip!, diff --git a/lib/notification_factory/template.rb b/lib/notification_factory/template.rb index b10a9ecac..7da6e8efe 100644 --- a/lib/notification_factory/template.rb +++ b/lib/notification_factory/template.rb @@ -4,113 +4,26 @@ class NotificationFactory::Template examples how to use - message_subject = NotificationFactory::Template.new( - { - ticket: Ticket.first, - }, - 'de-de', + cleaned_template = NotificationFactory::Template.new( 'some template <%= d "ticket.title", false %> <%= c "fqdn", false %>', - false - ).render - - message_body = NotificationFactory::Template.new( - { - ticket: Ticket.first, - }, - 'de-de', - 'some template <%= d "ticket.title", true %> <%= c "fqdn", true %>', - ).render + ).to_s =end - def initialize(objects, locale, template, escape = true) - @objects = objects - @locale = locale || 'en-us' + def initialize(template) @template = template - @escape = escape end - def render - ERB.new(@template).result(binding) + def to_s + strip_html + + @template end - # d - data of object - # d('user.firstname', htmlEscape) - def d(key, escape = nil) - - # do validaton, ignore some methodes - if key =~ /(`|\.(|\s*)(save|destroy|delete|remove|drop|update\(|update_att|create\(|new|all|where|find))/i - return "#{key} (not allowed)" - end - - value = nil - object_methods = key.split('.') - object_name = object_methods.shift.to_sym - object_refs = @objects[object_name] - object_methods_s = '' - object_methods.each { |method| - if object_methods_s != '' - object_methods_s += '.' - end - object_methods_s += method - - # if method exists - if !object_refs.respond_to?( method.to_sym ) - value = "\#{#{object_name}.#{object_methods_s} / no such method}" - break - end - object_refs = object_refs.send( method.to_sym ) - } - placeholder = if !value - object_refs - else - value - end - return placeholder if escape == false || (escape.nil? && !@escape) - h placeholder - end - - # c - config - # c('fqdn', htmlEscape) - def c(key, escape = nil) - config = Setting.get(key) - return config if escape == false || (escape.nil? && !@escape) - h config - end - - # t - translation - # t('yes', htmlEscape) - def t(key, escape = nil) - translation = Translation.translate(@locale, key) - return translation if escape == false || (escape.nil? && !@escape) - h translation - end - - # a_html - article body in html - # a_html(article) - def a_html(article) - content_type = d "#{article}.content_type", false - if content_type =~ /html/ - return d "#{article}.body", false - end - d("#{article}.body", false).text2html - end - - # a_text - article body in text - # a_text(article) - def a_text(article) - content_type = d "#{article}.content_type", false - body = d "#{article}.body", false - if content_type =~ /html/ - body = body.html2text - end - (body.strip + "\n").gsub(/^(.*?)$/, '> \\1') - end - - # h - htmlEscape - # h('fqdn', htmlEscape) - def h(key) - return key if !key - CGI.escapeHTML(key.to_s) + def strip_html + # some browsers start adding HTML tags + # fixes https://github.com/zammad/zammad/issues/385 + @template.gsub!(%r{#\{\s*<[^>]+>([^<]+)]+>\s*\}}, '\1') + @template.gsub!(/#\{\s*<[^>]+>([^<]+)\s*\}/, '\1') end end diff --git a/test/unit/notification_factory_renderer_test.rb b/test/unit/notification_factory_renderer_test.rb new file mode 100644 index 000000000..992018a72 --- /dev/null +++ b/test/unit/notification_factory_renderer_test.rb @@ -0,0 +1,86 @@ +# encoding: utf-8 +require 'test_helper' + +class NotificationFactoryRendererTest < ActiveSupport::TestCase + + # TODO: should be mocked somehow + Translation.load('de-de') + + # RSpec incoming! + def described_class + NotificationFactory::Renderer + end + + Group = Struct.new(:name) + State = Struct.new(:name) + User = Struct.new(:firstname, :lastname, :longname, :fullname) + Ticket = Struct.new(:id, :title, :group, :owner, :state) + + group = Group.new('Users') + state = State.new('new') + owner = User.new('Notificationxxx', 'Agent1yyy', 'Notificationxxx Agent1yyy', 'Notificationxxx Agent1yyy (Zammad)') + current_user = User.new('CurrentUserxxx', 'Agent2yyy', 'CurrentUserxxx Agent2yyy', 'CurrentUserxxx Agent2yyy (Zammad)') + recipient = User.new('Recipientxxx', 'Customer1yyy', 'Recipientxxx Customer1yyy', 'Recipientxxx Customer1yyy (Zammad)') + ticket = Ticket.new(1, 'Welcome to Zammad!', group, owner, state) + + test 'replace object attribute' do + + template = "<%= d 'ticket.title' %>" + + result = described_class.new( + { + ticket: ticket, + }, + 'en-us', + template, + ).render + + assert_equal(CGI.escapeHTML(ticket.title), result) + end + + test 'config' do + + setting = 'fqdn' + template = "<%= c '#{setting}' %>" + + result = described_class.new( + { + ticket: ticket, + }, + 'en-us', + template, + ).render + + assert_equal(Setting.get(setting), result) + end + + test 'translation' do + + template = "<%= t 'new' %>" + + result = described_class.new( + { + ticket: ticket, + }, + 'de-de', + template, + ).render + + assert_equal('neu', result) + end + + test 'chained function calls' do + + template = "<%= t d 'ticket.state.name' %>" + + result = described_class.new( + { + ticket: ticket, + }, + 'de-de', + template, + ).render + + assert_equal('neu', result) + end +end diff --git a/test/unit/notification_factory_template_test.rb b/test/unit/notification_factory_template_test.rb new file mode 100644 index 000000000..f5bf4ebea --- /dev/null +++ b/test/unit/notification_factory_template_test.rb @@ -0,0 +1,40 @@ +# encoding: utf-8 +require 'test_helper' + +class NotificationFactoryTemplateTest < ActiveSupport::TestCase + + # RSpec incoming! + def described_class + NotificationFactory::Template + end + + test 'regular browser html' do + + # ensures https://github.com/zammad/zammad/issues/385 + template_before = '<%= d "#{ticket.id}" %>' + template_after = '<%= d "ticket.id" %>' + + result = described_class.new(template_before).to_s + assert_equal(template_after, result) + end + + test 'spaced browser html' do + + # ensures https://github.com/zammad/zammad/issues/385 + template_before = '<%= d "#{ ticket.id } " %>' + template_after = '<%= d "ticket.id " %>' + + result = described_class.new(template_before).to_s + assert_equal(template_after, result) + end + + test 'broken browser html' do + + # ensures https://github.com/zammad/zammad/issues/385 + template_before = '<%= d "#{ticket.id }" %>' + template_after = '<%= d "ticket.id " %>' + + result = described_class.new(template_before).to_s + assert_equal(template_after, result) + end +end