Merge pull request #208 from twitter/dont-bust-cache-for-idempotent-changes

If a policy modification is idempotent, don't dup the config

Author: oreoshake

lib/secure_headers.rb

@@ -47,12 +47,15 @@ class << self
     # additions - a hash containing directives. e.g.
     #    script_src: %w(another-host.com)
     def override_content_security_policy_directives(request, additions)
-      config = config_for(request).dup
-      if config.csp == OPT_OUT
-        config.csp = {}
+      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)
       end
-      config.csp.merge!(additions)
-      override_secure_headers_request_config(request, config)
     end
 
     # Public: appends source values to the current configuration. If no value
@@ -62,9 +65,12 @@ def override_content_security_policy_directives(request, additions)
     # additions - a hash containing directives. e.g.
     #    script_src: %w(another-host.com)
     def append_content_security_policy_directives(request, additions)
-      config = config_for(request).dup
-      config.csp = CSP.combine_policies(config.csp, additions)
-      override_secure_headers_request_config(request, config)
+      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
     end
 
     # Public: override X-Frame-Options settings for this request.

lib/secure_headers/headers/content_security_policy.rb

@@ -157,13 +157,7 @@ def make_header(config, user_agent)
         [header.name, header.value]
       end
 
-      # Public: Validates that the configuration has a valid type, or that it is a valid
-      # source expression.
-      #
-      # Private: validates that a source expression:
-      # 1. has a valid name
-      # 2. is an array of strings
-      # 3. does not contain any depreated, now invalid values (inline, eval, self, none)
+      # Public: Validates each source expression.
       #
       # Does not validate the invididual values of the source expression (e.g.
       # script_src => h*t*t*p: will not raise an exception)
@@ -179,6 +173,17 @@ def validate_config!(config)
         end
       end
 
+      # Public: determine if merging +additions+ will cause a change to the
+      # actual value of the config.
+      #
+      # e.g. config = { script_src: %w(example.org google.com)} and
+      # additions = { script_src: %w(google.com)} then idempotent_additions? would return
+      # 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
+      end
+
       # Public: combine the values from two different configs.
       #
       # original - the main config
@@ -208,11 +213,11 @@ def combine_policies(original, additions)
         # when each hash contains a value for a given key.
         original.merge(additions) do |directive, lhs, rhs|
           if source_list?(directive)
-            lhs | rhs
+            (lhs.to_a + rhs).uniq.compact
           else
             rhs
           end
-        end
+        end.reject { |_, value| value.nil? || value == [] } # this mess prevents us from adding empty directives.
       end
 
       private

spec/lib/secure_headers/headers/content_security_policy_spec.rb

@@ -109,6 +109,19 @@ module SecureHeaders
       end
     end
 
+    describe "#idempotent_additions?" do
+      specify { expect(ContentSecurityPolicy.idempotent_additions?(OPT_OUT, script_src: %w(b.com))).to be false }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, script_src: %w(c.com))).to be false }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, style_src: %w(b.com))).to be false }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, script_src: %w(a.com b.com c.com))).to be false }
+
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, script_src: %w(b.com))).to be true }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, script_src: %w(b.com a.com))).to be true }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, script_src: %w())).to be true }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, script_src: [nil])).to be true }
+      specify { expect(ContentSecurityPolicy.idempotent_additions?({script_src: %w(a.com b.com)}, style_src: [nil])).to be true }
+    end
+
     describe "#value" do
       it "discards 'none' values if any other source expressions are present" do
         csp = ContentSecurityPolicy.new(default_opts.merge(frame_src: %w('self' 'none')))