Add better_html to ensure our ERB usage is safe (#4860)

rubygems · Jul 8, 2024 · 07fbafd · 07fbafd
1 parent 4f1d665
commit 07fbafd
Show file tree

Hide file tree

Showing 25 changed files with 80 additions and 23 deletions.
diff --git a/Gemfile b/Gemfile
@@ -79,6 +79,7 @@ gem "observer", "~> 0.1.2" # launchdarkly-server-sdk-8.0.0
 gem "sprockets-rails", "~> 3.5"
 gem "importmap-rails", "~> 2.0"
 gem "stimulus-rails", "~> 1.3" # this adds stimulus-loading.js so it must be available at runtime
+gem "better_html", "~> 2.1"
 
 group :assets, :development do
   gem "tailwindcss-rails", "~> 2.6"

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -131,6 +131,13 @@ GEM
     base64 (0.2.0)
     bcrypt (3.1.20)
     benchmark-ips (2.12.0)
+    better_html (2.1.1)
+      actionview (>= 6.0)
+      activesupport (>= 6.0)
+      ast (~> 2.0)
+      erubi (~> 1.4)
+      parser (>= 2.4)
+      smart_properties
     bigdecimal (3.1.8)
     bindata (2.5.0)
     bitarray (1.2.0)
@@ -673,6 +680,7 @@ GEM
       simplecov (~> 0.19)
     simplecov-html (0.12.3)
     simplecov_json_formatter (0.1.4)
+    smart_properties (1.17.0)
     snaky_hash (2.0.1)
       hashie
       version_gem (~> 1.1, >= 1.1.1)
@@ -775,6 +783,7 @@ DEPENDENCIES
   aws-sdk-s3 (~> 1.156)
   aws-sdk-sqs (~> 1.80)
   bcrypt (~> 3.1)
+  better_html (~> 2.1)
   bootsnap (~> 1.18)
   brakeman (~> 6.1)
   browser (~> 6.0)
@@ -913,6 +922,7 @@ CHECKSUMS
   base64 (0.2.0) sha256=0f25e9b21a02a0cc0cea8ef92b2041035d39350946e8789c562b2d1a3da01507
   bcrypt (3.1.20) sha256=8410f8c7b3ed54a3c00cd2456bf13917d695117f033218e2483b2e40b0784099
   benchmark-ips (2.12.0) sha256=09dd4d5be05db42470e7e7b01be7310564073a054e35d9d9ec7840c523f3dbcb
+  better_html (2.1.1) sha256=046c3551d1488a3f2939a7cac6fabf2bde08c32e135c91fcd683380118e5af55
   bigdecimal (3.1.8) sha256=a89467ed5a44f8ae01824af49cbc575871fa078332e8f77ea425725c1ffe27be
   bindata (2.5.0) sha256=29dccb8ba1cc9de148f24bb88930840c62db56715f0f80eccadd624d9f3d2623
   bitarray (1.2.0) sha256=7f9f31fadbd87bf51544cf13058e81cd6ec408ff40f127902cef3d6767b23f11
@@ -1130,6 +1140,7 @@ CHECKSUMS
   simplecov-cobertura (2.1.0) sha256=2c6532e34df2e38a379d72cef9a05c3b16c64ce90566beebc6887801c4ad3f02
   simplecov-html (0.12.3) sha256=4b1aad33259ffba8b29c6876c12db70e5750cb9df829486e4c6e5da4fa0aa07b
   simplecov_json_formatter (0.1.4) sha256=529418fbe8de1713ac2b2d612aa3daa56d316975d307244399fa4838c601b428
+  smart_properties (1.17.0) sha256=f9323f8122e932341756ddec8e0ac9ec6e238408a7661508be99439ca6d6384b
   snaky_hash (2.0.1) sha256=1ac87ec157fcfe7a460e821e0cd48ae1e6f5e3e082ab520f03f31a9259dbdc31
   sprockets (4.2.1) sha256=951b13dd2f2fcae840a7184722689a803e0ff9d2702d902bd844b196da773f97
   sprockets-rails (3.5.1) sha256=c44626cb3887a1a8b572ca258685db33b4ebd041aa73428a716eac444ee5ef48

diff --git a/app/controllers/totps_controller.rb b/app/controllers/totps_controller.rb
@@ -15,7 +15,7 @@ def new
     session[:totp_seed] = @seed
     session[:totp_seed_expire] = Gemcutter::MFA_KEY_EXPIRY.from_now.utc.to_i
     text = ROTP::TOTP.new(@seed, issuer: issuer).provisioning_uri(current_user.email)
-    @qrcode_svg = RQRCode::QRCode.new(text, level: :l).as_svg(module_size: 6)
+    @qrcode_svg = RQRCode::QRCode.new(text, level: :l).as_svg(module_size: 6).html_safe # rubocop:disable Rails/OutputSafety
   end
 
   def create

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
@@ -1,4 +1,6 @@
 module ApplicationHelper
+  include BetterHtml::Helpers
+
   def page_title
     combo = "#{t :title} | #{t :subtitle}"
     # If instance variable @title_for_header_only is present then it is added to combo title string

diff --git a/app/javascript/src/transitive_dependencies.js b/app/javascript/src/transitive_dependencies.js
@@ -1,12 +1,14 @@
 import $ from "jquery";
 
 $(document).on('click', '.deps_expanded-link', function () {
+  try {
   var current = $(this);
-  var gem_id = $(this).attr('data-gem_id');
-  var version_id = $(this).attr('data-version');
+  var gem_id = this.dataset.gemId;
+  var version_id = this.dataset.version;
+  const url = "/gems/"+gem_id+"/versions/"+version_id+"/dependencies.json";
   $.ajax({
     type: "get",
-    url: "/gems/"+gem_id+"/versions/"+version_id+"/dependencies.json",
+    url: url,
     success: function(resp) {
       renderDependencies(resp, current);
     },
@@ -15,6 +17,9 @@ $(document).on('click', '.deps_expanded-link', function () {
       current.parent().next().next().html(error_message);
     }
   });
+} catch (e) {
+  alert(e);
+}
 });
 
 function renderDependencies(resp, current) {

diff --git a/app/views/dependencies/_dependencies.html.erb b/app/views/dependencies/_dependencies.html.erb
@@ -5,7 +5,7 @@
     <ul class="deps">
       <li>
         <span>
-          <span class="deps_expanded deps_expanded-link" data-gem_id="<%= name %>" data-version="<%= version %>"></span>
+          <span class="deps_expanded deps_expanded-link" <%= html_attributes("data-gem-id" => name, "data-version" => version) %>></span>
         </span>
         <%= link_to "/gems/#{name}/versions/#{version}", target: :_blank do %>
           <span class="deps_item"><%= name %> <%= version %>

diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb
@@ -110,7 +110,7 @@
               <%= content_for_title @title, @title_url %>
 
               <% if @subtitle %>
-                <i class="page__subheading"><%= @subtitle.html_safe %></i>
+                <i class="page__subheading"><%= @subtitle %></i>
               <% end %>
             </h1>
           <% end %>

diff --git a/app/views/layouts/component_preview.html.erb b/app/views/layouts/component_preview.html.erb
@@ -17,7 +17,7 @@
         <h1 class="t-display page__heading">
           <%= content_for_title @title, @title_url %>
           <% if @subtitle %>
-            <i class="page__subheading"><%= @subtitle.html_safe %></i>
+            <i class="page__subheading"><%= @subtitle %></i>
           <% end %>
         </h1>
       <% end %>

diff --git a/app/views/layouts/mailer.html.erb b/app/views/layouts/mailer.html.erb
@@ -107,7 +107,7 @@
 
 										<div class="hide-for-mobile">
 											<div class="text-nav" style="color:#000000; font-family:Arial, sans-serif; min-width:auto !important; font-size:12px; line-height:22px; text-align:center">
-												<a href=<%= root_url %> target="_blank" rel="noopener" class="link-white" style="color:#000000; text-decoration:none"><span class="link-white" style="color:#000000; text-decoration:none">HOME</span></a>
+												<a href="<%= root_url %>" target="_blank" rel="noopener" class="link-white" style="color:#000000; text-decoration:none"><span class="link-white" style="color:#000000; text-decoration:none">HOME</span></a>
 												&nbsp;&nbsp;|&nbsp;&nbsp;
 												<a href="https://guides.rubygems.org/" target="_blank" rel="noopener" class="link-white" style="color:#000000; text-decoration:none"><span class="link-white" style="color:#000000; text-decoration:none">GUIDES</span></a>
 												&nbsp;&nbsp;|&nbsp;&nbsp;

diff --git a/app/views/mailer/email_confirmation.html.erb b/app/views/mailer/email_confirmation.html.erb
@@ -60,7 +60,7 @@
 </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href=<%= update_email_confirmations_url(token: @user.confirmation_token.html_safe) %> target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">VERIFY</span></a>
+                          <a href="<%= update_email_confirmations_url(token: @user.confirmation_token) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">VERIFY</span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

diff --git a/app/views/mailer/mfa_notification.html.erb b/app/views/mailer/mfa_notification.html.erb
@@ -34,7 +34,7 @@
 </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href=<%= new_totp_url %> target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">ENABLE MULTI-FACTOR AUTHENTICATION (MFA)</span></a>
+                          <a href="<%= new_totp_url %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">ENABLE MULTI-FACTOR AUTHENTICATION (MFA)</span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

diff --git a/app/views/mailer/mfa_recommendation_announcement.html.erb b/app/views/mailer/mfa_recommendation_announcement.html.erb
@@ -91,7 +91,7 @@
                         </td>
                         <td bgcolor="#e9573f">
                           <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                            <a href=<%= new_totp_url %> target="_blank" class="link-white" style="color:#ffffff; text-decoration:none">
+                            <a href="<%= new_totp_url %>" target="_blank" class="link-white" style="color:#ffffff; text-decoration:none">
                               <span class="link-white" style="color:#ffffff; text-decoration:none">
                                 <%= "ENABLE MFA" if @user.mfa_disabled? %>
                                 <%= "STRENGTHEN MFA LEVEL" if @user.mfa_ui_only? %>

diff --git a/app/views/mailer/mfa_required_popular_gems_announcement.html.erb b/app/views/mailer/mfa_required_popular_gems_announcement.html.erb
@@ -78,7 +78,7 @@
                         </td>
                         <td bgcolor="#e9573f">
                           <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                            <a href=<%= new_totp_url %> target="_blank" class="link-white" style="color:#ffffff; text-decoration:none">
+                            <a href="<%= new_totp_url %>" target="_blank" class="link-white" style="color:#ffffff; text-decoration:none">
                               <span class="link-white" style="color:#ffffff; text-decoration:none">
                                 <%= "ENABLE MFA" if @user.mfa_disabled? %>
                                 <%= "STRENGTHEN MFA LEVEL" if @user.mfa_ui_only? %>

diff --git a/app/views/mailer/mfa_required_soon_announcement.html.erb b/app/views/mailer/mfa_required_soon_announcement.html.erb
@@ -73,7 +73,7 @@
                         </td>
                         <td bgcolor="#e9573f">
                           <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                            <a href=<%= new_totp_url %> target="_blank" class="link-white" style="color:#ffffff; text-decoration:none">
+                            <a href="<%= new_totp_url %>" target="_blank" class="link-white" style="color:#ffffff; text-decoration:none">
                               <span class="link-white" style="color:#ffffff; text-decoration:none">
                                 <%= "ENABLE MFA" if @user.mfa_disabled? %>
                                 <%= "STRENGTHEN MFA LEVEL" if @user.mfa_ui_only? %>

diff --git a/app/views/owners_mailer/new_ownership_requests.html.erb b/app/views/owners_mailer/new_ownership_requests.html.erb
@@ -34,7 +34,7 @@
                       </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href=<%= rubygem_adoptions_url(@rubygem) %> target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none"><%= t("mailer.new_ownership_requests.button") %></span></a>
+                          <a href="<%= rubygem_adoptions_url(@rubygem) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none"><%= t("mailer.new_ownership_requests.button") %></span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

diff --git a/app/views/owners_mailer/ownership_confirmation.html.erb b/app/views/owners_mailer/ownership_confirmation.html.erb
@@ -29,7 +29,7 @@
                       </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href=<%= confirm_rubygem_owners_url(@rubygem.slug, token: @ownership.token.html_safe) %> target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">VERIFY</span></a>
+                          <a href="<%= confirm_rubygem_owners_url(@rubygem.slug, token: @ownership.token) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">VERIFY</span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

diff --git a/app/views/owners_mailer/ownership_request_approved.html.erb b/app/views/owners_mailer/ownership_request_approved.html.erb
@@ -26,7 +26,7 @@
                         </td>
                         <td bgcolor="#e9573f">
                           <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                            <a href=<%= rubygem_owners_url(@rubygem.slug) %> target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none"><%= t("mailer.new_ownership_requests.owners_page") %></span></a>
+                            <a href="<%= rubygem_owners_url(@rubygem.slug) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none"><%= t("mailer.new_ownership_requests.owners_page") %></span></a>
                           </div>
                         </td>
                         <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

diff --git a/app/views/pages/data.html.erb b/app/views/pages/data.html.erb
@@ -1,6 +1,6 @@
 <% @title = t('.title') %>
 
-<div class="t-body", id="data-dump">
+<div class="t-body" id="data-dump">
   <p>We provide weekly dumps of the RubyGems.org PostgreSQL data. This dump is sanitized, removing all user information.</p>
   <p>The <a href="https://github.com/rubygems/rubygems.org/tree/master/script/load-pg-dump">load-pg-dump</a> script is
      available as an example of how to to download and load the most recent weekly dump.</p>

diff --git a/app/views/password_mailer/change_password.html.erb b/app/views/password_mailer/change_password.html.erb
@@ -29,7 +29,7 @@
 </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href="<%= edit_password_url(token: @user.confirmation_token.html_safe) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">CHANGE PASSWORD</span></a>
+                          <a href="<%= edit_password_url(token: @user.confirmation_token) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">CHANGE PASSWORD</span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

diff --git a/app/views/rubygems/_gem_members.html.erb b/app/views/rubygems/_gem_members.html.erb
@@ -9,7 +9,7 @@
       <% if rubygem.owners.without_mfa.present? %>
         <% if current_user.mfa_disabled? %>
           <span class="gem__users__mfa-text mfa-warn">
-            <%= t '.self_no_mfa_warning_html'.html_safe %>
+            <%= t '.self_no_mfa_warning_html' %>
           </span>
         <% else %>
           <span class="gem__users__mfa-text mfa-warn"><%= t '.not_using_mfa_warning_show' %></span>

diff --git a/app/views/searches/show.html.erb b/app/views/searches/show.html.erb
@@ -25,7 +25,7 @@
     <% @subtitle = t('.subtitle', :query => content_tag(:em, h(params[:query]))) %>
 
     <header class="gems__header push--s">
-      <p class="gems__meter"><%= page_entries_info(@gems, :entry_name => 'gem').html_safe %></p>
+      <p class="gems__meter"><%= page_entries_info(@gems, :entry_name => 'gem') %></p>
     </header>
 
     <%= render partial: 'aggregations', locals: { gems: @gems } %>
@@ -35,9 +35,9 @@
       <div class='search__suggestions'>
         <p>
           <%= t(".suggestion") %>
-          <%= suggestions.map do |term|
+          <%= to_sentence(suggestions.map do |term|
             link_to term, search_path(params: { query: term }), only_path: true
-          end.to_sentence(last_word_connector: ' or ').html_safe %>?
+          end, last_word_connector: ' or ') %>?
         </p>
       </div>
     <% end %>

diff --git a/app/views/totps/new.html.erb b/app/views/totps/new.html.erb
@@ -2,7 +2,7 @@
 
 <div class="t-body l-overflow">
   <div class="l-half--l l-half--l--center">
-    <%= raw @qrcode_svg %>
+    <%= @qrcode_svg %>
   </div>
   <div>
     <p><%= t('.scan_prompt') %></p>

diff --git a/config/initializers/better_html.rb b/config/initializers/better_html.rb
@@ -0,0 +1,5 @@
+BetterHtml.configure do |config|
+  config.template_exclusion_filter = proc { |filename|
+    filename.include?("avo")
+  }
+end
diff --git a/test/unit/erb_implementation_test.rb b/test/unit/erb_implementation_test.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class ErbImplementationTest < ActiveSupport::TestCase
+  ERB_GLOB = File.join(
+    "app", "views", "**", "{*.htm,*.html,*.htm.erb,*.html.erb,*.html+*.erb}"
+  )
+
+  Dir[ERB_GLOB, base: Rails.root].each do |filename|
+    test "html errors in #{filename}" do
+      data = Rails.root.join(filename).read
+      BetterHtml::BetterErb::ErubiImplementation.new(data, filename:).validate!
+    end
+  end
+end
diff --git a/test/unit/erb_safety_test.rb b/test/unit/erb_safety_test.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "better_html/test_helper/safe_erb_tester"
+
+class ErbSafetyTest < ActiveSupport::TestCase
+  include BetterHtml::TestHelper::SafeErbTester
+  ERB_GLOB = File.join(
+    "app", "views", "**", "{*.htm,*.html,*.htm.erb,*.html.erb,*.html+*.erb}"
+  )
+
+  Dir[ERB_GLOB, base: Rails.root].each do |filename|
+    test "missing javascript escapes in #{filename}" do
+      assert_erb_safety(Rails.root.join(filename).read, filename:)
+    end
+  end
+end