critical_issues#168
Merged
Merged
Conversation
What the code does: ContainsSuccessEventReferences() checks if an expression references a success event by doing expression.Contains(eventName) for each available event name.
The problem: If you have events named "Test" and "Test2", and an expression contains "Test2", the check for "Test" will also match because "Test2".Contains("Test") is true. This means a rule expression that references Test2 will incorrectly get the boolean value for Test injected as a scoped parameter.
What the code does: ContainsRuleReferences() checks if an expression references another rule by doing expression.Contains($"@{ruleName}").
The problem: Same substring issue. If you have rules named "Test" and "Test2", an expression containing "@test2" will match the check for "@test" because "@test2".Contains("@test") is true. The wrong rule result gets injected as a scoped parameter.
What the code does: CompileRuleWithRuleResults() does processedExpression = processedExpression.Replace($"@{kvp.Key}", kvp.Key); to strip the @ prefix from rule references before compiling.
The problem: Replace is global and substring-based. If a rule is named "Count", and the expression contains "DiscountAmount", it becomes "DisountAmount" after replacement — corrupting the expression.
What the code does: MemCache.Set() uses a ConcurrentDictionary for cached values and a ConcurrentQueue for eviction order. When adding/updating a key, it enqueues a new (key, expiry) tuple. The eviction loop dequeues from the queue and removes matching entries from the dictionary. The problem: When AddOrUpdate updates an existing key with a new expiry, the old queue entry for that key is orphaned — it stays in the queue forever. Over time the queue grows unbounded with stale entries. The eviction loop can also spin forever if the queue head is stale (key already removed or updated) and _cacheDictionary.Count stays above _config.SizeLimit.
What the code does: GetCompiledRulesKey() builds a cache key as $"{workflowName}-" + string.Join("-", ruleParams.Select(c => $"{c.Name}_{c.Type.Name}")).
The problem: Type.Name is not unique. List<String> and List<Int32> both have Type.Name == "List1". Two different RuleParameter` arrays with different generic type arguments would produce the same cache key, causing compiled rules to be incorrectly shared.
What the code does: RulesCache.GetWorkflow() checks if workflow.WorkflowsToInject has entries, then mutates workflow.Rules by concatenating injected rules directly into the cached Workflow object. The problem: The cached Workflow object is modified in-place. Multiple calls accumulate duplicate rules (injected rules get added again and again). It's not thread-safe — two threads calling GetWorkflow simultaneously can interleave the concat operations. The base workflow becomes permanently modified, so even fresh executions see the accumulated rules.
What the code does: ExecuteActionAsync() recursively calls itself for child results: await ExecuteActionAsync(ruleResult.ChildResults). The outer call has a cancellationToken parameter, but it's not passed to the recursive call. The problem: If a cancellation is requested while processing nested/child rules, the child rule actions continue running. The cancellation token is effectively ignored for any rule hierarchy beyond the top level.
What the code does: ActionBase.ExecuteAndReturnResultAsync() catches exceptions during action execution and, when EnableExceptionAsErrorMessage = true, wraps them: new Exception($"Exception while executing {GetType().Name}: {ex.Message}", ex).
The problem: The new exception's message only contains ex.Message. Any custom data on the original exception (beyond InnerException) is buried. The outer ActionResult.Exception gets this wrapped exception, so calling code sees the generic message, not the original. The report says "losing custom exception data" — technically ex.Data is still accessible via InnerException, but it's one hop away and the top-level exception is a generic wrapper.
What the code does: RuleExpressionParser.Parse() takes ParameterExpression[] parameters and passes it directly to new ExpressionParser(parameters, expression, new object[] { }, config).Parse(returnType).
The problem: No validation on parameters. If it's null or contains null elements, ExpressionParser will throw a NullReferenceException or ArgumentNullException from deep inside System.Linq.Dynamic.Core. The caller gets an unhelpful stack trace.
What the code does: ExecuteAllRulesAsync() calls Array.Sort(ruleParams, (RuleParameter a, RuleParameter b) => string.Compare(a.Name, b.Name)); on the ruleParams array passed by the caller. The problem: Array.Sort sorts in-place. Since ruleParams is a reference to the caller's array (whether passed as params RuleParameter[] or directly), the caller's array order is permanently modified. This is a surprising side effect — callers who constructed their RuleParameter array in a specific order will find it reordered after the call.
What the code does: RulesEngine constructor takes string[] jsonConfig and deserializes with JsonSerializer.Deserialize<Workflow>(item) using default System.Text.Json options.
The problem: Default System.Text.Json is case-sensitive. If someone sends camelCase JSON ({"workflowName": "..."} instead of {"WorkflowName": "..."}), deserialization silently fails — properties end up null. The old Microsoft fork used Newtonsoft.Json which was case-insensitive by default, so this is a migration gotcha.
What the code does: ActionFactory.Get() throws KeyNotFoundException($"Action with name: {name} does not exist") when an action name isn't found.
The problem: Bare string in exception message. Not a typed exception, not a resource string, not localizable. Minor but consistent with good practice.
…Queue<...>(). The problem: Fixed the Set() method to eliminate the queue-based eviction, but Clear() still creates a new queue that is never used elsewhere. The field is dead weight now.
What the code does: Inside the foreach (var lp in localParams) loop, parametersArray = parameters.ToArray() is reassigned every iteration. The problem: This creates a new array on every scoped param. With many local params, this is O(n²) array allocations. The array is only needed for expressionBuilder.Parse().
What the code does: "(\\$\\(.*?\\))" matches $(propertyName) in error messages. The problem: The .*? is non-greedy but the pattern can still match across multiple $(...) segments in edge cases. Also, there's no validation that the matched content is a valid property reference. Solution: More restrictive pattern: @"\$\(([^)]+)\)" — matches $( then any chars except ) then ). This prevents malformed matches.
What the code does: JsonSerializer.Deserialize<Workflow>(item) with case-insensitive options (per your fix). The problem: If the JSON is malformed, this throws JsonException directly to the caller with no context about which config item failed.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
critical and major issues found and fixed