- 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

@@ -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,

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(