Merge pull request #386 from twitter/deprecate-get

Deprecate Configuration#get

Author: oreoshake

Gemfile

@@ -19,4 +19,5 @@ group :guard do
   gem "growl"
   gem "guard-rspec", platforms: [:ruby_19, :ruby_20, :ruby_21, :ruby_22, :ruby_23, :ruby_24]
   gem "rb-fsevent"
+  gem "terminal-notifier-guard"
 end

lib/secure_headers.rb

@@ -196,7 +196,7 @@ def header_hash_for(request)
     #
     # name - the name of the previously configured override.
     def use_secure_headers_override(request, name)
-      if config = Configuration.get(name)
+      if config = Configuration.get(name, internal: true)
         override_secure_headers_request_config(request, config)
       else
         raise ArgumentError.new("no override by the name of #{name} has been configured")
@@ -228,7 +228,7 @@ def content_security_policy_style_nonce(request)
     # Falls back to the global config
     def config_for(request, prevent_dup = false)
       config = request.env[SECURE_HEADERS_CONFIG] ||
-        Configuration.get(Configuration::DEFAULT_CONFIG)
+        Configuration.get(Configuration::DEFAULT_CONFIG, internal: true)
 
 
       # Global configs are frozen, per-request configs are not. When we're not

lib/secure_headers/configuration.rb

@@ -28,7 +28,7 @@ def default(&block)
       #
       # Returns: the newly created config
       def override(name, base = DEFAULT_CONFIG, &block)
-        unless get(base)
+        unless get(base, internal: true)
           raise NotYetConfiguredError, "#{base} policy not yet supplied"
         end
         override = @configurations[base].dup
@@ -40,7 +40,11 @@ def override(name, base = DEFAULT_CONFIG, &block)
       #
       # Returns the configuration with a given name or raises a
       # NotYetConfiguredError if `default` has not been called.
-      def get(name = DEFAULT_CONFIG)
+      def get(name = DEFAULT_CONFIG, internal: false)
+        unless internal
+          Kernel.warn "#{Kernel.caller.first}: [DEPRECATION] `#get` is deprecated. It will be removed in the next major release. Use SecureHeaders::Configuration.dup to retrieve the default config."
+        end
+
         if @configurations.nil?
           raise NotYetConfiguredError, "Default policy not yet supplied"
         end

lib/secure_headers/headers/clear_site_data.rb

@@ -48,7 +48,7 @@ def validate_config!(config)
       #
       # Returns a String of quoted values that are comma separated.
       def make_header_value(types)
-        types.map { |t| "\"#{t}\""}.join(", ")
+        types.map { |t| %("#{t}") }.join(", ")
       end
     end
   end

spec/lib/secure_headers/configuration_spec.rb

@@ -9,15 +9,20 @@ module SecureHeaders
     end
 
     it "has a default config" do
-      expect(Configuration.get(Configuration::DEFAULT_CONFIG)).to_not be_nil
+      expect(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true)).to_not be_nil
+    end
+
+    it "warns when using deprecated internal-ish #get API" do
+      expect(Kernel).to receive(:warn).once.with(/`#get` is deprecated/)
+      Configuration.get(Configuration::DEFAULT_CONFIG)
     end
 
     it "has an 'noop' config" do
-      expect(Configuration.get(Configuration::NOOP_CONFIGURATION)).to_not be_nil
+      expect(Configuration.get(Configuration::NOOP_CONFIGURATION, internal: true)).to_not be_nil
     end
 
     it "precomputes headers upon creation" do
-      default_config = Configuration.get(Configuration::DEFAULT_CONFIG)
+      default_config = Configuration.get(Configuration::DEFAULT_CONFIG, internal: true)
       header_hash = default_config.cached_headers.each_with_object({}) do |(key, value), hash|
         header_name, header_value = if key == :csp
           value["Chrome"]
@@ -35,8 +40,8 @@ module SecureHeaders
         # do nothing, just copy it
       end
 
-      config = Configuration.get(:test_override)
-      noop = Configuration.get(Configuration::NOOP_CONFIGURATION)
+      config = Configuration.get(:test_override, internal: true)
+      noop = Configuration.get(Configuration::NOOP_CONFIGURATION, internal: true)
       [:csp, :csp_report_only, :cookies].each do |key|
         expect(config.send(key)).to eq(noop.send(key))
       end
@@ -47,24 +52,24 @@ module SecureHeaders
         config.x_content_type_options = OPT_OUT
       end
 
-      expect(Configuration.get.cached_headers).to_not eq(Configuration.get(:test_override).cached_headers)
+      expect(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).cached_headers).to_not eq(Configuration.get(:test_override, internal: true).cached_headers)
     end
 
     it "stores an override of the global config" do
       Configuration.override(:test_override) do |config|
         config.x_frame_options = "DENY"
       end
 
-      expect(Configuration.get(:test_override)).to_not be_nil
+      expect(Configuration.get(:test_override, internal: true)).to_not be_nil
     end
 
     it "deep dup's config values when overriding so the original cannot be modified" do
       Configuration.override(:override) do |config|
         config.csp[:default_src] << "'self'"
       end
 
-      default = Configuration.get
-      override = Configuration.get(:override)
+      default = Configuration.get(Configuration::DEFAULT_CONFIG, internal: true)
+      override = Configuration.get(:override, internal: true)
 
       expect(override.csp.directive_value(:default_src)).not_to be(default.csp.directive_value(:default_src))
     end
@@ -78,9 +83,9 @@ module SecureHeaders
         config.csp = config.csp.merge(script_src: %w(example.org))
       end
 
-      original_override = Configuration.get(:override)
+      original_override = Configuration.get(:override, internal: true)
       expect(original_override.csp.to_h).to eq(default_src: %w('self'), script_src: %w('self'))
-      override_config = Configuration.get(:second_override)
+      override_config = Configuration.get(:second_override, internal: true)
       expect(override_config.csp.to_h).to eq(default_src: %w('self'), script_src: %w('self' example.org))
     end
 
@@ -101,7 +106,7 @@ module SecureHeaders
         config.cookies = OPT_OUT
       end
 
-      config = Configuration.get
+      config = Configuration.get(Configuration::DEFAULT_CONFIG, internal: true)
       expect(config.cookies).to eq(OPT_OUT)
     end
 
@@ -110,7 +115,7 @@ module SecureHeaders
         config.cookies = {httponly: true, secure: true, samesite: {lax: false}}
       end
 
-      config = Configuration.get
+      config = Configuration.get(Configuration::DEFAULT_CONFIG, internal: true)
       expect(config.cookies).to eq({httponly: true, secure: true, samesite: {lax: false}})
     end
   end

spec/lib/secure_headers/headers/policy_management_spec.rb

@@ -152,7 +152,7 @@ module SecureHeaders
             script_src: %w('self'),
           }
         end
-        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get.csp.to_h, style_src: %w(anothercdn.com))
+        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).csp.to_h, style_src: %w(anothercdn.com))
         csp = ContentSecurityPolicy.new(combined_config)
         expect(csp.name).to eq(ContentSecurityPolicyConfig::HEADER_NAME)
         expect(csp.value).to eq("default-src https:; script-src 'self'; style-src https: anothercdn.com")
@@ -167,7 +167,7 @@ module SecureHeaders
           }.freeze
         end
         report_uri = "https://report-uri.io/asdf"
-        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get.csp.to_h, report_uri: [report_uri])
+        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).csp.to_h, report_uri: [report_uri])
         csp = ContentSecurityPolicy.new(combined_config, USER_AGENTS[:firefox])
         expect(csp.value).to include("report-uri #{report_uri}")
       end
@@ -183,7 +183,7 @@ module SecureHeaders
         non_default_source_additions = ContentSecurityPolicy::NON_FETCH_SOURCES.each_with_object({}) do |directive, hash|
           hash[directive] = %w("http://example.org)
         end
-        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get.csp.to_h, non_default_source_additions)
+        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).csp.to_h, non_default_source_additions)
 
         ContentSecurityPolicy::NON_FETCH_SOURCES.each do |directive|
           expect(combined_config[directive]).to eq(%w("http://example.org))
@@ -198,7 +198,7 @@ module SecureHeaders
             report_only: false
           }
         end
-        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get.csp.to_h, report_only: true)
+        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).csp.to_h, report_only: true)
         csp = ContentSecurityPolicy.new(combined_config, USER_AGENTS[:firefox])
         expect(csp.name).to eq(ContentSecurityPolicyReportOnlyConfig::HEADER_NAME)
       end
@@ -211,7 +211,7 @@ module SecureHeaders
             block_all_mixed_content: false
           }
         end
-        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get.csp.to_h, block_all_mixed_content: true)
+        combined_config = ContentSecurityPolicy.combine_policies(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).csp.to_h, block_all_mixed_content: true)
         csp = ContentSecurityPolicy.new(combined_config)
         expect(csp.value).to eq("default-src https:; block-all-mixed-content; script-src 'self'")
       end
@@ -221,7 +221,7 @@ module SecureHeaders
           config.csp = OPT_OUT
         end
         expect do
-          ContentSecurityPolicy.combine_policies(Configuration.get.csp.to_h, script_src: %w(anothercdn.com))
+          ContentSecurityPolicy.combine_policies(Configuration.get(Configuration::DEFAULT_CONFIG, internal: true).csp.to_h, script_src: %w(anothercdn.com))
         end.to raise_error(ContentSecurityPolicyConfigError)
       end
     end

spec/lib/secure_headers/middleware_spec.rb

@@ -50,7 +50,7 @@ module SecureHeaders
       end
       request = Rack::Request.new({})
       SecureHeaders.use_secure_headers_override(request, "my_custom_config")
-      expect(request.env[SECURE_HEADERS_CONFIG]).to be(Configuration.get("my_custom_config"))
+      expect(request.env[SECURE_HEADERS_CONFIG]).to be(Configuration.get("my_custom_config", internal: true))
       _, env = middleware.call request.env
       expect(env[ContentSecurityPolicyConfig::HEADER_NAME]).to match("example.org")
     end
@@ -66,7 +66,7 @@ module SecureHeaders
       end
 
       it "allows opting out of cookie protection with OPT_OUT alone" do
-        Configuration.default { |config| config.cookies = OPT_OUT}
+        Configuration.default { |config| config.cookies = OPT_OUT }
 
         # do NOT make this request https. non-https requests modify a config,
         # causing an exception when operating on OPT_OUT. This ensures we don't

spec/lib/secure_headers/view_helpers_spec.rb

@@ -67,7 +67,7 @@ def content_tag(type, content = nil, options = nil, &block)
     end
 
     if options.is_a?(Hash)
-      options = options.map {|k, v| " #{k}=#{v}"}
+      options = options.map { |k, v| " #{k}=#{v}" }
     end
     "<#{type}#{options}>#{content}</#{type}>"
   end