Fixed issue #385 - Invalid (html) content in notification templates breaks application.

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_subject = NotificationFactory::Renderer.new(data[:objects], data[:locale], template[:subject], false).render
-    message_body = NotificationFactory::Template.new(data[:objects], data[:locale], template[:body]).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,

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 <b><%= d "ticket.title", false %></b> <%= c "fqdn", false %>',
+      false
+    ).render
+
+    message_body = NotificationFactory::Renderer.new(
+      {
+        ticket: Ticket.first,
+      },
+      'de-de',
+      'some template <b><%= d "ticket.title", true %></b> <%= 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

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_subject = NotificationFactory::Renderer.new(data[:objects], data[:locale], template[:subject]).render
-    message_body = NotificationFactory::Template.new(data[:objects], data[:locale], template[:body]).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!,

lib/notification_factory/template.rb

@@ -4,113 +4,26 @@ class NotificationFactory::Template
 
 examples how to use
 
-    message_subject = NotificationFactory::Template.new(
+    cleaned_template = NotificationFactory::Template.new(
-      {
-        ticket: Ticket.first,
-      },
-      'de-de',
       'some template <b><%= d "ticket.title", false %></b> <%= c "fqdn", false %>',
-      false
+    ).to_s
-    ).render
-
-    message_body = NotificationFactory::Template.new(
-      {
-        ticket: Ticket.first,
-      },
-      'de-de',
-      'some template <b><%= d "ticket.title", true %></b> <%= c "fqdn", true %>',
-    ).render
 
 =end
 
-  def initialize(objects, locale, template, escape = true)
+  def initialize(template)
-    @objects = objects
-    @locale = locale || 'en-us'
     @template = template
-    @escape = escape
   end
 
-  def render
+  def to_s
-    ERB.new(@template).result(binding)
+    strip_html
+
+    @template
   end
 
-  # d - data of object
+  def strip_html
-  # d('user.firstname', htmlEscape)
+    # some browsers start adding HTML tags
-  def d(key, escape = nil)
+    # fixes https://github.com/zammad/zammad/issues/385
-
+    @template.gsub!(%r{#\{\s*<[^>]+>([^<]+)</[^>]+>\s*\}}, '\1')
-    # do validaton, ignore some methodes
+    @template.gsub!(/#\{\s*<[^>]+>([^<]+)\s*\}/, '\1')
-    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)
   end
 end

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('Notification<b>xxx</b>', 'Agent1<b>yyy</b>', 'Notification<b>xxx</b> Agent1<b>yyy</b>', 'Notification<b>xxx</b> Agent1<b>yyy</b> (Zammad)')
+  current_user = User.new('CurrentUser<b>xxx</b>', 'Agent2<b>yyy</b>', 'CurrentUser<b>xxx</b> Agent2<b>yyy</b>', 'CurrentUser<b>xxx</b> Agent2<b>yyy</b> (Zammad)')
+  recipient    = User.new('Recipient<b>xxx</b>', 'Customer1<b>yyy</b>', 'Recipient<b>xxx</b> Customer1<b>yyy</b>', 'Recipient<b>xxx</b> Customer1<b>yyy</b> (Zammad)')
+  ticket       = Ticket.new(1, '<b>Welcome to Zammad!</b>', 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

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 "#{<a href="http://ticket.id" title="http://ticket.id" target="_blank">ticket.id</a>}" %>'
+    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 "#{   <a href="http://ticket.id" title="http://ticket.id" target="_blank">ticket.id  </a>}     " %>'
+    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 "#{<a href="http://ticket.id" title="http://ticket.id" target="_blank">ticket.id  }" %>'
+    template_after  = '<%= d "ticket.id  " %>'
+
+    result = described_class.new(template_before).to_s
+    assert_equal(template_after, result)
+  end
+end