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 e7d06497a..b950d6fc9 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; } @@ -27,11 +27,11 @@ public sealed class ExecApprovalEvaluation public bool AllAllowlistResolutionsMatched { 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/ExecApprovalsContracts.cs b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs index 9a845ef7a..2ffd011e1 100644 --- a/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs +++ b/src/OpenClaw.Shared/ExecApprovals/ExecApprovalsContracts.cs @@ -37,7 +37,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 50e1bef9e..c5aa978d4 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); @@ -92,14 +92,17 @@ 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 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"); @@ -107,9 +110,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 @@ -162,8 +166,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) @@ -176,14 +178,23 @@ 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. + // Each side effect is independently best-effort so a failure in one does not skip the other. + if (persistAllowlistEntry && context.Security == ExecSecurity.Allowlist) + { + 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 " + @@ -197,7 +208,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"; @@ -206,6 +217,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, 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( @@ -230,7 +272,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, @@ -238,7 +280,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 d7b7f344d..6fadfd8b1 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, @@ -75,7 +75,7 @@ internal ExecApprovalsStore( // ── 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) { if (_legacyFilePath is not null && File.Exists(_legacyFilePath) && !File.Exists(_filePath)) @@ -87,6 +87,81 @@ 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. + // 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? resolvedPath) + { + if (string.IsNullOrEmpty(pattern)) return Task.FromResult(false); + var key = NormalizeAgentId(agentId); + var buckets = key == "*" ? new[] { "*" } : new[] { key, "*" }; + return UpdateFileAsync(file => + { + var changed = false; + foreach (var bucketKey in buckets) + { + if (!file.Agents!.TryGetValue(bucketKey, out var agent) || agent?.Allowlist is null) + continue; + foreach (var entry in agent.Allowlist) + { + if (!string.Equals(entry.Pattern?.Trim(), pattern.Trim(), + StringComparison.OrdinalIgnoreCase)) + continue; + entry.LastUsedAt = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); + 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) @@ -171,7 +246,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}"); @@ -312,6 +387,55 @@ 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 + { + // 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) + { + _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) @@ -394,7 +518,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) { @@ -410,7 +534,6 @@ internal static List NormalizeAllowlistEntries( Id = entry.Id, Pattern = pattern, LastUsedAt = entry.LastUsedAt, - LastUsedCommand = entry.LastUsedCommand, LastResolvedPath = entry.LastResolvedPath, }); } 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 { diff --git a/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs b/tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs index cc9d513cb..a5064ecc7 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); @@ -479,6 +482,221 @@ 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 LastUsedAt. + [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].LastUsedAt); + } + + // 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].LastUsedAt); + } + + // 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].LastUsedAt); + } + + // 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].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("\"lastUsedAt\"", 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 79b9695d4..3b5355ab0 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; @@ -826,6 +827,367 @@ public void ResolveReadOnly_LegacyAskFallback_MapsToSecurity(string legacyValue, Assert.Equal(expected, resolved.Defaults.AskFallback); } + // ── 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", "/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("/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", null); + + Assert.False(result); + Assert.Null(store.ResolveReadOnly("main").Allowlist[0].LastUsedAt); + } + + [Fact] + public async Task RecordAllowlistUseAsync_AgentNotPresent_ReturnsFalse() + { + WriteFile("""{"version":1,"agents":{}}"""); + var store = Store(); + var result = await store.RecordAllowlistUseAsync("nonexistent", "**/git.exe", 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", 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", null); + + var allowlist = store.ResolveReadOnly("main").Allowlist; + 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 + // 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", "/usr/bin/git"); + + Assert.True(result); + var entry = store.ResolveReadOnly("main").Allowlist.Single(); + Assert.Equal(id, entry.Id); + 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", null); + + Assert.True(result); + var json = File.ReadAllText(FilePath); + var lastUsedCount = System.Text.RegularExpressions.Regex.Matches(json, "\"lastUsedAt\"").Count; + Assert.Equal(2, lastUsedCount); + } + + [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", 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", 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); + // lastUsedCommand must never appear in the persisted file (security regression guard). + Assert.DoesNotContain("lastUsedCommand", json, StringComparison.OrdinalIgnoreCase); + } + + [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 + } + } + + [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")); + } + // ── State dir: tilde-only expansion ────────────────────────────────────── ///