Merge pull request #182 from mallowlabs/id/182

Fixed: Miss escaping when message body contains ' with as_redmine_ticket_link_filter

config/initializers/ruby_patch.rb

@@ -6,12 +6,3 @@ class Array
   end
 end
 
-class CGI
-  class << self
-    alias_method :orig_escapeHTML, :escapeHTML
-    def escapeHTML(str)
-      orig_escapeHTML(str).gsub("'", "&#39;")
-    end
-  end
-end if RUBY_VERSION < '2.0.0'
-

lib/asakusa_satellite/filter.rb

@@ -33,6 +33,11 @@ def children(doc, &f)
     end
     private :children
 
+    def escapeText(str)
+      REXML::Text::normalize(str || '')
+    end
+    private :escapeText
+
     def process(message, room)
       all_process ||= @filter_config.filters.map{|c|
         @plugins.find{|p|
@@ -42,8 +47,8 @@ def process(message, room)
 
       # process order
       # 1. process_all for all lines
-      raw_lines = CGI.escapeHTML(message.body || "").split("\n")
-      lines = all_process.reduce(raw_lines) do| lines, process|
+      raw_lines = escapeText(message.body).split("\n")
+      lines = all_process.reduce(raw_lines) do |lines, process|
         if process.respond_to? :process_all
           process.process_all(lines, :message => message, :room => room)
         else
@@ -53,11 +58,11 @@ def process(message, room)
 
       # 2. process for each text node
       body = lines.to_a.join("<br />")
-      doc  = all_process.reduce(REXML::Document.new "<as>#{body}</as>") do|doc, process|
+      doc  = all_process.reduce(REXML::Document.new "<as>#{body}</as>") do |doc, process|
         if process.respond_to? :process
-          doc.each_element('/as/text()').each do|node|
+          doc.each_element('/as/text()').each do |node|
             s = process.process(node.to_s, :message => message, :room => room)
-            children(REXML::Document.new("<as>#{s}</as>")).each do|x|
+            children(REXML::Document.new("<as>#{s}</as>")).each do |x|
               node.parent.insert_before node, x
             end
             node.remove
@@ -68,16 +73,16 @@ def process(message, room)
 
       # hack for some browser.
       # Convert <iframe /> to <iframe></iframe>
-      %w(iframe script div).each do|name|
-        doc.each_element("//#{name}") do|node|
+      %w(iframe script div).each do |name|
+        doc.each_element("//#{name}") do |node|
           node << REXML::Text.new('')
         end
       end
 
       children(doc).join
     rescue => e
       Rails.logger.error e
-      message.body
+      escapeText(message.body)
     end
 
     def add_filter(klass, config)
@@ -88,6 +93,6 @@ def [](name)
       @filter_config.filters.find { |c| c['name'] == name }
     end
 
-    module_function :initialize!, :process, :add_filter, :[], :children
+    module_function :initialize!, :process, :add_filter, :[], :children, :escapeText
   end
 end

plugins/as_code_highlight_filter/lib/code_highlight_filter.rb

@@ -3,7 +3,7 @@ class CodeHighlightFilter < AsakusaSatellite::Filter::Base
 
   def process_all(lines, opts={})
     lang,*body = lines
-    content = CGI.unescapeHTML(body.join("\n"))
+    content = REXML::Text::unnormalize(body.join("\n"))
     case lang
     when "graphviz::","graph::"
       %(<img class="graphviz" src="http://chart.googleapis.com/chart?cht=gv&amp;chl=#{CGI.escape content}" />)

plugins/as_redmine_ticket_link_filter/lib/redmine_ticket_link_filter.rb

@@ -7,6 +7,7 @@ class AsakusaSatellite::Filter::RedmineTicketLinkFilter < AsakusaSatellite::Filt
   def process(line, opts={})
     room = opts[:room]
     info = room.yaml[:redmine_ticket]
+    return line if info.blank?
     line.gsub(/#(\d+)/) {|id|
       ticket $1, id, info
     }
@@ -24,7 +25,7 @@ def ticket(id, ref, info)
       begin
         open(api.to_s) do|io|
           hash = JSON.parse(io.read)
-          subject = CGI::escapeHTML hash["issue"]["subject"]
+          subject = REXML::Text::normalize hash["issue"]["subject"]
           if subject.respond_to? :force_encoding
             subject.force_encoding 'utf-8'
           end

spec/lib/asakusa_satellite/filter_spec.rb

@@ -12,6 +12,12 @@ def process(text, opts={})
   end
 end
 
+class NumberFilter < AsakusaSatellite::Filter::Base
+  def process(text, opts={})
+    text.gsub('#','<strong>#</strong>')
+  end
+end
+
 describe AsakusaSatellite::Filter do
   make = lambda do|text|
     @room    = Room.new
@@ -38,13 +44,13 @@ def process(text, opts={})
 
     describe 'apos string' do
       subject { make["'"] }
-      it { should == "&#39;" }
+      it { should == "&apos;" }
     end
   end
 
   describe "passing tags" do
     before do
-      CGI.stub(:escapeHTML){|x| x }
+      AsakusaSatellite::Filter.stub(:escapeText){|x| x }
     end
 
     describe 'filter text' do
@@ -99,13 +105,37 @@ def process(text, opts={})
   end
 
   describe 'return "as is" when error occured' do
+    context "without tags" do
+      before do
+        filter_config = AsakusaSatellite::Filter::FilterConfig.new([{'name' => 'error_filter'}])
+        AsakusaSatellite::Filter.initialize!(filter_config)
+        AsakusaSatellite::Filter.add_filter ErrorFilter,{}
+      end
+      subject { AsakusaSatellite::Filter.process(Message.new(:body => 'text'), nil) }
+
+      it { should == 'text' }
+    end
+    context "with tags" do
+      before do
+        filter_config = AsakusaSatellite::Filter::FilterConfig.new([{'name' => 'error_filter'}])
+        AsakusaSatellite::Filter.initialize!(filter_config)
+        AsakusaSatellite::Filter.add_filter ErrorFilter,{}
+      end
+      subject { AsakusaSatellite::Filter.process(Message.new(:body => '<script />'), nil) }
+
+      it { should == '&lt;script /&gt;' }
+    end
+  end
+
+  describe 'valid escaping' do
     before do
-      AsakusaSatellite::Filter.initialize!([{'name' => 'error_filter'}])
-      AsakusaSatellite::Filter.add_filter ErrorFilter,{}
+      filter_config = AsakusaSatellite::Filter::FilterConfig.new([{'name' => 'number_filter'}])
+      AsakusaSatellite::Filter.initialize!(filter_config)
+      AsakusaSatellite::Filter.add_filter NumberFilter,{}
     end
-    subject { AsakusaSatellite::Filter.process(Message.new(:body => 'text'), nil) }
+    subject { AsakusaSatellite::Filter.process(Message.new(:body => "'#"), nil) }
 
-    it { should == 'text' }
+    it { should == '&apos;<strong>#</strong>' }
   end
 
 end