From e2b6db2f4811b480c083f791126c555b07900f55 Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Sun, 18 Jan 2026 23:06:25 +1100 Subject: [PATCH] Fix geo rule re-enablement bug When rules expire and are disabled by ExpiredRulesCleanupJob, the system was unable to re-enable them due to unique index constraints. This caused geo-based blocking to stop working in production. Implemented find-or-update-or-create pattern in WafPolicy#create_rule_for_network_range: - Re-enables disabled rules and sets new expiration (7 days) - Extends expiration for enabled rules - Creates new rules with 7-day TTL - Handles race conditions gracefully Added test coverage for all three scenarios. Co-Authored-By: Claude Sonnet 4.5 --- app/models/waf_policy.rb | 124 ++++++++++++++++++++++----------- test/models/waf_policy_test.rb | 71 ++++++++++++++++++- 2 files changed, 153 insertions(+), 42 deletions(-) diff --git a/app/models/waf_policy.rb b/app/models/waf_policy.rb index 0f958e6..f828ae2 100644 --- a/app/models/waf_policy.rb +++ b/app/models/waf_policy.rb @@ -159,49 +159,23 @@ validate :targets_must_be_array return nil end - # Try to create the rule, handling duplicates gracefully - begin - rule = Rule.create!( - waf_rule_type: 'network', - waf_action: policy_action.to_sym, - network_range: network_range, - waf_policy: self, - user: user, - source: "policy", - metadata: build_rule_metadata(network_range), - priority: network_range.prefix_length - ) - rescue ActiveRecord::RecordNotUnique - # Rule already exists (created by another job or earlier in this job) - # Find and return the existing rule - Rails.logger.debug "Rule already exists for #{network_range.cidr} with policy #{name}" - return Rule.find_by( - waf_rule_type: 'network', - waf_action: policy_action, - network_range: network_range, - waf_policy: self, - source: "policy" - ) + # Find existing rule (enabled or disabled) + existing_rule = Rule.find_by( + waf_rule_type: 'network', + waf_action: policy_action, + network_range: network_range, + waf_policy: self, + source: "policy" + ) + + if existing_rule + # Re-enable disabled rules or extend expiration for enabled rules + update_existing_rule(existing_rule) + return existing_rule end - # Handle redirect/challenge specific data - if redirect_action? && additional_data['redirect_url'] - rule.update!( - metadata: rule.metadata.merge( - redirect_url: additional_data['redirect_url'], - redirect_status: additional_data['redirect_status'] || 302 - ) - ) - elsif challenge_action? - rule.update!( - metadata: rule.metadata.merge( - challenge_type: additional_data['challenge_type'] || 'captcha', - challenge_message: additional_data['challenge_message'] - ) - ) - end - - rule + # Create new rule + create_new_rule(network_range) end def create_rule_for_event(event) @@ -451,6 +425,74 @@ validate :targets_must_be_array end end + def update_existing_rule(rule) + updates = { updated_at: Time.current } + + # Re-enable if disabled + unless rule.enabled? + updates[:enabled] = true + Rails.logger.info "Re-enabling rule ##{rule.id} for #{rule.network_range.cidr}" + end + + # Set/extend expiration to 7 days from now + # (Can be made configurable later via policy.rule_ttl field) + updates[:expires_at] = 7.days.from_now + + rule.update!(updates) unless updates.empty? + end + + def create_new_rule(network_range) + begin + rule = Rule.create!( + waf_rule_type: 'network', + waf_action: policy_action.to_sym, + network_range: network_range, + waf_policy: self, + user: user, + source: "policy", + metadata: build_rule_metadata(network_range), + priority: network_range.prefix_length, + expires_at: 7.days.from_now # Set expiration for new rules + ) + rescue ActiveRecord::RecordNotUnique + # Race condition: rule created between find_by and create + # Retry by finding and updating + existing_rule = Rule.find_by( + waf_rule_type: 'network', + waf_action: policy_action, + network_range: network_range, + waf_policy: self, + source: "policy" + ) + + if existing_rule + update_existing_rule(existing_rule) + return existing_rule + else + raise # Re-raise if we still can't find it + end + end + + # Handle redirect/challenge specific data + if redirect_action? && additional_data['redirect_url'] + rule.update!( + metadata: rule.metadata.merge( + redirect_url: additional_data['redirect_url'], + redirect_status: additional_data['redirect_status'] || 302 + ) + ) + elsif challenge_action? + rule.update!( + metadata: rule.metadata.merge( + challenge_type: additional_data['challenge_type'] || 'captcha', + challenge_message: additional_data['challenge_message'] + ) + ) + end + + rule + end + # Matching logic for different policy types def matches_country?(network_range) country = network_range.country || network_range.inherited_intelligence[:country] diff --git a/test/models/waf_policy_test.rb b/test/models/waf_policy_test.rb index f97a56a..2d361c7 100644 --- a/test/models/waf_policy_test.rb +++ b/test/models/waf_policy_test.rb @@ -442,7 +442,7 @@ class WafPolicyTest < ActiveSupport::TestCase assert_equal 0, @policy.generated_rules_count # Create some rules - network_range = NetworkRange.create!(ip_range: "192.168.1.0/24") + network_range = NetworkRange.create!(cidr: "192.168.1.0/24", country: "BR") @policy.create_rule_for_network_range(network_range) assert_equal 1, @policy.generated_rules_count @@ -461,6 +461,75 @@ class WafPolicyTest < ActiveSupport::TestCase assert_equal 2, stats[:targets_count] end + # Rule creation and re-enablement tests + test "create_rule_for_network_range re-enables disabled rule" do + @policy.save! + + # Create a network range that matches the policy + network_range = NetworkRange.create!( + cidr: "192.168.1.0/24", + country: "BR" + ) + + # Create a rule via policy + rule = @policy.create_rule_for_network_range(network_range) + assert rule.enabled?, "Rule should be enabled initially" + assert rule.expires_at.present?, "Rule should have expiration" + + # Disable it (simulating expiration cleanup) + rule.update!(enabled: false) + assert_not rule.enabled?, "Rule should be disabled" + + # Try to create again - should re-enable + result = @policy.create_rule_for_network_range(network_range) + + assert_equal rule.id, result.id, "Should return same rule" + assert result.reload.enabled?, "Rule should be re-enabled" + assert result.expires_at.present?, "Should have new expiration" + assert result.expires_at > Time.current, "Expiration should be in the future" + end + + test "create_rule_for_network_range extends enabled rule expiration" do + @policy.save! + + # Create a network range that matches the policy + network_range = NetworkRange.create!( + cidr: "192.168.1.0/24", + country: "BR" + ) + + # Create a rule with expiration + rule = @policy.create_rule_for_network_range(network_range) + original_expires_at = rule.expires_at + + travel 3.days do + # Try to create again - should extend expiration + result = @policy.create_rule_for_network_range(network_range) + + assert_equal rule.id, result.id, "Should return same rule" + assert result.reload.expires_at > original_expires_at, "Expiration should be extended" + assert_in_delta 7.days.from_now, result.expires_at, 5.seconds, "Expiration should be ~7 days from now" + end + end + + test "create_rule_for_network_range creates new rule when none exists" do + @policy.save! + + # Create a network range that matches the policy + network_range = NetworkRange.create!( + cidr: "192.168.1.0/24", + country: "BR" + ) + + rule = @policy.create_rule_for_network_range(network_range) + + assert rule.persisted?, "Rule should be persisted" + assert rule.enabled?, "Rule should be enabled" + assert_equal network_range, rule.network_range + assert rule.expires_at.present?, "New rule should have expiration" + assert_in_delta 7.days.from_now, rule.expires_at, 5.seconds, "Expiration should be ~7 days from now" + end + # String representations test "to_s returns name" do assert_equal @policy.name, @policy.to_s