From 5b7b9f40ae2e0b656f0a5fdbd8106370a233ebc9 Mon Sep 17 00:00:00 2001
From: benjamin wil <bw@benjaminwil.info>
Date: Sat, 13 Feb 2021 16:04:42 -0800
Subject: [PATCH] Only escape HTML from breadcrumbs. Resolves #1658 (#1663)

---
 lib/gollum/views/overview.rb |  4 +-
 lib/gollum/views/page.rb     | 13 +++---
 test/test_overview_view.rb   | 31 ++++++++++--
 test/test_page_view.rb       | 91 +++++++++++++++++++++++++++---------
 4 files changed, 103 insertions(+), 36 deletions(-)

diff --git a/lib/gollum/views/overview.rb b/lib/gollum/views/overview.rb
index 87c8ced4..0c651296 100644
--- a/lib/gollum/views/overview.rb
+++ b/lib/gollum/views/overview.rb
@@ -26,9 +26,9 @@ module Precious
             title = crumb.basename
 
             if title == path.basename
-              breadcrumb << %{<li class="breadcrumb-item" aria-current="page">#{CGI.escape(title.to_s)}</li>}
+              breadcrumb << %{<li class="breadcrumb-item" aria-current="page">#{CGI.escapeHTML(title.to_s)}</li>}
             else
-              breadcrumb << %{<li class="breadcrumb-item"><a href="#{overview_path}/#{crumb}/">#{CGI.escape(title.to_s)}</a></li>}
+              breadcrumb << %{<li class="breadcrumb-item"><a href="#{overview_path}/#{crumb}/">#{CGI.escapeHTML(title.to_s)}</a></li>}
             end
           end
           breadcrumb << %{</ol></nav>}
diff --git a/lib/gollum/views/page.rb b/lib/gollum/views/page.rb
index 98c21f6b..65dc3bb7 100644
--- a/lib/gollum/views/page.rb
+++ b/lib/gollum/views/page.rb
@@ -16,15 +16,10 @@ module Precious
       DEFAULT_AUTHOR = 'you'
       @@to_xml       = { :save_with => Nokogiri::XML::Node::SaveOptions::DEFAULT_XHTML ^ 1, :indent => 0, :encoding => 'UTF-8' }
 
-      def title
-        h1 = @h1_title ? page_header_from_content(@content) : false
-        h1 || @page.url_path_title # url_path_title is the metadata title if present, otherwise the filename-based title
-      end
-
       def page_header
         title
       end
-            
+
       def breadcrumb
         path = Pathname.new(@page.url_path).parent
         return '' if path.to_s == '.'
@@ -32,7 +27,7 @@ module Precious
         path.descend do |crumb|
           element = "#{crumb.basename}"
           next if element == @page.title
-          breadcrumb << %{<li class="breadcrumb-item"><a href="#{overview_path}/#{crumb}/">#{CGI.escape(element.to_s)}</a></li>}
+          breadcrumb << %{<li class="breadcrumb-item"><a href="#{overview_path}/#{crumb}/">#{CGI.escapeHTML(element.to_s)}</a></li>}
         end
         breadcrumb << %{</ol></nav>}
         breadcrumb.join("\n")
@@ -269,6 +264,10 @@ module Precious
         result << "</tr>\n</table>\n"
       end
 
+      def title
+        h1 = @h1_title ? page_header_from_content(@content) : false
+        h1 || @page.url_path_title # url_path_title is the metadata title if present, otherwise the filename-based title
+      end
     end
   end
 end
diff --git a/test/test_overview_view.rb b/test/test_overview_view.rb
index a0956fef..4189ed4c 100644
--- a/test/test_overview_view.rb
+++ b/test/test_overview_view.rb
@@ -43,12 +43,22 @@ context "Precious::Views::Overview" do
     @page.instance_variable_set("@base_url", "")
     assert_equal "<nav aria-label=\"Breadcrumb\"><ol><li class=\"breadcrumb-item\"><a href=\"/gollum/overview\">Home</a></li>\n<li class=\"breadcrumb-item\"><a href=\"/gollum/overview/Mordor/\">Mordor</a></li>\n<li class=\"breadcrumb-item\"><a href=\"/gollum/overview/Mordor/Eye-Of-Sauron/\">Eye-Of-Sauron</a></li>\n<li class=\"breadcrumb-item\" aria-current=\"page\">Saruman</li>\n</ol></nav>", @page.breadcrumb
   end
-  
-  test 'guard against malicious filenames' do
-    malicious_title = '<img src=x onerror=alert(1) />'
-    @page.instance_variable_set("@path", malicious_title)
+
+  test "breadcrumbs guard against malicious filenames" do
+    malicious_path = '<script>alert("malicious-content");/Very Bad'
+    @page.instance_variable_set("@path", malicious_path)
     @page.instance_variable_set("@base_url", "")
-    assert @page.breadcrumb.include?(">%3Cimg+src%3Dx+onerror%3Dalert%281%29+</a>")
+
+    refute_includes @page.breadcrumb, malicious_path
+    assert_includes @page.breadcrumb, ">&lt;script&gt;alert(&quot;malicious-content&quot;);</a>"
+  end
+
+  test "breadcrumbs retain unicode and ASCII characters" do
+    title = "数学 📘"
+    @page.instance_variable_set("@path", title)
+    @page.instance_variable_set("@base_url", "")
+
+    assert_includes @page.breadcrumb, title
   end
 
   test "breadcrumb with no path" do
@@ -79,6 +89,17 @@ context "Precious::Views::Overview" do
     assert_equal result[:name], 'Orc'
   end
 
+  test "files_folders retain unicode and ASCII characters" do
+    @page.instance_variable_set("@path", "Mordor")
+    @page.instance_variable_set("@base_url", "")
+    @page.instance_variable_set("@results", [
+      FakePageResult.new("Mordor/Eye-Of-Sauron-👁️-数学.md")
+    ])
+    result = @page.files_folders.first
+
+    assert result[:name], "Eye Of Sauron 👁️ 数学"
+  end
+
   test "base url" do
     # based on test "files_folders"
     @page.instance_variable_set("@path", "Mordor")
diff --git a/test/test_page_view.rb b/test/test_page_view.rb
index 7a299e19..2e1119e7 100644
--- a/test/test_page_view.rb
+++ b/test/test_page_view.rb
@@ -13,15 +13,41 @@ context "Precious::Views::Page" do
     FileUtils.rm_rf(@path)
   end
 
-  test 'guard against malicious filenames' do
-    malicious_title = '<img src=x onerror=alert(1) />'
-    @wiki.write_page(malicious_title, :markdown, 'Is Bilbo a hobbit? Why certainly!')
-    page = @wiki.page(malicious_title)
+  test "breadcrumbs guard against malicious input" do
+    malicious_path = '<script>alert("malicious-content");/Very Bad'
+    @wiki.write_page(malicious_path, :markdown, 'Is Bilbo a hobbit? Why certainly!')
+    page = @wiki.page(malicious_path)
     @view = Precious::Views::Page.new
     @view.instance_variable_set :@page, page
     @view.instance_variable_set :@content, page.formatted_data
     @view.instance_variable_set :@h1_title, false
-    assert @view.breadcrumb.include?(">%3Cimg+src%3Dx+onerror%3Dalert%281%29+</a>")
+
+    refute_includes @view.breadcrumb, malicious_path
+    assert_includes @view.breadcrumb, ">&lt;script&gt;alert(&quot;malicious-content&quot;);</a>"
+  end
+
+  test "breadcrumbs retain unicode and ASCII characters" do
+    path = "数学 📘/Age of Bilbo"
+    @wiki.write_page(path, :markdown, "How old is Bilbo?")
+    page = @wiki.page(path)
+    @view = Precious::Views::Page.new
+    @view.instance_variable_set :@page, page
+    @view.instance_variable_set :@content, page.formatted_data
+    @view.instance_variable_set :@h1_title, false
+
+    assert_include @view.breadcrumb, "数学 📘"
+  end
+
+  test "page header retains unicde and ASCII characters" do
+    title = "数学 📘"
+    @wiki.write_page(title, :markdown, "How old is Bilbo?")
+    page = @wiki.page(title)
+    @view = Precious::Views::Page.new
+    @view.instance_variable_set :@page, page
+    @view.instance_variable_set :@content, page.formatted_data
+    @view.instance_variable_set :@h1_title, false
+
+    assert @view.page_header, "数学 📘"
   end
 
   test "h1 title sanitizes correctly" do
@@ -35,10 +61,46 @@ context "Precious::Views::Page" do
     @view.instance_variable_set :@h1_title, true
 
     # Test page_header_from_content(@content)
-    actual = @view.title
-    assert_equal '1 & 2', actual
+    assert @view.page_header, "1 & 2"
   end
 
+  test "page header uses filename when h1_title is false" do
+    title = "H1"
+    contents = <<~TEXT
+      # First H1 header
+      # Second H1 header
+    TEXT
+
+    @wiki.write_page(title, :markdown, contents, commit_details)
+    page = @wiki.page(title)
+
+    @view = Precious::Views::Page.new
+    @view.instance_variable_set :@page, page
+    @view.instance_variable_set :@content, page.formatted_data
+    @view.instance_variable_set :@h1_title, false
+
+    assert_equal @view.page_header, "H1"
+  end
+
+  test "page header uses filename when h1_title is true" do
+    contents = <<~TEXT
+      # First H1 header
+      # Second H1 header
+    TEXT
+
+    @wiki.write_page("H1", :markdown, contents, commit_details)
+    page = @wiki.page("H1")
+
+    @view = Precious::Views::Page.new
+    @view.instance_variable_set :@page, page
+    @view.instance_variable_set :@content, page.formatted_data
+    @view.instance_variable_set :@h1_title, true
+
+    assert_equal @view.page_header, "First H1 header"
+  end
+
+
+
   test "metadata is rendered into a table" do
     title = 'metadata test'
     @wiki.write_page(title, :markdown, "---\nsome: metadata\nhere: for you\n---\n# Some markdown\nIn this doc")
@@ -104,21 +166,6 @@ EOS
     assert_equal "594e928cc5dcb6d833dfb86bb36076fd4a84eea7", @view.id
   end
 
-  test "h1 title can be disabled" do
-    title = 'H1'
-    @wiki.write_page(title, :markdown, '# 1 & 2 <script>alert("js")</script>' + "\n # 3", commit_details)
-    page = @wiki.page(title)
-
-    @view = Precious::Views::Page.new
-    @view.instance_variable_set :@page, page
-    @view.instance_variable_set :@content, page.formatted_data
-    @view.instance_variable_set :@h1_title, false
-
-    # Title is based on file name when h1_title is false.
-    actual = @view.title
-    assert_equal title, actual
-  end
-
   test "breadcrumbs" do
     @wiki.write_page('subdir/BC Test 1', :markdown, 'Test', commit_details)
     page = @wiki.page('subdir/BC Test 1')