Tighten CSP around script-src and style-src

Instead of allowing every src, require scripts from unknown sources to have have a nonce. This makes it harder to exploit potential XSS vulnverabilities as the attacker needs to somehow know the nonce beforehand. This also adds the nonce to all internal scripts. As per #3913 (comment) the assets may be hosted outside of the main app. Adding the nonce continues to support this usecase. A nonce is incompatible with `unsafe-inline` (which only style has). Adapt the chart to not assign inline styles directly. Extensions that load external scripts/styles must either vendor or add the nonce to their tags. Closes #6268
sidekiq · Apr 29, 2024 · 5b0b969 · 5b0b969
1 parent 30786e0
commit 5b0b969
Show file tree

Hide file tree

Showing 11 changed files with 84 additions and 37 deletions.
diff --git a/lib/sidekiq/web.rb b/lib/sidekiq/web.rb
@@ -5,11 +5,13 @@
 require "sidekiq"
 require "sidekiq/api"
 require "sidekiq/paginator"
+require "sidekiq/web/public_helpers"
 require "sidekiq/web/helpers"
 
 require "sidekiq/web/router"
 require "sidekiq/web/action"
 require "sidekiq/web/application"
+require "sidekiq/web/content_security_policy_nonce"
 require "sidekiq/web/csrf_protection"
 
 require "rack/content_length"
@@ -158,12 +160,14 @@ def build
           header_rules: rules
         m.each { |middleware, block| use(*middleware, &block) }
         use Sidekiq::Web::CsrfProtection unless $TESTING
+        use Sidekiq::Web::ContentSecurityPolicyNonce unless $TESTING
         run WebApplication.new(klass)
       end
     end
   end
 
   Sidekiq::WebApplication.helpers WebHelpers
+  Sidekiq::WebApplication.helpers PublicWebHelpers
   Sidekiq::WebApplication.helpers Sidekiq::Paginator
 
   Sidekiq::WebAction.class_eval <<-RUBY, __FILE__, __LINE__ + 1

diff --git a/lib/sidekiq/web/application.rb b/lib/sidekiq/web/application.rb
@@ -5,7 +5,8 @@ class WebApplication
     extend WebRouter
 
     REDIS_KEYS = %w[redis_version uptime_in_days connected_clients used_memory_human used_memory_peak_human]
-    CSP_HEADER = [
+    NONCE_PLACEHOLDER = "nonce-placeholder"
+    CSP_HEADER_TEMPLATE = [
       "default-src 'self' https: http:",
       "child-src 'self'",
       "connect-src 'self' https: http: wss: ws:",
@@ -15,8 +16,8 @@ class WebApplication
       "manifest-src 'self'",
       "media-src 'self'",
       "object-src 'none'",
-      "script-src 'self' https: http:",
-      "style-src 'self' https: http: 'unsafe-inline'",
+      "script-src 'self' 'nonce-#{NONCE_PLACEHOLDER}'",
+      "style-src 'self' 'nonce-#{NONCE_PLACEHOLDER}'",
       "worker-src 'self'",
       "base-uri 'self'"
     ].join("; ").freeze
@@ -428,13 +429,17 @@ def call(env)
           Rack::CONTENT_TYPE => "text/html",
           Rack::CACHE_CONTROL => "private, no-store",
           Web::CONTENT_LANGUAGE => action.locale,
-          Web::CONTENT_SECURITY_POLICY => CSP_HEADER
+          Web::CONTENT_SECURITY_POLICY => process_csp(env, CSP_HEADER_TEMPLATE)
         }
         # we'll let Rack calculate Content-Length for us.
         [200, headers, [resp]]
       end
     end
 
+    def process_csp(env, input)
+      input.gsub(NONCE_PLACEHOLDER, env[:csp_nonce])
+    end
+
     def self.helpers(mod = nil, &block)
       if block
         WebAction.class_eval(&block)

diff --git a/lib/sidekiq/web/content_security_policy_nonce.rb b/lib/sidekiq/web/content_security_policy_nonce.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+require "securerandom"
+
+module Sidekiq
+  class Web
+    class ContentSecurityPolicyNonce
+      def initialize(app, options = nil)
+        @app = app
+      end
+
+      def call(env)
+        env[:csp_nonce] = SecureRandom.base64(16)
+        @app.call(env)
+      end
+    end
+  end
+end
diff --git a/lib/sidekiq/web/public_helpers.rb b/lib/sidekiq/web/public_helpers.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+module Sidekiq
+  module PublicWebHelpers
+    def csp_nonce
+      env[:csp_nonce]
+    end
+  end
+end
diff --git a/test/web_helpers_test.rb b/test/web_helpers_test.rb
@@ -5,6 +5,7 @@
 
 class Helpers
   include Sidekiq::WebHelpers
+  include Sidekiq::PublicWebHelpers
 
   def initialize(params = {})
     @thehash = default.merge(params)

diff --git a/test/web_test.rb b/test/web_test.rb
@@ -68,8 +68,8 @@ def job_params(job, score)
     get "/", {}
     policies = last_response.headers["Content-Security-Policy"].split("; ")
     assert_includes(policies, "connect-src 'self' https: http: wss: ws:")
-    assert_includes(policies, "style-src 'self' https: http: 'unsafe-inline'")
-    assert_includes(policies, "script-src 'self' https: http:")
+    assert_includes(policies, "style-src 'self' 'nonce-")
+    assert_includes(policies, "script-src 'self' 'nonce-")
     assert_includes(policies, "object-src 'none'")
   end
 

diff --git a/web/assets/javascripts/dashboard-charts.js b/web/assets/javascripts/dashboard-charts.js
@@ -108,17 +108,27 @@ class RealtimeChart extends DashboardChart {
   }
 
   renderLegend(dp) {
-    this.legend.innerHTML = `
-      <span>
-        <span class="swatch" style="background-color: ${dp[0].dataset.borderColor};"></span>
-        <span>${dp[0].dataset.label}: ${dp[0].formattedValue}</span>
-      </span>
-      <span>
-        <span class="swatch" style="background-color: ${dp[1].dataset.borderColor};"></span>
-        <span>${dp[1].dataset.label}: ${dp[1].formattedValue}</span>
-      </span>
-      <span class="time">${dp[0].label}</span>
-    `;
+    const entry1 = this.legendEntry(dp[0]);
+    const entry2 = this.legendEntry(dp[1]);
+    const time = document.createElement("span");
+    time.classList.add("time");
+    time.innerText = dp[0].label;
+
+    this.legend.replaceChildren(entry1, entry2, time)
+  }
+
+  legendEntry(dp) {
+    const wrapper = document.createElement("span");
+
+    const swatch = document.createElement("span");
+    swatch.classList.add("swatch");
+    swatch.style.backgroundColor = dp.dataset.borderColor;
+    wrapper.appendChild(swatch)
+
+    const label = document.createElement("span");
+    label.innerText = `${dp.dataset.label}: ${dp.formattedValue}`;
+    wrapper.appendChild(label)
+    return wrapper;
   }
 
   renderCursor(dp) {
@@ -179,4 +189,4 @@ class RealtimeChart extends DashboardChart {
   if (hc != null) {
     var htc = new DashboardChart(hc, JSON.parse(hc.textContent))
     window.historyChart = htc
-  }
+  }
diff --git a/web/views/dashboard.erb b/web/views/dashboard.erb
@@ -1,4 +1,4 @@
-<script type="text/javascript" src="<%= root_path %>javascripts/dashboard.js"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/dashboard.js" nonce="<%= csp_nonce %>"></script>
 <div class= "dashboard clearfix">
   <h3 >
     <%= t('Dashboard') %>
@@ -99,7 +99,7 @@
   </div>
 </div>
 
-<script type="text/javascript" src="<%= root_path %>javascripts/chart.min.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/chartjs-plugin-annotation.min.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/base-charts.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/dashboard-charts.js"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/chart.min.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/chartjs-plugin-annotation.min.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/base-charts.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/dashboard-charts.js" nonce="<%= csp_nonce %>"></script>
diff --git a/web/views/layout.erb b/web/views/layout.erb
@@ -5,20 +5,20 @@
     <meta charset="utf-8" />
     <meta name="viewport" content="width=device-width,initial-scale=1.0" />
 
-    <link href="<%= root_path %>stylesheets/bootstrap.css" media="screen" rel="stylesheet" type="text/css" />
+    <link href="<%= root_path %>stylesheets/bootstrap.css" media="screen" rel="stylesheet" type="text/css" nonce="<%= csp_nonce %>" />
     <% if rtl? %>
-    <link href="<%= root_path %>stylesheets/bootstrap-rtl.min.css" media="screen" rel="stylesheet" type="text/css"/>
+    <link href="<%= root_path %>stylesheets/bootstrap-rtl.min.css" media="screen" rel="stylesheet" type="text/css" nonce="<%= csp_nonce %>"/>
     <% end %>
 
-    <link href="<%= root_path %>stylesheets/application.css" media="screen" rel="stylesheet" type="text/css" />
-    <link href="<%= root_path %>stylesheets/application-dark.css" media="screen and (prefers-color-scheme: dark)" rel="stylesheet" type="text/css" />
+    <link href="<%= root_path %>stylesheets/application.css" media="screen" rel="stylesheet" type="text/css" nonce="<%= csp_nonce %>" />
+    <link href="<%= root_path %>stylesheets/application-dark.css" media="screen and (prefers-color-scheme: dark)" rel="stylesheet" type="text/css" nonce="<%= csp_nonce %>" />
     <% if rtl? %>
-    <link href="<%= root_path %>stylesheets/application-rtl.css" media="screen" rel="stylesheet" type="text/css" />
+    <link href="<%= root_path %>stylesheets/application-rtl.css" media="screen" rel="stylesheet" type="text/css" nonce="<%= csp_nonce %>" />
     <% end %>
 
     <link rel="apple-touch-icon" href="<%= root_path %>images/apple-touch-icon.png">
     <link rel="shortcut icon" type="image/ico" href="<%= root_path %>images/favicon.ico" />
-    <script type="text/javascript" src="<%= root_path %>javascripts/application.js"></script>
+    <script type="text/javascript" src="<%= root_path %>javascripts/application.js" nonce="<%= csp_nonce %>"></script>
     <meta name="google" content="notranslate" />
     <%= display_custom_head %>
   </head>

diff --git a/web/views/metrics.erb b/web/views/metrics.erb
@@ -1,6 +1,6 @@
-<script type="text/javascript" src="<%= root_path %>javascripts/chart.min.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/chartjs-plugin-annotation.min.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/base-charts.js"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/chart.min.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/chartjs-plugin-annotation.min.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/base-charts.js" nonce="<%= csp_nonce %>"></script>
 
 <div class="header-container">
   <div class="page-title-container">
@@ -88,4 +88,4 @@
 
 <!--p><small>Data from <%= @query_result.starts_at %> to <%= @query_result.ends_at %></small></p-->
 
-<script type="text/javascript" src="<%= root_path %>javascripts/metrics.js"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/metrics.js" nonce="<%= csp_nonce %>"></script>
diff --git a/web/views/metrics_for_job.erb b/web/views/metrics_for_job.erb
@@ -1,6 +1,6 @@
-<script type="text/javascript" src="<%= root_path %>javascripts/chart.min.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/chartjs-plugin-annotation.min.js"></script>
-<script type="text/javascript" src="<%= root_path %>javascripts/base-charts.js"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/chart.min.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/chartjs-plugin-annotation.min.js" nonce="<%= csp_nonce %>"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/base-charts.js" nonce="<%= csp_nonce %>"></script>
 
 <%
   job_result = @query_result.job_results[@name]
@@ -56,4 +56,4 @@
   <div class="alert alert-success"><%= t('NoJobMetricsFound') %></div>
 <% end %>
 
-<script type="text/javascript" src="<%= root_path %>javascripts/metrics.js"></script>
+<script type="text/javascript" src="<%= root_path %>javascripts/metrics.js" nonce="<%= csp_nonce %>"></script>