feat(remediation): support custom remediations #139
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Replace iota-based Remediation enum with string-based system - Add weight system: Allow=0, Unknown=1, Captcha=10, Ban=20 - Implement string deduplication using pointers to reduce allocations - Add remediation_weights configuration option for custom remediations - Update RemediationMap to use string keys instead of enum - Update GetRemediationAndOrigin to compare weights for priority - Update all dataset and SPOA code to use string-based remediations - Fix metrics and AppSec checks to use > WeightAllow instead of > WeightUnknown to properly include custom remediations that default to Unknown weight Fixes #136
…lication Change RemediationMap from map[string]string to map[remediation.Remediation]string. This leverages the deduplicated string pointers in Remediation structs to automatically reduce memory allocations when storing remediations in maps. Benefits: - Automatic string deduplication via shared *string pointers - Reduced memory allocations in RemediationMap - Type-safe map keys - Efficient pointer-based comparisons Updated all methods (Add, Remove, HasRemediationWithOrigin, GetRemediationAndOrigin) and Contains methods to use remediation.Remediation types directly.
…n comparisons Tests were comparing remediation.Remediation with strings after Contains methods were updated to return remediation.Remediation directly. Updated assertions to use IsEqual() method for proper type-safe comparisons.
Replace all r.String() == "..." comparisons with r.IsEqual(remediation.X) for type-safe remediation comparisons. Updated switch statement to use if/else chain with IsEqual() checks.
Add nolint comment to suppress gocritic ifElseChain warning. We use if-else with IsEqual() for type-safe remediation comparisons instead of switch on String() to maintain type safety.
Replace if-else chain with switch statement on remediation.Remediation struct. Since Remediation is comparable (all fields are comparable), we can switch on it directly using the known remediation constants (Allow, Ban, Captcha).
Replace if-else chain with switch statement on remediation.Remediation struct. Since Remediation is comparable (all fields are comparable), we can switch on it directly using the known remediation constants (Allow, Ban, Captcha). This satisfies the linter's ifElseChain warning while maintaining type safety.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the remediation system from a simple uint8 enum to a struct-based approach with configurable weights to support custom remediations. The implementation aims to enable users to define custom remediation types (e.g., "MFA") with weights that slot between built-in types (Allow=0, Unknown=1, Captcha=10, Ban=20). However, the implementation has several critical bugs that will prevent it from working correctly.
Key Issues:
- Critical: Struct-based Remediations used as map keys will fail equality checks due to pointer comparison
- Critical: Direct
!=and switch statement comparisons won't work with struct types - Critical: SetWeight() configuration happens too late - built-in constants already have cached weights
- Missing: No test coverage for the new remediation implementation
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/remediation/root.go | Complete rewrite of Remediation from uint8 to struct with weight-based comparison and string pointer deduplication. Introduces registry for managing weights and deduplicated strings. Critical bugs in map key usage and weight caching. |
| pkg/cfg/config.go | Adds RemediationWeights configuration field and applies custom weights via SetWeight(). Documentation missing; weight configuration timing issue prevents built-in weight overrides. |
| pkg/spoa/root.go | Updates remediation comparisons to use new IsEqual() and IsWeighted() methods. Direct != comparison on line 317 will fail with struct types. |
| pkg/dataset/types.go | Updates RemediationMap to use Remediation struct as keys with weight-based priority via IsHigher(). Map key comparison will fail due to pointer inequality. |
| pkg/dataset/root.go | Converts between Remediation and string representations in Add/Remove operations. Inconsistent approach between IP/Range (string) and Country (struct) operations. |
| pkg/dataset/ipmap.go | Changes operation structs to store remediation as string, converting back to Remediation when needed for map operations. |
| pkg/dataset/bart_types.go | Changes operation structs to store remediation as string, converting back to Remediation when needed for map operations. |
| pkg/dataset/root_test.go | Updates test assertions to use IsEqual() method for remediation comparison. |
| pkg/dataset/metrics_test.go | Updates test assertions to use IsEqual() method for remediation comparison. |
| pkg/dataset/benchmark_test.go | Updates benchmark code to use .String() method when creating test operations, and Compare() method for assertions. |
Comments suppressed due to low confidence (1)
pkg/dataset/root.go:134
- Inconsistency in how Remediation is passed to batch operations. IP and Range operations convert to string (
remediationName), but Country operations pass the Remediation struct directly (r). This makes the codebase harder to maintain and understand. Consider using a consistent approach across all operation types.
remediationName := r.String() // Convert to string for operations structs
switch scope {
case "ip":
ip, err := netip.ParseAddr(*decision.Value)
if err != nil {
log.Errorf("Error parsing IP address %s: %s", *decision.Value, err.Error())
continue
}
// Check for no-op: same IP, same remediation, same origin already exists
if d.IPMap.HasRemediation(ip, r, origin) {
// Exact duplicate - skip processing (no-op)
continue
}
ipType := "ipv4"
if ip.Is6() {
ipType = "ipv6"
}
// Check if we're overwriting an existing decision with different origin
if existingR, existingOrigin, found := d.IPMap.Contains(ip); found && existingR.IsEqual(r) && existingOrigin != origin {
// Decrement old origin's metric before incrementing new one
// Label order: origin, ip_type, scope (as defined in metrics.go)
metrics.TotalActiveDecisions.WithLabelValues(existingOrigin, ipType, "ip").Dec()
}
// Individual IPs go to IPMap for memory efficiency
ipOps = append(ipOps, IPAddOp{IP: ip, Origin: origin, R: remediationName, IPType: ipType})
// Label order: origin, ip_type, scope (as defined in metrics.go)
metrics.TotalActiveDecisions.WithLabelValues(origin, ipType, "ip").Inc()
case "range":
prefix, err := netip.ParsePrefix(*decision.Value)
if err != nil {
log.Errorf("Error parsing prefix %s: %s", *decision.Value, err.Error())
continue
}
// Check for no-op: same prefix, same remediation, same origin already exists
if d.RangeSet.HasRemediation(prefix, r, origin) {
// Exact duplicate - skip processing (no-op)
continue
}
ipType := "ipv4"
if prefix.Addr().Is6() {
ipType = "ipv6"
}
// Check if we're overwriting an existing decision with different origin
if existingOrigin, found := d.RangeSet.GetOriginForRemediation(prefix, r); found && existingOrigin != origin {
// Decrement old origin's metric before incrementing new one
// Label order: origin, ip_type, scope (as defined in metrics.go)
metrics.TotalActiveDecisions.WithLabelValues(existingOrigin, ipType, "range").Dec()
}
// Ranges go to BART for LPM support
rangeOps = append(rangeOps, BartAddOp{Prefix: prefix, Origin: origin, R: remediationName, IPType: ipType, Scope: "range"})
// Label order: origin, ip_type, scope (as defined in metrics.go)
metrics.TotalActiveDecisions.WithLabelValues(origin, ipType, "range").Inc()
case "country":
// Clone country code to break reference to Decision struct memory
cn := strings.Clone(*decision.Value)
// Check for no-op: same country, same remediation, same origin already exists
if d.CNSet.HasRemediation(cn, r, origin) {
// Exact duplicate - skip processing (no-op)
continue
}
// Check if we're overwriting an existing decision with different origin
if existingR, existingOrigin := d.CNSet.Contains(cn); existingR.IsEqual(r) && existingOrigin != "" && existingOrigin != origin {
// Decrement old origin's metric before incrementing new one
// Label order: origin, ip_type, scope (as defined in metrics.go)
metrics.TotalActiveDecisions.WithLabelValues(existingOrigin, "", "country").Dec()
}
cnOps = append(cnOps, cnOp{cn: cn, origin: origin, r: r})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix IsEqual() to compare both name pointer and weight for true equality - Add comprehensive documentation for RemediationWeights configuration - Add clarifying comments about struct comparison and custom remediation handling - Note: Direct struct comparison (r != remediation.X) works because Remediation is comparable (all fields are comparable: *string and int)
- Add HasSameWeight() method as suggested by Copilot for checking weight equality - Update GetRemediationAndOrigin() to handle remediations with same weight using alphabetical order as a deterministic tie-breaker - Add documentation explaining tie-breaking behavior when remediations have same weight - This ensures deterministic behavior when users configure multiple remediations with the same weight value
Add documentation explaining that all Remediation instances must be created via New() or FromString() to ensure proper deduplication for map key equality. Direct struct initialization will create different string pointers, causing map lookups to fail. This addresses Copilot's concern about map key comparison - the deduplication mechanism ensures all Remediation instances with the same name share the same *string pointer, making map key equality work correctly.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
pkg/dataset/types.go:97
- The
GetRemediationAndOrigin()function returns a zero-valuedRemediationwhen the map is empty. This zero-value hasname == nil, which could cause nil pointer dereferences when calling.String()or other methods.
Issue: If RemediationMap is empty, line 96 returns a zero-valued Remediation and an empty string. Calling .String() on this will hit the nil check on line 144 of internal/remediation/root.go and return "allow", but this is misleading - an empty map should not imply "allow".
Recommendation: Return an explicit value to indicate no remediation found, or check for empty map before calling this function. Consider:
func (rM RemediationMap) GetRemediationAndOrigin() (remediation.Remediation, string) {
if len(rM) == 0 {
return remediation.Allow, ""
}
// ... rest of implementation
}func (rM RemediationMap) GetRemediationAndOrigin() (remediation.Remediation, string) {
var maxRemediation remediation.Remediation
var maxOrigin string
first := true
for r, origin := range rM {
if first {
maxRemediation = r
maxOrigin = origin
first = false
continue
}
// Compare by weight first
if r.IsHigher(maxRemediation) {
maxRemediation = r
maxOrigin = origin
} else if r.HasSameWeight(maxRemediation) {
// Tie-breaker: use alphabetical order of name for deterministic behavior
if r.String() < maxRemediation.String() {
maxRemediation = r
maxOrigin = origin
}
}
}
return maxRemediation, maxOrigin
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
+21
| type Remediation struct { | ||
| name *string // Pointer to deduplicated string | ||
| weight int // Weight for comparison (higher = more severe) | ||
| } |
There was a problem hiding this comment.
The weight is stored redundantly in both the Remediation struct and the globalRegistry.weights map. This creates potential for inconsistency and doesn't align with the claimed "deduplication" benefits.
Issues:
- When
SetWeight()is called after aRemediationhas been created, existing instances retain their old weight while new instances get the new weight. - This breaks the invariant that all instances with the same name should behave identically.
- The PR description claims "Automatic deduplication" and "Reduced allocations", but storing weight in the struct doesn't achieve this.
Recommendation: Remove the weight field from the struct and always look it up from the registry:
func (r Remediation) Weight() int {
return GetWeight(*r.name)
}This ensures consistency and truly achieves deduplication - only the name pointer is stored per instance.
| if !ok { | ||
| // Weight not found, default to Unknown | ||
| weight = WeightUnknown | ||
| } |
There was a problem hiding this comment.
Data race: The globalRegistry.weights map is accessed without proper synchronization. In New(), lines 131-134 read from weights after releasing the lock on line 114. Between these lines, another goroutine could call SetWeight() and modify the map, causing a concurrent map read/write panic.
Problematic sequence:
defer globalRegistry.mu.Unlock() // Line 114 - lock is released
// ... (lines 115-130)
weight, ok := globalRegistry.weights[name] // Line 131 - unsynchronized read!Solution: Move the weight lookup inside the critical section before unlocking:
globalRegistry.mu.Lock()
defer globalRegistry.mu.Unlock()
// ... get/create deduped string ...
weight := globalRegistry.weights[name]
if weight == 0 {
weight = WeightUnknown
}
return Remediation{name: deduped, weight: weight}
Suggested change
| } | |
| } |
Comment on lines
1
to
202
| package remediation | ||
|
|
||
| // The order matters since we use slices.Max to get the max value | ||
| import ( | ||
| "sync" | ||
| ) | ||
|
|
||
| // Default weights for built-in remediations | ||
| // Allow=0, Unknown=1, then expand others to allow custom remediations to slot in between | ||
| const ( | ||
| Allow Remediation = iota // Allow remediation | ||
| Unknown // Unknown remediation (Unknown is used to have a value for remediation we don't support EG "MFA") | ||
| Captcha // Captcha remediation | ||
| Ban // Ban remediation | ||
| WeightAllow = 0 | ||
| WeightUnknown = 1 | ||
| WeightCaptcha = 10 | ||
| WeightBan = 20 | ||
| ) | ||
|
|
||
| // Remediation represents a remediation type as a string | ||
| // We use string pointers for deduplication to reduce allocations | ||
| type Remediation struct { | ||
| name *string // Pointer to deduplicated string | ||
| weight int // Weight for comparison (higher = more severe) | ||
| } | ||
|
|
||
| // registry manages deduplicated remediation strings and their weights | ||
| type registry struct { | ||
| mu sync.RWMutex | ||
| strings map[string]*string // Maps string to its deduplicated pointer | ||
| weights map[string]int // Maps remediation name to its weight | ||
| } | ||
|
|
||
| var globalRegistry = ®istry{ | ||
| strings: make(map[string]*string), | ||
| weights: make(map[string]int), | ||
| } | ||
|
|
||
| // Built-in remediation constants (for convenience) | ||
| // Initialized to nil, will be set in init() | ||
| var ( | ||
| Allow Remediation | ||
| Unknown Remediation | ||
| Captcha Remediation | ||
| Ban Remediation | ||
| ) | ||
|
|
||
| type Remediation uint8 // Remediation type is smallest uint to save space | ||
| //nolint:gochecknoinits // init() is required to initialize package-level vars after weights are set | ||
| func init() { | ||
| // Initialize built-in remediations with default weights | ||
| globalRegistry.mu.Lock() | ||
| defer globalRegistry.mu.Unlock() | ||
|
|
||
| // Set weights FIRST before creating strings | ||
| globalRegistry.weights["allow"] = WeightAllow | ||
| globalRegistry.weights["unknown"] = WeightUnknown | ||
| globalRegistry.weights["captcha"] = WeightCaptcha | ||
| globalRegistry.weights["ban"] = WeightBan | ||
|
|
||
| // Pre-create deduplicated strings for built-in remediations | ||
| // Must create new string variables for each to avoid pointer aliasing | ||
| allowStr := "allow" | ||
| unknownStr := "unknown" | ||
| captchaStr := "captcha" | ||
| banStr := "ban" | ||
|
|
||
| globalRegistry.strings["allow"] = &allowStr | ||
| globalRegistry.strings["unknown"] = &unknownStr | ||
| globalRegistry.strings["captcha"] = &captchaStr | ||
| globalRegistry.strings["ban"] = &banStr | ||
|
|
||
| // Now initialize the package-level vars directly (we already hold the lock) | ||
| // This avoids deadlock since New() would try to acquire the lock again | ||
| Allow = Remediation{name: &allowStr, weight: WeightAllow} | ||
| Unknown = Remediation{name: &unknownStr, weight: WeightUnknown} | ||
| Captcha = Remediation{name: &captchaStr, weight: WeightCaptcha} | ||
| Ban = Remediation{name: &banStr, weight: WeightBan} | ||
| } | ||
|
|
||
| // SetWeight sets a custom weight for a remediation (for configuration) | ||
| func SetWeight(name string, weight int) { | ||
| globalRegistry.mu.Lock() | ||
| defer globalRegistry.mu.Unlock() | ||
|
|
||
| globalRegistry.weights[name] = weight | ||
| // Ensure deduplicated string exists | ||
| if _, exists := globalRegistry.strings[name]; !exists { | ||
| // Create new string copy on heap to ensure pointer remains valid | ||
| nameCopy := name | ||
| globalRegistry.strings[name] = &nameCopy | ||
| } | ||
| } | ||
|
|
||
| // GetWeight returns the weight for a remediation name | ||
| func GetWeight(name string) int { | ||
| globalRegistry.mu.RLock() | ||
| defer globalRegistry.mu.RUnlock() | ||
|
|
||
| if weight, exists := globalRegistry.weights[name]; exists { | ||
| return weight | ||
| } | ||
| // Default to Unknown weight for unknown remediations | ||
| return WeightUnknown | ||
| } | ||
|
|
||
| // New creates a new Remediation from a string. | ||
| // Uses deduplicated string pointers to reduce allocations and ensure map key equality. | ||
| // | ||
| // IMPORTANT: All Remediation instances must be created via New() or FromString() to ensure | ||
| // proper deduplication. Direct struct initialization will create different string pointers, | ||
| // causing map lookups (e.g., in RemediationMap) to fail even for the same remediation name. | ||
| // | ||
| // The deduplication works by storing a single *string pointer per unique remediation name | ||
| // in globalRegistry.strings. All subsequent calls with the same name return Remediation | ||
| // instances with the same name pointer, ensuring map key equality works correctly. | ||
| func New(name string) Remediation { | ||
| globalRegistry.mu.Lock() | ||
| defer globalRegistry.mu.Unlock() | ||
|
|
||
| // Get or create deduplicated string pointer | ||
| deduped, exists := globalRegistry.strings[name] | ||
| if !exists { | ||
| // Create new deduplicated string. The variable escapes to heap when stored in | ||
| // the package-level map, ensuring the pointer remains valid for map key comparisons. | ||
| nameCopy := name | ||
| deduped = &nameCopy | ||
| globalRegistry.strings[name] = deduped | ||
| // Set default weight if not configured | ||
| if _, hasWeight := globalRegistry.weights[name]; !hasWeight { | ||
| globalRegistry.weights[name] = WeightUnknown | ||
| } | ||
| } | ||
|
|
||
| // Read weight from registry (may have been set in init() or SetWeight()) | ||
| weight, ok := globalRegistry.weights[name] | ||
| if !ok { | ||
| // Weight not found, default to Unknown | ||
| weight = WeightUnknown | ||
| } | ||
| return Remediation{ | ||
| name: deduped, | ||
| weight: weight, | ||
| } | ||
| } | ||
|
|
||
| // String returns the remediation name | ||
| func (r Remediation) String() string { | ||
| switch r { | ||
| case Ban: | ||
| return "ban" | ||
| case Captcha: | ||
| return "captcha" | ||
| case Unknown: | ||
| return "unknown" | ||
| default: | ||
| return "allow" | ||
| if r.name == nil { | ||
| return "allow" // Default fallback | ||
| } | ||
| return *r.name | ||
| } | ||
|
|
||
| // Weight returns the weight of the remediation | ||
| func (r Remediation) Weight() int { | ||
| return r.weight | ||
| } | ||
|
|
||
| // Compare returns: | ||
| // - negative if r < other | ||
| // - zero if r == other | ||
| // - positive if r > other | ||
| func (r Remediation) Compare(other Remediation) int { | ||
| return r.weight - other.weight | ||
| } | ||
|
|
||
| // IsHigher returns true if r has a higher weight than other | ||
| func (r Remediation) IsHigher(other Remediation) bool { | ||
| return r.weight > other.weight | ||
| } | ||
|
|
||
| // IsLower returns true if r has a lower weight than other | ||
| func (r Remediation) IsLower(other Remediation) bool { | ||
| return r.weight < other.weight | ||
| } | ||
|
|
||
| // IsEqual returns true if r represents the same remediation as other. | ||
| // This compares both the name pointer (for deduplicated identity) and weight. | ||
| // Two remediations are equal if they have the same name pointer and weight. | ||
| func (r Remediation) IsEqual(other Remediation) bool { | ||
| return r.name == other.name && r.weight == other.weight | ||
| } | ||
|
|
||
| // HasSameWeight returns true if r has the same weight as other. | ||
| // This is useful for checking if two different remediations have the same priority. | ||
| // Note: Two remediations with the same weight will be compared by name (alphabetical) | ||
| // as a tie-breaker when determining priority. | ||
| func (r Remediation) HasSameWeight(other Remediation) bool { | ||
| return r.weight == other.weight | ||
| } | ||
|
|
||
| // IsWeighted returns true if r is not Allow (has weight > Allow) | ||
| // This is useful for checking if a remediation should be applied | ||
| func (r Remediation) IsWeighted() bool { | ||
| return r.weight > WeightAllow | ||
| } | ||
|
|
||
| // FromString creates a Remediation from a string (alias for New for backward compatibility) | ||
| func FromString(s string) Remediation { | ||
| switch s { | ||
| case "ban": | ||
| return Ban | ||
| case "captcha": | ||
| return Captcha | ||
| case "allow": | ||
| return Allow | ||
| default: | ||
| return Unknown | ||
| } | ||
| return New(s) | ||
| } | ||
|
|
||
| // IsZero returns true if the remediation is zero-valued | ||
| func (r Remediation) IsZero() bool { | ||
| return r.name == nil | ||
| } |
There was a problem hiding this comment.
The entire internal/remediation package lacks test coverage. This is concerning given the complexity of the new implementation with:
- Thread-safe global registry with locks
- String pointer deduplication
- Weight management
- Multiple comparison methods
- Configuration integration
Critical test cases missing:
- Concurrent calls to
New()andSetWeight() - Map key equality verification (deduplication working correctly)
- Custom weight configuration and its interaction with built-in constants
- Zero-value
Remediationbehavior - Weight updates after initialization
Recommendation: Add comprehensive unit tests for the remediation package, especially given the PR is marked as "untested purely draft".
|
|
||
| scope := strings.ToLower(*decision.Scope) | ||
| r := remediation.FromString(*decision.Type) | ||
| remediationName := r.String() // Convert to string for storage |
There was a problem hiding this comment.
[nitpick] The comment "Convert to string for storage" is misleading. The actual issue is that operation structs now use string instead of Remediation, requiring this conversion.
Suggestion: Update comment to: // Get string representation for operation struct
Suggested change
| remediationName := r.String() // Convert to string for storage | |
| remediationName := r.String() // Get string representation for operation struct |
Comment on lines
+317
to
318
| // Note: Direct struct comparison works because Remediation is comparable | ||
| if r != remediation.Captcha && r != remediation.Allow { |
There was a problem hiding this comment.
Inconsistent use of struct comparison vs IsEqual(). Line 318 uses direct struct comparison (r != remediation.Captcha), but elsewhere in the file (e.g., line 221, 650), IsEqual() is used.
Issue: The comment on line 317 states "Direct struct comparison works because Remediation is comparable", which is true, but then the codebase inconsistently uses IsEqual() in other places. This creates confusion about which approach should be used.
Recommendation: Standardize on one approach throughout the codebase. Either:
- Always use direct comparison (
==,!=) since structs are comparable, or - Always use
IsEqual()for semantic clarity
The mixed approach is confusing and error-prone for future maintenance.
Suggested change
| // Note: Direct struct comparison works because Remediation is comparable | |
| if r != remediation.Captcha && r != remediation.Allow { | |
| // Use IsEqual for semantic clarity and consistency | |
| if !r.IsEqual(remediation.Captcha) && !r.IsEqual(remediation.Allow) { |
Comment on lines
+311
to
314
| r, origin := d.CNSet.Contains(cn) | ||
| return r, origin | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The refactored CheckCN() function is unnecessarily verbose. The intermediate variables r and origin are immediately returned without modification.
Before (implied from diff context):
return d.CNSet.Contains(cn)After (lines 311-312):
r, origin := d.CNSet.Contains(cn)
return r, originThis change adds no value and reduces readability. The original one-liner was clearer.
Recommendation: Revert to the original one-liner: return d.CNSet.Contains(cn)
Suggested change
| r, origin := d.CNSet.Contains(cn) | |
| return r, origin | |
| } | |
| return d.CNSet.Contains(cn) | |
| } |
|
|
||
| // If remediation is ban/captcha we dont need to create a request to send to appsec unless always send is on | ||
| if r > remediation.Unknown && !matchedHost.AppSec.AlwaysSend { | ||
| // If remediation is not allow, we dont need to create a request to send to appsec unless always send is on |
There was a problem hiding this comment.
Typo in comment: "we dont need" should be "we don't need" (missing apostrophe).
Similarly on line 328.
Suggested change
| // If remediation is not allow, we dont need to create a request to send to appsec unless always send is on | |
| // If remediation is not allow, we don't need to create a request to send to appsec unless always send is on |
Comment on lines
+76
to
+88
| // SetWeight sets a custom weight for a remediation (for configuration) | ||
| func SetWeight(name string, weight int) { | ||
| globalRegistry.mu.Lock() | ||
| defer globalRegistry.mu.Unlock() | ||
|
|
||
| globalRegistry.weights[name] = weight | ||
| // Ensure deduplicated string exists | ||
| if _, exists := globalRegistry.strings[name]; !exists { | ||
| // Create new string copy on heap to ensure pointer remains valid | ||
| nameCopy := name | ||
| globalRegistry.strings[name] = &nameCopy | ||
| } | ||
| } |
There was a problem hiding this comment.
The SetWeight function creates or updates weights, but doesn't update existing Remediation instances that were created before the weight change. Combined with the fact that package-level constants (Allow, Ban, etc.) are initialized in init() with cached weights, calling SetWeight on built-in remediations will create inconsistent behavior.
Example:
// In init(): Ban is created with weight 20
// Later in config loading:
SetWeight("ban", 25) // Updates registry but Ban constant still has weight 20
r1 := remediation.Ban // weight = 20 (from init)
r2 := remediation.FromString("ban") // weight = 25 (from registry)
r1.IsEqual(r2) // FALSE - different weightsThis is a fundamental design issue with caching weights in the struct. See also comment on lines 18-21 about removing the weight field.
|
|
||
| // Apply custom remediation weights if configured | ||
| if config.RemediationWeights != nil { | ||
| for remediationName, weight := range config.RemediationWeights { |
There was a problem hiding this comment.
The configuration loading order creates a race condition. Custom weights are applied in NewConfig() (line 97-102) after the init() function has already initialized the built-in constants with default weights. This means:
- If the config sets a custom weight for "ban", it updates the registry but
Banconstant already has weight 20 cached in its struct - New
Remediationinstances created viaFromString("ban")will get the custom weight - But comparisons between the constant
Banand new instances will compare different weights
Example failure scenario:
// Config sets ban weight to 25
remediation.SetWeight("ban", 25)
r := remediation.FromString("ban") // gets weight 25
if r.IsEqual(remediation.Ban) { // false! Ban has weight 20
// This never executes
}Solution: Document that custom weights for built-in remediations are not supported, or redesign to not cache weights in the struct (see comment on lines 18-21).
Suggested change
| for remediationName, weight := range config.RemediationWeights { | |
| for remediationName, weight := range config.RemediationWeights { | |
| // Prevent custom weights for built-in remediations to avoid inconsistent behavior | |
| switch remediationName { | |
| case remediation.Ban.Name, remediation.Captcha.Name, remediation.Bypass.Name, remediation.Stream.Name: | |
| return nil, fmt.Errorf("custom weights for built-in remediation '%s' are not supported", remediationName) | |
| } |
|
|
||
| scope := strings.ToLower(*decision.Scope) | ||
| r := remediation.FromString(*decision.Type) | ||
| remediationName := r.String() // Convert to string for operations structs |
There was a problem hiding this comment.
[nitpick] The variable name suggests conversion, but this is actually just accessing a method. The comment "Convert to string for operations structs" is misleading.
Suggestion: Either rename to remediationStr or remove the comment, as .String() is a standard method call, not a conversion process.
Suggested change
| remediationName := r.String() // Convert to string for operations structs | |
| remediationName := r.String() |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow custom
remediation.RemediationChanges
Benefits
*stringpointers deduplicate map keysuntested purely draft for now