Post-Review Fixes: Security, Memory Leaks, Documentation, and API Bugs

[asulwer]

A comprehensive review of the RoslynRules codebase identified multiple issues across security, memory management, thread safety, documentation accuracy, and API design. This issue consolidates all findings for batch fixing.

---

🔴 Critical

1. Security — Code Injection Risk

User-supplied expression strings are embedded raw into generated C# code without sanitization. A malicious expression like "); File.Delete(@"C:\data"); //" can execute arbitrary code.

Root cause: CodeGenerator.Generate() interpolates the raw expression string directly into the generated method body. AssemblyCompiler.BuildReferences() loads ALL currently loaded assemblies including System.IO, System.Diagnostics.Process, etc., giving expressions broad runtime access.

Fix needed:

• Document the risk prominently (SECURITY.md + README warning)
• Consider restricting loaded assemblies to a whitelist
• Add Roslyn syntax tree validation to reject dangerous expression patterns (invocations on File, Process, etc.)

Location: RoslynRules/Compiler/CodeGenerator.cs, RoslynRules/Compiler/AssemblyCompiler.cs

2. Memory Leak — Assembly Unloading

Every unique expression compilation loads a new assembly into the current AppDomain via Assembly.Load(assemblyBytes). These assemblies are never unloaded. In a long-running app with dynamic rule generation, this causes unbounded memory growth.

Root cause: Standard Assembly.Load loads into the default AppDomain. .NET Framework cannot unload individual assemblies. .NET Core+ supports AssemblyLoadContext for collectible assemblies but it is not used.

Fix needed:

• Use AssemblyLoadContext for .NET Core+ to enable assembly unloading
• Or document this as a known limitation with mitigation guidance (restart thresholds, expression reuse)

Location: RoslynRules/Compiler/DelegateFactory.cs:15, RoslynRules/Compiler/AssemblyCompiler.cs:73

---

🟡 High

3. Workflow.ExecuteParallel Ignores Dependencies

Workflow.ExecuteParallel() does NOT use GetExecutionOrder(). It runs all active rules in parallel sorted only by priority, skipping the dependency chain entirely. Rules with DependsOnRuleId may execute before their dependencies, causing RuleContext misses and incorrect results.

Note: ExecuteParallelAsync() handles dependencies correctly. The sync version should match that behavior.

Fix needed:

• Make ExecuteParallel use GetExecutionOrder() and execute independent rules in parallel while respecting dependency ordering
• Or document the limitation if intentional

Location: RoslynRules/Models/Workflow.cs (~line 312)

4. ExpressionCompiler Caching Race Condition

In ExpressionCompiler.Compile<TDelegate>(), the cache check and write are in separate locks. Two threads can compile the same expression simultaneously. The second thread's result overwrites the first in cache. Not dangerous (same result), but wastes CPU.

Current code:

lock (_lock) { if (_cache.TryGetValue(cacheKey, out var cached)) return (TDelegate)cached; }
// ... compilation happens OUTSIDE lock ...
lock (_lock) { _cache[cacheKey] = del; }

Fix needed:

• Use ConcurrentDictionary.GetOrAdd() with a factory delegate, OR
• Use double-checked locking where compilation happens inside the lock for cache misses

Location: RoslynRules/Compiler/ExpressionCompiler.cs:47-52

5. Sync-over-Async Blocking

Rule.ExecuteCore() uses Task.Run(() => ExecuteCoreInternal(...)) then blocks with .Wait() for timeout handling. This wastes thread-pool threads. Same pattern in CompiledAsyncFunc.Invoke() which blocks on .GetAwaiter().GetResult() — potential for thread-pool starvation and deadlocks in ASP.NET / UI contexts.

Fix needed:

• Replace Task.Run + .Wait() with CancellationTokenSource.CancelAfter() for timeout
• Document that sync execution of async delegates blocks; recommend ExecuteAsync() for async expressions
• Add [Obsolete("Use ExecuteAsync for async expressions to avoid blocking.")] or analyzer warning

Locations:

• RoslynRules/Models/Rule.cs:456-469 (sync timeout)
• RoslynRules/Models/CompiledDelegate.cs:77-79 (CompiledAsyncFunc.Invoke)
• RoslynRules/Models/CompiledDelegate.cs:108-110 (CompiledAsyncAction.Invoke)

---

🟢 Medium — Documentation

6. Duplicate Result Caching Section in README

The "Result Caching" section appears twice in README.md.

Fix: Remove the duplicate.

7. Missing API Reference Entries

The following public APIs are undocumented in docs/api-reference.md:

• Rule.ClearCache() — forces cache invalidation
• Rule.ExecuteAsync() — async rule execution
• Workflow.ExecuteBufferedAsync() — buffered async execution
• RuleBatch interface methods — missing most IRuleEngine aliases

8. Incorrect Documentation

• RulesException inheritance: docs say inherits InvalidOperationException, actually inherits Exception
• Workflow.ExecuteParallel docs claim "respects dependency chains" — it does not (see item Bump DynamicExpresso.Core from 2.19.2 to 2.19.3 #3)
• Child rules are part of the cached RuleResult but docs say "Child rules are still evaluated independently" — clarify that child rules ARE evaluated independently but their results ARE cached as part of the parent result
• RuleContext.StoreResult is public but intended for internal engine use — should be documented as "for internal use; do not call directly"

9. Missing Project Files

• CONTRIBUTING.md — contributor guidelines
• SECURITY.md — security policy and expression injection disclosure
• CHANGELOG.md — version history and breaking changes

10. JSON Loader Uses Reflection to Bypass Private Setters

JsonRuleLoader uses typeof(Rule).GetProperty("Id")?.SetValue(...) to restore IDs from JSON. This is fragile — breaks with trimming/AOT and is sensitive to refactoring.

Fix needed:

• Consider making Id init-only (init accessor) or adding a deserialization constructor
• Or use [JsonInclude] attribute with private setters

Location: RoslynRules/Extensions/JsonRuleLoader.cs:86-89, 153-156

11. Null-Forgiving Operator on Reflection Results

DelegateFactory uses ! on reflection results that could legitimately be null if generated code doesn't match expectations:

var type = assembly.GetType("ExpressionAssembly")!;
var method = type.GetMethod("Evaluate")!;

Fix needed: Replace with null checks and meaningful exceptions.

Location: RoslynRules/Compiler/DelegateFactory.cs:19-20

---

🟢 Medium — Tests

12. Missing Test Coverage

• Security tests: No tests verify that malicious expressions fail or are sandboxed
• Performance/stress tests: No load tests, memory leak tests, or concurrent compilation stress tests
• Race condition tests: ExpressionCompiler double-compile scenario not tested
• Edge cases: Null RuleParameter.Value, deeply nested JSON round-trip, empty string expressions with only whitespace

---

Acceptance Criteria

• SECURITY.md created with expression injection disclosure
• README updated with prominent security warning
• Workflow.ExecuteParallel fixed to respect dependency chains OR documented as limitation
• ExpressionCompiler cache race condition fixed
• Sync timeout implementation uses CancellationTokenSource.CancelAfter() instead of Task.Run + .Wait()
• Duplicate Result Caching section removed from README
• Rule.ClearCache(), Rule.ExecuteAsync(), Workflow.ExecuteBufferedAsync() documented in api-reference.md
• RulesException inheritance docs corrected
• CONTRIBUTING.md, CHANGELOG.md created
• JsonRuleLoader reflection replaced with init-only setters or [JsonInclude]
• DelegateFactory null-forgiving operators replaced with null checks
• Tests added for security (malicious expression rejection) and concurrent compilation

Estimated Effort

• Security docs + README fixes: 1-2 hours
• ExecuteParallel dependency fix: 2-3 hours
• ExpressionCompiler race fix: 1 hour
• Sync timeout refactor: 2-3 hours
• Documentation corrections: 2 hours
• JsonRuleLoader refactor: 1-2 hours
• New tests: 2-3 hours
• Total: ~12-16 hours