Address PR review feedback: Fix edge cases and improve consistency

- Register NOOP_OVERRIDE in disable! to avoid ArgumentError
- Clear @default_config when disable! is called after default
- Move clear_disabled inside class << self for consistency
- Add comprehensive edge case tests (calling disable! after default, default after disable!, interaction with overrides/dup)
- Document behavior in method comments

Co-authored-by: fletchto99 <[email protected]>

Author: Copilot

lib/secure_headers/configuration.rb

@@ -11,10 +11,22 @@ class IllegalPolicyModificationError < StandardError; end
     class << self
       # Public: Disable secure_headers entirely. When disabled, no headers will be set.
       #
+      # Note: This should be called before Configuration.default. Calling it after
+      # Configuration.default has been set will clear the default configuration.
+      #
       # Returns nothing
       def disable!
+        # Clear any existing default config to maintain consistency
+        remove_instance_variable(:@default_config) if defined?(@default_config)
+
         @disabled = true
         @noop_config = create_noop_config.freeze
+
+        # Ensure the built-in NOOP override is available even if `default` has never been called
+        @overrides ||= {}
+        unless @overrides.key?(NOOP_OVERRIDE)
+          @overrides[NOOP_OVERRIDE] = method(:create_noop_config_block)
+        end
       end
 
       # Public: Check if secure_headers is disabled

spec/lib/secure_headers/configuration_spec.rb

@@ -138,6 +138,66 @@ module SecureHeaders
         Configuration.disable!
         expect { Configuration.send(:default_config) }.not_to raise_error
       end
+
+      it "registers the NOOP_OVERRIDE when disabled without calling default" do
+        Configuration.disable!
+        expect(Configuration.overrides(Configuration::NOOP_OVERRIDE)).to_not be_nil
+      end
+
+      it "clears existing default config when called after default" do
+        Configuration.default do |config|
+          config.csp = { default_src: %w('self'), script_src: %w('self') }
+        end
+
+        Configuration.disable!
+
+        expect(Configuration.disabled?).to be true
+        expect(Configuration.instance_variable_defined?(:@default_config)).to be false
+      end
+
+      it "allows default to be called after disable! has been invoked" do
+        Configuration.disable!
+        reset_config
+
+        expect {
+          Configuration.default do |config|
+            config.csp = { default_src: %w('self'), script_src: %w('self') }
+          end
+        }.not_to raise_error
+
+        # After reset_config, disabled? returns nil (not false) because @disabled is removed
+        expect(Configuration.disabled?).to be_falsy
+        expect(Configuration.instance_variable_defined?(:@default_config)).to be true
+      end
+
+      it "works correctly with dup when library is disabled" do
+        Configuration.disable!
+        config = Configuration.dup
+
+        Configuration::CONFIG_ATTRIBUTES.each do |attr|
+          expect(config.instance_variable_get("@#{attr}")).to eq(OPT_OUT)
+        end
+      end
+
+      it "does not interfere with override mechanism" do
+        Configuration.disable!
+
+        # Should be able to use opt_out_of_all_protection without error
+        request = Rack::Request.new("HTTP_X_FORWARDED_SSL" => "on")
+        expect {
+          SecureHeaders.opt_out_of_all_protection(request)
+        }.not_to raise_error
+      end
+
+      it "interacts correctly with named overrides when disabled" do
+        Configuration.disable!
+
+        Configuration.override(:test_override) do |config|
+          config.x_frame_options = "DENY"
+        end
+
+        expect(Configuration.overrides(:test_override)).to_not be_nil
+      end
     end
   end
 end

spec/spec_helper.rb

@@ -53,11 +53,11 @@ def clear_overrides
       def clear_appends
         remove_instance_variable(:@appends) if defined?(@appends)
       end
-    end
 
-    def self.clear_disabled
-      remove_instance_variable(:@disabled) if defined?(@disabled)
-      remove_instance_variable(:@noop_config) if defined?(@noop_config)
+      def clear_disabled
+        remove_instance_variable(:@disabled) if defined?(@disabled)
+        remove_instance_variable(:@noop_config) if defined?(@noop_config)
+      end
     end
   end
 end