Skip to content

Commit

Permalink
Security: Escape strings where recommended by SafeERB
Browse files Browse the repository at this point in the history
The SafeERB plugin attempts to automatically detect view code which
fails to properly escape HTML.  You can find information here:

  http://wiki.rubyonrails.com/rails/pages/Safe+ERB

I'm using a version of SafeERB modified by Matthew Bass, which can be
found on github:

  http://github.com/pelargir/safe_erb/tree/master

My local copy is modified to avoid some false positives, and isn't ready
for production use yet.  But here's the first batch of changes it
recommended.  Note that some of these changes weren't really necessary--
some of the values we're wrapping can't actually contain HTML
metacharacters, at least not in normal locales.

Also note that SafeERB is only useful for the normal view code in places
like /admin, and that it won't help us with Liquid plugins in the front
end.  But it's a start.
  • Loading branch information
emk committed Dec 11, 2008
1 parent f9d3c10 commit a83309d
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 22 deletions.
4 changes: 2 additions & 2 deletions app/views/admin/articles/_page_nav.rhtml
Expand Up @@ -3,7 +3,7 @@
<div id="page-nav">
<ul id="act-nav" class="clear">
<% if controller.controller_name == 'comments' && controller.action_name == 'index' && @comments.size > 0 -%>
<li><%= link_to_remote "Delete these #{@filter != 'all' ? @filter : ''} comments", :confirm => "Are you sure you wish to delete all #{@filter != 'all' ? @filter : ''} comments?",
<li><%= link_to_remote h("Delete these #{@filter != 'all' ? @filter : ''} comments"), :confirm => "Are you sure you wish to delete all #{@filter != 'all' ? @filter : ''} comments?",
:url => { :controller => 'comments', :action => 'destroy', :id => @article }, :with => "ArticleForm.getAvailableComments().toQueryString('comment')"
%></li>
<% end -%>
Expand Down Expand Up @@ -57,4 +57,4 @@
<% end -%>
</ul>
</div>
<% end unless @article && @article.new_record? && @article.comments.size == 0 -%>
<% end unless @article && @article.new_record? && @article.comments.size == 0 -%>
2 changes: 1 addition & 1 deletion app/views/admin/assets/index.rhtml
Expand Up @@ -52,7 +52,7 @@
<h3>Some stats</h3>
<p>
You have a uploaded a total of <strong><%= pluralize site.assets.count, 'asset'%></strong>, using
<strong><%= number_to_human_size site.assets.sum(:size) %></strong>.
<strong><%=h number_to_human_size site.assets.sum(:size) %></strong>.
</p>
</div>
<% end %>
4 changes: 2 additions & 2 deletions app/views/admin/comments/index.rhtml
Expand Up @@ -2,7 +2,7 @@

<h3 style="border-bottom:1px solid #ccc;padding:5px">
<% if @article -%>
Comments on <%= link_to @article.title, edit_article_path(@article), :style => 'border:none' %> <span class="right"><%= @article.published? ? link_to(image_tag('/images/mephisto/icons/24-zoom-in.png', :style => 'vertical-align: middle'), @site.permalink_for(@article), :style => 'border:none;') : '&nbsp;' %></span>
Comments on <%= link_to h(@article.title), edit_article_path(@article), :style => 'border:none' %> <span class="right"><%= @article.published? ? link_to(image_tag('/images/mephisto/icons/24-zoom-in.png', :style => 'vertical-align: middle'), @site.permalink_for(@article), :style => 'border:none;') : '&nbsp;' %></span>
<% else -%>
Comments for all articles
<% end -%>
Expand All @@ -22,7 +22,7 @@ Comments for all articles
<blockquote><p>"<%= strip_tags(comment.body) %>"</p></blockquote>
<% end -%>
<span class="meta">
<cite>&mdash; <%= author_link_for comment %><%= %( (#{comment.author_email})) unless comment.author_email.blank? %> said <%= time_ago_in_words comment.created_at %> ago</cite>
<cite>&mdash; <%= author_link_for comment %><%=h %( (#{comment.author_email})) unless comment.author_email.blank? %> said <%=h time_ago_in_words comment.created_at %> ago</cite>

<%= link_to_remote 'Edit', :url => edit_article_comment_path(comment.article, comment), :method => :get %> |
<% if comment.approved? -%>
Expand Down
6 changes: 3 additions & 3 deletions app/views/admin/design/_form.rhtml
@@ -1,16 +1,16 @@
<div class="group">
<dl>
<dt>
<%= label_tag :data, labels[:data] %>
<%= label_tag :data, h(labels[:data]) %>
<p class="hint"><%= hint %></p>
</dt>
<dd><%= text_area_tag :data, h(attachment && attachment.file? ? attachment.read : params[:data]), :class => 'fat', :rows => 20 %></dd>
<% if controller.action_name == 'index' -%>
<dt>
<%= label_tag :filename, labels[:filename] %>
<%= label_tag :filename, h(labels[:filename]) %>
<p class="hint">You can create one of three types of files: Liquid (*.liquid), CSS (*.css), and Javascript (*.js).</p>
</dt>
<dd><%= text_field_tag :filename, params[:filename] %></dd>
<% end -%>
</dl>
</div>
</div>
8 changes: 4 additions & 4 deletions app/views/admin/design/_sidebar.rhtml
Expand Up @@ -4,18 +4,18 @@
*_layout suffix (e.g custom_layout).</p>
<ul id="attachments">
<% @theme.templates.template_types(@theme.extension).each do |template| -%>
<li><%= link_to template, url_for_theme(:controller => 'templates', :action => 'edit', :filename => template) %></li>
<li><%= link_to h(template), url_for_theme(:controller => 'templates', :action => 'edit', :filename => template) %></li>
<% end -%>
<% @theme.templates.custom(@theme.extension).each_with_index do |template, i| -%>
<li id="templates-<%= i %>">
<%= delete_link :templates, template, "templates-#{i}" %>
<%= link_to template, url_for_theme(:controller => 'templates', :action => 'edit', :filename => template) %>
<%= link_to h(template), url_for_theme(:controller => 'templates', :action => 'edit', :filename => template) %>
</li>
<% end -%>
<% @theme.resources.reject { |r| @theme.resources.image?(r) }.each_with_index do |resource, i| -%>
<li id="resources-<%= i %>">
<%= delete_link :resources, resource.basename.to_s, "resources-#{i}" %>
<%= link_to resource.basename, url_for_theme(:controller => 'resources', :action => 'edit', :filename => resource.basename) %>
<%= link_to h(resource.basename), url_for_theme(:controller => 'resources', :action => 'edit', :filename => resource.basename) %>
</li>
<% end -%>
</ul>
Expand All @@ -25,7 +25,7 @@
<p>Select an image to use in your template.</p>
<ul id="attachments">
<% @theme.resources.select { |r| @theme.resources.image?(r) }.each_with_index do |image, i| -%>
<li id="images-<%= i %>"><%= delete_link :resources, image.basename.to_s, "images-#{i}" %> <%= image.basename %> </li>
<li id="images-<%= i %>"><%= delete_link :resources, h(image.basename.to_s), "images-#{i}" %> <%= h(image.basename) %> </li>
<% end -%>
</ul>

Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/overview/index.rhtml
Expand Up @@ -49,7 +49,7 @@
<h3>Recent activity</h3>
<ul class="slist" id="activity">
<% @users.each do |user| -%>
<li style="clear:right;"><%= avatar_for user %><%= link_to who(user.login), :controller => 'users', :action => 'show', :id => user %><br /> showed up <%= distance_of_time_in_words_to_now(user.updated_at) %> ago</li>
<li style="clear:right;"><%= avatar_for user %><%= link_to who(user.login), :controller => 'users', :action => 'show', :id => user %><br /> showed up <%=h distance_of_time_in_words_to_now(user.updated_at) %> ago</li>
<% end -%>
</ul>
</div>
Expand Down
8 changes: 4 additions & 4 deletions app/views/admin/themes/_theme.rhtml
@@ -1,12 +1,12 @@
<li class="theme<%= ' current' if theme.current? %>" id="theme-<%= theme_counter %>">
<li class="theme<%=h ' current' if theme.current? %>" id="theme-<%= theme_counter %>">
<h3>
<span title="stored in /<%= theme.name %>"><%=h theme.title %></span>
<span title="stored in /<%=h theme.name %>"><%=h theme.title %></span>
<span class="thememeta">
<% unless theme.version.blank? -%>v<%=h theme.version %> |<% end -%>
by <%= theme.linked_author.blank? ? 'unknown' : theme.linked_author %>
by <%=h theme.linked_author.blank? ? 'unknown' : theme.linked_author %>
</span>
</h3>
<a id="theme-dialog-<%= theme_counter %>" class="theme_dialog">
<img src="<%= url_for(:controller => '/admin/themes', :action => 'preview_for', :id => theme) %>" alt="Theme preview" title="<%=h theme.title %> (v<%=h theme.version %>)" />
</a>
</li>
</li>
10 changes: 5 additions & 5 deletions app/views/layouts/application.rhtml
Expand Up @@ -3,7 +3,7 @@
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-type" content="text/html; charset=utf-8" />
<title><%= site.title %>: Admin <%= controller.controller_name %></title>
<title><%=h site.title %>: Admin <%=h controller.controller_name %></title>
<%= stylesheet_link_tag 'mephisto/mephisto' %>
<%= javascript_include_tag 'mephisto/prototype', 'mephisto/effects', 'mephisto/dragdrop', 'mephisto/lowpro', 'mephisto/application' %>
<script type="text/javascript">Mephisto.root = '<%= relative_url_root %>'; <%= init_mephisto_authenticity_token %></script>
Expand Down Expand Up @@ -36,7 +36,7 @@
<li><%= link_to 'Articles', :controller => '/admin/articles' %></li>
<li><%= link_to 'Assets', :controller => '/admin/assets' %></li>
<% Mephisto::Plugin.tabs.each do |tab| -%>
<li><%= link_to tab.first, tab.last %></li>
<li><%= link_to h(tab.first), tab.last %></li>
<% end -%>
</ul>
<% if admin? -%>
Expand All @@ -49,7 +49,7 @@
<li><%= link_to 'Plugins', :controller => '/admin/plugins' %></li>
<% end -%>
<% Mephisto::Plugin.admin_tabs.each do |tab| -%>
<li><%= link_to tab.first.to_s.tableize.humanize, tab.last %></li>
<li><%= link_to h(tab.first.to_s.tableize.humanize), tab.last %></li>
<% end -%>
</ul>
<% end -%>
Expand Down Expand Up @@ -80,8 +80,8 @@
<!-- div.left -->
<div id="main">
<div id="flashes">
<div id="flash-errors" style="display: none;"><%= flash[:error] %></div>
<div id="flash-notice" style="display:none"><%= flash[:notice] %></div>
<div id="flash-errors" style="display: none;"><%=h flash[:error] %></div>
<div id="flash-notice" style="display:none"><%=h flash[:notice] %></div>
</div>
<!-- begin action nav -->
<%= yield :action_nav %>
Expand Down

0 comments on commit a83309d

Please sign in to comment.