From a9e807e3a06364531f7c8d6d04f9bd72cafbec01 Mon Sep 17 00:00:00 2001 From: Kasperczyk Date: Sun, 19 Jul 2026 10:55:18 +0200 Subject: [PATCH 1/3] feat: pluggable rule store, security-analyzer hardening, and CI quality gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What and why Acts on a critical review of `/src` and `/.github` (see `plan.md`). Closes the high-, medium- and low-priority items: a pluggable persistence seam, a real security-gate gap, CI/quality hardening, and API conveniences. Fully backward compatible on disk; all changes verified by the test suite. ## Highlights ### Pluggable persistence — `IRuleStore` (feat) - New public `IRuleStore`, immutable `RuleRecord`, `StoredRule`, and default `FileRuleStore`; inject a custom backend via `RuleEngineOptions.Store` so several engine instances can share one store (DB/object storage). `StorePath` remains the default file-store path. - Integrity (source hashing, tamper detection) moved from the store into the engine (`RuleHash`, `IsSourceTampered`), so a custom store is pure persistence. Internal `RuleStore`/`RuleMetadata` removed; the engine now works with immutable records. On-disk format unchanged (`.meta.json` / `.cs` / `.rule.json`). ### Security gate hardening (fix) - `SecurityPolicy.Default` now bans `System.AppContext` — a real gap: the open root `System.*` surface let it (and its `GetData` / `BaseDirectory`) through the analyzer. Confirmed by a failing test first, then fixed. - New adversarial corpus for escapes the existing suite missed: reflection via an instance `GetType()` chain, `typeof(X).Assembly`, a banned type worn as a generic argument, a banned call hidden in a lambda / local function — plus positive controls (Math/Guid/DateTime/Regex still pass). ### Runtime trust boundary (feat + docs) - `Resolve` now documents the boundary: the engine guards its own `AppliesTo`/`Priority` calls, but a resolved implementation's methods run unguarded on the caller's thread; acceptance tests and the approval gate are the real defense. - New opt-in `RuleExecution.TryInvoke(func, timeout, out result, out error)` for a bounded, exception-isolated call. ### Correctness & conveniences - `NewId` now uses a full 128-bit GUID instead of a 48-bit slice — removes a silent-overwrite risk in the store. - `_disposed` marked `volatile` (visible to lock-free `Resolve`). - Async offload wrappers for the compile-bound operations: `AddRuleFromSourceAsync`, `AddJsonRuleFromSourceAsync`, `ApproveAsync`, `EnableAsync`. - `GetRule(id)` — single-rule lookup, cheaper than filtering `GetRules()`. ### CI / quality gates - `TreatWarningsAsErrors` (whole tree, builds 0/0) and `RestorePackagesWithLockFile`; committed `packages.lock.json` for all three projects; CI restore runs `--locked-mode`. - Library gets `AnalysisLevel=latest` + `AnalysisModeSecurity=All` (0 findings). - CodeQL switched to `build-mode: manual` with a real build for deeper dataflow analysis. ## Testing `dotnet test` — **173/173** on net8.0 and net10.0 (was 151). Full solution builds 0 warnings / 0 errors under warnings-as-errors. `dotnet restore --locked-mode` passes. ## Notes for reviewers - **Public API growth** (`IRuleStore`, `RuleRecord`, `StoredRule`, `FileRuleStore`, `RuleEngineOptions.Store`, `RuleExecution`, `GetRule`, four `*Async` methods) — a minor bump. - **Behavior change:** the default policy now rejects `System.AppContext`. Intentional tightening, not an API break. - Cross-instance coordination is still pull-based (an instance sees others' changes on its next `ReloadFromStore`); a notification-driven reload is left as a follow-up a custom store can provide. - `plan.md` is included as the review record — drop it from the PR if you'd rather not ship review notes in the repo. --- .github/workflows/ci.yml | 4 +- .github/workflows/codeql.yml | 17 +- AGENTS.md | 2 +- Directory.Build.props | 5 + README.md | 32 +- samples/RuleCraft.Sample/packages.lock.json | 65 ++++ src/RuleCraft/Engine/RuleEngine.cs | 146 +++++--- src/RuleCraft/Engine/RuleEngineOptions.cs | 35 +- src/RuleCraft/FileRuleStore.cs | 132 +++++++ src/RuleCraft/IRuleStore.cs | 92 +++++ src/RuleCraft/RuleCraft.csproj | 7 + src/RuleCraft/RuleExecution.cs | 53 +++ src/RuleCraft/Security/SecurityPolicy.cs | 1 + src/RuleCraft/Store/RuleMetadata.cs | 58 --- src/RuleCraft/Store/RuleStore.cs | 146 -------- src/RuleCraft/packages.lock.json | 96 +++++ tests/RuleCraft.Tests/CustomRuleStoreTests.cs | 101 +++++ .../RuleCraft.Tests/EngineConvenienceTests.cs | 73 ++++ tests/RuleCraft.Tests/RuleExecutionTests.cs | 51 +++ .../SecurityAnalyzerAdversarialTests.cs | 141 +++++++ tests/RuleCraft.Tests/packages.lock.json | 352 ++++++++++++++++++ 21 files changed, 1336 insertions(+), 273 deletions(-) create mode 100644 samples/RuleCraft.Sample/packages.lock.json create mode 100644 src/RuleCraft/FileRuleStore.cs create mode 100644 src/RuleCraft/IRuleStore.cs create mode 100644 src/RuleCraft/RuleExecution.cs delete mode 100644 src/RuleCraft/Store/RuleMetadata.cs delete mode 100644 src/RuleCraft/Store/RuleStore.cs create mode 100644 src/RuleCraft/packages.lock.json create mode 100644 tests/RuleCraft.Tests/CustomRuleStoreTests.cs create mode 100644 tests/RuleCraft.Tests/EngineConvenienceTests.cs create mode 100644 tests/RuleCraft.Tests/RuleExecutionTests.cs create mode 100644 tests/RuleCraft.Tests/SecurityAnalyzerAdversarialTests.cs create mode 100644 tests/RuleCraft.Tests/packages.lock.json diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dcc58da..401810b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,8 +32,10 @@ jobs: dotnet-version: 8.0.x global-json-file: global.json + # Locked mode: fail if the resolved graph differs from the committed packages.lock.json, so a + # dependency cannot change between a green PR and the merge without the lockfile changing too. - name: Restore - run: dotnet restore RuleCraft.slnx + run: dotnet restore RuleCraft.slnx --locked-mode # The whole solution, so a break in the sample is a build failure and not a surprise later. - name: Build diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 5bec7d2..6c24c48 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -28,10 +28,19 @@ jobs: uses: github/codeql-action/init@v4 with: languages: csharp - # Buildless: CodeQL reads the sources directly, so no SDK setup and no second Release - # build. Switch to `manual` (plus a setup-dotnet + dotnet build step) if analysis of the - # generated/compiled surface ever turns out to need a real build. - build-mode: none + # Traced build (not buildless): this library's whole point is the code it compiles and + # runs, so CodeQL sees more with a real build's dataflow than by reading sources alone. + build-mode: manual + + # Both inputs on purpose, same as ci.yml: setup-dotnet installs each, global.json picks the SDK. + - name: Set up .NET + uses: actions/setup-dotnet@v6 + with: + dotnet-version: 8.0.x + global-json-file: global.json + + - name: Build + run: dotnet build RuleCraft.slnx --configuration Release - name: Analyze uses: github/codeql-action/analyze@v4 diff --git a/AGENTS.md b/AGENTS.md index b10e607..c032f85 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ the load-context handling are the design — not incidental plumbing. ``` src/RuleCraft/ the library, shipped as a single DLL -tests/RuleCraft.Tests/ xunit suite (151 tests, no network, no API key) +tests/RuleCraft.Tests/ xunit suite (173 tests, no network, no API key) samples/RuleCraft.Sample/ ASP.NET Core minimal API + review console ``` diff --git a/Directory.Build.props b/Directory.Build.props index 78a4014..97069d9 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -7,5 +7,10 @@ false + + true + + true diff --git a/README.md b/README.md index 8dcbecd..ca65b9e 100644 --- a/README.md +++ b/README.md @@ -205,10 +205,11 @@ The engine is thread-safe and meant to be a singleton: resolution is lock-free, removing rules swaps an immutable snapshot, and mutations of one rule are serialized — an `Approve` racing a `Reject` cannot leave a rule live but recorded as rejected. -**Only the two generation methods are `async`**, because only they do I/O (the call to the LLM). -Compiling, parsing, testing and approving are CPU-bound and run on the calling thread: an `Approve` -costs a Roslyn compile, and the API says so rather than hiding it behind a `Task` that was never -asynchronous. Wrap those calls in `Task.Run` if a request thread must not block. +**Only rule generation is truly `async`** — it does I/O (the call to the LLM). Compiling, parsing, +testing and approving are CPU-bound and run on the calling thread: an `Approve` costs a Roslyn +compile, and the API says so rather than hiding it behind a `Task` that was never asynchronous. When +a request thread must not block, use the offload wrappers — `AddRuleFromSourceAsync`, `ApproveAsync`, +`EnableAsync` — which run that work on `Task.Run` for you. `Dispose()` unloads every rule assembly the engine loaded — worth doing if you build engines per scope (a test suite, say); a singleton normally lives as long as the process. @@ -222,7 +223,13 @@ scope (a test suite, say); a singleton normally lives as long as the process. | **Static** — a class in your repo | `AddStaticRule(new BulkOrderRule())` | already compiled | no | no — git is the gate | re-register at startup | All three land in the same registry and compete purely by priority — the dispatcher cannot tell -them apart, and `GetRules()` lists them side by side. +them apart, and `GetRules()` lists them side by side. `GetRule(id)` fetches one. + +**Where rules live.** By default, a `rules/` folder on disk (`StorePath`), reloaded at startup by +`ReloadFromStore()`. If several instances of your app should share one set of rules — approve on one, +run it on the others — point `RuleEngineOptions.Store` at your own `IRuleStore` (backed by a database, +say) instead of the default file store. Each instance picks up the others' changes on its next +`ReloadFromStore()`. A **static** rule is just a class implementing `IRule`: @@ -461,8 +468,8 @@ concepts. Depth, size and node count are bounded, so a rule cannot burn CPU on e **Compiled C# rules are not.** .NET has **no in-process sandbox**: approved rule code runs with the full permissions of your process. The security gate (reference whitelist + semantic-model analyzer banning `System.IO`, `System.Net`, `System.Reflection`, `System.Diagnostics`, interop, -threading, `Activator`, `Environment`, `unsafe`, `dynamic`, preprocessor directives, …) is a -**guardrail and review aid, not a sandbox**. +threading, `Activator`, `Environment`, `AppContext`, `unsafe`, `dynamic`, preprocessor directives, …) +is a **guardrail and review aid, not a sandbox**. The policy resolves most-specific-first — member, then type, then namespace — so it can hand out `System.Threading.Tasks` (an async contract cannot be implemented without naming `Task`) while @@ -479,7 +486,10 @@ are reasons the human approval step is not decoration. need it — so a catastrophically backtracking pattern in `AppliesTo` is a ReDoS on your hot path, triggered by whatever data hits it. The test harness times out a candidate that hangs *during validation*; it cannot help once the rule is live. Review regexes in rule code the way you would - in your own. + in your own. The engine guards its *own* calls into a rule (`AppliesTo`, `Priority`), but the + method you invoke on the resolved implementation runs on your thread with no timeout — wrap it in + `RuleExecution.TryInvoke(() => rule.GetDiscount(order), timeout, out var result, out _)` when a + request must not block on it. - **Kill the process by recursing.** `StackOverflowException` cannot be caught in .NET. The `try`/`catch` around every predicate makes a *throwing* rule harmless; a rule that recurses without a base case takes the process down regardless, and no in-process gate can change that. @@ -513,7 +523,9 @@ could equally well deploy code to the box. - **One `StorePath` per engine.** The default is a `rules/` folder relative to the process, so two engines over different contracts land in the same one. Each records the contract its rules were written against, ignores the other's, and logs an error at reload rather than quarantining what - is not its own — but the folder is still shared, and you should not rely on that politeness. + is not its own — but the folder is still shared, and you should not rely on that politeness. To + share one rule set across app instances on purpose, use a custom `IRuleStore` rather than the + default file store. - `Expression.Compile` is used for JSON field access, so a JSON-rule engine will not survive full AOT either. - Source files on disk are hashed; a tampered file is refused at approval and quarantined on @@ -528,7 +540,7 @@ could equally well deploy code to the box. src/RuleCraft/ the library (single DLL): engine, JSON-DSL parser/interpreter, Roslyn compiler, ALC loading, security analyzer, test harness, store, LLM generation -tests/RuleCraft.Tests/ xunit suite (151 tests, no network needed) +tests/RuleCraft.Tests/ xunit suite (173 tests, no network needed) samples/RuleCraft.Sample/ ASP.NET Core minimal API demo + review console ``` diff --git a/samples/RuleCraft.Sample/packages.lock.json b/samples/RuleCraft.Sample/packages.lock.json new file mode 100644 index 0000000..26a7ec7 --- /dev/null +++ b/samples/RuleCraft.Sample/packages.lock.json @@ -0,0 +1,65 @@ +{ + "version": 1, + "dependencies": { + "net10.0": { + "Anthropic": { + "type": "Direct", + "requested": "[12.36.0, )", + "resolved": "12.36.0", + "contentHash": "xkpZhBg5hJPYfa5smXbtOki8+6RoHn1uikSSrSJPviAlaD0Ac09md0adC9OBZb8xbszQ6J4KSI//ndP95S2zAg==", + "dependencies": { + "Microsoft.Extensions.AI.Abstractions": "10.5.1" + } + }, + "Microsoft.Extensions.AI": { + "type": "Direct", + "requested": "[10.8.0, )", + "resolved": "10.8.0", + "contentHash": "QfthL8e2X/Z6jRs4YyVS2+HTApJvhz725l35HYZ9cfwlEuQnpzY89NJoGNmpuNZn4LIvvQOQeZIbu3jggBXgUg==", + "dependencies": { + "Microsoft.Extensions.AI.Abstractions": "10.8.0", + "System.Numerics.Tensors": "10.0.10" + } + }, + "Microsoft.CodeAnalysis.Analyzers": { + "type": "Transitive", + "resolved": "5.3.0", + "contentHash": "KuLhbZwB0L8JikL86AE5VWEp3RLNjIcp+j8yz9EJ/UBgRz4+qDEjHg/tluRFbpYpD/e37BqaaNFbQ0vqawBwWQ==" + }, + "Microsoft.CodeAnalysis.Common": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "eWYNB5e92PSdkQ0xcmy2aLtrvBXNydnVi0Hj/VjaAely6XBqA3By+ClGAJaj4d16pzQmrXPLLK9RDVuS1Ec9xQ==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0" + } + }, + "Microsoft.CodeAnalysis.CSharp": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "r1DrKQ/L0xTw03wJrLr36AMQNslyaeEKBFyFmQcOKa8HX3YvmhY//JEUafb6IR/m0gmaUVCfBTWitKJRNb7YAA==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0", + "Microsoft.CodeAnalysis.Common": "[5.6.0]" + } + }, + "Microsoft.Extensions.AI.Abstractions": { + "type": "Transitive", + "resolved": "10.8.0", + "contentHash": "1tJZ5sAYrEq1YjNg8GrZSL1tVodsWRfrgybGsZo5DQsWQgLZ+dSR5uFowIS6vb/9zxGBPl7xpYq53QMjNObO9w==" + }, + "System.Numerics.Tensors": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "9Ildb4Y9maDztB0ZRGxsNShbpCQ2Tjv2dr4IjskBxpTGhH7aIz1+oC+Hl833ASs/htWwsH6myNe+sRpeyCzeMQ==" + }, + "rulecraft": { + "type": "Project", + "dependencies": { + "Microsoft.CodeAnalysis.CSharp": "[5.6.0, )", + "Microsoft.Extensions.AI.Abstractions": "[10.8.0, )" + } + } + } + } +} \ No newline at end of file diff --git a/src/RuleCraft/Engine/RuleEngine.cs b/src/RuleCraft/Engine/RuleEngine.cs index 2cd4299..d2d0b66 100644 --- a/src/RuleCraft/Engine/RuleEngine.cs +++ b/src/RuleCraft/Engine/RuleEngine.cs @@ -29,7 +29,7 @@ namespace RuleCraft; public sealed class RuleEngine : IDisposable where TContract : class { private readonly RuleEngineOptions _options; - private readonly RuleStore _store; + private readonly IRuleStore _store; private readonly RuleRegistry _registry = new(); private readonly RuleLocks _locks = new(); private readonly CSharpRuleKind _csharp; @@ -38,7 +38,9 @@ public sealed class RuleEngine : IDisposable where TContrac private IRuleKind? _json; private RulePrompts? _jsonPrompts; private TContract? _fallback; - private bool _disposed; + // Volatile: Dispose runs on one thread while Resolve reads this on others, so the write must be + // visible without a lock (Resolve is deliberately lock-free). + private volatile bool _disposed; // Reflected once per closed generic type, not per rule: the fingerprint walks every public // member of both types. @@ -50,7 +52,7 @@ public RuleEngine(RuleEngineOptions? options = null) { _options = (options ?? new RuleEngineOptions()).Snapshot(); _logger = _options.LoggerFactory.CreateLogger($"RuleCraft.RuleEngine<{typeof(TContract).Name}>"); - _store = new RuleStore(_options.StorePath, _logger); + _store = _options.Store ?? new FileRuleStore(_options.StorePath, _logger); _csharp = new CSharpRuleKind(_options, AcceptanceTests); } @@ -63,12 +65,12 @@ public RuleEngine(RuleEngineOptions? options = null) /// Null names mean metadata written before the fields existed: assume it is ours, as the store /// always did. /// - private static bool IsForAnotherContract(RuleMetadata metadata) => + private static bool IsForAnotherContract(RuleRecord metadata) => (metadata.ContractType is not null && metadata.ContractType != ContractTypeName) || (metadata.ContextType is not null && metadata.ContextType != ContextTypeName); /// Says which of the two happened, instead of guessing in the status reason. - private static string RevalidationFailure(RuleMetadata metadata) => + private static string RevalidationFailure(RuleRecord metadata) => metadata.ContractFingerprint == CurrentFingerprint ? "Failed revalidation: the rule no longer passes its own gates." : $"Failed revalidation: {ContractTypeName} or {ContextTypeName} changed shape since this rule was approved."; @@ -219,6 +221,20 @@ public RuleInfo AddRuleFromSource(string source, string? name = null, string? sp public RuleInfo AddJsonRuleFromSource(string source, string? name = null, string? spec = null) => AddFromSource(source, name, spec, RuleOrigin.Json); + /// + /// offloaded to the thread pool, so a request thread does not + /// block on the Roslyn compile. The token is honoured at entry (it cannot interrupt a compile + /// already under way). + /// + public Task AddRuleFromSourceAsync( + string source, string? name = null, string? spec = null, CancellationToken cancellationToken = default) => + Task.Run(() => AddRuleFromSource(source, name, spec), cancellationToken); + + /// + public Task AddJsonRuleFromSourceAsync( + string source, string? name = null, string? spec = null, CancellationToken cancellationToken = default) => + Task.Run(() => AddJsonRuleFromSource(source, name, spec), cancellationToken); + private RuleInfo AddFromSource(string source, string? name, string? spec, RuleOrigin origin) { ObjectDisposedException.ThrowIf(_disposed, this); @@ -244,7 +260,7 @@ private RuleInfo Persist( string id, string? name, string? spec, string source, PipelineOutcome outcome, RuleOrigin origin, string? modelId) { - var metadata = new RuleMetadata + var metadata = new RuleRecord { Id = id, // An explicit name wins; otherwise use the one the document gave itself. @@ -254,13 +270,16 @@ private RuleInfo Persist( Status = RuleStatus.PendingApproval, Priority = outcome.Priority, CreatedUtc = DateTimeOffset.UtcNow, + // Hash the exact source we hand the store; the store writes it verbatim, so this stays the + // signal the tamper check compares against. + SourceSha256 = RuleHash.Sha256Hex(source), ContractType = ContractTypeName, ContextType = ContextTypeName, ContractFingerprint = CurrentFingerprint, ModelId = modelId, Report = outcome.Report, }; - _store.Save(metadata, source); + _store.Save(new StoredRule(metadata, source)); _logger.LogInformation("Rule {RuleId} ('{Name}') stored as PendingApproval.", id, metadata.Name); return _options.AutoApprove @@ -296,6 +315,13 @@ public RuleInfo Approve(string ruleId, string approvedBy) } } + /// + /// offloaded to the thread pool: approval recompiles the rule, so it is as + /// expensive as adding one. The token is honoured at entry, not mid-compile. + /// + public Task ApproveAsync(string ruleId, string approvedBy, CancellationToken cancellationToken = default) => + Task.Run(() => Approve(ruleId, approvedBy), cancellationToken); + /// Rejects a pending rule. It stays on disk for audit but will never load. public RuleInfo Reject(string ruleId, string reason) { @@ -307,16 +333,15 @@ public RuleInfo Reject(string ruleId, string reason) if (metadata.Status != RuleStatus.PendingApproval) throw new RuleStateException($"Rule '{ruleId}' is {metadata.Status}; only PendingApproval rules can be rejected."); - metadata.Status = RuleStatus.Rejected; - metadata.StatusReason = reason; - _store.UpdateMetadata(metadata); + var rejected = metadata with { Status = RuleStatus.Rejected, StatusReason = reason }; + _store.Update(rejected); _logger.LogInformation("Rule {RuleId} rejected: {Reason}", ruleId, reason); - return ToInfo(metadata); + return ToInfo(rejected); } } private RuleInfo ApproveCore( - RuleMetadata metadata, string approvedBy, PipelineOutcome? preValidated) + RuleRecord metadata, string approvedBy, PipelineOutcome? preValidated) { // Both of these are host configuration problems rather than bad rules, so they throw before // anything is written to disk: a JSON rule with JSON support disabled, and a rule belonging @@ -330,7 +355,7 @@ private RuleInfo ApproveCore( var source = _store.ReadSource(metadata); - if (_store.IsSourceTampered(metadata)) + if (IsSourceTampered(metadata, source)) throw new RuleStateException( $"Source of rule '{metadata.Id}' was modified on disk after validation; refusing to load it."); @@ -339,10 +364,8 @@ private RuleInfo ApproveCore( var outcome = preValidated ?? kind.Validate(metadata.Id, source); if (outcome.Load is null) { - metadata.Status = RuleStatus.Quarantined; - metadata.StatusReason = RevalidationFailure(metadata); - metadata.Report = outcome.Report; - _store.UpdateMetadata(metadata); + var quarantined = metadata with { Report = outcome.Report }; + Quarantine(quarantined, RevalidationFailure(metadata)); throw new RuleValidationException( $"Rule '{metadata.Id}' failed revalidation and was quarantined.", outcome.Report); } @@ -350,18 +373,21 @@ private RuleInfo ApproveCore( var loaded = outcome.Load(); _registry.Add(metadata.Id, metadata.Name, metadata.Origin, loaded.Rule, loaded.Context); - metadata.Status = RuleStatus.Approved; - metadata.StatusReason = null; - metadata.ApprovedBy = approvedBy; - metadata.ApprovedAtUtc = DateTimeOffset.UtcNow; - metadata.Priority = outcome.Priority; - metadata.ContractType = ContractTypeName; - metadata.ContextType = ContextTypeName; - metadata.ContractFingerprint = CurrentFingerprint; - _store.UpdateMetadata(metadata); + var approved = metadata with + { + Status = RuleStatus.Approved, + StatusReason = null, + ApprovedBy = approvedBy, + ApprovedAtUtc = DateTimeOffset.UtcNow, + Priority = outcome.Priority, + ContractType = ContractTypeName, + ContextType = ContextTypeName, + ContractFingerprint = CurrentFingerprint, + }; + _store.Update(approved); _logger.LogInformation("Rule {RuleId} approved by {ApprovedBy} and loaded.", metadata.Id, approvedBy); - return ToInfo(metadata); + return ToInfo(approved); } /// @@ -391,6 +417,10 @@ public RuleInfo Enable(string ruleId, string enabledBy) } } + /// offloaded to the thread pool; like approval, it recompiles the rule. + public Task EnableAsync(string ruleId, string enabledBy, CancellationToken cancellationToken = default) => + Task.Run(() => Enable(ruleId, enabledBy), cancellationToken); + // ---------------------------------------------------------------- lifecycle /// @@ -412,10 +442,7 @@ public void RemoveRule(string ruleId) throw new RuleNotFoundException(ruleId); if (metadata is not null) - { - metadata.Status = RuleStatus.Disabled; - _store.UpdateMetadata(metadata); - } + _store.Update(metadata with { Status = RuleStatus.Disabled }); _logger.LogInformation("Rule {RuleId} removed (was loaded: {WasLoaded}).", ruleId, removed is not null); } @@ -467,17 +494,17 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) continue; } - if (_store.IsSourceTampered(metadata)) + var source = _store.ReadSource(metadata); + if (IsSourceTampered(metadata, source)) { Quarantine(metadata, "Source file on disk does not match the stored hash (tampered?)."); continue; } - var outcome = KindFor(metadata.Origin).Validate(metadata.Id, _store.ReadSource(metadata)); + var outcome = KindFor(metadata.Origin).Validate(metadata.Id, source); if (outcome.Load is null) { - metadata.Report = outcome.Report; - Quarantine(metadata, RevalidationFailure(metadata)); + Quarantine(metadata with { Report = outcome.Report }, RevalidationFailure(metadata)); continue; } @@ -507,17 +534,30 @@ public void Dispose() entry.Context?.Unload(); } - private void Quarantine(RuleMetadata metadata, string reason) + private void Quarantine(RuleRecord metadata, string reason) { - metadata.Status = RuleStatus.Quarantined; - metadata.StatusReason = reason; - _store.UpdateMetadata(metadata); + _store.Update(metadata with { Status = RuleStatus.Quarantined, StatusReason = reason }); _logger.LogWarning("Rule {RuleId} quarantined: {Reason}", metadata.Id, reason); } + // Integrity lives in the engine, not the store, so a custom IRuleStore stays pure persistence: + // compare the hash of what is on disk now to the one recorded when the source was saved. + private static bool IsSourceTampered(RuleRecord record, string source) => + record.SourceSha256 is not null && RuleHash.Sha256Hex(source) != record.SourceSha256; + // ---------------------------------------------------------------- dispatch /// Returns the implementation of the first rule in evaluation order that matches, the fallback, or null. + /// + /// Trust boundary: the engine guards its OWN calls into a rule — AppliesTo and + /// Priority run under try/catch here — but the implementation this returns is handed + /// straight back to you, and the method you then invoke on it runs on your thread with no + /// timeout, no exception isolation and no memory bound. For a compiled (LLM-generated) rule that + /// is unsandboxed code: a pathological branch can loop forever or throw. The real defense is the + /// acceptance tests and the human approval gate, not this call — a rule you would not hand a + /// production request to should not be Approved. Wrap the invocation yourself if a request thread + /// must never block on it. + /// public TContract? Resolve(TContext context) { ObjectDisposedException.ThrowIf(_disposed, this); @@ -597,6 +637,25 @@ private List> Match(TContext context) // ---------------------------------------------------------------- introspection + /// + /// One rule by id — a stored rule of any status (unless another engine left it in the folder), or + /// a static rule; null when there is no such rule. A single store lookup, so cheaper than + /// filtering when you only want one — map it to a details endpoint. + /// + public RuleInfo? GetRule(string id) + { + ObjectDisposedException.ThrowIf(_disposed, this); + + var orders = EvaluationOrders(); + + var metadata = _store.Find(id); + if (metadata is not null && !IsForAnotherContract(metadata)) + return ToInfo(metadata, orders); + + var entry = _registry.Snapshot.FirstOrDefault(e => e.Id == id && e.Origin == RuleOrigin.Static); + return entry is not null ? ToInfo(entry, orders) : null; + } + /// /// Every rule the engine knows about — stored ones (any status) and static ones — with the /// position each loaded rule occupies in the evaluation order. Loaded rules come first, in @@ -622,7 +681,7 @@ public IReadOnlyList GetRules() /// The store's rules minus any another engine left in the same folder. An engine reports on the /// rules it could actually run — listing a rule it would refuse to approve helps nobody. /// - private IEnumerable OwnRules() => _store.LoadAll().Where(m => !IsForAnotherContract(m)); + private IEnumerable OwnRules() => _store.LoadAll().Where(m => !IsForAnotherContract(m)); /// Loaded rule id → 0-based position in the current evaluation order. private Dictionary EvaluationOrders() @@ -634,7 +693,7 @@ private Dictionary EvaluationOrders() return orders; } - private RuleInfo ToInfo(RuleMetadata metadata, Dictionary? orders = null) => new( + private RuleInfo ToInfo(RuleRecord metadata, Dictionary? orders = null) => new( metadata.Id, metadata.Name, metadata.Status, @@ -668,7 +727,10 @@ private Dictionary EvaluationOrders() private static readonly ValidationReport EmptyReport = new([], [], []); - private static string NewId() => Guid.NewGuid().ToString("N")[..12]; + // Full 128 bits, not a truncated slice: the id names the store files a rule is saved under, so a + // collision would silently overwrite another rule. 48 bits (12 hex) reached a 1% birthday chance + // around a few million rules; the full GUID makes that a non-consideration. + private static string NewId() => Guid.NewGuid().ToString("N"); private static string? Coalesce(params string?[] candidates) => candidates.FirstOrDefault(c => !string.IsNullOrWhiteSpace(c)); diff --git a/src/RuleCraft/Engine/RuleEngineOptions.cs b/src/RuleCraft/Engine/RuleEngineOptions.cs index 8743643..1839215 100644 --- a/src/RuleCraft/Engine/RuleEngineOptions.cs +++ b/src/RuleCraft/Engine/RuleEngineOptions.cs @@ -19,6 +19,14 @@ public sealed class RuleEngineOptions /// public string StorePath { get; set; } = Path.Combine(Environment.CurrentDirectory, "rules"); + /// + /// Where rules are persisted. Leave null to use a over + /// ; set it to a custom (a shared database, say) + /// so several engine instances can see the same rules. When set, is ignored. + /// The reference is shared, not deep-copied, so the store must be thread-safe. + /// + public IRuleStore? Store { get; set; } + /// LLM used by AddRuleAsync(spec). Not required for source-based or store-reload flows. public IChatClient? ChatClient { get; set; } @@ -67,6 +75,7 @@ internal RuleEngineOptions Snapshot() AutoApprove = AutoApprove, SecurityPolicy = SecurityPolicy.Snapshot(), LoggerFactory = LoggerFactory, + Store = Store, }; foreach (var assembly in AdditionalReferenceAssemblies) @@ -77,18 +86,22 @@ internal RuleEngineOptions Snapshot() private void Validate() { - if (string.IsNullOrWhiteSpace(StorePath)) - throw new ArgumentException("StorePath must name a folder for the rule store.", nameof(StorePath)); - - // The folder itself is created lazily, on first write — but a malformed path should fail - // here, at the composition root, not deep inside the first request that adds a rule. - try - { - Path.GetFullPath(StorePath); - } - catch (Exception ex) + // A custom Store owns its own addressing; StorePath is only meaningful for the default file store. + if (Store is null) { - throw new ArgumentException($"StorePath '{StorePath}' is not a usable path.", nameof(StorePath), ex); + if (string.IsNullOrWhiteSpace(StorePath)) + throw new ArgumentException("StorePath must name a folder for the rule store.", nameof(StorePath)); + + // The folder itself is created lazily, on first write — but a malformed path should fail + // here, at the composition root, not deep inside the first request that adds a rule. + try + { + Path.GetFullPath(StorePath); + } + catch (Exception ex) + { + throw new ArgumentException($"StorePath '{StorePath}' is not a usable path.", nameof(StorePath), ex); + } } if (MaxGenerationAttempts < 1) diff --git a/src/RuleCraft/FileRuleStore.cs b/src/RuleCraft/FileRuleStore.cs new file mode 100644 index 0000000..c7707bf --- /dev/null +++ b/src/RuleCraft/FileRuleStore.cs @@ -0,0 +1,132 @@ +using System.Text; +using System.Text.Json; +using System.Text.Json.Serialization; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace RuleCraft; + +/// +/// The default : file-based persistence under one folder, two files per rule — +/// <id>.meta.json (the record) and the source, <id>.cs for compiled rules or +/// <id>.rule.json for JSON-DSL rules. Writes go through a temp file and a replacing move, +/// so a crash mid-write cannot leave a truncated file; source is written before the record, so a crash +/// between the two leaves an orphaned source (harmless) rather than a record pointing at nothing. +/// +public sealed class FileRuleStore : IRuleStore +{ + private const string MetadataSuffix = ".meta.json"; + + private static readonly JsonSerializerOptions JsonOptions = new() + { + WriteIndented = true, + PropertyNameCaseInsensitive = true, + Converters = { new JsonStringEnumConverter() }, + }; + + private readonly string _root; + private readonly ILogger _logger; + private readonly object _lock = new(); + + // No I/O here: constructing a store should not touch the disk, and one nobody writes to has no + // business creating a folder. The folder appears on the first Save. + public FileRuleStore(string root, ILogger? logger = null) + { + if (string.IsNullOrWhiteSpace(root)) + throw new ArgumentException("The store root must name a folder.", nameof(root)); + _root = root; + _logger = logger ?? NullLogger.Instance; + } + + public void Save(StoredRule rule) + { + ArgumentNullException.ThrowIfNull(rule); + lock (_lock) + { + Directory.CreateDirectory(_root); + WriteAtomic(SourcePath(rule.Record), rule.Source); + WriteMetadata(rule.Record); + } + } + + public void Update(RuleRecord record) + { + ArgumentNullException.ThrowIfNull(record); + lock (_lock) + { + WriteMetadata(record); + } + } + + public RuleRecord? Find(string id) + { + lock (_lock) + { + var path = MetadataPath(id); + if (!File.Exists(path)) + return null; + return JsonSerializer.Deserialize(File.ReadAllText(path, Encoding.UTF8), JsonOptions); + } + } + + public string ReadSource(RuleRecord record) + { + ArgumentNullException.ThrowIfNull(record); + lock (_lock) + { + return File.ReadAllText(SourcePath(record), Encoding.UTF8); + } + } + + /// + /// Every rule's record. A file that is not readable metadata is logged and skipped — a stray or + /// corrupt .json in the folder must not take down the application at startup. + /// + public IReadOnlyList LoadAll() + { + lock (_lock) + { + if (!Directory.Exists(_root)) + return []; + + var result = new List(); + + // Enumerate broadly and filter in code: a glob of "*.meta.json" can also match via + // Windows 8.3 short names. + foreach (var path in Directory.EnumerateFiles(_root, "*.json")) + { + if (!path.EndsWith(MetadataSuffix, StringComparison.OrdinalIgnoreCase)) + continue; + + try + { + var record = JsonSerializer.Deserialize( + File.ReadAllText(path, Encoding.UTF8), JsonOptions); + if (record is not null) + result.Add(record); + } + catch (Exception ex) + { + _logger.LogError(ex, "Skipping unreadable rule metadata file {Path}.", path); + } + } + + return result.OrderBy(m => m.CreatedUtc).ToList(); + } + } + + private void WriteMetadata(RuleRecord record) => + WriteAtomic(MetadataPath(record.Id), JsonSerializer.Serialize(record, JsonOptions)); + + private static void WriteAtomic(string path, string content) + { + var temporary = path + ".tmp"; + File.WriteAllText(temporary, content, Encoding.UTF8); + File.Move(temporary, path, overwrite: true); + } + + private string SourcePath(RuleRecord record) => + Path.Combine(_root, record.Id + (record.Origin == RuleOrigin.Json ? ".rule.json" : ".cs")); + + private string MetadataPath(string id) => Path.Combine(_root, id + MetadataSuffix); +} diff --git a/src/RuleCraft/IRuleStore.cs b/src/RuleCraft/IRuleStore.cs new file mode 100644 index 0000000..8ea1768 --- /dev/null +++ b/src/RuleCraft/IRuleStore.cs @@ -0,0 +1,92 @@ +using System.Security.Cryptography; +using System.Text; + +namespace RuleCraft; + +/// +/// A rule's persisted audit record — everything about a rule except its source text. Immutable: a +/// status change produces a new record via with, so a store never sees a half-mutated one. +/// The field names are the on-disk JSON contract for ; a custom store may +/// map them to columns however it likes, but must round-trip every one — the engine relies on +/// , / and +/// for integrity and contract-change detection. +/// +public sealed record RuleRecord +{ + public required string Id { get; init; } + + public required string Name { get; init; } + + public string? Spec { get; init; } + + /// What kind of rule this is; drives the source extension and which kind validates it. + public RuleOrigin Origin { get; init; } = RuleOrigin.Compiled; + + public RuleStatus Status { get; init; } + + public int Priority { get; init; } + + public DateTimeOffset CreatedUtc { get; init; } + + /// SHA-256 of the exact source bytes, set by the engine at save time; the tamper signal. + public string? SourceSha256 { get; init; } + + /// Contract/context type this rule was written against, by full name. Null in pre-versioned metadata. + public string? ContractType { get; init; } + + /// + public string? ContextType { get; init; } + + /// Hash of the contract/context API shape at approval time; distinguishes "stale" from "not mine". + public string? ContractFingerprint { get; init; } + + public string? ModelId { get; init; } + + public string? ApprovedBy { get; init; } + + public DateTimeOffset? ApprovedAtUtc { get; init; } + + /// Rejection reason / quarantine cause, when applicable. + public string? StatusReason { get; init; } + + public ValidationReport? Report { get; init; } +} + +/// A rule's record together with its source text — the unit a store persists on . +public sealed record StoredRule(RuleRecord Record, string Source); + +/// +/// Where an engine persists rules. The default is (one folder, two files +/// per rule); implement this to back rules with a shared database or object store so several engine +/// instances can see the same rules — set it on . +/// +/// Implementations must be thread-safe: an engine calls them from concurrent approval operations +/// (serialized per rule id, but not across ids) and from a lock-free read path is NOT one of them — +/// the store is never on Resolve. The engine owns integrity (hashing, tamper detection) and +/// contract checks; a store is pure persistence and must round-trip every +/// field and the source verbatim. +/// +public interface IRuleStore +{ + /// Creates or replaces a rule's source and record. Source is written before the record. + void Save(StoredRule rule); + + /// Replaces a rule's record only, leaving its source untouched — used for status changes. + void Update(RuleRecord record); + + /// The record for , or null if the store has no such rule. + RuleRecord? Find(string id); + + /// The source text for a rule. The record carries the origin a file store needs to find it. + string ReadSource(RuleRecord record); + + /// Every rule's record. Sources are read lazily via , not here. + IReadOnlyList LoadAll(); +} + +/// The one hash both the engine (when saving/verifying) and a store agree on. +internal static class RuleHash +{ + public static string Sha256Hex(string source) => + Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(source))); +} diff --git a/src/RuleCraft/RuleCraft.csproj b/src/RuleCraft/RuleCraft.csproj index 07b0495..9ef0de4 100644 --- a/src/RuleCraft/RuleCraft.csproj +++ b/src/RuleCraft/RuleCraft.csproj @@ -30,6 +30,13 @@ true + + + latest + All + + true diff --git a/src/RuleCraft/RuleExecution.cs b/src/RuleCraft/RuleExecution.cs new file mode 100644 index 0000000..e62d536 --- /dev/null +++ b/src/RuleCraft/RuleExecution.cs @@ -0,0 +1,53 @@ +using RuleCraft.Testing; + +namespace RuleCraft; + +/// +/// Opt-in bounded invocation for a resolved rule's implementation. +/// hands back an implementation whose methods run unguarded on your thread; wrap the call here when a +/// request thread must never block on a pathological (LLM-generated) rule. +/// +/// The same honest caveat as the validation harness applies: in-process code cannot be forcibly +/// stopped. On timeout this returns false promptly, but the runaway thread keeps running until +/// it returns on its own — a leak, not a kill. This is a backstop for latency, not a security +/// boundary; the security boundary is the acceptance tests and the human approval gate. +/// +public static class RuleExecution +{ + /// + /// Runs and waits up to . Returns true + /// with when it completed in time; false with + /// set to the unwrapped exception it threw, or a when it ran long. + /// + public static bool TryInvoke(Func invocation, TimeSpan timeout, out T? result, out Exception? error) + { + ArgumentNullException.ThrowIfNull(invocation); + if (timeout <= TimeSpan.Zero) + throw new ArgumentOutOfRangeException(nameof(timeout), timeout, "The timeout must be positive."); + + result = default; + error = null; + + var task = Task.Run(invocation); + try + { + // Wait returns true only if the body finished within the budget of its own accord — the + // same reasoning as TestExecution.RunWithTimeout: never trust a result produced after the + // deadline, and never fail a legitimately-slow one at the boundary. + if (!task.Wait(timeout)) + { + error = new TimeoutException( + $"Rule invocation exceeded {timeout.TotalSeconds:0.#}s (its thread may still be running)."); + return false; + } + + result = task.Result; + return true; + } + catch (Exception ex) + { + error = TestExecution.Unwrap(ex); + return false; + } + } +} diff --git a/src/RuleCraft/Security/SecurityPolicy.cs b/src/RuleCraft/Security/SecurityPolicy.cs index 8667c87..e419d76 100644 --- a/src/RuleCraft/Security/SecurityPolicy.cs +++ b/src/RuleCraft/Security/SecurityPolicy.cs @@ -79,6 +79,7 @@ public static SecurityPolicy Default policy.BannedTypes.UnionWith(new[] { "System.Activator", + "System.AppContext", "System.AppDomain", "System.Environment", "System.GC", diff --git a/src/RuleCraft/Store/RuleMetadata.cs b/src/RuleCraft/Store/RuleMetadata.cs deleted file mode 100644 index b19a620..0000000 --- a/src/RuleCraft/Store/RuleMetadata.cs +++ /dev/null @@ -1,58 +0,0 @@ -namespace RuleCraft.Store; - -/// Sidecar metadata persisted next to each rule's source file. -internal sealed class RuleMetadata -{ - public required string Id { get; set; } - - public required string Name { get; set; } - - public string? Spec { get; set; } - - /// - /// What kind of rule this is; drives the source file extension and which kind validates it. - /// Initialized (not defaulted by enum order) so metadata written before JSON rules existed - /// still reads back as Compiled. - /// - public RuleOrigin Origin { get; set; } = RuleOrigin.Compiled; - - public RuleStatus Status { get; set; } - - public int Priority { get; set; } - - public DateTimeOffset CreatedUtc { get; set; } - - public string? SourceSha256 { get; set; } - - /// - /// Contract and context types this rule was written against, by full name. Null in metadata - /// written before these fields existed, and then assumed to be the reading engine's own. - /// - /// Load-bearing because defaults to a folder relative - /// to the process: two engines over different contracts land in the same one unless told - /// otherwise, and each would read the other's rules as "no longer valid" and quarantine them — - /// a permanent disk write that fixing the configuration afterwards would not undo. - /// - public string? ContractType { get; set; } - - /// - public string? ContextType { get; set; } - - /// - /// Hash of the contract and context API shape at approval time. Same types, different hash means - /// the application changed underneath the rule — which is the difference between "this rule is - /// stale" and "this rule was never mine". - /// - public string? ContractFingerprint { get; set; } - - public string? ModelId { get; set; } - - public string? ApprovedBy { get; set; } - - public DateTimeOffset? ApprovedAtUtc { get; set; } - - /// Rejection reason / quarantine cause, when applicable. - public string? StatusReason { get; set; } - - public ValidationReport? Report { get; set; } -} diff --git a/src/RuleCraft/Store/RuleStore.cs b/src/RuleCraft/Store/RuleStore.cs deleted file mode 100644 index b9be31e..0000000 --- a/src/RuleCraft/Store/RuleStore.cs +++ /dev/null @@ -1,146 +0,0 @@ -using System.Security.Cryptography; -using System.Text; -using System.Text.Json; -using System.Text.Json.Serialization; -using Microsoft.Extensions.Logging; - -namespace RuleCraft.Store; - -/// -/// File-based persistence under the configured root, two files per rule: -/// <id>.meta.json (audit metadata) and the source — <id>.cs for compiled -/// rules or <id>.rule.json for JSON-DSL rules. Sources are the single source of truth: -/// rules are recompiled/reparsed from them on every reload. -/// -internal sealed class RuleStore -{ - private const string MetadataSuffix = ".meta.json"; - - private static readonly JsonSerializerOptions JsonOptions = new() - { - WriteIndented = true, - Converters = { new JsonStringEnumConverter() }, - }; - - private readonly string _root; - private readonly ILogger _logger; - private readonly object _lock = new(); - - // No I/O here: constructing an engine should not touch the disk, and a store nobody writes to - // has no business creating a folder. RuleEngineOptions already rejected a malformed path. - public RuleStore(string root, ILogger logger) - { - _root = root; - _logger = logger; - } - - public void Save(RuleMetadata metadata, string source) - { - lock (_lock) - { - Directory.CreateDirectory(_root); - - // Hash exactly the bytes we write — never round-trip the document through a serializer, - // or the reviewer stops seeing what is actually stored. - metadata.SourceSha256 = Hash(source); - - // Source before metadata: metadata is the index LoadAll reads, so a crash between the - // two leaves an orphaned source file (harmless) rather than a rule pointing at nothing. - WriteAtomic(SourcePath(metadata), source); - WriteMetadata(metadata); - } - } - - public void UpdateMetadata(RuleMetadata metadata) - { - lock (_lock) - { - WriteMetadata(metadata); - } - } - - public RuleMetadata? Find(string id) - { - lock (_lock) - { - var path = MetadataPath(id); - if (!File.Exists(path)) - return null; - return JsonSerializer.Deserialize(File.ReadAllText(path, Encoding.UTF8), JsonOptions); - } - } - - public string ReadSource(RuleMetadata metadata) - { - lock (_lock) - { - return File.ReadAllText(SourcePath(metadata), Encoding.UTF8); - } - } - - /// True when the on-disk source no longer matches the hash recorded at save time. - public bool IsSourceTampered(RuleMetadata metadata) => - metadata.SourceSha256 is not null && Hash(ReadSource(metadata)) != metadata.SourceSha256; - - /// - /// All rules in the store. A file that is not readable metadata is logged and skipped — - /// a stray or corrupt .json in the folder must not take down the application at startup. - /// - public IReadOnlyList LoadAll() - { - lock (_lock) - { - // Nothing has been saved yet; the folder appears on the first write. - if (!Directory.Exists(_root)) - return []; - - var result = new List(); - - // Enumerate broadly and filter in code: a glob of "*.meta.json" can also match via - // Windows 8.3 short names. - foreach (var path in Directory.EnumerateFiles(_root, "*.json")) - { - if (!path.EndsWith(MetadataSuffix, StringComparison.OrdinalIgnoreCase)) - continue; - - try - { - var metadata = JsonSerializer.Deserialize( - File.ReadAllText(path, Encoding.UTF8), JsonOptions); - if (metadata is not null) - result.Add(metadata); - } - catch (Exception ex) - { - _logger.LogError(ex, "Skipping unreadable rule metadata file {Path}.", path); - } - } - - return result.OrderBy(m => m.CreatedUtc).ToList(); - } - } - - private void WriteMetadata(RuleMetadata metadata) => - WriteAtomic(MetadataPath(metadata.Id), JsonSerializer.Serialize(metadata, JsonOptions)); - - /// - /// Writes via a temp file and a replacing move, so a crash mid-write cannot leave a truncated - /// file behind. A half-written .meta.json would be skipped by as corrupt, - /// silently dropping an approved rule from the application on the next restart — for a file - /// that doubles as the audit trail, "mostly written" is not good enough. - /// - private static void WriteAtomic(string path, string content) - { - var temporary = path + ".tmp"; - File.WriteAllText(temporary, content, Encoding.UTF8); - File.Move(temporary, path, overwrite: true); - } - - private string SourcePath(RuleMetadata metadata) => - Path.Combine(_root, metadata.Id + (metadata.Origin == RuleOrigin.Json ? ".rule.json" : ".cs")); - - private string MetadataPath(string id) => Path.Combine(_root, id + MetadataSuffix); - - private static string Hash(string source) => - Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(source))); -} diff --git a/src/RuleCraft/packages.lock.json b/src/RuleCraft/packages.lock.json new file mode 100644 index 0000000..f32bc1f --- /dev/null +++ b/src/RuleCraft/packages.lock.json @@ -0,0 +1,96 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Microsoft.CodeAnalysis.CSharp": { + "type": "Direct", + "requested": "[5.6.0, )", + "resolved": "5.6.0", + "contentHash": "r1DrKQ/L0xTw03wJrLr36AMQNslyaeEKBFyFmQcOKa8HX3YvmhY//JEUafb6IR/m0gmaUVCfBTWitKJRNb7YAA==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0", + "Microsoft.CodeAnalysis.Common": "[5.6.0]", + "System.Collections.Immutable": "10.0.1", + "System.Reflection.Metadata": "10.0.1" + } + }, + "Microsoft.Extensions.AI.Abstractions": { + "type": "Direct", + "requested": "[10.8.0, )", + "resolved": "10.8.0", + "contentHash": "1tJZ5sAYrEq1YjNg8GrZSL1tVodsWRfrgybGsZo5DQsWQgLZ+dSR5uFowIS6vb/9zxGBPl7xpYq53QMjNObO9w==", + "dependencies": { + "System.Text.Json": "10.0.10" + } + }, + "Microsoft.Extensions.DependencyInjection.Abstractions": { + "type": "Direct", + "requested": "[10.0.10, )", + "resolved": "10.0.10", + "contentHash": "z/2xXlFw2aLGjHyEm6E0tQ+In6VfzQzTrtArbQ2c0TQE16ZbyDCMGPvaUT9I0s8rgy9sRWlU2P9waW37qV04qA==" + }, + "Microsoft.Extensions.Logging.Abstractions": { + "type": "Direct", + "requested": "[10.0.10, )", + "resolved": "10.0.10", + "contentHash": "zkFxGYUvdxAvIKTyXHrmW+Sux53D4SezD9dMyZ6hrwwzPQJNuwCRy1f5W7AvYTqacEGhWF2XderRQG1OvbV8og==", + "dependencies": { + "Microsoft.Extensions.DependencyInjection.Abstractions": "10.0.10", + "System.Diagnostics.DiagnosticSource": "10.0.10" + } + }, + "Microsoft.CodeAnalysis.Analyzers": { + "type": "Transitive", + "resolved": "5.3.0", + "contentHash": "KuLhbZwB0L8JikL86AE5VWEp3RLNjIcp+j8yz9EJ/UBgRz4+qDEjHg/tluRFbpYpD/e37BqaaNFbQ0vqawBwWQ==" + }, + "Microsoft.CodeAnalysis.Common": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "eWYNB5e92PSdkQ0xcmy2aLtrvBXNydnVi0Hj/VjaAely6XBqA3By+ClGAJaj4d16pzQmrXPLLK9RDVuS1Ec9xQ==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0", + "System.Collections.Immutable": "10.0.1", + "System.Reflection.Metadata": "10.0.1" + } + }, + "System.Collections.Immutable": { + "type": "Transitive", + "resolved": "10.0.1", + "contentHash": "kdTe61B8P7i2M1pODC3MLbZ/CfFGjpC6c6jzxjQoB5DHZNewayCRqgFUmx3JKB6vLQtozpMQEiw+R5fO32Jv4g==" + }, + "System.Diagnostics.DiagnosticSource": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "cjtKi6ERMYWp6b9UTVPcwDT29PjKDtlM3W9OwnWL5abRsI8ku42Q2wqZoLIIXJnT/XF2s2CjuK8Nl4a3mmTxQQ==" + }, + "System.IO.Pipelines": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "7WX0W96y3dpQdYG4sEGdh38g3/0lOD4/dKbn2rRVOVzKhzoZUn2gKNIKaFeKWs8RCbpFfmmEWsRhSy95hMpvqA==" + }, + "System.Reflection.Metadata": { + "type": "Transitive", + "resolved": "10.0.1", + "contentHash": "zpcfT/wacPPhE17zcudozlxQtWN/84qyiMyZNGLnK4cj2IMBtLsZYwYjVnALUhPliwyUVj/P7kaZvBWYBCnf2Q==", + "dependencies": { + "System.Collections.Immutable": "10.0.1" + } + }, + "System.Text.Encodings.Web": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "o16m2YpDN/pjHsnxf9pTGwkpcuvjW8v1/wGUwJtM1c3QZUKm7ZEO/eYRJg7iIx6GxS2Zv9lAMHpiQwHDdgqauA==" + }, + "System.Text.Json": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "bmsO6UdYtBdtn32zYXfsh7KlyTIzV/3V9hdT9RIb4pXKgYOsNxXR+VbWigNwBtNFVGYGm6Hwmqw5a+/IWFd36Q==", + "dependencies": { + "System.IO.Pipelines": "10.0.10", + "System.Text.Encodings.Web": "10.0.10" + } + } + } + } +} \ No newline at end of file diff --git a/tests/RuleCraft.Tests/CustomRuleStoreTests.cs b/tests/RuleCraft.Tests/CustomRuleStoreTests.cs new file mode 100644 index 0000000..1fe330b --- /dev/null +++ b/tests/RuleCraft.Tests/CustomRuleStoreTests.cs @@ -0,0 +1,101 @@ +using RuleCraft; + +namespace RuleCraft.Tests; + +/// +/// A store nobody backs with a disk: proves the seam is real, and that two +/// engines sharing one store see each other's approved rules — the multi-instance story a single +/// process's file store cannot tell. +/// +public sealed class InMemoryRuleStore : IRuleStore +{ + private readonly object _lock = new(); + private readonly Dictionary _rules = new(StringComparer.Ordinal); + + public void Save(StoredRule rule) + { + lock (_lock) { _rules[rule.Record.Id] = rule; } + } + + public void Update(RuleRecord record) + { + // Update only ever follows a Save, so the source is already there — keep it, swap the record. + lock (_lock) { _rules[record.Id] = _rules[record.Id] with { Record = record }; } + } + + public RuleRecord? Find(string id) + { + lock (_lock) { return _rules.TryGetValue(id, out var stored) ? stored.Record : null; } + } + + public string ReadSource(RuleRecord record) + { + lock (_lock) { return _rules[record.Id].Source; } + } + + public IReadOnlyList LoadAll() + { + lock (_lock) { return _rules.Values.Select(s => s.Record).OrderBy(r => r.CreatedUtc).ToList(); } + } +} + +public class CustomRuleStoreTests +{ + private static RuleEngineOptions Options(IRuleStore store) => new() + { + Store = store, + TestTimeout = TimeSpan.FromSeconds(5), + }; + + [Fact] + public void A_custom_store_carries_a_rule_through_add_approve_and_resolve() + { + var store = new InMemoryRuleStore(); + using var engine = new RuleEngine(Options(store)); + + var info = engine.AddRuleFromSource(Fixtures.BigOrderRule); + Assert.Equal(RuleStatus.PendingApproval, info.Status); + + engine.Approve(info.Id, "reviewer"); + + var resolved = engine.Resolve(new TestOrder(150m, "alice", 1)); + Assert.NotNull(resolved); + Assert.Equal(0.10m, resolved!.GetDiscount(new TestOrder(150m, "alice", 1))); + } + + [Fact] + public void Two_engines_sharing_one_store_see_each_others_approved_rules() + { + var store = new InMemoryRuleStore(); + + using var writer = new RuleEngine(Options(store)); + var info = writer.AddRuleFromSource(Fixtures.BigOrderRule); + writer.Approve(info.Id, "reviewer"); + + // A second instance over the same store starts blank, then reloads — the cross-instance path + // a shared backend exists to serve. + using var reader = new RuleEngine(Options(store)); + Assert.Null(reader.Resolve(new TestOrder(150m, "alice", 1))); + + reader.ReloadFromStore(); + + var resolved = reader.Resolve(new TestOrder(150m, "alice", 1)); + Assert.NotNull(resolved); + Assert.Equal(0.10m, resolved!.GetDiscount(new TestOrder(150m, "alice", 1))); + } + + [Fact] + public void A_custom_store_still_gets_tamper_protection_from_the_engine() + { + var store = new InMemoryRuleStore(); + using var engine = new RuleEngine(Options(store)); + var info = engine.AddRuleFromSource(Fixtures.BigOrderRule); + + // Reach into the backing store and corrupt the source behind the engine's back, leaving the + // record (and its recorded hash) intact — exactly what the tamper check must catch. + var record = store.Find(info.Id)!; + store.Save(new StoredRule(record, "// tampered\n" + store.ReadSource(record))); + + Assert.Throws(() => engine.Approve(info.Id, "reviewer")); + } +} diff --git a/tests/RuleCraft.Tests/EngineConvenienceTests.cs b/tests/RuleCraft.Tests/EngineConvenienceTests.cs new file mode 100644 index 0000000..d0998c8 --- /dev/null +++ b/tests/RuleCraft.Tests/EngineConvenienceTests.cs @@ -0,0 +1,73 @@ +using RuleCraft; + +namespace RuleCraft.Tests; + +/// The low-ceremony conveniences: async offload wrappers and single-rule lookup. +public class EngineConvenienceTests +{ + private sealed class FlatStaticRule(decimal discount) : IRule, ITestDiscount + { + public bool AppliesTo(TestOrder context) => true; + public ITestDiscount Implementation => this; + public decimal GetDiscount(TestOrder order) => discount; + } + + [Fact] + public async Task AddRuleFromSourceAsync_then_ApproveAsync_loads_the_rule() + { + using var engine = new RuleEngine(Fixtures.Options()); + + var info = await engine.AddRuleFromSourceAsync(Fixtures.BigOrderRule); + Assert.Equal(RuleStatus.PendingApproval, info.Status); + + var approved = await engine.ApproveAsync(info.Id, "reviewer"); + Assert.Equal(RuleStatus.Approved, approved.Status); + + var resolved = engine.Resolve(new TestOrder(150m, "alice", 1)); + Assert.NotNull(resolved); + } + + [Fact] + public async Task An_already_cancelled_token_stops_the_async_add_before_it_runs() + { + using var engine = new RuleEngine(Fixtures.Options()); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAnyAsync( + () => engine.AddRuleFromSourceAsync(Fixtures.BigOrderRule, cancellationToken: cts.Token)); + } + + [Fact] + public void GetRule_returns_a_stored_rule_by_id() + { + using var engine = new RuleEngine(Fixtures.Options()); + var info = engine.AddRuleFromSource(Fixtures.BigOrderRule); + + var found = engine.GetRule(info.Id); + + Assert.NotNull(found); + Assert.Equal(info.Id, found!.Id); + Assert.Equal(RuleStatus.PendingApproval, found.Status); + } + + [Fact] + public void GetRule_returns_a_static_rule_by_id() + { + using var engine = new RuleEngine(Fixtures.Options()); + var info = engine.AddStaticRule(new FlatStaticRule(0.05m), "flat"); + + var found = engine.GetRule(info.Id); + + Assert.NotNull(found); + Assert.Equal(RuleOrigin.Static, found!.Origin); + Assert.True(found.IsLoaded); + } + + [Fact] + public void GetRule_returns_null_for_an_unknown_id() + { + using var engine = new RuleEngine(Fixtures.Options()); + Assert.Null(engine.GetRule("does-not-exist")); + } +} diff --git a/tests/RuleCraft.Tests/RuleExecutionTests.cs b/tests/RuleCraft.Tests/RuleExecutionTests.cs new file mode 100644 index 0000000..9ae85f4 --- /dev/null +++ b/tests/RuleCraft.Tests/RuleExecutionTests.cs @@ -0,0 +1,51 @@ +using System.Diagnostics; +using RuleCraft; + +namespace RuleCraft.Tests; + +public class RuleExecutionTests +{ + [Fact] + public void A_fast_invocation_returns_its_result() + { + var ok = RuleExecution.TryInvoke(() => 41 + 1, TimeSpan.FromSeconds(30), out var result, out var error); + + Assert.True(ok); + Assert.Equal(42, result); + Assert.Null(error); + } + + [Fact] + public void A_throwing_invocation_is_isolated_and_the_exception_is_unwrapped() + { + var ok = RuleExecution.TryInvoke( + () => throw new InvalidOperationException("boom"), TimeSpan.FromSeconds(30), out _, out var error); + + Assert.False(ok); + Assert.IsType(error); + Assert.Equal("boom", error!.Message); + } + + [Fact] + public void A_runaway_invocation_times_out_instead_of_blocking_forever() + { + var ok = RuleExecution.TryInvoke( + () => + { + var safety = Stopwatch.StartNew(); + while (safety.Elapsed < TimeSpan.FromSeconds(5)) { } + return 0; + }, + TimeSpan.FromMilliseconds(50), out _, out var error); + + Assert.False(ok); + Assert.IsType(error); + } + + [Fact] + public void A_nonpositive_timeout_is_rejected() + { + Assert.Throws( + () => RuleExecution.TryInvoke(() => 1, TimeSpan.Zero, out _, out _)); + } +} diff --git a/tests/RuleCraft.Tests/SecurityAnalyzerAdversarialTests.cs b/tests/RuleCraft.Tests/SecurityAnalyzerAdversarialTests.cs new file mode 100644 index 0000000..94ba69a --- /dev/null +++ b/tests/RuleCraft.Tests/SecurityAnalyzerAdversarialTests.cs @@ -0,0 +1,141 @@ +namespace RuleCraft.Tests; + +/// +/// A red-team corpus for the compiled-rule security gate: escapes that +/// does not already cover. The gate is a guardrail, not a sandbox (see SECURITY.md), so the invariant +/// is only ever "this never reaches the approval queue" — whether the compile step or the analyzer is +/// what refuses it. Each case that a benign rule must still be allowed is pinned by a positive control +/// at the bottom, so a future tightening cannot quietly ban legitimate arithmetic or string work. +/// +public class SecurityAnalyzerAdversarialTests +{ + private static string Wrap(string extraMembers) => + $$""" + using RuleCraft; + using RuleCraft.Tests; + + namespace RuleCraft.Generated.Adversarial; + + public sealed class AdversarialRule : IRule, ITestDiscount + { + public bool AppliesTo(TestOrder context) => true; + public ITestDiscount Implementation => this; + public decimal GetDiscount(TestOrder order) => 0m; + + {{extraMembers}} + } + + public sealed class SmokeTest : IRuleTest + { + public string Name => "smoke"; + public TestResult Run(TestContext context) => TestResult.Passed(); + } + """; + + private static RuleValidationException AssertRejected(string extraMembers) + { + var engine = new RuleEngine(Fixtures.Options()); + return Assert.Throws(() => engine.AddRuleFromSource(Wrap(extraMembers))); + } + + // -------------------------------------------------------------- reflection via a value, not a name + + [Fact] + public void Reflection_through_an_instance_GetType_is_rejected() + { + // GetType() itself is System.Object.GetType — not banned. The escape is what the returned + // System.Type gives you; every member on it must be refused, or reflection is one dot away. + var ex = AssertRejected("public object Reach() => \"x\".GetType().Assembly;"); + Assert.False(ex.Report.Success); + } + + [Fact] + public void Reflection_through_typeof_is_rejected() + { + var ex = AssertRejected("public object Reach() => typeof(TestOrder).Assembly;"); + Assert.False(ex.Report.Success); + } + + [Fact] + public void GetType_module_traversal_is_rejected() + { + var ex = AssertRejected("public object Reach() => this.GetType().Module;"); + Assert.False(ex.Report.Success); + } + + // -------------------------------------------------------------- a banned type worn as a type argument + + [Fact] + public void A_banned_type_as_a_generic_argument_is_rejected() + { + var ex = AssertRejected( + "public System.Collections.Generic.List Reach() => new();"); + Assert.Contains(ex.Report.SecurityFindings, f => f.Message.Contains("System.Type")); + } + + // -------------------------------------------------------------- banned call hidden inside a lambda + + [Fact] + public void A_banned_call_inside_a_lambda_body_is_rejected() + { + var ex = AssertRejected( + "public int Reach() { System.Func f = () => { System.IO.File.Delete(\"x\"); return 1; }; return f(); }"); + Assert.False(ex.Report.Success); + } + + [Fact] + public void A_banned_call_inside_a_local_function_is_rejected() + { + var ex = AssertRejected( + "public void Reach() { void Inner() => System.GC.Collect(); Inner(); }"); + Assert.Contains(ex.Report.SecurityFindings, f => f.Message.Contains("System.GC")); + } + + // -------------------------------------------------------------- the open root System.* surface + + [Fact] + public void AppContext_data_access_is_rejected() + { + // System.AppContext sits directly under the (unbanned) System root, so nothing but an explicit + // type ban stops it. GetData can read process-global switches — a data leak the guardrail owes + // a reviewer, even if it is not remote code execution. + var ex = AssertRejected("public object? Reach() => System.AppContext.GetData(\"x\");"); + Assert.Contains(ex.Report.SecurityFindings, f => f.Message.Contains("System.AppContext")); + } + + [Fact] + public void AppContext_base_directory_is_rejected() + { + var ex = AssertRejected("public string Reach() => System.AppContext.BaseDirectory;"); + Assert.Contains(ex.Report.SecurityFindings, f => f.Message.Contains("System.AppContext")); + } + + // -------------------------------------------------------------- positive controls: benign code stays legal + + [Fact] + public void Arithmetic_string_and_date_work_still_passes_the_gate() + { + var engine = new RuleEngine(Fixtures.Options()); + var source = Wrap( + """ + public decimal Compute(TestOrder o) => System.Math.Round(o.Total * 0.1m, 2); + public bool Named(TestOrder o) => o.Customer.StartsWith("v", System.StringComparison.OrdinalIgnoreCase); + public int Day() => System.DateTime.UtcNow.Day; + public string Id() => System.Guid.NewGuid().ToString("N"); + """); + + var exception = Record.Exception(() => engine.AddRuleFromSource(source)); + Assert.Null(exception); + } + + [Fact] + public void Regex_in_a_rule_still_passes_the_gate() + { + // Regex is deliberately allowed (business rules need it); ReDoS is documented as out of scope. + var engine = new RuleEngine(Fixtures.Options()); + var source = Wrap( + "public bool Reach(TestOrder o) => System.Text.RegularExpressions.Regex.IsMatch(o.Customer, \"^v\");"); + + Assert.Null(Record.Exception(() => engine.AddRuleFromSource(source))); + } +} diff --git a/tests/RuleCraft.Tests/packages.lock.json b/tests/RuleCraft.Tests/packages.lock.json new file mode 100644 index 0000000..276a0fe --- /dev/null +++ b/tests/RuleCraft.Tests/packages.lock.json @@ -0,0 +1,352 @@ +{ + "version": 1, + "dependencies": { + "net10.0": { + "coverlet.collector": { + "type": "Direct", + "requested": "[10.0.1, )", + "resolved": "10.0.1", + "contentHash": "27jXSV/0DbVqF5jDrAxuQFZ9oaz6gmG03p8ttxAFk+X0M4woFYj7MoWDLCna5EGLb0CE6OE7X6ZH3Wt5smTtaA==" + }, + "Microsoft.Extensions.DependencyInjection": { + "type": "Direct", + "requested": "[10.0.10, )", + "resolved": "10.0.10", + "contentHash": "ANyvsgkNBRvcJh2XLgn8veGmajf+8m0AbKK+HPWdRL1yraSNVVSmQhFntLtdz/C795jxqqup+k05cs/3jZQPOA==", + "dependencies": { + "Microsoft.Extensions.DependencyInjection.Abstractions": "10.0.10" + } + }, + "Microsoft.NET.Test.Sdk": { + "type": "Direct", + "requested": "[18.8.1, )", + "resolved": "18.8.1", + "contentHash": "dknJL3/9Y3t4XuCBqnc0PevPxgLsUMmVhjwup/b1HNovA8zWcj3XsfIf7c6p05363DWcqL7X/YhDL9B+Zymv1w==", + "dependencies": { + "Microsoft.CodeCoverage": "18.8.1", + "Microsoft.TestPlatform.TestHost": "18.8.1" + } + }, + "xunit": { + "type": "Direct", + "requested": "[2.9.3, )", + "resolved": "2.9.3", + "contentHash": "TlXQBinK35LpOPKHAqbLY4xlEen9TBafjs0V5KnA4wZsoQLQJiirCR4CbIXvOH8NzkW4YeJKP5P/Bnrodm0h9Q==", + "dependencies": { + "xunit.analyzers": "1.18.0", + "xunit.assert": "2.9.3", + "xunit.core": "[2.9.3]" + } + }, + "xunit.runner.visualstudio": { + "type": "Direct", + "requested": "[3.1.5, )", + "resolved": "3.1.5", + "contentHash": "tKi7dSTwP4m5m9eXPM2Ime4Kn7xNf4x4zT9sdLO/G4hZVnQCRiMTWoSZqI/pYTVeI27oPPqHBKYI/DjJ9GsYgA==" + }, + "Microsoft.CodeAnalysis.Analyzers": { + "type": "Transitive", + "resolved": "5.3.0", + "contentHash": "KuLhbZwB0L8JikL86AE5VWEp3RLNjIcp+j8yz9EJ/UBgRz4+qDEjHg/tluRFbpYpD/e37BqaaNFbQ0vqawBwWQ==" + }, + "Microsoft.CodeAnalysis.Common": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "eWYNB5e92PSdkQ0xcmy2aLtrvBXNydnVi0Hj/VjaAely6XBqA3By+ClGAJaj4d16pzQmrXPLLK9RDVuS1Ec9xQ==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0" + } + }, + "Microsoft.CodeAnalysis.CSharp": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "r1DrKQ/L0xTw03wJrLr36AMQNslyaeEKBFyFmQcOKa8HX3YvmhY//JEUafb6IR/m0gmaUVCfBTWitKJRNb7YAA==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0", + "Microsoft.CodeAnalysis.Common": "[5.6.0]" + } + }, + "Microsoft.CodeCoverage": { + "type": "Transitive", + "resolved": "18.8.1", + "contentHash": "Eclse/ZZjr4lmWzZFNN9h/OluhKL+SK/QbUyKUewgX139aGeyMEO/DkMPwuFs2MixvanTnz6891rF8UHDg+W4Q==" + }, + "Microsoft.Extensions.AI.Abstractions": { + "type": "Transitive", + "resolved": "10.8.0", + "contentHash": "1tJZ5sAYrEq1YjNg8GrZSL1tVodsWRfrgybGsZo5DQsWQgLZ+dSR5uFowIS6vb/9zxGBPl7xpYq53QMjNObO9w==" + }, + "Microsoft.Extensions.DependencyInjection.Abstractions": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "z/2xXlFw2aLGjHyEm6E0tQ+In6VfzQzTrtArbQ2c0TQE16ZbyDCMGPvaUT9I0s8rgy9sRWlU2P9waW37qV04qA==" + }, + "Microsoft.Extensions.Logging.Abstractions": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "zkFxGYUvdxAvIKTyXHrmW+Sux53D4SezD9dMyZ6hrwwzPQJNuwCRy1f5W7AvYTqacEGhWF2XderRQG1OvbV8og==", + "dependencies": { + "Microsoft.Extensions.DependencyInjection.Abstractions": "10.0.10" + } + }, + "Microsoft.TestPlatform.ObjectModel": { + "type": "Transitive", + "resolved": "18.8.1", + "contentHash": "qLbktNB1+b1XZLNJBTzaWVVJAd6PEzD7cgD406geMb6PcFZhp3EDNa1tctWx1+mtMU6MP/6ozVvFPC9vs2a9rw==" + }, + "Microsoft.TestPlatform.TestHost": { + "type": "Transitive", + "resolved": "18.8.1", + "contentHash": "FaQHPDTUOcE+SFTjssNPfrub2lT9Zyon4J2W/KLHt/efLJACb1TCeWXyOgh0D/4Q1e4n+S3E6mOKud+9nLZlEA==", + "dependencies": { + "Microsoft.TestPlatform.ObjectModel": "18.8.1" + } + }, + "xunit.abstractions": { + "type": "Transitive", + "resolved": "2.0.3", + "contentHash": "pot1I4YOxlWjIb5jmwvvQNbTrZ3lJQ+jUGkGjWE3hEFM0l5gOnBWS+H3qsex68s5cO52g+44vpGzhAt+42vwKg==" + }, + "xunit.analyzers": { + "type": "Transitive", + "resolved": "1.18.0", + "contentHash": "OtFMHN8yqIcYP9wcVIgJrq01AfTxijjAqVDy/WeQVSyrDC1RzBWeQPztL49DN2syXRah8TYnfvk035s7L95EZQ==" + }, + "xunit.assert": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "/Kq28fCE7MjOV42YLVRAJzRF0WmEqsmflm0cfpMjGtzQ2lR5mYVj1/i0Y8uDAOLczkL3/jArrwehfMD0YogMAA==" + }, + "xunit.core": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "BiAEvqGvyme19wE0wTKdADH+NloYqikiU0mcnmiNyXaF9HyHmE6sr/3DC5vnBkgsWaE6yPyWszKSPSApWdRVeQ==", + "dependencies": { + "xunit.extensibility.core": "[2.9.3]", + "xunit.extensibility.execution": "[2.9.3]" + } + }, + "xunit.extensibility.core": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "kf3si0YTn2a8J8eZNb+zFpwfoyvIrQ7ivNk5ZYA5yuYk1bEtMe4DxJ2CF/qsRgmEnDr7MnW1mxylBaHTZ4qErA==", + "dependencies": { + "xunit.abstractions": "2.0.3" + } + }, + "xunit.extensibility.execution": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "yMb6vMESlSrE3Wfj7V6cjQ3S4TXdXpRqYeNEI3zsX31uTsGMJjEw6oD5F5u1cHnMptjhEECnmZSsPxB6ChZHDQ==", + "dependencies": { + "xunit.extensibility.core": "[2.9.3]" + } + }, + "rulecraft": { + "type": "Project", + "dependencies": { + "Microsoft.CodeAnalysis.CSharp": "[5.6.0, )", + "Microsoft.Extensions.AI.Abstractions": "[10.8.0, )", + "Microsoft.Extensions.DependencyInjection.Abstractions": "[10.0.10, )", + "Microsoft.Extensions.Logging.Abstractions": "[10.0.10, )" + } + } + }, + "net8.0": { + "coverlet.collector": { + "type": "Direct", + "requested": "[10.0.1, )", + "resolved": "10.0.1", + "contentHash": "27jXSV/0DbVqF5jDrAxuQFZ9oaz6gmG03p8ttxAFk+X0M4woFYj7MoWDLCna5EGLb0CE6OE7X6ZH3Wt5smTtaA==" + }, + "Microsoft.Extensions.DependencyInjection": { + "type": "Direct", + "requested": "[10.0.10, )", + "resolved": "10.0.10", + "contentHash": "ANyvsgkNBRvcJh2XLgn8veGmajf+8m0AbKK+HPWdRL1yraSNVVSmQhFntLtdz/C795jxqqup+k05cs/3jZQPOA==", + "dependencies": { + "Microsoft.Extensions.DependencyInjection.Abstractions": "10.0.10" + } + }, + "Microsoft.NET.Test.Sdk": { + "type": "Direct", + "requested": "[18.8.1, )", + "resolved": "18.8.1", + "contentHash": "dknJL3/9Y3t4XuCBqnc0PevPxgLsUMmVhjwup/b1HNovA8zWcj3XsfIf7c6p05363DWcqL7X/YhDL9B+Zymv1w==", + "dependencies": { + "Microsoft.CodeCoverage": "18.8.1", + "Microsoft.TestPlatform.TestHost": "18.8.1" + } + }, + "xunit": { + "type": "Direct", + "requested": "[2.9.3, )", + "resolved": "2.9.3", + "contentHash": "TlXQBinK35LpOPKHAqbLY4xlEen9TBafjs0V5KnA4wZsoQLQJiirCR4CbIXvOH8NzkW4YeJKP5P/Bnrodm0h9Q==", + "dependencies": { + "xunit.analyzers": "1.18.0", + "xunit.assert": "2.9.3", + "xunit.core": "[2.9.3]" + } + }, + "xunit.runner.visualstudio": { + "type": "Direct", + "requested": "[3.1.5, )", + "resolved": "3.1.5", + "contentHash": "tKi7dSTwP4m5m9eXPM2Ime4Kn7xNf4x4zT9sdLO/G4hZVnQCRiMTWoSZqI/pYTVeI27oPPqHBKYI/DjJ9GsYgA==" + }, + "Microsoft.CodeAnalysis.Analyzers": { + "type": "Transitive", + "resolved": "5.3.0", + "contentHash": "KuLhbZwB0L8JikL86AE5VWEp3RLNjIcp+j8yz9EJ/UBgRz4+qDEjHg/tluRFbpYpD/e37BqaaNFbQ0vqawBwWQ==" + }, + "Microsoft.CodeAnalysis.Common": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "eWYNB5e92PSdkQ0xcmy2aLtrvBXNydnVi0Hj/VjaAely6XBqA3By+ClGAJaj4d16pzQmrXPLLK9RDVuS1Ec9xQ==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0", + "System.Collections.Immutable": "10.0.1", + "System.Reflection.Metadata": "10.0.1" + } + }, + "Microsoft.CodeAnalysis.CSharp": { + "type": "Transitive", + "resolved": "5.6.0", + "contentHash": "r1DrKQ/L0xTw03wJrLr36AMQNslyaeEKBFyFmQcOKa8HX3YvmhY//JEUafb6IR/m0gmaUVCfBTWitKJRNb7YAA==", + "dependencies": { + "Microsoft.CodeAnalysis.Analyzers": "5.3.0", + "Microsoft.CodeAnalysis.Common": "[5.6.0]", + "System.Collections.Immutable": "10.0.1", + "System.Reflection.Metadata": "10.0.1" + } + }, + "Microsoft.CodeCoverage": { + "type": "Transitive", + "resolved": "18.8.1", + "contentHash": "Eclse/ZZjr4lmWzZFNN9h/OluhKL+SK/QbUyKUewgX139aGeyMEO/DkMPwuFs2MixvanTnz6891rF8UHDg+W4Q==" + }, + "Microsoft.Extensions.AI.Abstractions": { + "type": "Transitive", + "resolved": "10.8.0", + "contentHash": "1tJZ5sAYrEq1YjNg8GrZSL1tVodsWRfrgybGsZo5DQsWQgLZ+dSR5uFowIS6vb/9zxGBPl7xpYq53QMjNObO9w==", + "dependencies": { + "System.Text.Json": "10.0.10" + } + }, + "Microsoft.Extensions.DependencyInjection.Abstractions": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "z/2xXlFw2aLGjHyEm6E0tQ+In6VfzQzTrtArbQ2c0TQE16ZbyDCMGPvaUT9I0s8rgy9sRWlU2P9waW37qV04qA==" + }, + "Microsoft.Extensions.Logging.Abstractions": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "zkFxGYUvdxAvIKTyXHrmW+Sux53D4SezD9dMyZ6hrwwzPQJNuwCRy1f5W7AvYTqacEGhWF2XderRQG1OvbV8og==", + "dependencies": { + "Microsoft.Extensions.DependencyInjection.Abstractions": "10.0.10", + "System.Diagnostics.DiagnosticSource": "10.0.10" + } + }, + "Microsoft.TestPlatform.ObjectModel": { + "type": "Transitive", + "resolved": "18.8.1", + "contentHash": "qLbktNB1+b1XZLNJBTzaWVVJAd6PEzD7cgD406geMb6PcFZhp3EDNa1tctWx1+mtMU6MP/6ozVvFPC9vs2a9rw==" + }, + "Microsoft.TestPlatform.TestHost": { + "type": "Transitive", + "resolved": "18.8.1", + "contentHash": "FaQHPDTUOcE+SFTjssNPfrub2lT9Zyon4J2W/KLHt/efLJACb1TCeWXyOgh0D/4Q1e4n+S3E6mOKud+9nLZlEA==", + "dependencies": { + "Microsoft.TestPlatform.ObjectModel": "18.8.1" + } + }, + "System.Collections.Immutable": { + "type": "Transitive", + "resolved": "10.0.1", + "contentHash": "kdTe61B8P7i2M1pODC3MLbZ/CfFGjpC6c6jzxjQoB5DHZNewayCRqgFUmx3JKB6vLQtozpMQEiw+R5fO32Jv4g==" + }, + "System.Diagnostics.DiagnosticSource": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "cjtKi6ERMYWp6b9UTVPcwDT29PjKDtlM3W9OwnWL5abRsI8ku42Q2wqZoLIIXJnT/XF2s2CjuK8Nl4a3mmTxQQ==" + }, + "System.IO.Pipelines": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "7WX0W96y3dpQdYG4sEGdh38g3/0lOD4/dKbn2rRVOVzKhzoZUn2gKNIKaFeKWs8RCbpFfmmEWsRhSy95hMpvqA==" + }, + "System.Reflection.Metadata": { + "type": "Transitive", + "resolved": "10.0.1", + "contentHash": "zpcfT/wacPPhE17zcudozlxQtWN/84qyiMyZNGLnK4cj2IMBtLsZYwYjVnALUhPliwyUVj/P7kaZvBWYBCnf2Q==", + "dependencies": { + "System.Collections.Immutable": "10.0.1" + } + }, + "System.Text.Encodings.Web": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "o16m2YpDN/pjHsnxf9pTGwkpcuvjW8v1/wGUwJtM1c3QZUKm7ZEO/eYRJg7iIx6GxS2Zv9lAMHpiQwHDdgqauA==" + }, + "System.Text.Json": { + "type": "Transitive", + "resolved": "10.0.10", + "contentHash": "bmsO6UdYtBdtn32zYXfsh7KlyTIzV/3V9hdT9RIb4pXKgYOsNxXR+VbWigNwBtNFVGYGm6Hwmqw5a+/IWFd36Q==", + "dependencies": { + "System.IO.Pipelines": "10.0.10", + "System.Text.Encodings.Web": "10.0.10" + } + }, + "xunit.abstractions": { + "type": "Transitive", + "resolved": "2.0.3", + "contentHash": "pot1I4YOxlWjIb5jmwvvQNbTrZ3lJQ+jUGkGjWE3hEFM0l5gOnBWS+H3qsex68s5cO52g+44vpGzhAt+42vwKg==" + }, + "xunit.analyzers": { + "type": "Transitive", + "resolved": "1.18.0", + "contentHash": "OtFMHN8yqIcYP9wcVIgJrq01AfTxijjAqVDy/WeQVSyrDC1RzBWeQPztL49DN2syXRah8TYnfvk035s7L95EZQ==" + }, + "xunit.assert": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "/Kq28fCE7MjOV42YLVRAJzRF0WmEqsmflm0cfpMjGtzQ2lR5mYVj1/i0Y8uDAOLczkL3/jArrwehfMD0YogMAA==" + }, + "xunit.core": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "BiAEvqGvyme19wE0wTKdADH+NloYqikiU0mcnmiNyXaF9HyHmE6sr/3DC5vnBkgsWaE6yPyWszKSPSApWdRVeQ==", + "dependencies": { + "xunit.extensibility.core": "[2.9.3]", + "xunit.extensibility.execution": "[2.9.3]" + } + }, + "xunit.extensibility.core": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "kf3si0YTn2a8J8eZNb+zFpwfoyvIrQ7ivNk5ZYA5yuYk1bEtMe4DxJ2CF/qsRgmEnDr7MnW1mxylBaHTZ4qErA==", + "dependencies": { + "xunit.abstractions": "2.0.3" + } + }, + "xunit.extensibility.execution": { + "type": "Transitive", + "resolved": "2.9.3", + "contentHash": "yMb6vMESlSrE3Wfj7V6cjQ3S4TXdXpRqYeNEI3zsX31uTsGMJjEw6oD5F5u1cHnMptjhEECnmZSsPxB6ChZHDQ==", + "dependencies": { + "xunit.extensibility.core": "[2.9.3]" + } + }, + "rulecraft": { + "type": "Project", + "dependencies": { + "Microsoft.CodeAnalysis.CSharp": "[5.6.0, )", + "Microsoft.Extensions.AI.Abstractions": "[10.8.0, )", + "Microsoft.Extensions.DependencyInjection.Abstractions": "[10.0.10, )", + "Microsoft.Extensions.Logging.Abstractions": "[10.0.10, )" + } + } + } + } +} \ No newline at end of file From d2aea91b38c6abf41eddea0edfe04e4d2b543b1e Mon Sep 17 00:00:00 2001 From: Kasperczyk Date: Sun, 19 Jul 2026 11:14:56 +0200 Subject: [PATCH 2/3] fix lint --- src/RuleCraft/Engine/RuleEngine.cs | 28 +++++++++++-------- src/RuleCraft/FileRuleStore.cs | 16 +++++++++-- tests/RuleCraft.Tests/CustomRuleStoreTests.cs | 12 ++++++++ 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/RuleCraft/Engine/RuleEngine.cs b/src/RuleCraft/Engine/RuleEngine.cs index d2d0b66..31ce989 100644 --- a/src/RuleCraft/Engine/RuleEngine.cs +++ b/src/RuleCraft/Engine/RuleEngine.cs @@ -19,9 +19,10 @@ namespace RuleCraft; /// compiled C# (), JSON-DSL (, requires /// ) and static host code (). /// -/// Only the two generation methods are asynchronous, because only they do I/O — the call to the -/// LLM. Everything else (compiling, parsing, running tests, approving) is CPU-bound work that -/// runs on the calling thread; offload it with Task.Run if a request thread must not block. +/// Only rule generation is truly asynchronous, because only it does I/O — the call to the LLM. +/// Everything else (compiling, parsing, running tests, approving) is CPU-bound work that runs on the +/// calling thread; the ...FromSourceAsync// +/// wrappers offload it to Task.Run for a request thread that must not block. /// /// The engine is a thread-safe singleton: is lock-free, and mutations /// are serialized per rule id. unloads every rule assembly it owns. @@ -170,7 +171,7 @@ public RuleInfo AddStaticRule(IRule rule, string? name = nu _logger.LogInformation( "Static rule {RuleId} ('{Name}') registered from host code with priority {Priority}.", - entry.Id, entry.Name, rule.Priority); + entry.Id, ForLog(entry.Name), rule.Priority); return ToInfo(entry, EvaluationOrders()); } @@ -280,7 +281,7 @@ private RuleInfo Persist( Report = outcome.Report, }; _store.Save(new StoredRule(metadata, source)); - _logger.LogInformation("Rule {RuleId} ('{Name}') stored as PendingApproval.", id, metadata.Name); + _logger.LogInformation("Rule {RuleId} ('{Name}') stored as PendingApproval.", id, ForLog(metadata.Name)); return _options.AutoApprove ? ApproveCore(metadata, approvedBy: "auto-approve", preValidated: outcome) @@ -335,7 +336,7 @@ public RuleInfo Reject(string ruleId, string reason) var rejected = metadata with { Status = RuleStatus.Rejected, StatusReason = reason }; _store.Update(rejected); - _logger.LogInformation("Rule {RuleId} rejected: {Reason}", ruleId, reason); + _logger.LogInformation("Rule {RuleId} rejected: {Reason}", ForLog(ruleId), ForLog(reason)); return ToInfo(rejected); } } @@ -385,7 +386,7 @@ private RuleInfo ApproveCore( ContractFingerprint = CurrentFingerprint, }; _store.Update(approved); - _logger.LogInformation("Rule {RuleId} approved by {ApprovedBy} and loaded.", metadata.Id, approvedBy); + _logger.LogInformation("Rule {RuleId} approved by {ApprovedBy} and loaded.", metadata.Id, ForLog(approvedBy)); return ToInfo(approved); } @@ -444,7 +445,7 @@ public void RemoveRule(string ruleId) if (metadata is not null) _store.Update(metadata with { Status = RuleStatus.Disabled }); - _logger.LogInformation("Rule {RuleId} removed (was loaded: {WasLoaded}).", ruleId, removed is not null); + _logger.LogInformation("Rule {RuleId} removed (was loaded: {WasLoaded}).", ForLog(ruleId), removed is not null); } } @@ -477,7 +478,7 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) "Rule {RuleId} ('{Name}') was written against {StoredContract}/{StoredContext} but this " + "engine serves {Contract}/{Context}; skipping it. Two engines are sharing a StorePath — " + "give each its own.", - metadata.Id, metadata.Name, metadata.ContractType, metadata.ContextType, + metadata.Id, ForLog(metadata.Name), ForLog(metadata.ContractType), ForLog(metadata.ContextType), ContractTypeName, ContextTypeName); continue; } @@ -490,7 +491,7 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) _logger.LogError( "Rule {RuleId} ('{Name}') is a JSON rule but JSON rules are not enabled; skipping it. " + "Call EnableJsonRules(...) before ReloadFromStore().", - metadata.Id, metadata.Name); + metadata.Id, ForLog(metadata.Name)); continue; } @@ -510,7 +511,7 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) var loaded = outcome.Load(); _registry.Add(metadata.Id, metadata.Name, metadata.Origin, loaded.Rule, loaded.Context); - _logger.LogInformation("Rule {RuleId} ('{Name}') reloaded from store.", metadata.Id, metadata.Name); + _logger.LogInformation("Rule {RuleId} ('{Name}') reloaded from store.", metadata.Id, ForLog(metadata.Name)); } } } @@ -732,6 +733,11 @@ private Dictionary EvaluationOrders() // around a few million rules; the full GUID makes that a non-consideration. private static string NewId() => Guid.NewGuid().ToString("N"); + // User-controlled text (ids from callers, rule names, rejection reasons, approver strings) can + // carry newlines that forge a second, fake log line; flatten CR/LF before it reaches a template. + private static string ForLog(string? value) => + value is null ? string.Empty : value.Replace('\r', ' ').Replace('\n', ' '); + private static string? Coalesce(params string?[] candidates) => candidates.FirstOrDefault(c => !string.IsNullOrWhiteSpace(c)); } diff --git a/src/RuleCraft/FileRuleStore.cs b/src/RuleCraft/FileRuleStore.cs index c7707bf..5072cab 100644 --- a/src/RuleCraft/FileRuleStore.cs +++ b/src/RuleCraft/FileRuleStore.cs @@ -126,7 +126,19 @@ private static void WriteAtomic(string path, string content) } private string SourcePath(RuleRecord record) => - Path.Combine(_root, record.Id + (record.Origin == RuleOrigin.Json ? ".rule.json" : ".cs")); + Path.Combine(_root, SafeId(record.Id) + (record.Origin == RuleOrigin.Json ? ".rule.json" : ".cs")); - private string MetadataPath(string id) => Path.Combine(_root, id + MetadataSuffix); + private string MetadataPath(string id) => Path.Combine(_root, SafeId(id) + MetadataSuffix); + + // A rule id becomes a file name under the store root. Engine-minted ids are GUIDs, but Find, + // Approve and friends take an id straight from the caller — an HTTP route value, say — so a value + // like "../../secret" must not be allowed to climb out of the folder. Require a single, plain path + // segment and reject anything else before it reaches Path.Combine. + private static string SafeId(string id) + { + if (string.IsNullOrEmpty(id) || id is "." or ".." + || !string.Equals(id, Path.GetFileName(id), StringComparison.Ordinal)) + throw new ArgumentException($"'{id}' is not a valid rule id: it must be a single path segment.", nameof(id)); + return id; + } } diff --git a/tests/RuleCraft.Tests/CustomRuleStoreTests.cs b/tests/RuleCraft.Tests/CustomRuleStoreTests.cs index 1fe330b..fe33aed 100644 --- a/tests/RuleCraft.Tests/CustomRuleStoreTests.cs +++ b/tests/RuleCraft.Tests/CustomRuleStoreTests.cs @@ -84,6 +84,18 @@ public void Two_engines_sharing_one_store_see_each_others_approved_rules() Assert.Equal(0.10m, resolved!.GetDiscount(new TestOrder(150m, "alice", 1))); } + [Theory] + [InlineData("../escape")] + [InlineData("a/b")] + [InlineData("..")] + public void The_file_store_refuses_a_rule_id_that_could_escape_its_folder(string hostileId) + { + var store = new FileRuleStore(Fixtures.NewStorePath()); + + // Find is the reachable path: Approve/Reject/Enable pass a caller-supplied id straight to it. + Assert.Throws(() => store.Find(hostileId)); + } + [Fact] public void A_custom_store_still_gets_tamper_protection_from_the_engine() { From 943ba5c0df202c069c672c5f687f47b78c16ebca Mon Sep 17 00:00:00 2001 From: Kasperczyk Date: Sun, 19 Jul 2026 11:14:56 +0200 Subject: [PATCH 3/3] fix(security): harden file store paths and log output --- src/RuleCraft/Engine/RuleEngine.cs | 28 +++++++++++-------- src/RuleCraft/FileRuleStore.cs | 16 +++++++++-- tests/RuleCraft.Tests/CustomRuleStoreTests.cs | 12 ++++++++ 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/RuleCraft/Engine/RuleEngine.cs b/src/RuleCraft/Engine/RuleEngine.cs index d2d0b66..31ce989 100644 --- a/src/RuleCraft/Engine/RuleEngine.cs +++ b/src/RuleCraft/Engine/RuleEngine.cs @@ -19,9 +19,10 @@ namespace RuleCraft; /// compiled C# (), JSON-DSL (, requires /// ) and static host code (). /// -/// Only the two generation methods are asynchronous, because only they do I/O — the call to the -/// LLM. Everything else (compiling, parsing, running tests, approving) is CPU-bound work that -/// runs on the calling thread; offload it with Task.Run if a request thread must not block. +/// Only rule generation is truly asynchronous, because only it does I/O — the call to the LLM. +/// Everything else (compiling, parsing, running tests, approving) is CPU-bound work that runs on the +/// calling thread; the ...FromSourceAsync// +/// wrappers offload it to Task.Run for a request thread that must not block. /// /// The engine is a thread-safe singleton: is lock-free, and mutations /// are serialized per rule id. unloads every rule assembly it owns. @@ -170,7 +171,7 @@ public RuleInfo AddStaticRule(IRule rule, string? name = nu _logger.LogInformation( "Static rule {RuleId} ('{Name}') registered from host code with priority {Priority}.", - entry.Id, entry.Name, rule.Priority); + entry.Id, ForLog(entry.Name), rule.Priority); return ToInfo(entry, EvaluationOrders()); } @@ -280,7 +281,7 @@ private RuleInfo Persist( Report = outcome.Report, }; _store.Save(new StoredRule(metadata, source)); - _logger.LogInformation("Rule {RuleId} ('{Name}') stored as PendingApproval.", id, metadata.Name); + _logger.LogInformation("Rule {RuleId} ('{Name}') stored as PendingApproval.", id, ForLog(metadata.Name)); return _options.AutoApprove ? ApproveCore(metadata, approvedBy: "auto-approve", preValidated: outcome) @@ -335,7 +336,7 @@ public RuleInfo Reject(string ruleId, string reason) var rejected = metadata with { Status = RuleStatus.Rejected, StatusReason = reason }; _store.Update(rejected); - _logger.LogInformation("Rule {RuleId} rejected: {Reason}", ruleId, reason); + _logger.LogInformation("Rule {RuleId} rejected: {Reason}", ForLog(ruleId), ForLog(reason)); return ToInfo(rejected); } } @@ -385,7 +386,7 @@ private RuleInfo ApproveCore( ContractFingerprint = CurrentFingerprint, }; _store.Update(approved); - _logger.LogInformation("Rule {RuleId} approved by {ApprovedBy} and loaded.", metadata.Id, approvedBy); + _logger.LogInformation("Rule {RuleId} approved by {ApprovedBy} and loaded.", metadata.Id, ForLog(approvedBy)); return ToInfo(approved); } @@ -444,7 +445,7 @@ public void RemoveRule(string ruleId) if (metadata is not null) _store.Update(metadata with { Status = RuleStatus.Disabled }); - _logger.LogInformation("Rule {RuleId} removed (was loaded: {WasLoaded}).", ruleId, removed is not null); + _logger.LogInformation("Rule {RuleId} removed (was loaded: {WasLoaded}).", ForLog(ruleId), removed is not null); } } @@ -477,7 +478,7 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) "Rule {RuleId} ('{Name}') was written against {StoredContract}/{StoredContext} but this " + "engine serves {Contract}/{Context}; skipping it. Two engines are sharing a StorePath — " + "give each its own.", - metadata.Id, metadata.Name, metadata.ContractType, metadata.ContextType, + metadata.Id, ForLog(metadata.Name), ForLog(metadata.ContractType), ForLog(metadata.ContextType), ContractTypeName, ContextTypeName); continue; } @@ -490,7 +491,7 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) _logger.LogError( "Rule {RuleId} ('{Name}') is a JSON rule but JSON rules are not enabled; skipping it. " + "Call EnableJsonRules(...) before ReloadFromStore().", - metadata.Id, metadata.Name); + metadata.Id, ForLog(metadata.Name)); continue; } @@ -510,7 +511,7 @@ public void ReloadFromStore(CancellationToken cancellationToken = default) var loaded = outcome.Load(); _registry.Add(metadata.Id, metadata.Name, metadata.Origin, loaded.Rule, loaded.Context); - _logger.LogInformation("Rule {RuleId} ('{Name}') reloaded from store.", metadata.Id, metadata.Name); + _logger.LogInformation("Rule {RuleId} ('{Name}') reloaded from store.", metadata.Id, ForLog(metadata.Name)); } } } @@ -732,6 +733,11 @@ private Dictionary EvaluationOrders() // around a few million rules; the full GUID makes that a non-consideration. private static string NewId() => Guid.NewGuid().ToString("N"); + // User-controlled text (ids from callers, rule names, rejection reasons, approver strings) can + // carry newlines that forge a second, fake log line; flatten CR/LF before it reaches a template. + private static string ForLog(string? value) => + value is null ? string.Empty : value.Replace('\r', ' ').Replace('\n', ' '); + private static string? Coalesce(params string?[] candidates) => candidates.FirstOrDefault(c => !string.IsNullOrWhiteSpace(c)); } diff --git a/src/RuleCraft/FileRuleStore.cs b/src/RuleCraft/FileRuleStore.cs index c7707bf..5072cab 100644 --- a/src/RuleCraft/FileRuleStore.cs +++ b/src/RuleCraft/FileRuleStore.cs @@ -126,7 +126,19 @@ private static void WriteAtomic(string path, string content) } private string SourcePath(RuleRecord record) => - Path.Combine(_root, record.Id + (record.Origin == RuleOrigin.Json ? ".rule.json" : ".cs")); + Path.Combine(_root, SafeId(record.Id) + (record.Origin == RuleOrigin.Json ? ".rule.json" : ".cs")); - private string MetadataPath(string id) => Path.Combine(_root, id + MetadataSuffix); + private string MetadataPath(string id) => Path.Combine(_root, SafeId(id) + MetadataSuffix); + + // A rule id becomes a file name under the store root. Engine-minted ids are GUIDs, but Find, + // Approve and friends take an id straight from the caller — an HTTP route value, say — so a value + // like "../../secret" must not be allowed to climb out of the folder. Require a single, plain path + // segment and reject anything else before it reaches Path.Combine. + private static string SafeId(string id) + { + if (string.IsNullOrEmpty(id) || id is "." or ".." + || !string.Equals(id, Path.GetFileName(id), StringComparison.Ordinal)) + throw new ArgumentException($"'{id}' is not a valid rule id: it must be a single path segment.", nameof(id)); + return id; + } } diff --git a/tests/RuleCraft.Tests/CustomRuleStoreTests.cs b/tests/RuleCraft.Tests/CustomRuleStoreTests.cs index 1fe330b..fe33aed 100644 --- a/tests/RuleCraft.Tests/CustomRuleStoreTests.cs +++ b/tests/RuleCraft.Tests/CustomRuleStoreTests.cs @@ -84,6 +84,18 @@ public void Two_engines_sharing_one_store_see_each_others_approved_rules() Assert.Equal(0.10m, resolved!.GetDiscount(new TestOrder(150m, "alice", 1))); } + [Theory] + [InlineData("../escape")] + [InlineData("a/b")] + [InlineData("..")] + public void The_file_store_refuses_a_rule_id_that_could_escape_its_folder(string hostileId) + { + var store = new FileRuleStore(Fixtures.NewStorePath()); + + // Find is the reachable path: Approve/Reject/Enable pass a caller-supplied id straight to it. + Assert.Throws(() => store.Find(hostileId)); + } + [Fact] public void A_custom_store_still_gets_tamper_protection_from_the_engine() {