Filter arbitrary class names and ids in rendered HTML output (#25503).

* Disallow setting arbitrary classes and ids via Textile syntax
* Only allow valid/supported languages for syntax highlighted code blocks

Patch by Jan Schulz-Hofen.

git-svn-id: http://svn.redmine.org/redmine/trunk@16502 e93f8b46-1217-0410-a6f0-8f06a7374b81

6 files changed, 80 insertions, 14 deletions

diff --git a/lib/redmine/wiki_formatting/markdown/formatter.rb b/lib/redmine/wiki_formatting/markdown/formatter.rb
index 4afbc2fdd..bfb04774c 100644
--- a/lib/redmine/wiki_formatting/markdown/formatter.rb
+++ b/lib/redmine/wiki_formatting/markdown/formatter.rb
@@ -35,7 +35,7 @@ module Redmine
         end
 
         def block_code(code, language)
-          if language.present?
+          if language.present? && Redmine::SyntaxHighlighting.language_supported?(language)
             "<pre><code class=\"#{CGI.escapeHTML language} syntaxhl\">" +
               Redmine::SyntaxHighlighting.highlight_by_language(code, language) +
               "</code></pre>"
diff --git a/lib/redmine/wiki_formatting/textile/formatter.rb b/lib/redmine/wiki_formatting/textile/formatter.rb
index 5862a1c62..8ff623a73 100644
--- a/lib/redmine/wiki_formatting/textile/formatter.rb
+++ b/lib/redmine/wiki_formatting/textile/formatter.rb
@@ -121,8 +121,14 @@ module Redmine
             text.gsub!(/<redpre#(\d+)>/) do
               content = @pre_list[$1.to_i]
               if content.match(/<code\s+class="(\w+)">\s?(.+)/m)
-                content = "<code class=\"#{$1} syntaxhl\">" +
-                  Redmine::SyntaxHighlighting.highlight_by_language($2, $1)
+                language = $1
+                text = $2
+                if Redmine::SyntaxHighlighting.language_supported?(language)
+                  content = "<code class=\"#{language} syntaxhl\">" +
+                    Redmine::SyntaxHighlighting.highlight_by_language(text, language)
+                else
+                  content = "<code>#{ERB::Util.h(text)}"
+                end
               end
               content
             end
diff --git a/lib/redmine/wiki_formatting/textile/redcloth3.rb b/lib/redmine/wiki_formatting/textile/redcloth3.rb
index bcb796ec6..d0bd217d3 100644
--- a/lib/redmine/wiki_formatting/textile/redcloth3.rb
+++ b/lib/redmine/wiki_formatting/textile/redcloth3.rb
@@ -494,7 +494,15 @@ class RedCloth3 < String
         style << "text-align:#{ h_align( $& ) };" if text =~ A_HLGN
 
         cls, id = $1, $2 if cls =~ /^(.*?)#(.*)$/
-        
+
+        # add wiki-class- and wiki-id- to classes and ids to prevent setting of
+        # arbitrary classes and ids
+        cls = cls.split(/\s+/).map do |c|
+          c.starts_with?('wiki-class-') ? c : "wiki-class-#{c}"
+        end.join(' ') if cls
+
+        id = id.starts_with?('wiki-id-') ? id : "wiki-id-#{id}" if id
+
         atts = ''
         atts << " style=\"#{ style.join }\"" unless style.empty?
         atts << " class=\"#{ cls }\"" unless cls.to_s.empty?
@@ -1097,7 +1105,7 @@ class RedCloth3 < String
                         first.match(/<#{ OFFTAGS }([^>]*)>/)
                         tag = $1
                         $2.to_s.match(/(class\=("[^"]+"|'[^']+'))/i)
-                        tag << " #{$1}" if $1
+                        tag << " #{$1}" if $1 && tag == 'code'
                         @pre_list << "<#{ tag }>#{ aftertag }"
                     end
                 elsif $1 and codepre > 0
diff --git a/test/unit/helpers/application_helper_test.rb b/test/unit/helpers/application_helper_test.rb
index a0d88eb5d..500fbd86e 100644
--- a/test/unit/helpers/application_helper_test.rb
+++ b/test/unit/helpers/application_helper_test.rb
@@ -117,7 +117,8 @@ class ApplicationHelperTest < Redmine::HelperTest
     to_test = {
       '!http://foo.bar/image.jpg!' => '<img src="http://foo.bar/image.jpg" alt="" />',
       'floating !>http://foo.bar/image.jpg!' => 'floating <span style="float:right"><img src="http://foo.bar/image.jpg" alt="" /></span>',
-      'with class !(some-class)http://foo.bar/image.jpg!' => 'with class <img src="http://foo.bar/image.jpg" class="some-class" alt="" />',
+      'with class !(some-class)http://foo.bar/image.jpg!' => 'with class <img src="http://foo.bar/image.jpg" class="wiki-class-some-class" alt="" />',
+      'with class !(wiki-class-foo)http://foo.bar/image.jpg!' => 'with class <img src="http://foo.bar/image.jpg" class="wiki-class-foo" alt="" />',
       'with style !{width:100px;height:100px}http://foo.bar/image.jpg!' => 'with style <img src="http://foo.bar/image.jpg" style="width:100px;height:100px;" alt="" />',
       'with title !http://foo.bar/image.jpg(This is a title)!' => 'with title <img src="http://foo.bar/image.jpg" title="This is a title" alt="This is a title" />',
       'with title !http://foo.bar/image.jpg(This is a double-quoted "title")!' => 'with title <img src="http://foo.bar/image.jpg" title="This is a double-quoted &quot;title&quot;" alt="This is a double-quoted &quot;title&quot;" />',
@@ -911,11 +912,11 @@ RAW
       "<pre><div>content</div></pre>" => "<pre>&lt;div&gt;content&lt;/div&gt;</pre>",
       "HTML comment: <!-- no comments -->" => "<p>HTML comment: &lt;!-- no comments --&gt;</p>",
       "<!-- opening comment" => "<p>&lt;!-- opening comment</p>",
-      # remove attributes except class
-      "<pre class='foo'>some text</pre>" => "<pre class='foo'>some text</pre>",
-      '<pre class="foo">some text</pre>' => '<pre class="foo">some text</pre>',
-      "<pre class='foo bar'>some text</pre>" => "<pre class='foo bar'>some text</pre>",
-      '<pre class="foo bar">some text</pre>' => '<pre class="foo bar">some text</pre>',
+      # remove attributes including class
+      "<pre class='foo'>some text</pre>" => "<pre>some text</pre>",
+      '<pre class="foo">some text</pre>' => '<pre>some text</pre>',
+      "<pre class='foo bar'>some text</pre>" => "<pre>some text</pre>",
+      '<pre class="foo bar">some text</pre>' => '<pre>some text</pre>',
       "<pre onmouseover='alert(1)'>some text</pre>" => "<pre>some text</pre>",
       # xss
       '<pre><code class=""onmouseover="alert(1)">text</code></pre>' => '<pre><code>text</code></pre>',
diff --git a/test/unit/lib/redmine/wiki_formatting/markdown_formatter_test.rb b/test/unit/lib/redmine/wiki_formatting/markdown_formatter_test.rb
index b8aa37ff2..77dd93c7d 100644
--- a/test/unit/lib/redmine/wiki_formatting/markdown_formatter_test.rb
+++ b/test/unit/lib/redmine/wiki_formatting/markdown_formatter_test.rb
@@ -70,6 +70,15 @@ STR
     end
   end
 
+  def test_should_not_allow_invalid_language_for_code_blocks
+    text = <<-STR
+~~~foo
+test
+~~~
+STR
+    assert_equal "<pre>test\n</pre>", @formatter.new(text).to_html
+  end
+
   def test_external_links_should_have_external_css_class
     text = 'This is a [link](http://example.net/)'
     assert_equal '<p>This is a <a href="http://example.net/" class="external">link</a></p>', @formatter.new(text).to_html.strip
diff --git a/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb b/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb
index 548a41de3..7e5ef8cb2 100644
--- a/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb
+++ b/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb
@@ -44,9 +44,7 @@ class Redmine::WikiFormatting::TextileFormatterTest < ActionView::TestCase
       '*two*words*'           => '<strong>two*words</strong>',
       '*two * words*'         => '<strong>two * words</strong>',
       '*two* *words*'         => '<strong>two</strong> <strong>words</strong>',
-      '*(two)* *(words)*'     => '<strong>(two)</strong> <strong>(words)</strong>',
-      # with class
-      '*(foo)two words*'      => '<strong class="foo">two words</strong>'
+      '*(two)* *(words)*'     => '<strong>(two)</strong> <strong>(words)</strong>'
     )
   end
 
@@ -535,6 +533,50 @@ STR
     assert_match /\Ah1.\tHeading 1\s+Content 1\z/, @formatter.new(text).get_section(1).first
   end
 
+  def test_should_not_allow_arbitrary_class_attribute_on_offtags
+    %w(code pre kbd).each do |tag|
+      assert_html_output({"<#{tag} class=\"foo\">test</#{tag}>" => "<#{tag}>test</#{tag}>"}, false)
+    end
+
+    assert_html_output({"<notextile class=\"foo\">test</notextile>" => "test"}, false)
+  end
+
+  def test_should_allow_valid_language_class_attribute_on_code_tags
+    assert_html_output({"<code class=\"ruby\">test</code>" => "<code class=\"ruby syntaxhl\"><span class=\"CodeRay\">test</span></code>"}, false)
+  end
+
+  def test_should_not_allow_valid_language_class_attribute_on_non_code_offtags
+    %w(pre kbd).each do |tag|
+      assert_html_output({"<#{tag} class=\"ruby\">test</#{tag}>" => "<#{tag}>test</#{tag}>"}, false)
+    end
+
+    assert_html_output({"<notextile class=\"ruby\">test</notextile>" => "test"}, false)
+  end
+
+  def test_should_prefix_class_attribute_on_tags
+    assert_html_output({
+      '!(foo)test.png!' => "<p><img src=\"test.png\" class=\"wiki-class-foo\" alt=\"\" /></p>",
+      '%(foo)test%'     => "<p><span class=\"wiki-class-foo\">test</span></p>",
+      'p(foo). test'    => "<p class=\"wiki-class-foo\">test</p>",
+      '|(foo). test|'   => "<table>\n\t\t<tr>\n\t\t\t<td class=\"wiki-class-foo\">test</td>\n\t\t</tr>\n\t</table>",
+    }, false)
+  end
+
+  def test_should_prefix_id_attribute_on_tags
+    assert_html_output({
+      '!(#foo)test.png!' => "<p><img src=\"test.png\" id=\"wiki-id-foo\" alt=\"\" /></p>",
+      '%(#foo)test%'     => "<p><span id=\"wiki-id-foo\">test</span></p>",
+      'p(#foo). test'    => "<p id=\"wiki-id-foo\">test</p>",
+      '|(#foo). test|'   => "<table>\n\t\t<tr>\n\t\t\t<td id=\"wiki-id-foo\">test</td>\n\t\t</tr>\n\t</table>",
+    }, false)
+  end
+
+  def test_should_not_prefix_class_and_id_attributes_already_prefixed
+    assert_html_output({
+      '!(wiki-class-foo#wiki-id-bar)test.png!' => "<p><img src=\"test.png\" class=\"wiki-class-foo\" id=\"wiki-id-bar\" alt=\"\" /></p>",
+    }, false)
+  end
+
   private
 
   def assert_html_output(to_test, expect_paragraph = true)