Merge pull request #390 from twitter/no-more-sniffing

Remove all useragent sniffing

Author: oreoshake

docs/upgrading-to-6-0.md

@@ -48,3 +48,7 @@ Prior to 6.0.0 you could conceivably, though unlikely, have `Configure#default`
 Since the first commit, reducing browser console messages was a goal. It led to overly complicated and error-prone UA sniffing. Nowadays, consoles warn on completely legitimate use of features meant to be backwards compatible. So the goal is impossible and the impact is negative, so eliminating code using sniffing is a goal.
 
 The first example: we will now send `'unsafe-inline'` along with nonce source expressions. This will generate warnings in some consoles but is 100% valid use and was a design goal of CSP in the early days. The concept of versioning CSP lost out and so we're left with backward compatibility as our only option.
+
+## No more frame-src/child-src magic
+
+First there was frame-src. Then there was child-src which deprecated frame-src. Then child-src became deprecated in favor of frame-src and worker-src. In the meantime, every browser did something different. For a while, it was recommended to set child-src and frame-src but the values had to be identical. secure_headers would sniff the UA to determine whether to use frame or child src. That can lead to confusing things like setting frame-src but seeing child-src. If the child-src and frame-src did not match up, an error is raised. This can be very confusing when using dynamic overrides ("Do we use child-src or frame-src?" => :boom:). Now that the dust has settled, I think we can stop sniffing UAs and just go with a straightforward application.

lib/secure_headers.rb

@@ -15,7 +15,6 @@
 require "secure_headers/middleware"
 require "secure_headers/railtie"
 require "secure_headers/view_helper"
-require "useragent"
 require "singleton"
 require "secure_headers/configuration"
 
@@ -149,8 +148,7 @@ def header_hash_for(request)
       prevent_dup = true
       config = config_for(request, prevent_dup)
       config.validate_config!
-      user_agent = UserAgent.parse(request.user_agent)
-      headers = config.generate_headers(user_agent)
+      headers = config.generate_headers
 
       if request.scheme != HTTPS
         HTTPS_HEADER_CLASSES.each do |klass|

lib/secure_headers/configuration.rb

@@ -194,11 +194,11 @@ def override(name = nil, &block)
       self
     end
 
-    def generate_headers(user_agent)
+    def generate_headers
       headers = {}
       HEADERABLE_ATTRIBUTES.each do |attr|
         klass = CONFIG_ATTRIBUTES_TO_HEADER_CLASSES[attr]
-        header_name, value = klass.make_header(instance_variable_get("@#{attr}"), user_agent)
+        header_name, value = klass.make_header(instance_variable_get("@#{attr}"))
         if header_name && value
           headers[header_name] = value
         end

lib/secure_headers/headers/content_security_policy.rb

@@ -1,19 +1,12 @@
 # frozen_string_literal: true
 require_relative "policy_management"
 require_relative "content_security_policy_config"
-require "useragent"
 
 module SecureHeaders
   class ContentSecurityPolicy
     include PolicyManagement
 
-    # constants to be used for version-specific UA sniffing
-    VERSION_46 = ::UserAgent::Version.new("46")
-    VERSION_10 = ::UserAgent::Version.new("10")
-    FALLBACK_VERSION = ::UserAgent::Version.new("0")
-
-    def initialize(config = nil, user_agent = OTHER)
-      user_agent ||= OTHER
+    def initialize(config = nil)
       @config = if config.is_a?(Hash)
         if config[:report_only]
           ContentSecurityPolicyReportOnlyConfig.new(config || DEFAULT_CONFIG)
@@ -26,12 +19,6 @@ def initialize(config = nil, user_agent = OTHER)
         config
       end
 
-      @parsed_ua = if user_agent.is_a?(UserAgent::Browsers::Base)
-        user_agent
-      else
-        UserAgent.parse(user_agent)
-      end
-      @frame_src = normalize_child_frame_src
       @preserve_schemes = @config.preserve_schemes
       @script_nonce = @config.script_nonce
       @style_nonce = @config.style_nonce
@@ -56,20 +43,10 @@ def value
 
     private
 
-    def normalize_child_frame_src
-      if @config.frame_src && @config.child_src && @config.frame_src != @config.child_src
-        raise ArgumentError, "#{Kernel.caller.first}: both :child_src and :frame_src supplied and do not match. This can lead to inconsistent behavior across browsers."
-      end
-
-      @config.frame_src || @config.child_src
-    end
-
     # Private: converts the config object into a string representing a policy.
     # Places default-src at the first directive and report-uri as the last. All
     # others are presented in alphabetical order.
     #
-    # Unsupported directives are filtered based on the user agent.
-    #
     # Returns a content security policy header value.
     def build_value
       directives.map do |directive_name|
@@ -125,18 +102,7 @@ def build_media_type_list_directive(directive)
     #
     # Returns a string representing a directive.
     def build_source_list_directive(directive)
-      source_list = case directive
-      when :child_src
-        if supported_directives.include?(:child_src)
-          @frame_src
-        end
-      when :frame_src
-        unless supported_directives.include?(:child_src)
-          @frame_src
-        end
-      else
-        @config.directive_value(directive)
-      end
+      source_list = @config.directive_value(directive)
 
       if source_list != OPT_OUT && source_list && source_list.any?
         normalized_source_list = minify_source_list(directive, source_list)
@@ -219,13 +185,13 @@ def append_nonce(source_list, nonce)
       source_list
     end
 
-    # Private: return the list of directives that are supported by the user agent,
+    # Private: return the list of directives,
     # starting with default-src and ending with report-uri.
     def directives
       [
         DEFAULT_SRC,
-        BODY_DIRECTIVES.select { |key| supported_directives.include?(key) },
-        REPORT_URI
+        BODY_DIRECTIVES,
+        REPORT_URI,
       ].flatten
     end
 
@@ -234,25 +200,6 @@ def strip_source_schemes(source_list)
       source_list.map { |source_expression| source_expression.sub(HTTP_SCHEME_REGEX, "") }
     end
 
-    # Private: determine which directives are supported for the given user agent.
-    #
-    # Add UA-sniffing special casing here.
-    #
-    # Returns an array of symbols representing the directives.
-    def supported_directives
-      @supported_directives ||= if VARIATIONS[@parsed_ua.browser]
-        if @parsed_ua.browser == "Firefox" && ((@parsed_ua.version || FALLBACK_VERSION) >= VERSION_46)
-          VARIATIONS["FirefoxTransitional"]
-        elsif @parsed_ua.browser == "Safari" && ((@parsed_ua.version || FALLBACK_VERSION) >= VERSION_10)
-          VARIATIONS["SafariTransitional"]
-        else
-          VARIATIONS[@parsed_ua.browser]
-        end
-      else
-        VARIATIONS[OTHER]
-      end
-    end
-
     def symbol_to_hyphen_case(sym)
       sym.to_s.tr("_", "-")
     end

lib/secure_headers/headers/policy_management.rb

@@ -81,58 +81,12 @@ def self.included(base)
       UPGRADE_INSECURE_REQUESTS
     ].flatten.freeze
 
-    EDGE_DIRECTIVES = DIRECTIVES_1_0
-    SAFARI_DIRECTIVES = DIRECTIVES_1_0
-    SAFARI_10_DIRECTIVES = DIRECTIVES_2_0
-
-    FIREFOX_UNSUPPORTED_DIRECTIVES = [
-      BLOCK_ALL_MIXED_CONTENT,
-      CHILD_SRC,
-      WORKER_SRC,
-      PLUGIN_TYPES
-    ].freeze
-
-    FIREFOX_46_DEPRECATED_DIRECTIVES = [
-      FRAME_SRC
-    ].freeze
-
-    FIREFOX_46_UNSUPPORTED_DIRECTIVES = [
-      BLOCK_ALL_MIXED_CONTENT,
-      WORKER_SRC,
-      PLUGIN_TYPES
-    ].freeze
-
-    FIREFOX_DIRECTIVES = (
-      DIRECTIVES_3_0 - FIREFOX_UNSUPPORTED_DIRECTIVES
-    ).freeze
-
-    FIREFOX_46_DIRECTIVES = (
-      DIRECTIVES_3_0 - FIREFOX_46_UNSUPPORTED_DIRECTIVES - FIREFOX_46_DEPRECATED_DIRECTIVES
-    ).freeze
-
-    CHROME_DIRECTIVES = (
-      DIRECTIVES_3_0
-    ).freeze
-
     ALL_DIRECTIVES = (DIRECTIVES_1_0 + DIRECTIVES_2_0 + DIRECTIVES_3_0).uniq.sort
 
     # Think of default-src and report-uri as the beginning and end respectively,
     # everything else is in between.
     BODY_DIRECTIVES = ALL_DIRECTIVES - [DEFAULT_SRC, REPORT_URI]
 
-    VARIATIONS = {
-      "Chrome" => CHROME_DIRECTIVES,
-      "Opera" => CHROME_DIRECTIVES,
-      "Firefox" => FIREFOX_DIRECTIVES,
-      "FirefoxTransitional" => FIREFOX_46_DIRECTIVES,
-      "Safari" => SAFARI_DIRECTIVES,
-      "SafariTransitional" => SAFARI_10_DIRECTIVES,
-      "Edge" => EDGE_DIRECTIVES,
-      "Other" => CHROME_DIRECTIVES
-    }.freeze
-
-    OTHER = "Other".freeze
-
     DIRECTIVE_VALUE_TYPES = {
       BASE_URI                  => :source_list,
       BLOCK_ALL_MIXED_CONTENT   => :boolean,
@@ -199,9 +153,9 @@ module ClassMethods
       #
       # Returns a default policy if no configuration is provided, or a
       # header name and value based on the config.
-      def make_header(config, user_agent = nil)
+      def make_header(config)
         return if config.nil? || config == OPT_OUT
-        header = new(config, user_agent)
+        header = new(config)
         [header.name, header.value]
       end
 
@@ -303,17 +257,11 @@ def populate_fetch_source_with_default!(original, additions)
           # Don't set a default if directive has an existing value
           next if original[directive]
           if FETCH_SOURCES.include?(directive)
-            original[directive] = default_for(directive, original)
+            original[directive] = original[DEFAULT_SRC]
           end
         end
       end
 
-      def default_for(directive, original)
-        return original[FRAME_SRC] if directive == CHILD_SRC && original[FRAME_SRC]
-        return original[CHILD_SRC] if directive == FRAME_SRC && original[CHILD_SRC]
-        original[DEFAULT_SRC]
-      end
-
       def source_list?(directive)
         DIRECTIVE_VALUE_TYPES[directive] == :source_list
       end

secure_headers.gemspec

@@ -16,5 +16,4 @@ Gem::Specification.new do |gem|
   gem.test_files    = gem.files.grep(%r{^(test|spec|features)/})
   gem.require_paths = ["lib"]
   gem.add_development_dependency "rake"
-  gem.add_dependency "useragent", ">= 0.15.0"
 end

spec/lib/secure_headers/headers/content_security_policy_spec.rb

@@ -116,73 +116,10 @@ module SecureHeaders
         ContentSecurityPolicy.new(default_src: %w('self'), frame_src: %w('self')).value
       end
 
-      it "raises an error when child-src and frame-src are supplied but are not equal" do
-        expect {
-          ContentSecurityPolicy.new(default_src: %w('self'), child_src: %w(child-src.com), frame_src: %w(frame-src,com)).value
-        }.to raise_error(ArgumentError)
-      end
-
       it "supports strict-dynamic" do
-        csp = ContentSecurityPolicy.new({default_src: %w('self'), script_src: [ContentSecurityPolicy::STRICT_DYNAMIC], script_nonce: 123456}, USER_AGENTS[:chrome])
+        csp = ContentSecurityPolicy.new({default_src: %w('self'), script_src: [ContentSecurityPolicy::STRICT_DYNAMIC], script_nonce: 123456})
         expect(csp.value).to eq("default-src 'self'; script-src 'strict-dynamic' 'nonce-123456' 'unsafe-inline'")
       end
-
-      context "browser sniffing" do
-        let (:complex_opts) do
-          (ContentSecurityPolicy::ALL_DIRECTIVES - [:frame_src]).each_with_object({}) do |directive, hash|
-            hash[directive] = ["#{directive.to_s.gsub("_", "-")}.com"]
-          end.merge({
-            block_all_mixed_content: true,
-            upgrade_insecure_requests: true,
-            script_src: %w(script-src.com),
-            script_nonce: 123456,
-            sandbox: %w(allow-forms),
-            plugin_types: %w(application/pdf)
-          })
-        end
-
-        it "does not filter any directives for Chrome" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:chrome])
-          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; block-all-mixed-content; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; manifest-src manifest-src.com; media-src media-src.com; object-src object-src.com; plugin-types application/pdf; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; upgrade-insecure-requests; worker-src worker-src.com; report-uri report-uri.com")
-        end
-
-        it "does not filter any directives for Opera" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:opera])
-          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; block-all-mixed-content; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; manifest-src manifest-src.com; media-src media-src.com; object-src object-src.com; plugin-types application/pdf; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; upgrade-insecure-requests; worker-src worker-src.com; report-uri report-uri.com")
-        end
-
-        it "filters blocked-all-mixed-content, child-src, and plugin-types for firefox" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:firefox])
-          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; frame-src child-src.com; img-src img-src.com; manifest-src manifest-src.com; media-src media-src.com; object-src object-src.com; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
-        end
-
-        it "filters blocked-all-mixed-content, frame-src, and plugin-types for firefox 46 and higher" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:firefox46])
-          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; manifest-src manifest-src.com; media-src media-src.com; object-src object-src.com; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
-        end
-
-        it "child-src value is copied to frame-src, adds 'unsafe-inline', filters base-uri, blocked-all-mixed-content, upgrade-insecure-requests, child-src, form-action, frame-ancestors, hash sources, and plugin-types for Edge" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:edge])
-          expect(policy.value).to eq("default-src default-src.com; connect-src connect-src.com; font-src font-src.com; frame-src child-src.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; report-uri report-uri.com")
-        end
-
-        it "child-src value is copied to frame-src, adds 'unsafe-inline', filters base-uri, blocked-all-mixed-content, upgrade-insecure-requests, child-src, form-action, frame-ancestors, hash sources, and plugin-types for safari" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:safari6])
-          expect(policy.value).to eq("default-src default-src.com; connect-src connect-src.com; font-src font-src.com; frame-src child-src.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; report-uri report-uri.com")
-        end
-
-        it "adds 'unsafe-inline', filters  blocked-all-mixed-content, upgrade-insecure-requests, and hash sources for safari 10 and higher" do
-          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:safari10])
-          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; plugin-types application/pdf; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; report-uri report-uri.com")
-        end
-
-        it "falls back to standard Firefox defaults when the useragent version is not present" do
-          ua = USER_AGENTS[:firefox].dup
-          allow(ua).to receive(:version).and_return(nil)
-          policy = ContentSecurityPolicy.new(complex_opts, ua)
-          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; frame-src child-src.com; img-src img-src.com; manifest-src manifest-src.com; media-src media-src.com; object-src object-src.com; sandbox allow-forms; script-src script-src.com 'nonce-123456' 'unsafe-inline'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
-        end
-      end
     end
   end
 end

spec/lib/secure_headers/headers/policy_management_spec.rb

@@ -190,7 +190,7 @@ module SecureHeaders
         report_uri = "https://report-uri.io/asdf"
         default_policy = Configuration.dup
         combined_config = ContentSecurityPolicy.combine_policies(default_policy.csp.to_h, report_uri: [report_uri])
-        csp = ContentSecurityPolicy.new(combined_config, USER_AGENTS[:firefox])
+        csp = ContentSecurityPolicy.new(combined_config)
         expect(csp.value).to include("report-uri #{report_uri}")
       end
 
@@ -223,7 +223,7 @@ module SecureHeaders
         end
         default_policy = Configuration.dup
         combined_config = ContentSecurityPolicy.combine_policies(default_policy.csp.to_h, report_only: true)
-        csp = ContentSecurityPolicy.new(combined_config, USER_AGENTS[:firefox])
+        csp = ContentSecurityPolicy.new(combined_config)
         expect(csp.name).to eq(ContentSecurityPolicyReportOnlyConfig::HEADER_NAME)
       end
 

spec/lib/secure_headers_spec.rb

@@ -127,27 +127,6 @@ module SecureHeaders
         expect(hash[XFrameOptions::HEADER_NAME]).to eq(XFrameOptions::SAMEORIGIN)
       end
 
-      it "produces a UA-specific CSP when overriding (and busting the cache)" do
-        Configuration.default do |config|
-          config.csp = {
-            default_src: %w('self'),
-            script_src: %w('self'),
-            child_src: %w('self')
-          }
-        end
-        firefox_request = Rack::Request.new(request.env.merge("HTTP_USER_AGENT" => USER_AGENTS[:firefox]))
-
-        # append an unsupported directive
-        SecureHeaders.override_content_security_policy_directives(firefox_request, {plugin_types: %w(application/pdf)})
-        # append a supported directive
-        SecureHeaders.override_content_security_policy_directives(firefox_request, {script_src: %w('self')})
-
-        hash = SecureHeaders.header_hash_for(firefox_request)
-
-        # child-src is translated to frame-src
-        expect(hash[ContentSecurityPolicyConfig::HEADER_NAME]).to eq("default-src 'self'; frame-src 'self'; script-src 'self'")
-      end
-
       it "produces a hash of headers with default config" do
         Configuration.default
         hash = SecureHeaders.header_hash_for(request)
@@ -197,22 +176,6 @@ module SecureHeaders
           expect(hash[ContentSecurityPolicyConfig::HEADER_NAME]).to eq("default-src 'self'; script-src mycdn.com 'unsafe-inline' anothercdn.com")
         end
 
-        it "child-src and frame-src must match" do
-          Configuration.default do |config|
-            config.csp = {
-              default_src: %w('self'),
-              frame_src: %w(frame_src.com),
-              script_src: %w('self')
-            }
-          end
-
-          SecureHeaders.append_content_security_policy_directives(chrome_request, child_src: %w(child_src.com))
-
-          expect {
-            SecureHeaders.header_hash_for(chrome_request)
-          }.to raise_error(ArgumentError)
-        end
-
         it "supports named appends" do
           Configuration.default do |config|
             config.csp = {