Port padrino helpers over to SafeBuffers

This patch ports all helpers over to use SafeBuffers. The changes
follow one general rule: all tag helpers like tag and content_tag
escape everything by default while block helpers like content_for
and form_for assume that the given content is already escaped.

Adds two new methods: safe_content_tag (which assumes that content
is already safe) and mark_safe, which marks a given String or Array
as safe.

Fixes test applications where necessary.

padrino-helpers/lib/padrino-helpers/form_builder/abstract_form_builder.rb

@@ -122,7 +122,7 @@ def radio_button_group(field, options={})
 
         # f.check_box :remember_me, :value => 'true', :uncheck_value => '0'
         def check_box(field, options={})
-          html =""
+          html = ActiveSupport::SafeBuffer.new
           unchecked_value = options.delete(:uncheck_value) || '0'
           options.reverse_merge!(:id => field_id(field), :value => '1')
           options.reverse_merge!(:checked => true) if values_matches_field?(field, options[:value])

padrino-helpers/lib/padrino-helpers/form_helpers.rb

@@ -54,7 +54,7 @@ def fields_for(object, settings={}, &block)
         instance = builder_instance(object, settings)
         fields_html = capture_html(instance, &block)
         fields_html << instance.hidden_field(:id) if instance.send(:nested_object_id)
-        concat_content fields_html
+        concat_safe_content fields_html
       end
 
       ##
@@ -81,7 +81,7 @@ def form_tag(url, options={}, &block)
         options['accept-charset'] ||= 'UTF-8'
         inner_form_html  = hidden_form_method_field(desired_method)
         inner_form_html += capture_html(&block)
-        concat_content content_tag(:form, inner_form_html, options)
+        concat_content content_tag(:form, mark_safe(inner_form_html), options)
       end
 
       ##
@@ -125,8 +125,8 @@ def hidden_form_method_field(desired_method)
       def field_set_tag(*args, &block)
         options = args.extract_options!
         legend_text = args[0].is_a?(String) ? args.first : nil
-        legend_html = legend_text.blank? ? '' : content_tag(:legend, legend_text)
-        field_set_content = legend_html + capture_html(&block)
+        legend_html = legend_text.blank? ? ActiveSupport::SafeBuffer.new : content_tag(:legend, legend_text)
+        field_set_content = legend_html + mark_safe(capture_html(&block))
         concat_content content_tag(:fieldset, field_set_content, options)
       end
 
@@ -199,10 +199,10 @@ def error_messages_for(*objects)
               }
             }.join
 
-            contents = ''
+            contents = ActiveSupport::SafeBuffer.new
             contents << content_tag(options[:header_tag] || :h2, header_message) unless header_message.blank?
             contents << content_tag(:p, message) unless message.blank?
-            contents << content_tag(:ul, error_messages)
+            contents << safe_content_tag(:ul, error_messages)
 
             content_tag(:div, contents, html)
           end
@@ -276,10 +276,11 @@ def error_message_on(object, field, options={})
       # @api public
       def label_tag(name, options={}, &block)
         options.reverse_merge!(:caption => "#{name.to_s.humanize}: ", :for => name)
-        caption_text = options.delete(:caption)
-        caption_text << "<span class='required'>*</span> " if options.delete(:required)
+        caption_text = options.delete(:caption).html_safe
+        caption_text.safe_concat "<span class='required'>*</span> " if options.delete(:required)
+
         if block_given? # label with inner content
-          label_content = caption_text + capture_html(&block)
+          label_content = caption_text.concat capture_html(&block)
           concat_content(content_tag(:label, label_content, options))
         else # regular label
           content_tag(:label, caption_text, options)
@@ -631,7 +632,7 @@ def select_tag(name, options={})
         end
         select_options_html = select_options_html.unshift(blank_option(prompt)) if select_options_html.is_a?(Array)
         options.merge!(:name => "#{options[:name]}[]") if options[:multiple]
-        content_tag(:select, select_options_html, options)
+        safe_content_tag(:select, select_options_html, options)
       end
 
       ##

padrino-helpers/lib/padrino-helpers/output_helpers.rb

@@ -26,6 +26,8 @@ def render(engine, *) # @private
       ##
       # Captures the html from a block of template code for any available handler.
       #
+      # Be aware that trusting the html is up to the caller.
+      #
       # @param [Object] *args
       #   Objects yield to the captured block
       # @param [Proc] &block
@@ -37,6 +39,12 @@ def render(engine, *) # @private
       #   capture_html(&block) => "...html..."
       #   capture_html(object_for_block, &block) => "...html..."
       #
+      # @example
+      #   ActiveSupport::SafeBuffer.new + capture_html { "<foo>" }
+      #   # => "&lt;foo&gt;"
+      #   ActiveSupport::SafeBuffer.safe_concat + capture_html { "<foo>" }
+      #   # => "<foo>"
+      #
       # @api semipublic
       def capture_html(*args, &block)
         handler = find_proper_handler
@@ -53,7 +61,9 @@ def capture_html(*args, &block)
       ##
       # Outputs the given text to the templates buffer directly.
       #
-      # @param [String] text
+      # The output might be subject to escaping, if it is not marked as safe.
+      #
+      # @param [String,SafeBuffer] text
       #   Text to concatenate to the buffer.
       #
       # @example
@@ -70,6 +80,21 @@ def concat_content(text="")
       end
       alias :concat :concat_content
 
+      ##
+      # Outputs the given text to the templates buffer directly,
+      # assuming that it is safe.
+      #
+      # @param [String] text
+      #   Text to concatenate to the buffer.
+      #
+      # @example
+      #   concat_safe_content("This will be output to the template buffer")
+      #
+      # @api semipublic
+      def concat_safe_content(text="")
+        concat_content text.html_safe
+      end
+
       ##
       # Returns true if the block is from a supported template type; false otherwise.
       # Used to determine if html should be returned or concatenated to the view.
@@ -146,7 +171,7 @@ def content_for?(key)
       def yield_content(key, *args)
         blocks = content_blocks[key.to_sym]
         return nil if blocks.empty?
-        blocks.map { |content| capture_html(*args, &content) }.join
+        mark_safe(blocks.map { |content| capture_html(*args, &content) }.join)
       end
 
       protected
@@ -170,6 +195,21 @@ def content_blocks
         def find_proper_handler
           OutputHelpers.handlers.map { |h| h.new(self) }.find { |h| h.engines.include?(current_engine) && h.is_type? }
         end
+
+        ##
+        # Marks a String or a collection of Strings as safe. `nil` is accepted
+        # but ignored.
+        #
+        # @param [String, Array<String>] the values to be marked safe.
+        #
+        # @return [ActiveSupport::SafeBuffer, Array<ActiveSupport::SafeBuffer>]
+        def mark_safe(value)
+          if value.respond_to? :map!
+            value.map!{|v| v.html_safe if v }
+          else
+            value.html_safe if value
+          end
+        end
     end # OutputHelpers
   end # Helpers
 end # Padrino

padrino-helpers/lib/padrino-helpers/output_helpers/erb_handler.rb

@@ -28,7 +28,7 @@ def is_type?
         #   @handler.capture_from_template(&block) => "...html..."
         #
         def capture_from_template(*args, &block)
-          self.output_buffer, _buf_was = "", self.output_buffer
+          self.output_buffer, _buf_was = ActiveSupport::SafeBuffer.new, self.output_buffer
           block.call(*args)
           ret = eval("@_out_buf", block.binding)
           self.output_buffer = _buf_was

padrino-helpers/lib/padrino-helpers/render_helpers.rb

@@ -46,12 +46,12 @@ def partial(template, options={})
             counter += 1
             options[:locals].merge!(object_name => member, "#{object_name}_counter".to_sym => counter)
             render(explicit_engine, template_path, options.dup)
-          }.join("\n")
+          }.join("\n").html_safe
         else
           if member = options.delete(:object)
             options[:locals].merge!(object_name => member)
           end
-          render(explicit_engine, template_path, options.dup)
+          render(explicit_engine, template_path, options.dup).html_safe
         end
       end
       alias :render_partial :partial

padrino-helpers/lib/padrino-helpers/tag_helpers.rb

@@ -42,6 +42,12 @@ module TagHelpers
         :confirm
       ]
 
+      ##
+      # A html_safe newline string to avoid allocating a new on each
+      # concatenation.
+      #
+      NEWLINE = "\n".html_safe.freeze
+
       ##
       # Creates an HTML tag with given name, content, and options
       #
@@ -104,14 +110,30 @@ def content_tag(name, content = nil, options = nil, &block)
           content = capture_html(&block)
         end
 
-        content = content.join("\n") if content.respond_to?(:join)
-
         options    = parse_data_options(name, options)
         attributes = tag_attributes(options)
-        output = "<#{name}#{attributes}>#{content}</#{name}>"
+        output = ActiveSupport::SafeBuffer.new
+        output.safe_concat "<#{name}#{attributes}>"
+        if content.respond_to? :each
+          content.each { |c| output.concat c; output.safe_concat NEWLINE }
+        else
+          output.concat content
+        end
+        output.safe_concat "</#{name}>"
+
         block_is_template?(block) ? concat_content(output) : output
       end
 
+      ##
+      # Like #content_tag, but assumes its input to be safe and doesn't
+      # escape. It also returns safe html.
+      #
+      # @see #content_tag
+      #
+      def safe_content_tag(name, content = nil, options = nil, &block)
+        mark_safe(content_tag(name, mark_safe(content), options, &block))
+      end
+
       ##
       # Creates an HTML input field with the given type and options
       #
@@ -200,7 +222,7 @@ def input_tag(type, options = {})
       def tag(name, options = nil, open = false)
         options    = parse_data_options(name, options)
         attributes = tag_attributes(options)
-        "<#{name}#{attributes}#{open ? '>' : ' />'}"
+        "<#{name}#{attributes}#{open ? '>' : ' />'}".html_safe
       end
 
       private

padrino-helpers/test/fixtures/markup_app/app.rb

@@ -2,12 +2,16 @@
 require 'haml'
 require 'erubis'
 require 'slim'
+require 'padrino-core/application/rendering/extensions/erubis'
+require 'padrino-core/application/rendering/extensions/haml'
 
 class MarkupDemo < Sinatra::Base
   register Padrino::Helpers
 
   configure do
     set :root, File.dirname(__FILE__)
+    set :erb, :engine_class => Padrino::Erubis::SafeBufferTemplate
+    set :haml, :escape_html => true
   end
 
   get '/:engine/:file' do
@@ -23,15 +27,15 @@ def show(kind, template)
 
     def captured_content(&block)
       content_html = capture_html(&block)
-      "<p>#{content_html}</p>"
+      "<p>#{content_html}</p>".html_safe
     end
 
     def concat_in_p(content_html)
-      concat_content "<p>#{content_html}</p>"
+      concat_safe_content "<p>#{content_html}</p>"
     end
 
     def determine_block_is_template(name, &block)
-      concat_content "<p class='is_template'>The #{name} block passed in is a template</p>" if block_is_template?(block)
+      concat_safe_content "<p class='is_template'>The #{name} block passed in is a template</p>" if block_is_template?(block)
     end
 
     def ruby_not_template_block

padrino-helpers/test/fixtures/markup_app/views/current_engine.haml

@@ -1,5 +1,5 @@
 %p.start= current_engine
 %p.haml= haml :'partials/_haml'
-%p.erb= erb  :'partials/_erb'
+%p.erb= erb :'partials/_erb'
 %p.slim= slim :'partials/_slim'
 %p.end= current_engine

padrino-helpers/test/fixtures/render_app/app.rb

@@ -16,6 +16,8 @@ class RenderDemo < Padrino::Application
   configure do
     set :logging, false
     set :padrino_logging, false
+    set :erb, :engine_class => Padrino::Erubis::SafeBufferTemplate
+    set :haml, :escape_html => true
   end
 
   # get current engines from partials

padrino-helpers/test/test_tag_helpers.rb

@@ -53,6 +53,17 @@ def app
       assert_has_tag('p.large#star', :content => "Demo") { actual_html }
     end
 
+    should "escape non-html-safe content" do
+      actual_html = content_tag(:p, :class => 'large', :id => 'star') { "<>" }
+      assert_has_tag('p.large#star') { actual_html }
+      assert_match('&lt;&gt;', actual_html)
+    end
+
+    should "not escape html-safe content" do
+      actual_html = content_tag(:p, :class => 'large', :id => 'star') { "<>" }
+      assert_has_tag('p.large#star', :content => "<>") { actual_html }
+    end
+
     should "support tags with erb" do
       visit '/erb/content_tag'
       assert_have_selector :p, :content => "Test 1", :class => 'test', :id => 'test1'