From de30a5c1b1e8dbbc794c5ff616f97bb1bc3073ff Mon Sep 17 00:00:00 2001 From: Martin Gruner Date: Thu, 23 Sep 2021 12:04:18 +0200 Subject: [PATCH] Maintenance: Improve template rendering. --- lib/notification_factory/mailer.rb | 9 ++++--- lib/notification_factory/renderer.rb | 7 ++--- lib/notification_factory/slack.rb | 9 ++++--- lib/notification_factory/template.rb | 10 ++++--- .../notification_factory/renderer.rb | 5 ++-- .../lib/notification_factory/renderer_spec.rb | 21 ++++++++++++--- .../lib/notification_factory/template_spec.rb | 26 ++++++++++++++++++- 7 files changed, 69 insertions(+), 18 deletions(-) diff --git a/lib/notification_factory/mailer.rb b/lib/notification_factory/mailer.rb index 3d0ee633d..b9e0154ac 100644 --- a/lib/notification_factory/mailer.rb +++ b/lib/notification_factory/mailer.rb @@ -324,7 +324,8 @@ returns locale: data[:locale], timezone: data[:timezone], template: template[:subject], - escape: false + escape: false, + trusted: true, ).render # strip off the extra newline at the end of the subject to avoid =0A suffixes (see #2726) @@ -334,7 +335,8 @@ returns objects: data[:objects], locale: data[:locale], timezone: data[:timezone], - template: template[:body] + template: template[:body], + trusted: true, ).render if !data[:raw] @@ -348,7 +350,8 @@ returns objects: data[:objects], locale: data[:locale], timezone: data[:timezone], - template: application_template + template: application_template, + trusted: true, ).render end { diff --git a/lib/notification_factory/renderer.rb b/lib/notification_factory/renderer.rb index 669279bd9..bc6e96f6e 100644 --- a/lib/notification_factory/renderer.rb +++ b/lib/notification_factory/renderer.rb @@ -13,7 +13,8 @@ examples how to use locale: 'de-de', timezone: 'America/Port-au-Prince', template: 'some template #{ticket.title} {config.fqdn}', - escape: false + escape: false, + trusted: false, # Allow ERB tags in the template? ).render message_body = NotificationFactory::Renderer.new( @@ -27,11 +28,11 @@ examples how to use =end - def initialize(objects:, template:, locale: nil, timezone: nil, escape: true) + def initialize(objects:, template:, locale: nil, timezone: nil, escape: true, trusted: false) # rubocop:disable Metrics/ParameterLists @objects = objects @locale = locale || Locale.default @timezone = timezone || Setting.get('timezone_default') - @template = NotificationFactory::Template.new(template, escape) + @template = NotificationFactory::Template.new(template, escape, trusted) @escape = escape end diff --git a/lib/notification_factory/slack.rb b/lib/notification_factory/slack.rb index f490da42e..e7a12a190 100644 --- a/lib/notification_factory/slack.rb +++ b/lib/notification_factory/slack.rb @@ -46,14 +46,16 @@ returns locale: data[:locale], timezone: data[:timezone], template: template[:subject], - escape: false + escape: false, + trusted: true ).render message_body = NotificationFactory::Renderer.new( objects: data[:objects], locale: data[:locale], timezone: data[:timezone], template: template[:body], - escape: false + escape: false, + trusted: true ).render if !data[:raw] @@ -68,7 +70,8 @@ returns locale: data[:locale], timezone: data[:timezone], template: application_template, - escape: false + escape: false, + trusted: true ).render end { diff --git a/lib/notification_factory/template.rb b/lib/notification_factory/template.rb index b7919e6c2..19e93835f 100644 --- a/lib/notification_factory/template.rb +++ b/lib/notification_factory/template.rb @@ -9,17 +9,21 @@ examples how to use cleaned_template = NotificationFactory::Template.new( 'some template #{ticket.title} #{config.fqdn}', true, + false, # Allow ERB tags in the template? ).to_s =end - def initialize(template, escape) + def initialize(template, escape, trusted) @template = template - @escape = escape + @escape = escape + @trusted = trusted end def to_s - @template.gsub(%r{\#{\s*(.*?)\s*}}m) do + result = @template + result.gsub!(%r{<%(?!%)}, '<%%') if !@trusted + result.gsub(%r{\#{\s*(.*?)\s*}}m) do # some browsers start adding HTML tags # fixes https://github.com/zammad/zammad/issues/385 input_template = $1.gsub(%r{\A<.+?>\s*|\s*<.+?>\z}, '') diff --git a/spec/factories/notification_factory/renderer.rb b/spec/factories/notification_factory/renderer.rb index c1f86ea47..244503265 100644 --- a/spec/factories/notification_factory/renderer.rb +++ b/spec/factories/notification_factory/renderer.rb @@ -2,11 +2,12 @@ FactoryBot.define do factory :notification_factory_renderer, class: 'NotificationFactory::Renderer' do - objects { nil } + objects { nil } locale { 'en-en' } template { '' } escape { true } + trusted { false } - initialize_with { new(objects: objects, locale: locale, template: template, escape: escape) } + initialize_with { new(objects: objects, locale: locale, template: template, escape: escape, trusted: trusted) } end end diff --git a/spec/lib/notification_factory/renderer_spec.rb b/spec/lib/notification_factory/renderer_spec.rb index 1785ba2a0..3fe093fec 100644 --- a/spec/lib/notification_factory/renderer_spec.rb +++ b/spec/lib/notification_factory/renderer_spec.rb @@ -12,6 +12,21 @@ RSpec.describe NotificationFactory::Renderer do expect(renderer.render).to eq '' end + context 'when rendering templates with ERB tags' do + + let(:template) { '<%% <%= "<%" %> %%>' } + + it 'ignores pre-existing ERB tags in an untrusted template' do + renderer = build :notification_factory_renderer, template: template + expect(renderer.render).to eq '<% <%= "<%" %> %%>' + end + + it 'executes pre-existing ERB tags in a trusted template' do + renderer = build :notification_factory_renderer, template: template, trusted: true + expect(renderer.render).to eq '<% <% %%>' + end + end + it 'correctly renders chained object references' do user = User.where(firstname: 'Nicole').first ticket = create :ticket, customer: user @@ -30,17 +45,17 @@ RSpec.describe NotificationFactory::Renderer do end it 'raises a StandardError when rendering a template with a broken syntax' do - renderer = build :notification_factory_renderer, template: 'test <% if %>', objects: {} + renderer = build :notification_factory_renderer, template: 'test <% if %>', objects: {}, trusted: true expect { renderer.render }.to raise_error(StandardError) end it 'raises a StandardError when rendering a template calling a non existant method' do - renderer = build :notification_factory_renderer, template: 'test <% Ticket.non_existant_method %>', objects: {} + renderer = build :notification_factory_renderer, template: 'test <% Ticket.non_existant_method %>', objects: {}, trusted: true expect { renderer.render }.to raise_error(StandardError) end it 'raises a StandardError when rendering a template referencing a non existant object' do - renderer = build :notification_factory_renderer, template: 'test <% NonExistantObject.first %>', objects: {} + renderer = build :notification_factory_renderer, template: 'test <% NonExistantObject.first %>', objects: {}, trusted: true expect { renderer.render }.to raise_error(StandardError) end diff --git a/spec/lib/notification_factory/template_spec.rb b/spec/lib/notification_factory/template_spec.rb index b8257c443..a8edc0219 100644 --- a/spec/lib/notification_factory/template_spec.rb +++ b/spec/lib/notification_factory/template_spec.rb @@ -4,9 +4,11 @@ require 'rails_helper' RSpec.describe NotificationFactory::Template do subject(:template) do - described_class.new(template_string, escape) + described_class.new(template_string, escape, trusted) end + let(:trusted) { false } + describe '#to_s' do context 'for empty input template (incl. whitespace-only)' do let(:template_string) { "\#{ }" } @@ -28,6 +30,28 @@ RSpec.describe NotificationFactory::Template do end end + context 'for sanitizing the template string' do + let(:escape) { false } + + context 'for strings containing ERB' do + let(:template_string) { '<%% <% "<%" %> <%# comment %> <%= "<%" %> <%- "" %> %%>' } + + context 'for untrusted templates' do + it 'mutes all pre-existing ERB tags' do + expect(template.to_s).to eq('<%% <%% "<%%" %> <%%# comment %> <%%= "<%%" %> <%%- "" %> %%>') + end + end + + context 'for trusted templates' do + let(:trusted) { true } + + it 'keeps all pre-existing ERB tags' do + expect(template.to_s).to eq(template_string) + end + end + end + end + context 'for input template using #t helper' do let(:template_string) { "\#{t('some text')}" } let(:escape) { false }