Merge pull request #232 from twitter/single-idempotency-check

Handle very dynamic policies better

Author: oreoshake

lib/secure_headers.rb

@@ -48,14 +48,12 @@ class << self
     #    script_src: %w(another-host.com)
     def override_content_security_policy_directives(request, additions)
       config = config_for(request)
-      unless CSP.idempotent_additions?(config.csp, additions)
-        config = config.dup
-        if config.csp == OPT_OUT
-          config.csp = {}
-        end
-        config.csp.merge!(additions)
-        override_secure_headers_request_config(request, config)
+      if config.current_csp == OPT_OUT
+        config.dynamic_csp = {}
       end
+
+      config.dynamic_csp = config.current_csp.merge(additions)
+      override_secure_headers_request_config(request, config)
     end
 
     # Public: appends source values to the current configuration. If no value
@@ -66,11 +64,8 @@ def override_content_security_policy_directives(request, additions)
     #    script_src: %w(another-host.com)
     def append_content_security_policy_directives(request, additions)
       config = config_for(request)
-      unless CSP.idempotent_additions?(config.csp, additions)
-        config = config.dup
-        config.csp = CSP.combine_policies(config.csp, additions)
-        override_secure_headers_request_config(request, config)
-      end
+      config.dynamic_csp = CSP.combine_policies(config.current_csp, additions)
+      override_secure_headers_request_config(request, config)
     end
 
     # Public: override X-Frame-Options settings for this request.
@@ -79,16 +74,16 @@ def append_content_security_policy_directives(request, additions)
     #
     # Returns the current config
     def override_x_frame_options(request, value)
-      default_config = config_for(request).dup
-      default_config.x_frame_options = value
-      override_secure_headers_request_config(request, default_config)
+      config = config_for(request)
+      config.update_x_frame_options(value)
+      override_secure_headers_request_config(request, config)
     end
 
     # Public: opts out of setting a given header by creating a temporary config
     # and setting the given headers config to OPT_OUT.
     def opt_out_of_header(request, header_key)
-      config = config_for(request).dup
-      config.send("#{header_key}=", OPT_OUT)
+      config = config_for(request)
+      config.opt_out(header_key)
       override_secure_headers_request_config(request, config)
     end
 
@@ -109,14 +104,11 @@ def opt_out_of_all_protection(request)
     # in Rack middleware.
     def header_hash_for(request)
       config = config_for(request)
-
-      headers = if cached_headers = config.cached_headers
-        use_cached_headers(cached_headers, request)
-      else
-        build_headers(config, request)
+      unless ContentSecurityPolicy.idempotent_additions?(config.csp, config.current_csp)
+        config.rebuild_csp_header_cache!(request.user_agent)
       end
 
-      headers
+      use_cached_headers(config.cached_headers, request)
     end
 
     # Public: specify which named override will be used for this request.
@@ -155,8 +147,14 @@ def content_security_policy_style_nonce(request)
     # Checks to see if a named override is used for this request, then
     # Falls back to the global config
     def config_for(request)
-      request.env[SECURE_HEADERS_CONFIG] ||
+      config = request.env[SECURE_HEADERS_CONFIG] ||
         Configuration.get(Configuration::DEFAULT_CONFIG)
+
+      if config.frozen?
+        config.dup
+      else
+        config
+      end
     end
 
     private
@@ -191,36 +189,17 @@ def header_classes_for(request)
       end
     end
 
-    # Private: do the heavy lifting of converting a configuration object
-    # to a hash of headers valid for this request.
-    #
-    # Returns a hash of header names / values.
-    def build_headers(config, request)
-      header_classes_for(request).each_with_object({}) do |klass, hash|
-        header_config = if config
-          config.fetch(klass::CONFIG_KEY)
-        end
-
-        header_name, value = if klass == CSP
-          make_header(klass, header_config, request.user_agent)
-        else
-          make_header(klass, header_config)
-        end
-        hash[header_name] = value if value
-      end
-    end
-
     # Private: takes a precomputed hash of headers and returns the Headers
     # customized for the request.
     #
     # Returns a hash of header names / values valid for a given request.
-    def use_cached_headers(default_headers, request)
+    def use_cached_headers(headers, request)
       header_classes_for(request).each_with_object({}) do |klass, hash|
-        if default_header = default_headers[klass::CONFIG_KEY]
+        if header = headers[klass::CONFIG_KEY]
           header_name, value = if klass == CSP
-            default_csp_header_for_ua(default_header, request)
+            csp_header_for_ua(header, request)
           else
-            default_header
+            header
           end
           hash[header_name] = value
         end
@@ -232,13 +211,8 @@ def use_cached_headers(default_headers, request)
     # headers - a hash of header_config_key => [header_name, header_value]
     #
     # Returns a CSP [header, value] array
-    def default_csp_header_for_ua(headers, request)
-      family = UserAgent.parse(request.user_agent).browser
-      if CSP::VARIATIONS.key?(family)
-        headers[family]
-      else
-        headers[CSP::OTHER]
-      end
+    def csp_header_for_ua(headers, request)
+      headers[CSP.ua_to_variation(UserAgent.parse(request.user_agent))]
     end
 
     # Private: optionally build a header with a given configure

lib/secure_headers/configuration.rb

@@ -3,6 +3,7 @@ class Configuration
     DEFAULT_CONFIG = :default
     NOOP_CONFIGURATION = "secure_headers_noop_config"
     class NotYetConfiguredError < StandardError; end
+    class IllegalPolicyModificationError < StandardError; end
     class << self
       # Public: Set the global default configuration.
       #
@@ -23,12 +24,12 @@ def default(&block)
       # if no value is supplied.
       #
       # Returns: the newly created config
-      def override(name, base = DEFAULT_CONFIG)
+      def override(name, base = DEFAULT_CONFIG, &block)
         unless get(base)
           raise NotYetConfiguredError, "#{base} policy not yet supplied"
         end
         override = @configurations[base].dup
-        yield(override)
+        override.instance_eval &block if block_given?
         add_configuration(name, override)
       end
 
@@ -43,18 +44,6 @@ def get(name = DEFAULT_CONFIG)
         @configurations[name]
       end
 
-      # Public: perform a basic deep dup. The shallow copy provided by dup/clone
-      # can lead to modifying parent objects.
-      def deep_copy(config)
-        config.each_with_object({}) do |(key, value), hash|
-          hash[key] = if value.is_a?(Array)
-            value.dup
-          else
-            value
-          end
-        end
-      end
-
       private
 
       # Private: add a valid configuration to the global set of named configs.
@@ -86,16 +75,39 @@ def add_noop_configuration
 
         add_configuration(NOOP_CONFIGURATION, noop_config)
       end
+
+      # Public: perform a basic deep dup. The shallow copy provided by dup/clone
+      # can lead to modifying parent objects.
+      def deep_copy(config)
+        config.each_with_object({}) do |(key, value), hash|
+          hash[key] = if value.is_a?(Array)
+            value.dup
+          else
+            value
+          end
+        end
+      end
+
+      # Private: convenience method purely DRY things up. The value may not be a
+      # hash (e.g. OPT_OUT, nil)
+      def deep_copy_if_hash(value)
+        if value.is_a?(Hash)
+          deep_copy(value)
+        else
+          value
+        end
+      end
     end
 
-    attr_accessor :hsts, :x_frame_options, :x_content_type_options,
-      :x_xss_protection, :csp, :x_download_options, :x_permitted_cross_domain_policies,
-      :hpkp, :secure_cookies
-    attr_reader :cached_headers
+    attr_writer :hsts, :x_frame_options, :x_content_type_options,
+      :x_xss_protection, :x_download_options, :x_permitted_cross_domain_policies,
+      :hpkp, :dynamic_csp, :secure_cookies
+
+    attr_reader :cached_headers, :csp, :dynamic_csp, :secure_cookies
 
     def initialize(&block)
       self.hpkp = OPT_OUT
-      self.csp = self.class.deep_copy(CSP::DEFAULT_CONFIG)
+      self.csp = self.class.send(:deep_copy, CSP::DEFAULT_CONFIG)
       instance_eval &block if block_given?
     end
 
@@ -104,33 +116,37 @@ def initialize(&block)
     # Returns a deep-dup'd copy of this configuration.
     def dup
       copy = self.class.new
-      copy.hsts = hsts
-      copy.x_frame_options = x_frame_options
-      copy.x_content_type_options = x_content_type_options
-      copy.x_xss_protection = x_xss_protection
-      copy.x_download_options = x_download_options
-      copy.x_permitted_cross_domain_policies = x_permitted_cross_domain_policies
-      copy.csp = if csp.is_a?(Hash)
-        self.class.deep_copy(csp)
-      else
-        csp
+      copy.secure_cookies = @secure_cookies
+      copy.csp = self.class.send(:deep_copy_if_hash, @csp)
+      copy.dynamic_csp = self.class.send(:deep_copy_if_hash, @dynamic_csp)
+      copy.cached_headers = self.class.send(:deep_copy_if_hash, @cached_headers)
+      copy
+    end
+
+    def opt_out(header)
+      send("#{header}=", OPT_OUT)
+      if header == CSP::CONFIG_KEY
+        dynamic_csp = OPT_OUT
       end
+      self.cached_headers.delete(header)
+    end
+
+    def update_x_frame_options(value)
+      self.cached_headers[XFrameOptions::CONFIG_KEY] = XFrameOptions.make_header(value)
+    end
 
-      copy.hpkp = if hpkp.is_a?(Hash)
-        self.class.deep_copy(hpkp)
-      else
-        hpkp
+    # Public: generated cached headers for a specific user agent.
+    def rebuild_csp_header_cache!(user_agent)
+      self.cached_headers[CSP::CONFIG_KEY] = {}
+      unless current_csp == OPT_OUT
+        user_agent = UserAgent.parse(user_agent)
+        variation = CSP.ua_to_variation(user_agent)
+        self.cached_headers[CSP::CONFIG_KEY][variation] = CSP.make_header(current_csp, user_agent)
       end
-      copy
     end
 
-    # Public: Retrieve a config based on the CONFIG_KEY for a class
-    #
-    # Returns the value if available, and returns a dup of any hash values.
-    def fetch(key)
-      config = send(key)
-      config = self.class.deep_copy(config) if config.is_a?(Hash)
-      config
+    def current_csp
+      @dynamic_csp || @csp
     end
 
     # Public: validates all configurations values.
@@ -139,24 +155,40 @@ def fetch(key)
     #
     # Returns nothing
     def validate_config!
-      StrictTransportSecurity.validate_config!(hsts)
-      ContentSecurityPolicy.validate_config!(csp)
-      XFrameOptions.validate_config!(x_frame_options)
-      XContentTypeOptions.validate_config!(x_content_type_options)
-      XXssProtection.validate_config!(x_xss_protection)
-      XDownloadOptions.validate_config!(x_download_options)
-      XPermittedCrossDomainPolicies.validate_config!(x_permitted_cross_domain_policies)
-      PublicKeyPins.validate_config!(hpkp)
+      StrictTransportSecurity.validate_config!(@hsts)
+      ContentSecurityPolicy.validate_config!(@csp)
+      XFrameOptions.validate_config!(@x_frame_options)
+      XContentTypeOptions.validate_config!(@x_content_type_options)
+      XXssProtection.validate_config!(@x_xss_protection)
+      XDownloadOptions.validate_config!(@x_download_options)
+      XPermittedCrossDomainPolicies.validate_config!(@x_permitted_cross_domain_policies)
+      PublicKeyPins.validate_config!(@hpkp)
     end
 
+    protected
+
+    def csp=(new_csp)
+      if self.dynamic_csp
+        raise IllegalPolicyModificationError, "You are attempting to modify CSP settings directly. Use dynamic_csp= isntead."
+      end
+
+      @csp = new_csp
+    end
+
+    def cached_headers=(headers)
+      @cached_headers = headers
+    end
+
+    private
+
     # Public: Precompute the header names and values for this configuraiton.
     # Ensures that headers generated at configure time, not on demand.
     #
     # Returns the cached headers
     def cache_headers!
       # generate defaults for the "easy" headers
       headers = (ALL_HEADERS_BESIDES_CSP).each_with_object({}) do |klass, hash|
-        config = fetch(klass::CONFIG_KEY)
+        config = instance_variable_get("@#{klass::CONFIG_KEY}")
         unless config == OPT_OUT
           hash[klass::CONFIG_KEY] = klass.make_header(config).freeze
         end
@@ -165,7 +197,7 @@ def cache_headers!
       generate_csp_headers(headers)
 
       headers.freeze
-      @cached_headers = headers
+      self.cached_headers = headers
     end
 
     # Private: adds CSP headers for each variation of CSP support.
@@ -175,11 +207,10 @@ def cache_headers!
     #
     # Returns nothing
     def generate_csp_headers(headers)
-      unless csp == OPT_OUT
+      unless @csp == OPT_OUT
         headers[CSP::CONFIG_KEY] = {}
-
+        csp_config = self.current_csp
         CSP::VARIATIONS.each do |name, _|
-          csp_config = fetch(CSP::CONFIG_KEY)
           csp = CSP.make_header(csp_config, UserAgent.parse(name))
           headers[CSP::CONFIG_KEY][name] = csp.freeze
         end

lib/secure_headers/headers/policy_management.rb

@@ -197,7 +197,7 @@ def validate_config!(config)
       # because google.com is already in the config.
       def idempotent_additions?(config, additions)
         return false if config == OPT_OUT
-        config.to_s == combine_policies(config, additions).to_s
+        config == combine_policies(config, additions)
       end
 
       # Public: combine the values from two different configs.
@@ -218,11 +218,19 @@ def combine_policies(original, additions)
           raise ContentSecurityPolicyConfigError.new("Attempted to override an opt-out CSP config.")
         end
 
-        original = original.dup if original.frozen?
+        original = Configuration.send(:deep_copy, original)
         populate_fetch_source_with_default!(original, additions)
         merge_policy_additions(original, additions)
       end
 
+      def ua_to_variation(user_agent)
+        if family = user_agent.browser && VARIATIONS.key?(family)
+          VARIATIONS[family]
+        else
+          OTHER
+        end
+      end
+
       private
 
       # merge the two hashes. combine (instead of overwrite) the array values