Hobo Hobo → Hobo 1.3 (Rails 3)

Comments and changes to this ticket

• 

  Domizio Demichelis October 31st, 2010 @ 03:40 PM

  • State changed from “new” to “investigating”
  • Milestone set to “Hobo 1.3 (Rails 3)”
  • Assigned user set to “Domizio Demichelis”
  • Milestone order changed from “197913” to “0”

  To me it seems that the real bug is that the string type doesn't get escaped, not the other way around. How do you know that a string or a text from the db is safe? Indeed it is not, so it should be escaped, specially because you are using string or text, which shouldn't contain html entities.

  I think that we should fix the string type because that should be escaped, and leave the text type as it is. If you want raw html, you should use the raw_html_string type (which I don't know whether is correctly rendered or not).

• 

  Domizio Demichelis October 31st, 2010 @ 05:28 PM

  Thank you, now it's clear.

  IMHO the to_html methods should escape the input (when needed) and always return html_safe strings, while the view should always escape this (without any condition), so the code will be simpler. Anyway if this is already html_safe it will not be escaped twice.

• 

  Domizio Demichelis October 31st, 2010 @ 06:35 PM

  • State changed from “investigating” to “open”

  I have reviewed the hobo_fields/types, but I have no time to test it, it whould be very helpful if you could try this patch:

  diff --git a/hobo_fields/lib/hobo_fields/types/email_address.rb b/hobo_fields/lib/hobo_fields/types/email_address.rb
  index c32255f..9e8adf0 100644
  --- a/hobo_fields/lib/hobo_fields/types/email_address.rb
  +++ b/hobo_fields/lib/hobo_fields/types/email_address.rb
  @@ -1,3 +1,4 @@
  +require 'active_support/core_ext/string/output_safety'
   module HoboFields
     module Types
       class EmailAddress < String
  @@ -13,7 +14,7 @@ module HoboFields
         end
   
         def to_html(xmldoctype = true)
  -        self.sub('@', " at ").gsub('.', ' dot ')
  +        ERB::Util.html_escape(self).sub('@', " at ").gsub('.', ' dot ').html_safe
         end
   
         HoboFields.register_type(:email_address, self)
  diff --git a/hobo_fields/lib/hobo_fields/types/raw_html_string.rb b/hobo_fields/lib/hobo_fields/types/raw_html_string.rb
  index 0196215..faa8aa4 100644
  --- a/hobo_fields/lib/hobo_fields/types/raw_html_string.rb
  +++ b/hobo_fields/lib/hobo_fields/types/raw_html_string.rb
  @@ -3,7 +3,7 @@ module HoboFields
       class RawHtmlString < HoboFields::Types::Text
   
         def to_html(xmldoctype = true)
  -        self
  +        self.html_safe
         end
   
         HoboFields.register_type(:raw_html, self)
  diff --git a/hobo_fields/lib/hobo_fields/types/raw_markdown_string.rb b/hobo_fields/lib/hobo_fields/types/raw_markdown_string.rb
  index b5f0163..06354d1 100644
  --- a/hobo_fields/lib/hobo_fields/types/raw_markdown_string.rb
  +++ b/hobo_fields/lib/hobo_fields/types/raw_markdown_string.rb
  @@ -5,7 +5,7 @@ module HoboFields
         HoboFields.register_type(:raw_markdown, self)
   
         def to_html(xmldoctype = true)
  -        blank? ? "" : Markdown.new(self).to_html
  +        blank? ? "" : Markdown.new(self).to_html.html_safe
         end
   
       end
  diff --git a/hobo_fields/lib/hobo_fields/types/text.rb b/hobo_fields/lib/hobo_fields/types/text.rb
  index 57a2aea..65f7cb4 100644
  --- a/hobo_fields/lib/hobo_fields/types/text.rb
  +++ b/hobo_fields/lib/hobo_fields/types/text.rb
  @@ -1,13 +1,12 @@
  +require 'active_support/core_ext/string/output_safety'
   module HoboFields
     module Types
       class Text < String
   
  -      HTML_ESCAPE = { '&' => '&amp;', '"' => '&quot;', '>' => '&gt;', '<' => '&lt;' }
  -
         COLUMN_TYPE = :text
   
         def to_html(xmldoctype = true)
  -        gsub(/[&"><]/) { |special| HTML_ESCAPE[special] }.gsub("\n", "<br#{xmldoctype ? ' /' : ''}>\n")
  +        ERB::Util.html_escape(self).gsub("\n", "<br#{xmldoctype ? ' /' : ''}>\n").html_safe
         end
   
         HoboFields.register_type(:text, self)
  diff --git a/hobo_fields/lib/hobo_fields/types/textile_string.rb b/hobo_fields/lib/hobo_fields/types/textile_string.rb
  index e251327..2f8edc5 100644
  --- a/hobo_fields/lib/hobo_fields/types/textile_string.rb
  +++ b/hobo_fields/lib/hobo_fields/types/textile_string.rb
  @@ -12,7 +12,7 @@ module HoboFields
           else
             textilized = RedCloth.new(self, [ :hard_breaks ])
             textilized.hard_breaks = true if textilized.respond_to?("hard_breaks=")
  -          textilized.to_html
  +          HoboFields::SanitizeHtml.sanitize(textilized.to_html)
           end
         end
  

  and if you have the time, please try the other types as well.

  Thank you.

• 

  Domizio Demichelis November 2nd, 2010 @ 01:45 AM

  Thank you for your suggestion and for your test cases: they have been very useful.

  Anyway about the logic of sanitizing/escaping/marking-html_safe field types, here are my conclusions:

  • The to_html method of a field type must always return a html_safe? string. Depending on the type itself and its common use, the result will be one of the following:

  • escaped string (html tags get escaped as html entities)

  • sanitized string (unsafe html tags get filtered out)
  • raw string (no changes)

  In details:

  Known HTML content fields:

  • HtmlString > sanitized
  • RawHtmlString > raw
  • MarkdownString > sanitized
  • RawMarkdownString > raw
  • TextileString > sanitized

  Non HTML content fields:

  • Text > escaped and "\n" translated to "<br/>"
  • EmailAddress > escaped and formatted as a "human-only" readable address
  • PasswordString > literal "[password hidden]" I18n translated

  Internal set content fields:

  • EnumString > raw and I18n translated
  • LifecycleState > raw and I18n translated

  I changed your tests accordingly.

• 

  Domizio Demichelis November 2nd, 2010 @ 01:48 AM

  • State changed from “open” to “resolved”

  (from [30006b63956b5bd2600facb86b3a23c255cce107]) xss: the to_htm methods of hobo_fields/types return safe html [#838 state:resolved] http://github.com/tablatom/hobo/commit/30006b63956b5bd2600facb86b3a...

• 

  Domizio Demichelis November 2nd, 2010 @ 10:33 PM

  Actually I had no intention to make it "1.1 compatible"... TBH it happened just because I didn't know about TranslationHelper#translate :-)

  I will investigate the possibility to use it framework-wide. Thank you for the comment.

• 

  Bryan Larsen July 12th, 2011 @ 05:05 PM

  Please try RC1, which I believe fixes your issue, Sebastian.

• 

  Bryan Larsen July 14th, 2011 @ 11:55 AM

  Sorry, I thought RC1 included https://github.com/tablatom/hobo/commit/b46ec8dfc12affac5d709ab458d...

  Please try the version direct from github, gem "hobo", :git => "git://github.com/tablatom/hobo.git", :branch => "rails3"