From 5e22499e44225c831661087b68de154deafef5c6 Mon Sep 17 00:00:00 2001
From: Thorsten Eckel <te@znuny.com>
Date: Sun, 19 Oct 2014 20:26:01 +0200
Subject: [PATCH] - Fixed typo in unit test filename. - Fixed exploitable eval
 call in callback replacement. - Added unit test for fixed exploitable eval
 call. - Improved callback regular expressions and made them more strict. -
 Fixed (now) broken i18n unit test. - Refactored code layout.

---
 lib/notification_factory.rb                   | 64 +++++++++++--------
 ...y_test.rb => notification_factory_test.rb} |  7 +-
 2 files changed, 43 insertions(+), 28 deletions(-)
 rename test/unit/{notifiation_factory_test.rb => notification_factory_test.rb} (91%)

diff --git a/lib/notification_factory.rb b/lib/notification_factory.rb
index fbeb6c254..3f594e08d 100644
--- a/lib/notification_factory.rb
+++ b/lib/notification_factory.rb
@@ -1,47 +1,57 @@
 module NotificationFactory
   def self.build(data)
 
-    data[:string].gsub!( /\#\{(.+?)\}/ ) { |s|
+    data[:string].gsub!( / \#\{ \s* ( .+? ) \s* \} /x ) { |placeholder|
+
+      # store possible callback to work with
+      # and check if it's valid for execution
+      original_string = $&
+      callback        = $1
+      callback_valid  = nil
 
       # use quoted text
-      callback = $1
-      callback.gsub!( /\.body$/ ) { |item|
-        item = item + '.word_wrap( :line_width => 82 ).message_quote.chomp'
+      callback.gsub!( /\A ( \w+ ) \.body \z/x ) { |item|
+        callback_valid = true
+        item           = item + '.word_wrap( :line_width => 82 ).message_quote.chomp'
       }
 
       # use config params
-      callback.gsub!( /^config\.(.+?)$/ ) { |item|
-        name = $1
-        item = "Setting.get('#{$1}')"
+      callback.gsub!( /\A config\.( [\w\.]+ ) \z/x ) { |item|
+        callback_valid = true
+        name           = $1
+        item           = "Setting.get('#{name}')"
       }
 
       # use object params
-      callback.gsub!( /^(.+?)(\..+?)$/ ) { |item|
+      callback.gsub!( /\A ( [\w]+ )( \.[\w\.]+ ) \z/x ) { |item|
+
         object_name   = $1
         object_method = $2
-        replace = nil
-        if data[:objects][object_name.to_sym]
-          replace = "data[:objects]['#{object_name}'.to_sym]#{object_method}"
-        else
-          replace = $1 + $2
-        end
-        item = replace
+
+        next if !data[:objects][object_name.to_sym]
+
+        callback_valid = true
+        item           = "data[:objects]['#{object_name}'.to_sym]#{object_method}"
       }
 
-      # replace value
-      begin
-        s = eval callback
-      rescue Exception => e
-        Rails.logger.error "can't eval #{callback}"
-        Rails.logger.error e.inspect
+      if callback_valid
+        # replace value
+        begin
+          placeholder = eval callback
+        rescue Exception => e
+          Rails.logger.error "Evaluation error caused by callback '#{callback}'"
+          Rails.logger.error e.inspect
+        end
+      else
+        placeholder = original_string
       end
     }
 
     # translate
-    data[:string].gsub!( /i18n\((.+?)\)/ ) { |s|
-      string = $1
-      locale = data[:locale] || 'en'
-      s = Translation.translate( locale, string )
+    data[:string].gsub!( /i18n\((.+?)\)/ ) { |placeholder|
+      string      = $1
+      locale      = data[:locale] || 'en'
+      placeholder = Translation.translate( locale, string )
     }
 
     return data[:string]
@@ -49,9 +59,9 @@ module NotificationFactory
 
   def self.send(data)
     sender = Setting.get('notification_sender')
-    a = Channel::IMAP.new
+    imap   = Channel::IMAP.new
     Rails.logger.info "NOTICE: SEND NOTIFICATION TO: #{data[:recipient][:email]}"
-    message = a.send(
+    message = imap.send(
       {
 #        :in_reply_to => self.in_reply_to,
         :from       => sender,
diff --git a/test/unit/notifiation_factory_test.rb b/test/unit/notification_factory_test.rb
similarity index 91%
rename from test/unit/notifiation_factory_test.rb
rename to test/unit/notification_factory_test.rb
index 824d021de..2ea040f26 100644
--- a/test/unit/notifiation_factory_test.rb
+++ b/test/unit/notification_factory_test.rb
@@ -36,7 +36,7 @@ class NotificationFactoryTest < ActiveSupport::TestCase
       },
       {
         :locale => 'de',
-        :string => 'i18n(#{"New"}) some text',
+        :string => 'i18n(New) some text',
         :result => 'Neu some text',
       },
       {
@@ -44,6 +44,11 @@ class NotificationFactoryTest < ActiveSupport::TestCase
         :string => '\'i18n(#{ticket.state.name})\' ticket state',
         :result => '\'neu\' ticket state',
       },
+      {
+        :locale => 'de',
+        :string => '\#{puts `ls`}',
+        :result => '\#{puts `ls`}',
+      },
     ]
     tests.each { |test|
       result = NotificationFactory.build(