consolidate escape sequence capturing in one validation, add suspende…

…rs to check if one snuck by
github · oreoshake · Jan 23, 2020 · Jan 23, 2020 · Jan 23, 2020 · Jan 23, 2020
commit 87dba71b9256db78ecab2b25a45d5526300ca213
diff --git a/lib/secure_headers/headers/content_security_policy.rb b/lib/secure_headers/headers/content_security_policy.rb
@@ -105,6 +105,11 @@ def build_source_list_directive(directive)
       source_list = @config.directive_value(directive)
       if source_list != OPT_OUT && source_list && source_list.any?
         minified_source_list = minify_source_list(directive, source_list).join(" ")
+        if minified_source_list =~ ESCAPE_SEQUENCE
+          # Should not happen. Just in case.
+          raise ContentSecurityPolicyConfigError.new("generated #{directive} contains a #{$1.inspect} in #{minified_source_list.inspect}")
+        end
+
         [symbol_to_hyphen_case(directive), minified_source_list].join(" ").strip
       end
     end

diff --git a/lib/secure_headers/headers/policy_management.rb b/lib/secure_headers/headers/policy_management.rb
@@ -329,7 +329,7 @@ def validate_sandbox_expression!(directive, sandbox_token_expression)
         return if boolean?(sandbox_token_expression) && sandbox_token_expression == true
         ensure_array_of_strings!(directive, sandbox_token_expression)
         valid = sandbox_token_expression.compact.all? do |v|
-          v.is_a?(String) && v.start_with?("allow-") && v !~ ESCAPE_SEQUENCE
+          v.is_a?(String) && v.start_with?("allow-")
         end
         if !valid
           raise ContentSecurityPolicyConfigError.new("#{directive} must be True or an array of zero or more sandbox token strings (ex. allow-forms). Was #{sandbox_token_expression.inspect}")
@@ -380,7 +380,18 @@ def ensure_valid_directive!(directive)
       end
 
       def ensure_array_of_strings!(directive, value)
-        if (!value.is_a?(Array) || !value.compact.all? { |v| v.is_a?(String) })
+        unless value.is_a?(Array)
+          raise ContentSecurityPolicyConfigError.new("#{directive} must be an array of strings")
+        end
+
+        all_valid_strings = value.compact.all? do |v|
+          if v =~ ESCAPE_SEQUENCE
+            raise ContentSecurityPolicyConfigError.new("#{directive} contains a #{$1.inspect} in #{v.inspect}")
+          end
+          v.is_a?(String)
+        end
+
+        unless all_valid_strings
           raise ContentSecurityPolicyConfigError.new("#{directive} must be an array of strings")
         end
       end
@@ -391,10 +402,6 @@ def ensure_valid_sources!(directive, source_expression)
           if ContentSecurityPolicy::DEPRECATED_SOURCE_VALUES.include?(expression)
             raise ContentSecurityPolicyConfigError.new("#{directive} contains an invalid keyword source (#{expression}). This value must be single quoted.")
           end
-
-          if expression =~ ESCAPE_SEQUENCE
-            raise ContentSecurityPolicyConfigError.new("#{directive} contains a #{$1.inspect} in #{expression.inspect}")
-          end
         end
       end
 

diff --git a/spec/lib/secure_headers/headers/policy_management_spec.rb b/spec/lib/secure_headers/headers/policy_management_spec.rb
@@ -113,7 +113,13 @@ module SecureHeaders
       it "disallows newlines in configs" do
         expect do
           ContentSecurityPolicy.validate_config!(ContentSecurityPolicyConfig.new(default_src: %w('self'), script_src: %w(mycdn.com), object_src: ["http://foo.com \n script-src *"]))
-        end.to raise_error(ContentSecurityPolicyConfigError, %r{object_src contains a "\\n" in "http://foo.com \\n script-src})
+        end.to raise_error(ContentSecurityPolicyConfigError, %r(object_src contains a "\\n" in "http://foo.com \\n script-src))
+      end
+
+      it "catches semicolons that somehow bypass validation" do
+        expect do
+          ContentSecurityPolicy.new(ContentSecurityPolicyConfig.new(default_src: %w('self'), script_src: %w(mycdn.com), object_src: ["http://foo.com ; script-src *"])).value
+        end.to raise_error(ContentSecurityPolicyConfigError, %(generated object_src contains a ";" in "foo.com ; script-src *"))
       end
 
       it "allows nil values" do
@@ -141,16 +147,16 @@ module SecureHeaders
         end.to raise_error(ContentSecurityPolicyConfigError, %(sandbox must be True or an array of zero or more sandbox token strings (ex. allow-forms). Was ["steve"]))
       end
 
-      it "accepts anything of the form allow-* as a sandbox value " do
+      it "accepts anything of the form allow-* as a sandbox value" do
         expect do
           ContentSecurityPolicy.validate_config!(ContentSecurityPolicyConfig.new(default_opts.merge(sandbox: ["allow-foo"])))
         end.to_not raise_error
       end
 
-      it "rejects escapes in sandbox value " do
+      it "rejects escapes in sandbox value" do
         expect do
           ContentSecurityPolicy.validate_config!(ContentSecurityPolicyConfig.new(default_opts.merge(sandbox: ["allow-; script-src foo"])))
-        end.to raise_error(ContentSecurityPolicyConfigError, %(sandbox must be True or an array of zero or more sandbox token strings (ex. allow-forms). Was ["allow-; script-src foo"]))
+        end.to raise_error(ContentSecurityPolicyConfigError, %(sandbox contains a ";" in "allow-; script-src foo"))
       end
 
       it "accepts true as a sandbox policy" do
@@ -168,10 +174,10 @@ module SecureHeaders
       it "rejects escape values as plugin-type value" do
         expect do
           ContentSecurityPolicy.validate_config!(ContentSecurityPolicyConfig.new(default_opts.merge(plugin_types: [";/script-src*"])))
-        end.to raise_error(ContentSecurityPolicyConfigError, "plugin_types must be an array of valid media types (ex. application/pdf)")
+        end.to raise_error(ContentSecurityPolicyConfigError, %(plugin_types contains a ";" in ";/script-src*"))
       end
 
-      it "accepts anything of the form type/subtype as a plugin-type value " do
+      it "accepts anything of the form type/subtype as a plugin-type value" do
         expect do
           ContentSecurityPolicy.validate_config!(ContentSecurityPolicyConfig.new(default_opts.merge(plugin_types: ["application/pdf"])))
         end.to_not raise_error