From faa8bb6e701ba82bf2c2573a7c6a73539057899e Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Fri, 22 May 2026 08:20:44 +0200 Subject: [PATCH 1/8] feat: add ExecApprovalsStore write path and wire coordinator side effects (PR8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the store write path (AddAllowlistEntryAsync, RecordAllowlistUseAsync) and wires the side-effect calls into ExecApprovalsCoordinator. Side effects fire strictly after the final allow decision is confirmed (both pass1 pre-approved and post-pass2 branches). Best-effort: never throw; any I/O failure degrades to a logged warning. UpdateFileAsync refuses to write a malformed file. SemaphoreSlim serializes intra-process writes. Pattern validation is non-empty only, matching macOS parity. New entries carry {id, pattern} only — lastUsedAt is absent on creation and stamped by RecordAllowlistUseAsync on first use (macOS addAllowlistEntry parity). Rail 19 preserved: coordinator not referenced in any production src/ file. Co-Authored-By: Claude Sonnet 4.6 --- .../ExecApprovals/ExecApprovalsCoordinator.cs | 66 ++++- .../ExecApprovals/ExecApprovalsStore.cs | 117 +++++++- .../ExecApprovalsCoordinatorTests.cs | 145 +++++++++ .../ExecApprovalsStoreTests.cs | 275 ++++++++++++++++++ 4 files changed, 584 insertions(+), 19 deletions(-) diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs index 3b8eba045..f06643d7a 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs @@ -8,10 +8,10 @@ namespace OpenClaw.Shared.ExecApprovals; // Full coordinator pipeline: validate → normalize → buildContext → evaluate(pass1) → -// prompt/fallback → [persistAllowlistEntry stub] → evaluate(pass2) → final decision. -// Rail 10: no WinUI types. Rail 17: SemaphoreSlim serializes the prompt+pass2 block. -// Rail 19: not wired in production src in PR7 — verified by test 15. -// Must be registered as singleton when wired (PR8+): the SemaphoreSlim is per-instance. +// prompt/fallback → evaluate(pass2) → side effects → final decision. +// UI-free: no WinUI types. A SemaphoreSlim serializes the prompt+pass2 block. +// Not wired in production src — verified by ProductionWiring_CoordinatorNotReferencedInSrc test. +// Must be registered as singleton when wired: the SemaphoreSlim is per-instance. public sealed class ExecApprovalsCoordinator : IExecApprovalV2Handler { private readonly ExecApprovalsStore _store; @@ -19,7 +19,7 @@ public sealed class ExecApprovalsCoordinator : IExecApprovalV2Handler private readonly IExecApprovalV2PromptHandler _prompt; private readonly IOpenClawLogger _logger; - // Serializes the prompt call + second-pass block (rail 17). + // Serializes the prompt call + second-pass block. // Does NOT protect validate/normalize/buildContext — those are stateless. private readonly SemaphoreSlim _promptLock = new(1, 1); @@ -90,14 +90,16 @@ public async Task HandleAsync(NodeInvokeRequest request, s identity.AllowAlwaysPatterns, matches); - // Step 4: first pass (approvalDecision always null in PR7 — CVE #8682, ADR-0002 Phase 2) + // Step 4: first pass (approvalDecision always null — pass2 decides based on user response) var pass1 = ExecApprovalEvaluator.Evaluate(context, null); if (pass1 is ExecHostPolicyDecision.DenyOutcome denyPass1) return LogAndReturn(denyPass1.Error, correlationId, promptAttempted: false, fallbackUsed: false, canonical: context.DisplayCommand); if (pass1 is ExecHostPolicyDecision.AllowOutcome) { - // Pre-approved path (security=Full, ask=Off or allowlist satisfied): skip prompt + // Pre-approved path (security=Full, ask=Off or allowlist satisfied): skip prompt. + // Side effects fire before the log line, after the final allow decision is confirmed. + await RecordAllowlistUsageAsync(context).ConfigureAwait(false); _logger.Info($"[EXEC-APPROVALS] [{correlationId}] path=new " + $"canonical=\"{SanitizeForLog(context.DisplayCommand)}\" decision=allow " + $"reason=approved fallbackUsed=false promptAttempted=false"); @@ -105,9 +107,10 @@ public async Task HandleAsync(NodeInvokeRequest request, s } // RequiresPromptOutcome → continue to prompt/fallback block - // Steps 5-7: prompt/fallback + second pass (critical section) + // Steps 5-8: prompt/fallback + second pass (critical section) + side effect flag bool promptAttempted = false; bool fallbackUsed = false; + bool persistAllowlistEntry = false; await _promptLock.WaitAsync().ConfigureAwait(false); try @@ -160,8 +163,6 @@ public async Task HandleAsync(NodeInvokeRequest request, s followupDecision = FallbackDecision(context, resolved.Defaults.AskFallback); } - // Step 6: AddAllowlistEntry stub (PR9 implements for AllowAlways + security==Allowlist) - // Step 7: second pass — must never return RequiresPrompt var pass2 = ExecApprovalEvaluator.Evaluate(context, followupDecision); if (pass2 is ExecHostPolicyDecision.DenyOutcome denyPass2) @@ -174,14 +175,18 @@ public async Task HandleAsync(NodeInvokeRequest request, s return LogAndReturn(ExecApprovalV2Result.InternalError("second-pass-requires-prompt"), correlationId, promptAttempted, fallbackUsed, canonical: context.DisplayCommand); } - // AllowOutcome → fall through to steps 8-10 + // pass2 is AllowOutcome — record whether AllowAlways was the prompt decision. + persistAllowlistEntry = followupDecision == ExecApprovalDecision.AllowAlways; } finally { _promptLock.Release(); } - // Step 8: RecordAllowlistUse stub (PR9) + // Step 8: side effects — strictly after the final allow decision. + if (persistAllowlistEntry && context.Security == ExecSecurity.Allowlist) + await PersistAllowlistEntriesAsync(context).ConfigureAwait(false); + await RecordAllowlistUsageAsync(context).ConfigureAwait(false); // Step 9: final allow log _logger.Info($"[EXEC-APPROVALS] [{correlationId}] path=new " + @@ -195,7 +200,7 @@ public async Task HandleAsync(NodeInvokeRequest request, s { // Outer safety net: any unhandled exception in buildContext, CanPresent, FallbackDecision, // or an out-of-range prompt outcome produces a typed deny instead of escaping HandleAsync. - // Rail 1: failures in the new path must never be silent or untyped. + // Failures must never be silent or untyped. var msg = $"[EXEC-APPROVALS] [{correlationId}] path=new " + $"canonical=\"\" decision=deny reason=unexpected-exception " + $"fallbackUsed=false promptAttempted=false"; @@ -204,6 +209,37 @@ public async Task HandleAsync(NodeInvokeRequest request, s } } + // Persists allowAlways patterns after an AllowAlways prompt decision (non-empty only). + // Caller guarantees Security == Allowlist (guard is in HandleAsync step 8). + private async Task PersistAllowlistEntriesAsync(ExecApprovalEvaluation context) + { + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var pattern in context.AllowAlwaysPatterns) + { + if (string.IsNullOrWhiteSpace(pattern) || !seen.Add(pattern)) continue; + await _store.AddAllowlistEntryAsync(context.AgentId, pattern).ConfigureAwait(false); + } + } + + // Updates lastUsed* metadata for every matched allowlist entry after a final allow. + // Guard mirrors macOS recordAllowlistMatches: no-op unless security=allowlist and satisfied. + private async Task RecordAllowlistUsageAsync(ExecApprovalEvaluation context) + { + if (context.Security != ExecSecurity.Allowlist || !context.AllowlistSatisfied) return; + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + for (var i = 0; i < context.AllowlistMatches.Count; i++) + { + var pattern = context.AllowlistMatches[i].Pattern; + if (string.IsNullOrEmpty(pattern) || !seen.Add(pattern)) continue; + var resolvedPath = i < context.AllowlistResolutions.Count + ? context.AllowlistResolutions[i].ResolvedPath + : null; + await _store.RecordAllowlistUseAsync( + context.AgentId, pattern, context.DisplayCommand, resolvedPath) + .ConfigureAwait(false); + } + } + // Fail-safe defaults when no UI is available (Saltzer/Schroeder fail-safe defaults, OWASP ASVS 4.1.4). // ask=Always → Deny: human approval is a precondition; without UI the only safe outcome is deny. private static ExecApprovalDecision FallbackDecision( @@ -228,7 +264,7 @@ private static ExecApprovalV2PromptRequest BuildPromptRequest( string correlationId) => new() { - DisplayCommand = context.DisplayCommand, // NOT sanitized — presenter's responsibility (rail 11) + DisplayCommand = context.DisplayCommand, // NOT sanitized — presenter's responsibility Cwd = identity.Cwd, Security = context.Security, Ask = context.Ask, @@ -236,7 +272,7 @@ private static ExecApprovalV2PromptRequest BuildPromptRequest( ResolvedPath = context.Resolution?.ResolvedPath, SessionKey = identity.SessionKey, CorrelationId = correlationId, - // Host omitted in PR7 (no gateway wiring yet) + // Host omitted (no gateway wiring yet) }; // Anti log-injection: replaces control characters in DisplayCommand before writing to logs. diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs index 376e5e20e..40e6a060e 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs @@ -10,7 +10,7 @@ namespace OpenClaw.Shared.ExecApprovals; // New store for exec-approvals.json. Separate from legacy ExecApprovalPolicy (exec-policy.json). -// PR4 scope: read path only. Write path (recordAllowlistUse) is added in PR9. +// Read path: ResolveReadOnly, LoadFile, EnsureFileAsync. Write path: AddAllowlistEntryAsync, RecordAllowlistUseAsync. public sealed class ExecApprovalsStore { // KebabCaseLower covers all macOS enum values: deny, allowlist, full, off, on-miss, always, @@ -44,7 +44,7 @@ public ExecApprovalsStore(string dataPath, IOpenClawLogger logger) // ── Public API ──────────────────────────────────────────────────────────── - // No side effects; does not create the file. Used by the evaluator (PR5). + // No side effects; does not create the file. public ExecApprovalsResolved ResolveReadOnly(string? agentId) { var result = LoadFile(); @@ -53,6 +53,75 @@ public ExecApprovalsResolved ResolveReadOnly(string? agentId) : ResolveFromFile(result.File, agentId); } + // Adds a new allowlist entry for the agent. Best-effort: never throws. + // Returns true if the entry is present after the call (added or already there), + // false if the pattern was empty or the write was skipped/failed. + // Pattern validation is non-empty only — parity with macOS. + public async Task AddAllowlistEntryAsync(string? agentId, string pattern) + { + var trimmed = pattern?.Trim(); + if (string.IsNullOrEmpty(trimmed)) + { + _logger.Debug("[EXEC-APPROVALS] AddAllowlistEntry skipped: empty pattern"); + return false; + } + var key = NormalizeAgentId(agentId); + bool alreadyPresent = false; + var wrote = await UpdateFileAsync(file => + { + var agents = file.Agents!; + if (!agents.TryGetValue(key, out var agent) || agent is null) + { + agent = new ExecApprovalsAgent(); + agents[key] = agent; + } + var allowlist = agent.Allowlist ??= []; + // Dedup case-insensitive — consistent with NormalizeAllowlistEntries (OrdinalIgnoreCase HashSet). + if (allowlist.Any(e => string.Equals( + e.Pattern?.Trim(), trimmed, StringComparison.OrdinalIgnoreCase))) + { + alreadyPresent = true; + return false; + } + allowlist.Add(new ExecAllowlistEntry + { + Id = Guid.NewGuid(), // parity with macOS UUID() + Pattern = trimmed, + // LastUsedAt intentionally absent: macOS addAllowlistEntry only sets {id, pattern}. + // RecordAllowlistUseAsync stamps it on first successful use. + }); + return true; + }).ConfigureAwait(false); + return wrote || alreadyPresent; + } + + // Updates lastUsed* metadata for every allowlist entry whose pattern matches. + // Best-effort: never throws. No-op if the agent or pattern is not found. + // Returns true if at least one entry was updated and saved; false otherwise. + public Task RecordAllowlistUseAsync( + string? agentId, string pattern, string command, string? resolvedPath) + { + if (string.IsNullOrEmpty(pattern)) return Task.FromResult(false); + var key = NormalizeAgentId(agentId); + return UpdateFileAsync(file => + { + if (!file.Agents!.TryGetValue(key, out var agent) || agent?.Allowlist is null) + return false; + var changed = false; + foreach (var entry in agent.Allowlist) + { + if (!string.Equals(entry.Pattern?.Trim(), pattern.Trim(), + StringComparison.OrdinalIgnoreCase)) + continue; + entry.LastUsedAt = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); + entry.LastUsedCommand = command; // STJ escapes control chars on serialize + entry.LastResolvedPath = resolvedPath; // Id and Pattern preserved + changed = true; + } + return changed; + }); + } + // Side-effecting resolve: creates the file if missing, initializes agents dict. // For startup / settings UI. Not used by the evaluator. public async Task ResolveAsync(string? agentId) @@ -129,7 +198,7 @@ private async Task EnsureFileAsync() return new ExecApprovalsFile { Version = 1, Agents = [] }; } - // socket intentionally omitted in Windows v1 (research doc 02 decision 3). + // socket intentionally omitted in Windows v1. var newFile = new ExecApprovalsFile { Version = 1, Agents = [] }; await SaveFileAsync(newFile).ConfigureAwait(false); _logger.Info($"[EXEC-APPROVALS] Created {_filePath}"); @@ -157,6 +226,46 @@ private async Task SaveFileAsync(ExecApprovalsFile file) } } + // Best-effort mutate-and-save. Serialized by the store lock. + // Never throws. Refuses to overwrite a malformed file. + // Returns true if the file was mutated and saved; false if the mutate was a no-op, + // the file was malformed/invalid, or any I/O failure occurred. + private async Task UpdateFileAsync(Func mutate) + { + await _lock.WaitAsync().ConfigureAwait(false); + try + { + var result = LoadFile(); + if (result.Status == LoadFileStatus.Invalid) + { + _logger.Warn("[EXEC-APPROVALS] Refusing to write exec-approvals.json: " + + "file is malformed or has an unsupported version"); + return false; + } + var file = result.Status == LoadFileStatus.Loaded && result.File is not null + ? result.File + : new ExecApprovalsFile { Version = 1, Agents = [] }; + file.Agents ??= new Dictionary(); + + if (!mutate(file)) return false; // no-op: nothing to persist + + await SaveFileAsync(file).ConfigureAwait(false); + return true; + } + catch (Exception ex) + { + // Any failure (incl. transient IOException on the atomic move) degrades to a + // logged warning. The atomic write guarantees the file on disk is never left corrupt. + _logger.Warn($"[EXEC-APPROVALS] exec-approvals.json write failed " + + $"({ex.Message}); side effect skipped"); + return false; + } + finally + { + _lock.Release(); + } + } + // ── Normalization ───────────────────────────────────────────────────────── private static ExecApprovalsFile Normalize(ExecApprovalsFile file) @@ -239,7 +348,7 @@ private static ExecApprovalsAgent WithNormalizedAllowlist(ExecApprovalsAgent age // Mirrors macOS normalizeAllowlistEntries. // dropInvalid=false: discard only null/empty patterns; keep non-empty ones regardless of validity. // dropInvalid=true: same in v1 — pattern validity beyond non-empty is enforced by the allowlist - // matcher in PR5, not here. The flag is preserved for API symmetry with macOS. + // matcher, not here. The flag is preserved for API symmetry with macOS. internal static List NormalizeAllowlistEntries( IEnumerable entries, bool dropInvalid) { diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs index b33a28a8d..b47d0032c 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs @@ -445,6 +445,151 @@ public async Task CanPresent_Throws_ReturnsInternalError_NotException() Assert.Contains(log.Errors, e => e.Contains("unexpected-exception")); } + // ── PR8: allowlist persistence and use recording ────────────────────────── + + // A. AllowAlways + security=allowlist → entry persisted in store. + [Fact] + public async Task AllowAlways_Allowlist_PersistsEntry() + { + WriteStoreFile("""{"version":1,"agents":{"main":{"security":"allowlist","ask":"always"}}}"""); + var result = await MakeCoordinator( + canPresent: AlwaysCanPresentEvaluator.Instance, + prompt: new FixedDecisionPromptHandler(ExecApprovalPromptOutcome.AllowAlways)) + .HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-A"); + + Assert.True(result.IsAllow); + var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + Assert.NotNull(resolved.Allowlist[0].Pattern); + Assert.Contains("cmd", resolved.Allowlist[0].Pattern, StringComparison.OrdinalIgnoreCase); + } + + // B. AllowAlways + security=full → guard fails, no allowlist entry written. + [Fact] + public async Task AllowAlways_SecurityFull_DoesNotPersist() + { + WriteStoreFile("""{"version":1,"defaults":{"security":"full","ask":"always"}}"""); + var result = await MakeCoordinator( + canPresent: AlwaysCanPresentEvaluator.Instance, + prompt: new FixedDecisionPromptHandler(ExecApprovalPromptOutcome.AllowAlways)) + .HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-B"); + + Assert.True(result.IsAllow); + var json = File.ReadAllText(Path.Combine(_dir, "exec-approvals.json")); + Assert.DoesNotContain("allowlist", json, StringComparison.OrdinalIgnoreCase); + } + + // C. Pre-approved path (pass1 = Allow) → RecordAllowlistUse fires and updates LastUsedCommand. + [Fact] + public async Task AllowPreapproved_RecordsAllowlistUse() + { + WriteStoreFile(""" + { + "version": 1, + "agents": { + "main": { + "security": "allowlist", + "ask": "off", + "allowlist": [{ "pattern": "**/cmd.exe" }] + } + } + } + """); + var result = await MakeCoordinator().HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-C"); + + Assert.True(result.IsAllow); + var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); + } + + // D. AllowOnce → persistAllowlistEntry=false, no entry written. + [Fact] + public async Task AllowOnce_DoesNotPersistEntry() + { + WriteStoreFile("""{"version":1,"agents":{"main":{"security":"allowlist","ask":"always"}}}"""); + var result = await MakeCoordinator( + canPresent: AlwaysCanPresentEvaluator.Instance, + prompt: new FixedDecisionPromptHandler(ExecApprovalPromptOutcome.AllowOnce)) + .HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-D"); + + Assert.True(result.IsAllow); + var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Empty(resolved.Allowlist); + } + + // E. AllowAlways called twice for the same command → exactly one entry (dedup in store). + [Fact] + public async Task AllowAlways_Idempotent_SingleEntry() + { + WriteStoreFile("""{"version":1,"agents":{"main":{"security":"allowlist","ask":"always"}}}"""); + var coordinator = MakeCoordinator( + canPresent: AlwaysCanPresentEvaluator.Instance, + prompt: new FixedDecisionPromptHandler(ExecApprovalPromptOutcome.AllowAlways)); + + await coordinator.HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-E1"); + await coordinator.HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-E2"); + + var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + } + + // F. Prompt path (ask=always + AllowlistSatisfied=true + AllowOnce) → + // RecordAllowlistUse fires in the post-pass2 branch (not just the pass1 branch). + [Fact] + public async Task AllowOnce_AllowlistSatisfied_RecordsUseInPostPass2Branch() + { + WriteStoreFile(""" + { + "version": 1, + "agents": { + "main": { + "security": "allowlist", + "ask": "always", + "allowlist": [{ "pattern": "**/cmd.exe" }] + } + } + } + """); + var result = await MakeCoordinator( + canPresent: AlwaysCanPresentEvaluator.Instance, + prompt: new FixedDecisionPromptHandler(ExecApprovalPromptOutcome.AllowOnce)) + .HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-F"); + + Assert.True(result.IsAllow); + var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); + } + + // G. Fallback path (canPresent=false) + AllowlistSatisfied=true → RecordAllowlistUse fires. + [Fact] + public async Task Fallback_AllowlistSatisfied_RecordsUse() + { + // askFallback=off → FallbackDecision=AllowOnce → pass2=Allow. AllowlistSatisfied=true + // because cmd.exe resolves and **/cmd.exe matches. RecordAllowlistUsageAsync must fire. + WriteStoreFile(""" + { + "version": 1, + "agents": { + "main": { + "security": "allowlist", + "ask": "always", + "askFallback": "off", + "allowlist": [{ "pattern": "**/cmd.exe" }] + } + } + } + """); + // canPresent=false (default) → fallback path; askFallback=off → AllowOnce → Allow + var result = await MakeCoordinator().HandleAsync(Req("""{"command":["cmd"]}"""), "pr8-G"); + + Assert.True(result.IsAllow); + var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); + } + // ── Test doubles ────────────────────────────────────────────────────────── private sealed class FixedDecisionPromptHandler : IExecApprovalV2PromptHandler diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs index 403af4cc2..e4c254781 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Text.Json; using System.Threading.Tasks; using OpenClaw.Shared; @@ -712,6 +713,280 @@ public void JsonOptions_ExecAskDeny_SerializesAsDeny() Assert.Contains("\"deny\"", json); } + // ── Write path: AddAllowlistEntryAsync ─────────────────────────────────── + + [Fact] + public async Task AddAllowlistEntryAsync_ExistingFile_AddsEntryWithIdAndMetadata() + { + WriteFile(MinimalFile()); + var store = Store(); + var result = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + + Assert.True(result); + var resolved = store.ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + var entry = resolved.Allowlist[0]; + Assert.Equal("**/git.exe", entry.Pattern); + Assert.NotNull(entry.Id); + Assert.Null(entry.LastUsedAt); // macOS addAllowlistEntry: {id, pattern} only — no lastUsedAt on creation + } + + [Fact] + public async Task AddAllowlistEntryAsync_DuplicatePattern_NotAddedCaseInsensitive() + { + WriteFile(MinimalFile()); + var store = Store(); + var first = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + var second = await store.AddAllowlistEntryAsync("main", "**/GIT.EXE"); + + Assert.True(first); + Assert.True(second); // already present → true + Assert.Single(store.ResolveReadOnly("main").Allowlist); + } + + [Fact] + public async Task AddAllowlistEntryAsync_EmptyOrWhitespacePattern_ReturnsFalse() + { + WriteFile(MinimalFile()); + var store = Store(); + + Assert.False(await store.AddAllowlistEntryAsync("main", "")); + Assert.False(await store.AddAllowlistEntryAsync("main", " ")); + Assert.Empty(store.ResolveReadOnly("main").Allowlist); + } + + [Fact] + public async Task AddAllowlistEntryAsync_NoFile_CreatesFileWithEntry() + { + var store = Store(); + var result = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + + Assert.True(result); + Assert.True(File.Exists(FilePath)); + var resolved = store.ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + Assert.Equal("**/git.exe", resolved.Allowlist[0].Pattern); + } + + [Fact] + public async Task AddAllowlistEntryAsync_MalformedFile_RefusesToWriteAndWarns() + { + WriteFile("{ bad json }"); + var original = File.ReadAllText(FilePath); + var store = Store(); + + var result = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + + Assert.False(result); + Assert.Equal(original, File.ReadAllText(FilePath)); + Assert.Contains(_log.Warnings, w => w.Contains("Refusing to write")); + } + + [Fact] + public async Task AddAllowlistEntryAsync_AtomicWrite_NoTempFileLeft() + { + WriteFile(MinimalFile()); + await Store().AddAllowlistEntryAsync("main", "**/git.exe"); + + Assert.Empty(Directory.GetFiles(_dir, "*.tmp")); + } + + [Fact] + public async Task AddAllowlistEntryAsync_NewAgent_CreatesAgentWithEntry() + { + WriteFile("""{"version":1,"agents":{}}"""); + var store = Store(); + var result = await store.AddAllowlistEntryAsync("agent-xyz", "**/git.exe"); + + Assert.True(result); + Assert.Single(store.ResolveReadOnly("agent-xyz").Allowlist); + } + + // ── Write path: RecordAllowlistUseAsync ────────────────────────────────── + + [Fact] + public async Task RecordAllowlistUseAsync_UpdatesMetadataAndPreservesIdAndPattern() + { + var id = Guid.NewGuid(); + WriteFile($$""" + { + "version": 1, + "agents": { + "main": { + "allowlist": [ + { "id": "{{id}}", "pattern": "**/git.exe" } + ] + } + } + } + """); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", "/usr/bin/git"); + + Assert.True(result); + var entry = store.ResolveReadOnly("main").Allowlist[0]; + Assert.Equal(id, entry.Id); + Assert.Equal("**/git.exe", entry.Pattern); + Assert.NotNull(entry.LastUsedAt); + Assert.Equal("git status", entry.LastUsedCommand); + Assert.Equal("/usr/bin/git", entry.LastResolvedPath); + } + + [Fact] + public async Task RecordAllowlistUseAsync_PatternNotPresent_ReturnsFalse() + { + WriteFile(""" + { + "version": 1, + "agents": { + "main": { "allowlist": [{ "pattern": "**/git.exe" }] } + } + } + """); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("main", "**/rg.exe", "rg foo", null); + + Assert.False(result); + Assert.Null(store.ResolveReadOnly("main").Allowlist[0].LastUsedCommand); + } + + [Fact] + public async Task RecordAllowlistUseAsync_AgentNotPresent_ReturnsFalse() + { + WriteFile("""{"version":1,"agents":{}}"""); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("nonexistent", "**/git.exe", "git status", null); + + Assert.False(result); + } + + [Fact] + public async Task RecordAllowlistUseAsync_MalformedFile_ReturnsFalse() + { + WriteFile("{ bad json }"); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + + Assert.False(result); + Assert.Equal("{ bad json }", File.ReadAllText(FilePath)); + } + + [Fact] + public async Task RecordAllowlistUseAsync_DoesNotTouchOtherEntries() + { + WriteFile(""" + { + "version": 1, + "agents": { + "main": { + "allowlist": [ + { "pattern": "**/git.exe" }, + { "pattern": "**/rg.exe" } + ] + } + } + } + """); + var store = Store(); + await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + + var allowlist = store.ResolveReadOnly("main").Allowlist; + Assert.NotNull(allowlist.First(e => e.Pattern == "**/git.exe").LastUsedCommand); + Assert.Null(allowlist.First(e => e.Pattern == "**/rg.exe").LastUsedCommand); + } + + [Fact] + public async Task RecordAllowlistUseAsync_BestEffort_IoExceptionReturnsFalse() + { + // Entry present so the mutate lambda returns true (pattern found → something to update). + // Without a matching entry the mutate would be a no-op and UpdateFileAsync would return + // false before reaching SaveFileAsync — that is the not-found path, not the IOException path. + WriteFile(""" + { + "version": 1, + "agents": { + "main": { + "allowlist": [{ "pattern": "**/git.exe" }] + } + } + } + """); + var store = Store(); + + // FileShare.Read: LoadFile succeeds; File.Move(tmp, target, overwrite:true) fails + // because the target is open without write/delete sharing → IOException degraded-save path. + using (var fs = new FileStream(FilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.Read)) + { + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + Assert.False(result); // IOException absorbed, no exception escaped + Assert.NotEmpty(_log.Warnings); // write failure logged as Warn + } + } + + // ── Round-trip and integration ──────────────────────────────────────────── + + [Fact] + public async Task AddAllowlistEntry_RoundTrip_ResolvedByReadPath() + { + WriteFile(MinimalFile()); + var store = Store(); + await store.AddAllowlistEntryAsync("main", "**/git.exe"); + + var resolved = store.ResolveReadOnly("main"); + Assert.Single(resolved.Allowlist); + Assert.Equal("**/git.exe", resolved.Allowlist[0].Pattern); + } + + [Fact] + public async Task RoundTrip_WrittenFileIsValidJsonWithCorrectFields() + { + WriteFile(MinimalFile()); + var store = Store(); + await store.AddAllowlistEntryAsync("main", "**/git.exe"); + // lastUsedAt is absent on creation; RecordAllowlistUseAsync stamps it on first use. + await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + + var json = File.ReadAllText(FilePath); + using var doc = System.Text.Json.JsonDocument.Parse(json); // valid JSON + Assert.Equal(1, doc.RootElement.GetProperty("version").GetInt32()); + // lastUsedAt must be a JSON number (Unix epoch ms), not a string. + var lastUsedAt = doc.RootElement + .GetProperty("agents").GetProperty("main") + .GetProperty("allowlist")[0].GetProperty("lastUsedAt"); + Assert.Equal(System.Text.Json.JsonValueKind.Number, lastUsedAt.ValueKind); + } + + [Fact] + public async Task AddAllowlistEntryAsync_Concurrency_SamePattern_SingleEntry() + { + WriteFile(MinimalFile()); + var store = Store(); + var tasks = Enumerable.Range(0, 5) + .Select(_ => store.AddAllowlistEntryAsync("main", "**/git.exe")) + .ToList(); + await Task.WhenAll(tasks); + + Assert.Single(store.ResolveReadOnly("main").Allowlist); + } + + [Fact] + public async Task AddAllowlistEntryAsync_BestEffort_IoExceptionReturnsFalse() + { + // FileShare.Read: LoadFile (File.ReadAllText / FileAccess.Read) succeeds because + // read-sharing is granted. File.Move(tmp, target, overwrite:true) fails because the + // target is open without write/delete sharing. This exercises the IOException degraded-save + // path, not the malformed-file refusal branch. + WriteFile(MinimalFile()); + var store = Store(); + + using (var fs = new FileStream(FilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.Read)) + { + var result = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + Assert.False(result); // IOException absorbed, no exception escaped + Assert.NotEmpty(_log.Warnings); // write failure logged as Warn + } + } + // ── Helpers ─────────────────────────────────────────────────────────────── private static string MinimalFile() => """{"version":1,"agents":{}}"""; From ce5589e37bb72ba3536d999a2a2023364d0bfefe Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Fri, 22 May 2026 08:20:56 +0200 Subject: [PATCH 2/8] chore: remove internal planning terminology from ExecApprovals comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strip rail codes, PR numbers, research doc references, D-labels, CVE/ADR tags, and other planning labels from all ExecApprovals source files. No logic changed — comments only. Co-Authored-By: Claude Sonnet 4.6 --- .../ExecApprovals/CanonicalCommandIdentity.cs | 18 +++++++++--------- .../ExecApprovals/ExecApprovalEvaluation.cs | 10 +++++----- .../ExecApprovalV2InputValidator.cs | 2 +- .../ExecApprovalV2NormalizationStep.cs | 12 ++++++------ .../ExecApprovals/ExecApprovalV2NullHandler.cs | 2 +- .../ExecApprovals/ExecApprovalV2Result.cs | 2 +- .../ExecApprovals/ExecCommandResolution.cs | 4 ++-- .../ExecShellWrapperNormalizer.cs | 4 ++-- .../ExecApprovals/ICanPresentEvaluator.cs | 10 +++++----- .../ExecApprovals/IExecApprovalV2Handler.cs | 4 ++-- 10 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/OpenClaw.Shared/ExecApprovals/CanonicalCommandIdentity.cs b/src/OpenClaw.Shared/ExecApprovals/CanonicalCommandIdentity.cs index 647b920d7..bb2199a56 100644 --- a/src/OpenClaw.Shared/ExecApprovals/CanonicalCommandIdentity.cs +++ b/src/OpenClaw.Shared/ExecApprovals/CanonicalCommandIdentity.cs @@ -2,36 +2,36 @@ namespace OpenClaw.Shared.ExecApprovals; -// Architectural barrier produced by PR3. +// Architectural barrier between raw requests and the evaluation pipeline. // Equivalent to ExecHostValidatedRequest in the macOS reference, extended with resolution outputs. -// No module from PR4 onward may accept ValidatedRunRequest as direct input (research doc 05 line 439). -// Rail 15: a single canonical representation reused across evaluation, logging, prompting, execution. +// No evaluation module may accept ValidatedRunRequest as direct input — this is the canonical handoff type. +// A single canonical representation reused across evaluation, logging, prompting, and execution. public sealed class CanonicalCommandIdentity { // ── Normalization outputs ───────────────────────────────────────────────── - // Argv exactly as produced by PR2 (no trimming; coding contract process-argv-semantics). + // Argv as received from the normalizer (no trimming; callers must not modify). public IReadOnlyList Command { get; } // Canonical display form generated from argv. Never rawCommand from the agent. - // Used by logging and prompting. Research doc 05 decision 2. + // Used by logging and prompting. public string DisplayCommand { get; } // Safe rawCommand for executable resolution. Null in Windows v1 (rawCommand not in - // system.run protocol; research doc 05 OQ-V4 / decision 10). + // the system.run protocol). public string? EvaluationRawCommand { get; } // ── Resolution outputs ──────────────────────────────────────────────────── - // Singular resolution for the state machine (PR5). + // Singular resolution for the state machine. // Null if the primary executable cannot be determined. public ExecCommandResolution? Resolution { get; } - // Per-segment resolutions for the allowlist matcher (PR4/PR5). + // Per-segment resolutions for the allowlist matcher. // Empty list means fail-closed — no allowlist satisfaction possible. public IReadOnlyList AllowlistResolutions { get; } - // Suggested allowlist patterns for prompt/UI (PR6). Not a security decision. + // Suggested allowlist patterns for prompt/UI. Not a security decision. public IReadOnlyList AllowAlwaysPatterns { get; } // ── Request context (carried from ValidatedRunRequest) ──────────────────── diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalEvaluation.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalEvaluation.cs index f477314a9..d5898f5d9 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalEvaluation.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalEvaluation.cs @@ -4,9 +4,9 @@ namespace OpenClaw.Shared.ExecApprovals; // Aggregated evaluation context passed to the stateless evaluator. -// Shape mirrors macOS ExecApprovalEvaluation struct (research doc 06). +// Shape mirrors macOS ExecApprovalEvaluation struct. // Derived fields are computed once in the constructor and must not be recomputed by callers. -// Research doc 06 stable conclusion 3: construction belongs to the coordinator (PR7), not the evaluator. +// Construction belongs to the coordinator, not the evaluator. public sealed class ExecApprovalEvaluation { public IReadOnlyList Command { get; } @@ -17,7 +17,7 @@ public sealed class ExecApprovalEvaluation public IReadOnlyDictionary? Env { get; } // Singular resolution — AllowlistResolutions[0], or null if the list is empty. - // Research doc 06: "resolution = allowlistResolutions.first" (not an independent call). + // Always the first element of AllowlistResolutions, not an independent resolver call. public ExecCommandResolution? Resolution { get; } public IReadOnlyList AllowlistResolutions { get; } @@ -25,11 +25,11 @@ public sealed class ExecApprovalEvaluation public IReadOnlyList AllowlistMatches { get; } // true iff security==allowlist && resolutions.Count>0 && matches.Count==resolutions.Count. - // Research doc 06 derivation rule — must not be re-derived outside the constructor. + // Derived once in the constructor — must not be re-derived outside it. public bool AllowlistSatisfied { get; } // First match when AllowlistSatisfied; null otherwise. - // Research doc 06 R5: AllowlistMatch must be null when AllowlistSatisfied is false. + // Must be null when AllowlistSatisfied is false. public ExecAllowlistEntry? AllowlistMatch { get; } // Always false in v1. Kept as part of the conceptual model; activation deferred. diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2InputValidator.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2InputValidator.cs index b7e2f33b8..7d455a161 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2InputValidator.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2InputValidator.cs @@ -4,7 +4,7 @@ namespace OpenClaw.Shared.ExecApprovals; /// -/// Phase 1 of the V2 exec approval pipeline: structural input validation (rail 18, step 1). +/// Phase 1 of the V2 exec approval pipeline: structural input validation. /// Parses a raw NodeInvokeRequest into a ValidatedRunRequest or returns validation-failed. /// Does not resolve executables, detect shell wrappers, or evaluate policy. /// diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NormalizationStep.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NormalizationStep.cs index 863315792..e6688229b 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NormalizationStep.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NormalizationStep.cs @@ -3,7 +3,7 @@ namespace OpenClaw.Shared.ExecApprovals; // Either a CanonicalCommandIdentity (IsResolved=true) or a typed denial (IsResolved=false). -// Produced by ExecApprovalV2Normalizer; consumed by the coordinator pipeline (PR7). +// Produced by ExecApprovalV2Normalizer; consumed by the coordinator pipeline. public sealed class ExecApprovalV2NormalizationOutcome { public bool IsResolved { get; } @@ -29,7 +29,7 @@ public static ExecApprovalV2NormalizationOutcome Fail(ExecApprovalV2Result error => new(error); } -// Rail 18 steps 2-4: normalize command form → resolve executable → build canonical identity. +// Steps 2-4 of the approval pipeline: normalize command form → resolve executable → build canonical identity. // Stateless — safe to call concurrently. public static class ExecApprovalV2Normalizer { @@ -39,10 +39,10 @@ public static ExecApprovalV2NormalizationOutcome Normalize(ValidatedRunRequest r var cwd = request.Cwd; var env = request.Env as IReadOnlyDictionary; - // displayCommand is always derived from argv, never from rawCommand (research doc 05 decision 2). + // displayCommand is always derived from argv, never from rawCommand. var displayCommand = ShellQuoting.FormatExecCommand(argv); - // rawCommand is null in Windows v1 (system.run does not carry it; research doc 05 OQ-V4). + // rawCommand is null in Windows v1 (system.run does not carry it). // EvaluationRawCommand stays null — correct and documented conservative output. string? evaluationRawCommand = null; @@ -50,7 +50,7 @@ public static ExecApprovalV2NormalizationOutcome Normalize(ValidatedRunRequest r var resolution = ExecCommandResolver.Resolve(argv, cwd, env); // Multi-segment resolution for allowlist. - // Empty list is fail-closed: no allowlist satisfaction possible (research doc 04 R2). + // Empty list is fail-closed: no allowlist satisfaction possible. // An empty list is NOT itself a denial at this step — the evaluator decides. var allowlistResolutions = ExecCommandResolver.ResolveForAllowlist( argv, evaluationRawCommand, cwd, env); @@ -58,7 +58,7 @@ public static ExecApprovalV2NormalizationOutcome Normalize(ValidatedRunRequest r // UX patterns for prompting. var allowAlwaysPatterns = ExecCommandResolver.ResolveAllowAlwaysPatterns(argv, cwd, env); - // Rail 6: if argv is non-empty but resolution is entirely impossible, deny. + // If argv is non-empty but resolution is entirely impossible, deny. // "Ambiguous or inconsistent" → typed deny, not silent allow. if (resolution is null && allowlistResolutions.Count == 0) return Fail("executable-resolution-failed"); diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NullHandler.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NullHandler.cs index e59d6412a..672bf70bc 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NullHandler.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2NullHandler.cs @@ -4,7 +4,7 @@ namespace OpenClaw.Shared.ExecApprovals; /// /// Default V2 handler: always returns . -/// Keeps the V2 path inert until a real handler is installed (rail 19). +/// Keeps the V2 path inert until a real handler is installed. /// Never throws, never falls through to legacy. /// public sealed class ExecApprovalV2NullHandler : IExecApprovalV2Handler diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2Result.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2Result.cs index 3c1658fbb..fa820ee3b 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2Result.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2Result.cs @@ -1,7 +1,7 @@ namespace OpenClaw.Shared.ExecApprovals; /// -/// Stable result codes for the V2 exec approval path (rail 7). +/// Stable result codes for the V2 exec approval path. /// public enum ExecApprovalV2Code { diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecCommandResolution.cs b/src/OpenClaw.Shared/ExecApprovals/ExecCommandResolution.cs index 59f53ab9a..0efebb984 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecCommandResolution.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecCommandResolution.cs @@ -69,7 +69,7 @@ internal static IReadOnlyList ResolveForAllowlist( { var token = ParseFirstToken(segment); if (token is null) return []; - // -EncodedCommand and aliases in segment position: fail-closed (research doc 04 S1). + // -EncodedCommand and aliases in segment position: fail-closed. if (SegmentUsesEncodedCommand(segment, token)) return []; var res = ResolveExecutable(token, cwd, env); if (res is null) return []; @@ -494,7 +494,7 @@ private static bool HasNonStandardColon(string path) private static string TryNormalizePath(string path) { // GetFullPath resolves . and .. but does not expand 8.3 short names. - // Full GetLongPathName P/Invoke is left as OQ-R1 in the research docs. + // Full GetLongPathName P/Invoke is a known gap — short names not expanded. try { return Path.GetFullPath(path); } catch { return path; } // hostile path must not throw out of resolution } diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs b/src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs index 71e36b472..328d7e25f 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecShellWrapperNormalizer.cs @@ -7,7 +7,7 @@ namespace OpenClaw.Shared.ExecApprovals; // Differs from the legacy ExecShellWrapperParser.Expand (BFS multi-level, string-based). // This normalizer operates on argv (IReadOnlyList) and performs one level of // wrapper detection, with recursive env-prefix unwrapping up to MaxWrapperDepth. -// Rail 18 step 2: normalize command form. +// Step 2 of the approval pipeline: normalize command form. internal static class ExecShellWrapperNormalizer { private enum WrapperKind { Posix, Cmd, PowerShell } @@ -35,7 +35,7 @@ internal sealed record ParsedWrapper(bool IsWrapper, string? InlineCommand); internal static readonly ParsedWrapper NotWrapper = new(false, null); // Detects a single-level shell wrapper in argv. - // rawCommand is always null in Windows v1 (not in system.run protocol; research doc 05 OQ-V4). + // rawCommand is always null in Windows v1 (not in the system.run protocol). // Detection is on argv only; rawCommand is accepted for API compatibility with future use. internal static ParsedWrapper Extract(IReadOnlyList command, string? rawCommand = null) => ExtractInner(command, rawCommand, 0); diff --git a/src/OpenClaw.Shared/ExecApprovals/ICanPresentEvaluator.cs b/src/OpenClaw.Shared/ExecApprovals/ICanPresentEvaluator.cs index 7e53c244e..1e162819f 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ICanPresentEvaluator.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ICanPresentEvaluator.cs @@ -1,20 +1,20 @@ namespace OpenClaw.Shared.ExecApprovals; // Determines whether the coordinator can present a UI prompt for this request. -// Doc 08 F1 lists four inputs to canPresent: requestSessionKey, activeSessionKey, +// Four inputs to canPresent: requestSessionKey, activeSessionKey, // lastInputSeconds, desktopInteractive. Only requestSessionKey is passed by the // coordinator — the other three are encapsulated inside the implementation: // activeSessionKey: provided by whatever tracks the active tray session. -// lastInputSeconds: read via Win32 GetLastInputInfo (OQ-F1). -// desktopInteractive: read via OpenInputDesktop / WTSQuerySessionInformation (OQ-F1). -// Keeping these out of the interface keeps the coordinator UI-free (rail 10) and +// lastInputSeconds: read via Win32 GetLastInputInfo. +// desktopInteractive: read via OpenInputDesktop / WTSQuerySessionInformation. +// Keeping these out of the interface keeps the coordinator UI-free and // testable without Win32. Must never throw — fail to false (no UI available). public interface ICanPresentEvaluator { bool CanPresent(string? requestSessionKey); } -// Default for PR7: UI not wired yet. Everything routes to FallbackDecision. +// Default: UI not wired. Everything routes to FallbackDecision. public sealed class AlwaysCannotPresentEvaluator : ICanPresentEvaluator { public static readonly AlwaysCannotPresentEvaluator Instance = new(); diff --git a/src/OpenClaw.Shared/ExecApprovals/IExecApprovalV2Handler.cs b/src/OpenClaw.Shared/ExecApprovals/IExecApprovalV2Handler.cs index e53501947..9da995de9 100644 --- a/src/OpenClaw.Shared/ExecApprovals/IExecApprovalV2Handler.cs +++ b/src/OpenClaw.Shared/ExecApprovals/IExecApprovalV2Handler.cs @@ -3,9 +3,9 @@ namespace OpenClaw.Shared.ExecApprovals; /// -/// Seam for the V2 exec approval path (rail 10: UI-free, no WinUI types). +/// Seam for the V2 exec approval path. Implementations must be UI-free (no WinUI types). /// Implementations decide whether a system.run request is allowed. -/// In PR1 only the NullHandler exists; real evaluation arrives in later PRs. +/// The NullHandler is the default; production wiring installs the real coordinator. /// public interface IExecApprovalV2Handler { From 11d9c691726fedb049e0eee8888ba24e39364359 Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Sat, 23 May 2026 09:02:09 +0200 Subject: [PATCH 3/8] chore: retrigger CI From 60411fe65f97024bb3258327686b810b44db9c78 Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Sat, 23 May 2026 09:36:02 +0200 Subject: [PATCH 4/8] test(exec-approvals): add runtime proof showing AllowAlways persistence and lastUsed recording End-to-end coordinator/store test using real filesystem I/O. Surfaces the on-disk exec-approvals.json content at three points (initial, post-AllowAlways, post-allowlist-hit) via ITestOutputHelper so the JSON is visible under `dotnet test ... --logger "console;verbosity=detailed"`. Demonstrates both side-effect paths: AllowAlways persists a new entry, and a later allowlist hit records lastUsed* metadata against the same entry id (dedup). Co-Authored-By: Claude Sonnet 4.6 --- .../ExecApprovalsCoordinatorTests.cs | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs index b47d0032c..171a4428c 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs @@ -6,6 +6,7 @@ using System.Threading; using System.Threading.Tasks; using Xunit; +using Xunit.Abstractions; using OpenClaw.Shared; using OpenClaw.Shared.ExecApprovals; @@ -19,11 +20,13 @@ namespace OpenClaw.Shared.Tests; public class ExecApprovalsCoordinatorTests : IDisposable { private readonly string _dir; + private readonly ITestOutputHelper _output; - public ExecApprovalsCoordinatorTests() + public ExecApprovalsCoordinatorTests(ITestOutputHelper output) { _dir = Path.Combine(Path.GetTempPath(), $"oca-coord-test-{Guid.NewGuid():N}"); Directory.CreateDirectory(_dir); + _output = output; } public void Dispose() => Directory.Delete(_dir, recursive: true); @@ -590,6 +593,48 @@ public async Task Fallback_AllowlistSatisfied_RecordsUse() Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); } + // End-to-end coordinator/store runtime proof using real filesystem I/O. + // Demonstrates the two side-effect paths via ITestOutputHelper, so the + // resulting JSON appears in `dotnet test ... --logger "console;verbosity=detailed"`: + // - AllowAlways persists a new allowlist entry into exec-approvals.json + // - A later allowlist hit records lastUsed* metadata + [Fact] + public async Task RuntimeProof_AllowAlways_PersistsAndRecordsLastUsed() + { + var filePath = Path.Combine(_dir, "exec-approvals.json"); + + WriteStoreFile("""{"version":1,"agents":{"main":{"security":"allowlist","ask":"always"}}}"""); + _output.WriteLine("=== Initial exec-approvals.json ==="); + _output.WriteLine(File.ReadAllText(filePath)); + + var coordinator = MakeCoordinator( + canPresent: AlwaysCanPresentEvaluator.Instance, + prompt: new FixedDecisionPromptHandler(ExecApprovalPromptOutcome.AllowAlways)); + + // Step 1: AllowAlways → entry persisted (no lastUsed* yet). + var first = await coordinator.HandleAsync(Req("""{"command":["cmd"]}"""), "proof-1"); + Assert.True(first.IsAllow); + + _output.WriteLine(""); + _output.WriteLine("=== After AllowAlways (correlationId=proof-1) ==="); + _output.WriteLine(File.ReadAllText(filePath)); + + // Step 2: Same command again → allowlist hit, lastUsed* recorded. + var second = await coordinator.HandleAsync(Req("""{"command":["cmd"]}"""), "proof-2"); + Assert.True(second.IsAllow); + + _output.WriteLine(""); + _output.WriteLine("=== After allowlist hit (correlationId=proof-2) ==="); + _output.WriteLine(File.ReadAllText(filePath)); + + var resolvedAfter = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); + Assert.Single(resolvedAfter.Allowlist); + Assert.NotNull(resolvedAfter.Allowlist[0].Pattern); + Assert.NotNull(resolvedAfter.Allowlist[0].LastUsedAt); + Assert.NotNull(resolvedAfter.Allowlist[0].LastUsedCommand); + Assert.NotNull(resolvedAfter.Allowlist[0].LastResolvedPath); + } + // ── Test doubles ────────────────────────────────────────────────────────── private sealed class FixedDecisionPromptHandler : IExecApprovalV2PromptHandler From bdd5481d914866be232472c8abed73e62f8d6a22 Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Sat, 23 May 2026 09:59:41 +0200 Subject: [PATCH 5/8] fix(exec-approvals): record allowlist use on wildcard bucket too ResolveReadOnly merges entries from agents["*"] into the resolved allowlist for any concrete agent, so a hit can be authorized by the wildcard bucket. RecordAllowlistUseAsync was only searching the concrete agent bucket, so wildcard-authorized executions never accumulated lastUsed* metadata. The method now iterates both the concrete agent bucket and "*", updating metadata wherever the pattern matches. If agentId is already "*", only the wildcard bucket is searched (no double-pass). Tests added: - Store: wildcard-only bucket hit updates metadata. - Store: same pattern in both buckets updates both entries. - Coordinator: end-to-end regression with allowlist living under "*". Co-Authored-By: Claude Sonnet 4.6 --- .../ExecApprovals/ExecApprovalsStore.cs | 25 +++++---- .../ExecApprovalsCoordinatorTests.cs | 29 +++++++++++ .../ExecApprovalsStoreTests.cs | 52 +++++++++++++++++++ 3 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs index 40e6a060e..52e71cd8e 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs @@ -98,25 +98,32 @@ public async Task AddAllowlistEntryAsync(string? agentId, string pattern) // Updates lastUsed* metadata for every allowlist entry whose pattern matches. // Best-effort: never throws. No-op if the agent or pattern is not found. // Returns true if at least one entry was updated and saved; false otherwise. + // Searches both the concrete agent bucket and the wildcard bucket ("*"), + // because ResolveReadOnly merges wildcard entries into the resolved allowlist — + // so a hit can be authorized by either source and metadata must follow. public Task RecordAllowlistUseAsync( string? agentId, string pattern, string command, string? resolvedPath) { if (string.IsNullOrEmpty(pattern)) return Task.FromResult(false); var key = NormalizeAgentId(agentId); + var buckets = key == "*" ? new[] { "*" } : new[] { key, "*" }; return UpdateFileAsync(file => { - if (!file.Agents!.TryGetValue(key, out var agent) || agent?.Allowlist is null) - return false; var changed = false; - foreach (var entry in agent.Allowlist) + foreach (var bucketKey in buckets) { - if (!string.Equals(entry.Pattern?.Trim(), pattern.Trim(), - StringComparison.OrdinalIgnoreCase)) + if (!file.Agents!.TryGetValue(bucketKey, out var agent) || agent?.Allowlist is null) continue; - entry.LastUsedAt = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); - entry.LastUsedCommand = command; // STJ escapes control chars on serialize - entry.LastResolvedPath = resolvedPath; // Id and Pattern preserved - changed = true; + foreach (var entry in agent.Allowlist) + { + if (!string.Equals(entry.Pattern?.Trim(), pattern.Trim(), + StringComparison.OrdinalIgnoreCase)) + continue; + entry.LastUsedAt = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); + entry.LastUsedCommand = command; // STJ escapes control chars on serialize + entry.LastResolvedPath = resolvedPath; // Id and Pattern preserved + changed = true; + } } return changed; }); diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs index 171a4428c..c76f25d29 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs @@ -635,6 +635,35 @@ public async Task RuntimeProof_AllowAlways_PersistsAndRecordsLastUsed() Assert.NotNull(resolvedAfter.Allowlist[0].LastResolvedPath); } + // Regression: wildcard-authorized hit must record lastUsed* on the wildcard bucket entry. + // ResolveReadOnly merges agents["*"] into the resolved allowlist for any concrete agent, + // so a request from "main" can be allow-matched by an entry living under "*". The store's + // record path must follow the same source — otherwise wildcard-authorized executions never + // accumulate usage metadata. + [Fact] + public async Task WildcardAllowlistHit_RecordsUseOnWildcardBucketEntry() + { + WriteStoreFile(""" + { + "version": 1, + "agents": { + "*": { + "security": "allowlist", + "ask": "off", + "allowlist": [{ "pattern": "**/cmd.exe" }] + } + } + } + """); + + var result = await MakeCoordinator().HandleAsync(Req("""{"command":["cmd"]}"""), "wildcard-1"); + + Assert.True(result.IsAllow); + var json = File.ReadAllText(Path.Combine(_dir, "exec-approvals.json")); + Assert.Contains("\"lastUsedCommand\"", json); + Assert.Contains("\"lastResolvedPath\"", json); + } + // ── Test doubles ────────────────────────────────────────────────────────── private sealed class FixedDecisionPromptHandler : IExecApprovalV2PromptHandler diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs index e4c254781..977a99d13 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs @@ -895,6 +895,58 @@ public async Task RecordAllowlistUseAsync_DoesNotTouchOtherEntries() Assert.Null(allowlist.First(e => e.Pattern == "**/rg.exe").LastUsedCommand); } + // ResolveReadOnly merges wildcard entries into the resolved allowlist, so a hit can be + // authorized by agents["*"]. RecordAllowlistUseAsync must follow the same source. + [Fact] + public async Task RecordAllowlistUseAsync_WildcardBucketOnly_UpdatesMetadata() + { + var id = Guid.NewGuid(); + WriteFile($$""" + { + "version": 1, + "agents": { + "*": { + "allowlist": [ + { "id": "{{id}}", "pattern": "**/git.exe" } + ] + } + } + } + """); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", "/usr/bin/git"); + + Assert.True(result); + var entry = store.ResolveReadOnly("main").Allowlist.Single(); + Assert.Equal(id, entry.Id); + Assert.Equal("git status", entry.LastUsedCommand); + Assert.Equal("/usr/bin/git", entry.LastResolvedPath); + Assert.NotNull(entry.LastUsedAt); + } + + // Same pattern in both buckets: both entries get metadata updated. The matcher cannot + // tell them apart structurally, and metadata is informative — not authorization-bearing. + [Fact] + public async Task RecordAllowlistUseAsync_PatternInBothBuckets_UpdatesBoth() + { + WriteFile(""" + { + "version": 1, + "agents": { + "main": { "allowlist": [{ "pattern": "**/git.exe" }] }, + "*": { "allowlist": [{ "pattern": "**/git.exe" }] } + } + } + """); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + + Assert.True(result); + var json = File.ReadAllText(FilePath); + var lastUsedCount = System.Text.RegularExpressions.Regex.Matches(json, "\"lastUsedCommand\"").Count; + Assert.Equal(2, lastUsedCount); + } + [Fact] public async Task RecordAllowlistUseAsync_BestEffort_IoExceptionReturnsFalse() { From a88d42833a62e875e8d27daece86c0e2279753d6 Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Tue, 26 May 2026 19:09:03 +0200 Subject: [PATCH 6/8] chore: retrigger CI sweep Co-Authored-By: Claude Sonnet 4.6 From ad4021d80197c86a7629b4afb2d8212727e2a32e Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Sun, 7 Jun 2026 21:47:37 +0200 Subject: [PATCH 7/8] fix(exec-approvals): drop lastUsedCommand and harden side-effect isolation lastUsedCommand persisted the full command text in exec-approvals.json, leaking tokens or secrets embedded in command arguments. Remove the field from ExecAllowlistEntry and the command parameter from RecordAllowlistUseAsync; lastUsedAt and lastResolvedPath provide sufficient operational metadata. Approval-store side effects (PersistAllowlistEntriesAsync, RecordAllowlistUsageAsync) were unguarded, so an unexpected exception could cause an already-approved command to be reported as InternalError. Each side effect in both the pre-approved (pass1) and post-prompt (step 8) paths is now wrapped in its own best-effort try/catch so a failure in one does not skip the other and never flips an allow to a deny. Add DoesNotContain("lastUsedCommand") assertion to the store round-trip test as a regression guard against accidental reintroduction of the field. Co-Authored-By: Claude Sonnet 4.6 --- .../ExecApprovals/ExecApprovalsContracts.cs | 1 - .../ExecApprovals/ExecApprovalsCoordinator.cs | 16 ++++++---- .../ExecApprovals/ExecApprovalsStore.cs | 4 +-- .../ExecApprovalsCoordinatorTests.cs | 11 ++++--- .../ExecApprovalsStoreTests.cs | 30 +++++++++---------- 5 files changed, 32 insertions(+), 30 deletions(-) diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs index b1fb05227..6d0dc5ec5 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs @@ -36,7 +36,6 @@ public sealed class ExecAllowlistEntry public Guid? Id { get; set; } public string? Pattern { get; set; } public double? LastUsedAt { get; set; } - public string? LastUsedCommand { get; set; } public string? LastResolvedPath { get; set; } } diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs index f06643d7a..9e5aaa513 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs @@ -98,8 +98,9 @@ public async Task HandleAsync(NodeInvokeRequest request, s if (pass1 is ExecHostPolicyDecision.AllowOutcome) { // Pre-approved path (security=Full, ask=Off or allowlist satisfied): skip prompt. - // Side effects fire before the log line, after the final allow decision is confirmed. - await RecordAllowlistUsageAsync(context).ConfigureAwait(false); + // Side effects are best-effort: a metadata write failure must not flip an allow to a deny. + try { await RecordAllowlistUsageAsync(context).ConfigureAwait(false); } + catch (Exception ex) { _logger.Warn($"[EXEC-APPROVALS] [{correlationId}] side-effect: record-usage failed (non-fatal): {ex.Message}"); } _logger.Info($"[EXEC-APPROVALS] [{correlationId}] path=new " + $"canonical=\"{SanitizeForLog(context.DisplayCommand)}\" decision=allow " + $"reason=approved fallbackUsed=false promptAttempted=false"); @@ -184,9 +185,14 @@ public async Task HandleAsync(NodeInvokeRequest request, s } // Step 8: side effects — strictly after the final allow decision. + // Each side effect is independently best-effort so a failure in one does not skip the other. if (persistAllowlistEntry && context.Security == ExecSecurity.Allowlist) - await PersistAllowlistEntriesAsync(context).ConfigureAwait(false); - await RecordAllowlistUsageAsync(context).ConfigureAwait(false); + { + try { await PersistAllowlistEntriesAsync(context).ConfigureAwait(false); } + catch (Exception ex) { _logger.Warn($"[EXEC-APPROVALS] [{correlationId}] side-effect: persist-entry failed (non-fatal): {ex.Message}"); } + } + try { await RecordAllowlistUsageAsync(context).ConfigureAwait(false); } + catch (Exception ex) { _logger.Warn($"[EXEC-APPROVALS] [{correlationId}] side-effect: record-usage failed (non-fatal): {ex.Message}"); } // Step 9: final allow log _logger.Info($"[EXEC-APPROVALS] [{correlationId}] path=new " + @@ -235,7 +241,7 @@ private async Task RecordAllowlistUsageAsync(ExecApprovalEvaluation context) ? context.AllowlistResolutions[i].ResolvedPath : null; await _store.RecordAllowlistUseAsync( - context.AgentId, pattern, context.DisplayCommand, resolvedPath) + context.AgentId, pattern, resolvedPath) .ConfigureAwait(false); } } diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs index 52e71cd8e..c83e79480 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs @@ -102,7 +102,7 @@ public async Task AddAllowlistEntryAsync(string? agentId, string pattern) // because ResolveReadOnly merges wildcard entries into the resolved allowlist — // so a hit can be authorized by either source and metadata must follow. public Task RecordAllowlistUseAsync( - string? agentId, string pattern, string command, string? resolvedPath) + string? agentId, string pattern, string? resolvedPath) { if (string.IsNullOrEmpty(pattern)) return Task.FromResult(false); var key = NormalizeAgentId(agentId); @@ -120,7 +120,6 @@ public Task RecordAllowlistUseAsync( StringComparison.OrdinalIgnoreCase)) continue; entry.LastUsedAt = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); - entry.LastUsedCommand = command; // STJ escapes control chars on serialize entry.LastResolvedPath = resolvedPath; // Id and Pattern preserved changed = true; } @@ -371,7 +370,6 @@ internal static List NormalizeAllowlistEntries( Id = entry.Id, Pattern = pattern, LastUsedAt = entry.LastUsedAt, - LastUsedCommand = entry.LastUsedCommand, LastResolvedPath = entry.LastResolvedPath, }); } diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs index c76f25d29..c5201e06f 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs @@ -482,7 +482,7 @@ public async Task AllowAlways_SecurityFull_DoesNotPersist() Assert.DoesNotContain("allowlist", json, StringComparison.OrdinalIgnoreCase); } - // C. Pre-approved path (pass1 = Allow) → RecordAllowlistUse fires and updates LastUsedCommand. + // C. Pre-approved path (pass1 = Allow) → RecordAllowlistUse fires and updates LastUsedAt. [Fact] public async Task AllowPreapproved_RecordsAllowlistUse() { @@ -503,7 +503,7 @@ public async Task AllowPreapproved_RecordsAllowlistUse() Assert.True(result.IsAllow); var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); Assert.Single(resolved.Allowlist); - Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); + Assert.NotNull(resolved.Allowlist[0].LastUsedAt); } // D. AllowOnce → persistAllowlistEntry=false, no entry written. @@ -562,7 +562,7 @@ public async Task AllowOnce_AllowlistSatisfied_RecordsUseInPostPass2Branch() Assert.True(result.IsAllow); var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); Assert.Single(resolved.Allowlist); - Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); + Assert.NotNull(resolved.Allowlist[0].LastUsedAt); } // G. Fallback path (canPresent=false) + AllowlistSatisfied=true → RecordAllowlistUse fires. @@ -590,7 +590,7 @@ public async Task Fallback_AllowlistSatisfied_RecordsUse() Assert.True(result.IsAllow); var resolved = new ExecApprovalsStore(_dir, NullLogger.Instance).ResolveReadOnly("main"); Assert.Single(resolved.Allowlist); - Assert.NotNull(resolved.Allowlist[0].LastUsedCommand); + Assert.NotNull(resolved.Allowlist[0].LastUsedAt); } // End-to-end coordinator/store runtime proof using real filesystem I/O. @@ -631,7 +631,6 @@ public async Task RuntimeProof_AllowAlways_PersistsAndRecordsLastUsed() Assert.Single(resolvedAfter.Allowlist); Assert.NotNull(resolvedAfter.Allowlist[0].Pattern); Assert.NotNull(resolvedAfter.Allowlist[0].LastUsedAt); - Assert.NotNull(resolvedAfter.Allowlist[0].LastUsedCommand); Assert.NotNull(resolvedAfter.Allowlist[0].LastResolvedPath); } @@ -660,7 +659,7 @@ public async Task WildcardAllowlistHit_RecordsUseOnWildcardBucketEntry() Assert.True(result.IsAllow); var json = File.ReadAllText(Path.Combine(_dir, "exec-approvals.json")); - Assert.Contains("\"lastUsedCommand\"", json); + Assert.Contains("\"lastUsedAt\"", json); Assert.Contains("\"lastResolvedPath\"", json); } diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs index 977a99d13..beb2b1dbf 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs @@ -821,14 +821,13 @@ public async Task RecordAllowlistUseAsync_UpdatesMetadataAndPreservesIdAndPatter } """); var store = Store(); - var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", "/usr/bin/git"); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "/usr/bin/git"); Assert.True(result); var entry = store.ResolveReadOnly("main").Allowlist[0]; Assert.Equal(id, entry.Id); Assert.Equal("**/git.exe", entry.Pattern); Assert.NotNull(entry.LastUsedAt); - Assert.Equal("git status", entry.LastUsedCommand); Assert.Equal("/usr/bin/git", entry.LastResolvedPath); } @@ -844,10 +843,10 @@ public async Task RecordAllowlistUseAsync_PatternNotPresent_ReturnsFalse() } """); var store = Store(); - var result = await store.RecordAllowlistUseAsync("main", "**/rg.exe", "rg foo", null); + var result = await store.RecordAllowlistUseAsync("main", "**/rg.exe", null); Assert.False(result); - Assert.Null(store.ResolveReadOnly("main").Allowlist[0].LastUsedCommand); + Assert.Null(store.ResolveReadOnly("main").Allowlist[0].LastUsedAt); } [Fact] @@ -855,7 +854,7 @@ public async Task RecordAllowlistUseAsync_AgentNotPresent_ReturnsFalse() { WriteFile("""{"version":1,"agents":{}}"""); var store = Store(); - var result = await store.RecordAllowlistUseAsync("nonexistent", "**/git.exe", "git status", null); + var result = await store.RecordAllowlistUseAsync("nonexistent", "**/git.exe", null); Assert.False(result); } @@ -865,7 +864,7 @@ public async Task RecordAllowlistUseAsync_MalformedFile_ReturnsFalse() { WriteFile("{ bad json }"); var store = Store(); - var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", null); Assert.False(result); Assert.Equal("{ bad json }", File.ReadAllText(FilePath)); @@ -888,11 +887,11 @@ public async Task RecordAllowlistUseAsync_DoesNotTouchOtherEntries() } """); var store = Store(); - await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + await store.RecordAllowlistUseAsync("main", "**/git.exe", null); var allowlist = store.ResolveReadOnly("main").Allowlist; - Assert.NotNull(allowlist.First(e => e.Pattern == "**/git.exe").LastUsedCommand); - Assert.Null(allowlist.First(e => e.Pattern == "**/rg.exe").LastUsedCommand); + Assert.NotNull(allowlist.First(e => e.Pattern == "**/git.exe").LastUsedAt); + Assert.Null(allowlist.First(e => e.Pattern == "**/rg.exe").LastUsedAt); } // ResolveReadOnly merges wildcard entries into the resolved allowlist, so a hit can be @@ -914,12 +913,11 @@ public async Task RecordAllowlistUseAsync_WildcardBucketOnly_UpdatesMetadata() } """); var store = Store(); - var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", "/usr/bin/git"); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "/usr/bin/git"); Assert.True(result); var entry = store.ResolveReadOnly("main").Allowlist.Single(); Assert.Equal(id, entry.Id); - Assert.Equal("git status", entry.LastUsedCommand); Assert.Equal("/usr/bin/git", entry.LastResolvedPath); Assert.NotNull(entry.LastUsedAt); } @@ -939,11 +937,11 @@ public async Task RecordAllowlistUseAsync_PatternInBothBuckets_UpdatesBoth() } """); var store = Store(); - var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", null); Assert.True(result); var json = File.ReadAllText(FilePath); - var lastUsedCount = System.Text.RegularExpressions.Regex.Matches(json, "\"lastUsedCommand\"").Count; + var lastUsedCount = System.Text.RegularExpressions.Regex.Matches(json, "\"lastUsedAt\"").Count; Assert.Equal(2, lastUsedCount); } @@ -969,7 +967,7 @@ public async Task RecordAllowlistUseAsync_BestEffort_IoExceptionReturnsFalse() // because the target is open without write/delete sharing → IOException degraded-save path. using (var fs = new FileStream(FilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.Read)) { - var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + var result = await store.RecordAllowlistUseAsync("main", "**/git.exe", null); Assert.False(result); // IOException absorbed, no exception escaped Assert.NotEmpty(_log.Warnings); // write failure logged as Warn } @@ -996,7 +994,7 @@ public async Task RoundTrip_WrittenFileIsValidJsonWithCorrectFields() var store = Store(); await store.AddAllowlistEntryAsync("main", "**/git.exe"); // lastUsedAt is absent on creation; RecordAllowlistUseAsync stamps it on first use. - await store.RecordAllowlistUseAsync("main", "**/git.exe", "git status", null); + await store.RecordAllowlistUseAsync("main", "**/git.exe", null); var json = File.ReadAllText(FilePath); using var doc = System.Text.Json.JsonDocument.Parse(json); // valid JSON @@ -1006,6 +1004,8 @@ public async Task RoundTrip_WrittenFileIsValidJsonWithCorrectFields() .GetProperty("agents").GetProperty("main") .GetProperty("allowlist")[0].GetProperty("lastUsedAt"); Assert.Equal(System.Text.Json.JsonValueKind.Number, lastUsedAt.ValueKind); + // lastUsedCommand must never appear in the persisted file (security regression guard). + Assert.DoesNotContain("lastUsedCommand", json, StringComparison.OrdinalIgnoreCase); } [Fact] From da0b255e5dbdab9389194125b18a3a6bb48eb75a Mon Sep 17 00:00:00 2001 From: AlexAlves87 Date: Fri, 12 Jun 2026 07:05:14 +0200 Subject: [PATCH 8/8] fix(exec-approvals): migrate legacy file before any store write UpdateFileAsync loaded the target file directly, so with a custom state dir configured a write could create a fresh exec-approvals.json at the target path while an unmigrated legacy file still existed. That would permanently block TryMigrateLegacyFile and silently orphan the legacy configuration. Mirror EnsureFileAsync: run migration first and refuse to write when the legacy file is unreadable (Blocked), consistent with the existing fail-closed read semantics. Co-Authored-By: Claude Fable 5 --- .../ExecApprovals/ExecApprovalsStore.cs | 9 +++++ .../ExecApprovalsStoreTests.cs | 35 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs index 524eafa9d..6fadfd8b1 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs @@ -396,6 +396,15 @@ private async Task UpdateFileAsync(Func mutate) await _lock.WaitAsync().ConfigureAwait(false); try { + // Migrate before any write: creating the target file here would permanently + // block TryMigrateLegacyFile and silently orphan the legacy configuration. + if (TryMigrateLegacyFile() == LegacyMigrationStatus.Blocked) + { + _logger.Warn("[EXEC-APPROVALS] Refusing to write exec-approvals.json: " + + "unmigrated legacy file is unreadable"); + return false; + } + var result = LoadFile(); if (result.Status == LoadFileStatus.Invalid) { diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs index 86cd0eb9a..0f87ddb62 100644 --- a/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs +++ b/tests/OpenClaw.Shared.Tests/ExecApprovalsStoreTests.cs @@ -1153,6 +1153,41 @@ public async Task AddAllowlistEntryAsync_BestEffort_IoExceptionReturnsFalse() } } + [Fact] + public async Task AddAllowlistEntryAsync_CustomStateDir_MigratesLegacyBeforeWriting() + { + WriteFile(MinimalFileWithAgent("main", "allowlist")); + var stateDir = Path.Combine(_dir, "custom-state"); + var store = Store(stateDir); + + var result = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + + Assert.True(result); + // Legacy file migrated first; the write lands on the migrated content, not a fresh file. + Assert.False(File.Exists(FilePath)); + Assert.True(File.Exists($"{FilePath}.migrated")); + var resolved = store.ResolveReadOnly("main"); + Assert.Equal(ExecSecurity.Allowlist, resolved.Defaults.Security); + Assert.Single(resolved.Allowlist); + Assert.Equal("**/git.exe", resolved.Allowlist[0].Pattern); + } + + [Fact] + public async Task AddAllowlistEntryAsync_CustomStateDir_UnreadableLegacy_RefusesToWrite() + { + WriteFile("{ bad json }"); + var stateDir = Path.Combine(_dir, "custom-state"); + var store = Store(stateDir); + + var result = await store.AddAllowlistEntryAsync("main", "**/git.exe"); + + Assert.False(result); + // No target file may be created: that would permanently block legacy migration. + Assert.False(File.Exists(Path.Combine(stateDir, "exec-approvals.json"))); + Assert.True(File.Exists(FilePath)); + Assert.Contains(_log.Warnings, w => w.Contains("Refusing to write")); + } + // ── Helpers ─────────────────────────────────────────────────────────────── private static string MinimalFile() => """{"version":1,"agents":{}}""";