Merge pull request #934 from nobu/h1-1321358-xss

Escape file names

Author: nobu

lib/rdoc/generator/template/darkfish/_head.rhtml

@@ -3,18 +3,18 @@
 <title><%= h @title %></title>
 
 <script type="text/javascript">
-  var rdoc_rel_prefix = "<%= asset_rel_prefix %>/";
-  var index_rel_prefix = "<%= rel_prefix %>/";
+  var rdoc_rel_prefix = "<%= h asset_rel_prefix %>/";
+  var index_rel_prefix = "<%= h rel_prefix %>/";
 </script>
 
-<script src="<%= asset_rel_prefix %>/js/navigation.js" defer></script>
-<script src="<%= asset_rel_prefix %>/js/search.js" defer></script>
-<script src="<%= asset_rel_prefix %>/js/search_index.js" defer></script>
-<script src="<%= asset_rel_prefix %>/js/searcher.js" defer></script>
-<script src="<%= asset_rel_prefix %>/js/darkfish.js" defer></script>
+<script src="<%= h asset_rel_prefix %>/js/navigation.js" defer></script>
+<script src="<%= h asset_rel_prefix %>/js/search.js" defer></script>
+<script src="<%= h asset_rel_prefix %>/js/search_index.js" defer></script>
+<script src="<%= h asset_rel_prefix %>/js/searcher.js" defer></script>
+<script src="<%= h asset_rel_prefix %>/js/darkfish.js" defer></script>
 
-<link href="<%= asset_rel_prefix %>/css/fonts.css" rel="stylesheet">
-<link href="<%= asset_rel_prefix %>/css/rdoc.css" rel="stylesheet">
+<link href="<%= h asset_rel_prefix %>/css/fonts.css" rel="stylesheet">
+<link href="<%= h asset_rel_prefix %>/css/rdoc.css" rel="stylesheet">
 <%- @options.template_stylesheets.each do |stylesheet| -%>
-<link href="<%= asset_rel_prefix %>/<%= File.basename stylesheet %>" rel="stylesheet">
+<link href="<%= h asset_rel_prefix %>/<%= File.basename stylesheet %>" rel="stylesheet">
 <%- end -%>

lib/rdoc/generator/template/darkfish/_sidebar_pages.rhtml

@@ -12,18 +12,18 @@
   <%- end.each do |n, files| -%>
     <%- f = files.shift -%>
     <%- if files.empty? -%>
-    <li><a href="<%= rel_prefix %>/<%= f.path %>"><%= h f.page_name %></a>
+    <li><a href="<%= rel_prefix %>/<%= h f.path %>"><%= h f.page_name %></a>
       <%- next -%>
     <%- end -%>
     <li><details<% if dir == n %> open<% end %>><summary><%
     if n == f.page_name
-      %><a href="<%= rel_prefix %>/<%= f.path %>"><%= h n %></a><%
+      %><a href="<%= rel_prefix %>/<%= h f.path %>"><%= h n %></a><%
     else
       %><%= h n %><% files.unshift(f)
     end %></summary>
     <ul class="link-list">
     <%- files.each do |f| -%>
-      <li><a href="<%= rel_prefix %>/<%= f.path %>"><%= h f.page_name %></a>
+      <li><a href="<%= rel_prefix %>/<%= h f.path %>"><%= h f.page_name %></a>
     <%- end -%>
     </ul></details>
   <%- end -%>

lib/rdoc/generator/template/darkfish/js/darkfish.js

@@ -54,7 +54,7 @@ function hookSearch() {
     var html = '';
 
     // TODO add relative path to <script> per-page
-    html += '<p class="search-match"><a href="' + index_rel_prefix + result.path + '">' + this.hlt(result.title);
+    html += '<p class="search-match"><a href="' + index_rel_prefix + this.escapeHTML(result.path) + '">' + this.hlt(result.title);
     if (result.params)
       html += '<span class="params">' + result.params + '</span>';
     html += '</a>';

lib/rdoc/generator/template/darkfish/js/search.js

@@ -101,7 +101,7 @@ Search.prototype = Object.assign({}, Navigation, new function() {
   }
 
   this.escapeHTML = function(html) {
-    return html.replace(/[&<>]/g, function(c) {
+    return html.replace(/[&<>"`']/g, function(c) {
       return '&#' + c.charCodeAt(0) + ';';
     });
   }

lib/rdoc/generator/template/darkfish/table_of_contents.rhtml

@@ -8,14 +8,14 @@
 <ul>
 <%- simple_files.sort.each do |file| -%>
   <li class="file">
-    <a href="<%= file.path %>"><%= h file.page_name %></a>
+    <a href="<%= h file.path %>"><%= h file.page_name %></a>
 <%
    # HACK table_of_contents should not exist on Document
    table = file.parse(file.comment).table_of_contents
    unless table.empty? then %>
     <ul>
 <%- table.each do |heading| -%>
-      <li><a href="<%= file.path %>#<%= heading.aref %>"><%= heading.plain_html %></a>
+      <li><a href="<%= h file.path %>#<%= heading.aref %>"><%= heading.plain_html %></a>
 <%-   end -%>
     </ul>
 <%- end -%>

test/rdoc/test_rdoc_generator_darkfish.rb

@@ -233,6 +233,29 @@ def test_generated_method_with_html_tag_yield
     assert_includes method_name, '{ |%<<script>alert("atui")</script>>, yield_arg| ... }'
   end
 
+  def test_generated_filename_with_html_tag
+    filename = '"><em>should be escaped'
+    begin # in @tmpdir
+      File.write(filename, '')
+    rescue SystemCallError
+      # ", <, > chars are prohibited as filename
+      return
+    else
+      File.unlink(filename)
+    end
+    @store.add_file filename
+    doc = @store.all_files.last
+    doc.parser = RDoc::Parser::Simple
+
+    @g.generate
+
+    Dir.glob("*.html", base: @tmpdir) do |html|
+      File.read(File.join(@tmpdir, html)).scan(/.*should be escaped.*/) do |line|
+        assert_not_include line, "<em>", html
+      end
+    end
+  end
+
   def test_template_stylesheets
     css = Tempfile.create(%W'hoge .css', Dir.mktmpdir('tmp', '.'))
     File.write(css, '')