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 <noreply@anthropic.com>
This commit is contained in:
@@ -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]
|
||||
|
||||
Reference in New Issue
Block a user