From 779ca761811e0fbd19f90aa3f1139a45fc342891 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 12 Apr 2026 21:31:41 +0000 Subject: [PATCH 1/3] fix(security): gate control-plane writes, hard-deny lifecycle files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An agent session edited ~/.netclaw/config/netclaw.json via file_write; the config watcher detected the change and triggered a full daemon restart, dropping the session mid-turn with no warning or attribution. ToolPathPolicy only protected secrets.json, webhooks/, and keys/ — the rest of the control plane was writable by any tool with the `file` grant, and Personal audience has no root-containment, so the deny list was the entire security boundary. Split ToolPathPolicy into three independent enforcement surfaces: write deny, read deny, and a narrow shell indicator list (so directory-scoped write-deny entries don't bleed into the substring scan that gates `shell_execute` — otherwise `ls ~/.netclaw/config` would regress). Tier 1 (hard deny, no approval possible): secrets, keys, netclaw.db, pid/lock files, restart-manifest.json. Corrupting these is unrecoverable and no legitimate agent flow edits them. Tier 2 (approval-gated via FilePathApprovalMatcher): everything under ~/.netclaw/config/ that isn't Tier 1 — netclaw.json, devices.json, tool-approvals.json, mcp-oauth-metadata.json, webhooks/*.json. The matcher inspects the target path and routes control-plane writes to a `file_write:control-plane` approval-mode key with per-path patterns so approving netclaw.json doesn't implicitly approve tool-approvals.json. Fail-closed default in GetMissingApprovalPolicyDefaultMode: when ApprovalPolicy is null on Personal audience, control-plane writes still require interactive approval, matching the existing fail-closed behavior for shell_execute. FileReadTool switches from IsDenied to the narrower IsReadDenied so the agent can still read netclaw.json for diagnostics while writes are gated. Error messages on all three file tools now name the offending path and point at the right escape hatch (`netclaw doctor --fix`, `netclaw secrets set`) instead of a generic "protected by security policy" string. --- .../Tools/ToolApprovalGateTests.cs | 120 ++++++++++++++ src/Netclaw.Actors/Tools/FileEditTool.cs | 9 +- .../Tools/FilePathApprovalMatcher.cs | 147 ++++++++++++++++++ src/Netclaw.Actors/Tools/FileReadTool.cs | 5 +- src/Netclaw.Actors/Tools/FileWriteTool.cs | 9 +- src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 43 ++++- src/Netclaw.Daemon/Program.cs | 41 ++++- .../ToolPathPolicyTests.cs | 147 ++++++++++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 16 ++ src/Netclaw.Security/ToolPathPolicy.cs | 100 ++++++++++-- 10 files changed, 605 insertions(+), 32 deletions(-) create mode 100644 src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs index 827f86de4..351d94dbd 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs @@ -155,4 +155,124 @@ public void Hard_denied_shell_command_is_blocked_before_approval() Assert.False(decision.NeedsApproval); Assert.Equal("hard_deny_self_destructive", decision.DenyReason); } + + // ── FilePathApprovalMatcher / control-plane approval tests ──────────── + + private const string ControlPlaneRoot = "/home/user/.netclaw/config"; + + private static ToolAccessPolicy CreateFileWritePolicy(ToolApprovalConfig? approvalPolicy = null) + { + var config = new ToolConfig(); + config.AudienceProfiles.Personal.ApprovalPolicy = approvalPolicy; + return new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + fileApprovalMatcher: new FilePathApprovalMatcher(ControlPlaneRoot)); + } + + private static INetclawTool FileWriteToolInstance() => new FileWriteTool(); + private static INetclawTool FileEditToolInstance() => new FileEditTool(); + + [Fact] + public void file_write_to_netclaw_json_requires_approval_under_fail_closed_default() + { + // ApprovalPolicy is null → GetMissingApprovalPolicyDefaultMode fires. + // For Personal + control-plane key, fail-closed default is Approval. + var policy = CreateFileWritePolicy(approvalPolicy: null); + var args = new Dictionary + { + ["Path"] = ControlPlaneRoot + "/netclaw.json", + ["Content"] = "{}" + }; + + var decision = policy.AuthorizeInvocation(FileWriteToolInstance(), PersonalContext(), args); + + Assert.True(decision.NeedsApproval); + Assert.NotNull(decision.ApprovalContext); + Assert.Contains( + decision.ApprovalContext!.UnapprovedPatterns, + p => p.StartsWith("file_write:control-plane:", StringComparison.Ordinal)); + } + + [Fact] + public void file_write_to_non_control_plane_path_auto_approves_under_null_policy() + { + var policy = CreateFileWritePolicy(approvalPolicy: null); + var args = new Dictionary + { + ["Path"] = "/tmp/scratch.txt", + ["Content"] = "hello" + }; + + var decision = policy.AuthorizeInvocation(FileWriteToolInstance(), PersonalContext(), args); + + Assert.True(decision.Allowed); + Assert.False(decision.NeedsApproval); + } + + [Fact] + public void file_edit_of_netclaw_json_requires_approval() + { + var policy = CreateFileWritePolicy(approvalPolicy: null); + var args = new Dictionary + { + ["Path"] = ControlPlaneRoot + "/netclaw.json", + ["OldString"] = "a", + ["NewString"] = "b" + }; + + var decision = policy.AuthorizeInvocation(FileEditToolInstance(), PersonalContext(), args); + + Assert.True(decision.NeedsApproval); + Assert.Contains( + decision.ApprovalContext!.UnapprovedPatterns, + p => p.StartsWith("file_edit:control-plane:", StringComparison.Ordinal)); + } + + [Fact] + public void file_write_emits_distinct_per_path_patterns() + { + // Per-path patterns so approving netclaw.json does not implicitly + // approve tool-approvals.json or devices.json. + var matcher = new FilePathApprovalMatcher(ControlPlaneRoot); + var netclawJson = matcher.ExtractPatterns("file_write", + new Dictionary { ["Path"] = ControlPlaneRoot + "/netclaw.json" }); + var toolApprovals = matcher.ExtractPatterns("file_write", + new Dictionary { ["Path"] = ControlPlaneRoot + "/tool-approvals.json" }); + var devices = matcher.ExtractPatterns("file_write", + new Dictionary { ["Path"] = ControlPlaneRoot + "/devices.json" }); + + Assert.NotEqual(netclawJson[0], toolApprovals[0]); + Assert.NotEqual(netclawJson[0], devices[0]); + Assert.NotEqual(toolApprovals[0], devices[0]); + } + + [Fact] + public void file_write_control_plane_approval_honors_explicit_auto_override() + { + // Escape hatch: operator who knows what they're doing can downgrade the + // control-plane key to Auto via ApprovalPolicy.ToolOverrides. + var approvalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["file_write:control-plane"] = ToolApprovalMode.Auto + } + }; + var policy = CreateFileWritePolicy(approvalPolicy); + var args = new Dictionary + { + ["Path"] = ControlPlaneRoot + "/netclaw.json", + ["Content"] = "{}" + }; + + var decision = policy.AuthorizeInvocation(FileWriteToolInstance(), PersonalContext(), args); + + Assert.True(decision.Allowed); + Assert.False(decision.NeedsApproval); + } } diff --git a/src/Netclaw.Actors/Tools/FileEditTool.cs b/src/Netclaw.Actors/Tools/FileEditTool.cs index cf2f07bc0..ff63b5819 100644 --- a/src/Netclaw.Actors/Tools/FileEditTool.cs +++ b/src/Netclaw.Actors/Tools/FileEditTool.cs @@ -10,11 +10,13 @@ namespace Netclaw.Actors.Tools; /// Makes targeted text replacements in an existing file without rewriting the entire file. /// Matches literal text (not regex). Fails if OldString is not found or is ambiguous. /// -[NetclawTool("file_edit", +[NetclawTool(ToolName, "Make targeted text replacements in an existing file without rewriting the entire file", Grant = "file")] public sealed partial class FileEditTool : NetclawTool { + public const string ToolName = "file_edit"; + private readonly ToolPathPolicy? _pathPolicy; private readonly ScopedFileAccessPolicy _fileAccessPolicy; @@ -54,7 +56,10 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon return accessError; if (_pathPolicy?.IsDenied(authorizedPath) == true) - return "Error: Access denied — this file is protected by security policy."; + return $"Error: Access denied: '{authorizedPath}' is part of Netclaw's control plane " + + "(secrets, keys, database, or lifecycle files) and cannot be modified by agent tools, " + + "even with approval. If the user wants this change, ask them to run a dedicated command " + + "(e.g. `netclaw doctor --fix`, `netclaw secrets set`) or edit the file directly."; if (!File.Exists(authorizedPath)) return $"Error: File not found: {authorizedPath}"; diff --git a/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs b/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs new file mode 100644 index 000000000..d181dbbd4 --- /dev/null +++ b/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs @@ -0,0 +1,147 @@ +using Netclaw.Security; + +namespace Netclaw.Actors.Tools; + +/// +/// Argument-aware approval matcher for the file_write and file_edit +/// tools. Inspects the target path and, when it lands inside Netclaw's control +/// plane (~/.netclaw/config/), routes the invocation to a distinct +/// approval-mode key so operators can gate control-plane writes without +/// requiring approval for every ordinary file write. +/// +/// +/// Tier split: Tier 1 files (secrets, keys, SQLite DB, pid/lock, restart +/// manifest) are hard-denied earlier by +/// and never reach this matcher. Tier 2 files — the rest of +/// ~/.netclaw/config/ — are routed here and surfaced through the +/// standard approval flow with per-path patterns so approving +/// netclaw.json does not implicitly approve tool-approvals.json. +/// +public sealed class FilePathApprovalMatcher : IToolApprovalMatcher +{ + public const string ControlPlaneModeKeySuffix = ":control-plane"; + public const string ControlPlanePatternPrefix = "control-plane:"; + + private readonly string _controlPlaneRoot; + + public FilePathApprovalMatcher(string controlPlaneRoot) + { + _controlPlaneRoot = NormalizePath(controlPlaneRoot); + } + + public string GetApprovalModeKey(string toolName, IDictionary? arguments) + { + return TryGetControlPlaneRelativePath(arguments, out _) + ? toolName + ControlPlaneModeKeySuffix + : toolName; + } + + public IReadOnlyList ExtractPatterns(string toolName, IDictionary? arguments) + { + if (TryGetControlPlaneRelativePath(arguments, out var relativePath)) + return [toolName + ControlPlaneModeKeySuffix + ":" + relativePath]; + + return [toolName]; + } + + public bool IsApproved(string toolName, IDictionary? arguments, IEnumerable approvedPatterns) + { + var patterns = ExtractPatterns(toolName, arguments); + foreach (var pattern in patterns) + { + var matched = false; + foreach (var approved in approvedPatterns) + { + if (string.Equals(pattern, approved, StringComparison.OrdinalIgnoreCase)) + { + matched = true; + break; + } + } + + if (!matched) + return false; + } + + return true; + } + + public string FormatForDisplay(string toolName, IDictionary? arguments) + { + if (TryGetPath(arguments, out var path)) + return $"{toolName}: {path}"; + + return toolName; + } + + private bool TryGetControlPlaneRelativePath( + IDictionary? arguments, + out string relativePath) + { + relativePath = string.Empty; + + if (!TryGetPath(arguments, out var rawPath)) + return false; + + if (!TryNormalizePath(rawPath, out var normalized)) + return false; + + if (!IsUnderRoot(normalized, _controlPlaneRoot)) + return false; + + relativePath = Path.GetRelativePath(_controlPlaneRoot, normalized) + .Replace(Path.DirectorySeparatorChar, '/'); + return true; + } + + private static bool TryGetPath(IDictionary? arguments, out string path) + { + path = string.Empty; + if (arguments is null) + return false; + + if (arguments.TryGetValue("Path", out var value) || arguments.TryGetValue("path", out value)) + { + if (value is string s && !string.IsNullOrWhiteSpace(s)) + { + path = s; + return true; + } + } + + return false; + } + + private static bool TryNormalizePath(string rawPath, out string normalized) + { + normalized = string.Empty; + try + { + var baseDir = Environment.CurrentDirectory; + var combined = Path.IsPathRooted(rawPath) + ? rawPath + : Path.Combine(baseDir, rawPath); + normalized = NormalizePath(combined); + return true; + } + catch + { + return false; + } + } + + private static string NormalizePath(string path) + => Path.GetFullPath(path).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + + private static bool IsUnderRoot(string candidate, string root) + { + if (!candidate.StartsWith(root, StringComparison.OrdinalIgnoreCase)) + return false; + + if (candidate.Length == root.Length) + return true; + + var boundary = candidate[root.Length]; + return boundary == Path.DirectorySeparatorChar || boundary == Path.AltDirectorySeparatorChar; + } +} diff --git a/src/Netclaw.Actors/Tools/FileReadTool.cs b/src/Netclaw.Actors/Tools/FileReadTool.cs index 783e6ee2b..72aca11b7 100644 --- a/src/Netclaw.Actors/Tools/FileReadTool.cs +++ b/src/Netclaw.Actors/Tools/FileReadTool.cs @@ -43,8 +43,9 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon if (!_fileAccessPolicy.TryResolveReadPath(args.Path, context, out var authorizedPath, out var accessError)) return accessError; - if (_pathPolicy?.IsDenied(authorizedPath) == true) - return "Error: Access denied — this file is protected by security policy."; + if (_pathPolicy?.IsReadDenied(authorizedPath) == true) + return $"Error: Access denied: '{authorizedPath}' contains credentials or keys " + + "and cannot be read by agent tools."; if (!File.Exists(authorizedPath)) return $"Error: File not found: {authorizedPath}"; diff --git a/src/Netclaw.Actors/Tools/FileWriteTool.cs b/src/Netclaw.Actors/Tools/FileWriteTool.cs index 1e4b526ac..ff6387d5c 100644 --- a/src/Netclaw.Actors/Tools/FileWriteTool.cs +++ b/src/Netclaw.Actors/Tools/FileWriteTool.cs @@ -9,11 +9,13 @@ namespace Netclaw.Actors.Tools; /// /// Writes content to a file as UTF-8, creating parent directories if needed. /// -[NetclawTool("file_write", +[NetclawTool(ToolName, "Write content to a file, creating parent directories if needed", Grant = "file")] public sealed partial class FileWriteTool : NetclawTool { + public const string ToolName = "file_write"; + private readonly ToolPathPolicy? _pathPolicy; private readonly ScopedFileAccessPolicy _fileAccessPolicy; @@ -45,7 +47,10 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon return accessError; if (_pathPolicy?.IsDenied(authorizedPath) == true) - return "Error: Access denied — this file is protected by security policy."; + return $"Error: Access denied: '{authorizedPath}' is part of Netclaw's control plane " + + "(secrets, keys, database, or lifecycle files) and cannot be modified by agent tools, " + + "even with approval. If the user wants this change, ask them to run a dedicated command " + + "(e.g. `netclaw doctor --fix`, `netclaw secrets set`) or edit the file directly."; try { diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index fa7ba915b..5034a8b26 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -13,13 +13,19 @@ public sealed class ToolAccessPolicy private readonly EffectivePolicyDefaults _defaults; private readonly ToolAudienceProfileResolver _profileResolver; private readonly ShellCommandPolicy? _shellCommandPolicy; + private readonly IToolApprovalMatcher _fileApprovalMatcher; - public ToolAccessPolicy(ToolConfig toolConfig, EffectivePolicyDefaults defaults, ShellCommandPolicy? shellCommandPolicy = null) + public ToolAccessPolicy( + ToolConfig toolConfig, + EffectivePolicyDefaults defaults, + ShellCommandPolicy? shellCommandPolicy = null, + IToolApprovalMatcher? fileApprovalMatcher = null) { _toolConfig = toolConfig; _defaults = defaults; _profileResolver = new ToolAudienceProfileResolver(toolConfig); _shellCommandPolicy = shellCommandPolicy; + _fileApprovalMatcher = fileApprovalMatcher ?? DefaultApprovalMatcher.Instance; } public IReadOnlyList FilterExposedTools( @@ -87,7 +93,7 @@ public ToolAccessDecision AuthorizeInvocation( return ToolAccessDecision.Deny("tool_not_allowed_for_audience_profile"); if (!IsShellTool(tool)) - return CheckApprovalGate(tool.Name, context, arguments, DefaultApprovalMatcher.Instance); + return CheckApprovalGate(tool.Name, context, arguments, SelectMatcherForTool(tool.Name)); var shellMode = ResolveShellMode(); if (shellMode == ShellExecutionMode.Off) @@ -134,8 +140,9 @@ private ToolAccessDecision CheckApprovalGate( var audience = ResolveAudience(context); var profile = ToolAudienceProfileDefaults.GetResolvedProfile(_toolConfig.AudienceProfiles, audience); var approvalPolicy = profile.ApprovalPolicy; - var mode = approvalPolicy?.GetEffectiveMode(toolName) - ?? GetMissingApprovalPolicyDefaultMode(toolName, audience); + var approvalModeKey = matcher.GetApprovalModeKey(toolName, arguments); + var mode = approvalPolicy?.GetEffectiveMode(approvalModeKey) + ?? GetMissingApprovalPolicyDefaultMode(approvalModeKey, audience); if (mode == ToolApprovalMode.Deny) return ToolAccessDecision.Deny("tool_denied_by_approval_policy"); @@ -162,16 +169,38 @@ private ToolAccessDecision CheckApprovalGate( return ToolAccessDecision.RequiresApproval(approvalContext); } - private static ToolApprovalMode GetMissingApprovalPolicyDefaultMode(string toolName, TrustAudience audience) + private static ToolApprovalMode GetMissingApprovalPolicyDefaultMode(string approvalModeKey, TrustAudience audience) { // Fail-closed for personal shell: if the approval policy was omitted, // still require interactive approval rather than silently auto-approving. - if (audience == TrustAudience.Personal && string.Equals(toolName, ShellTool.ToolName, StringComparison.Ordinal)) - return ToolApprovalMode.Approval; + if (audience == TrustAudience.Personal) + { + if (string.Equals(approvalModeKey, ShellTool.ToolName, StringComparison.Ordinal)) + return ToolApprovalMode.Approval; + + // Fail-closed for control-plane file writes: even if the operator did + // not explicitly configure an approval policy, writes into + // ~/.netclaw/config/** require interactive approval. Without this the + // agent can silently edit netclaw.json and trigger a daemon restart + // that drops the current session (see the session-file-blown incident). + if (approvalModeKey.EndsWith(FilePathApprovalMatcher.ControlPlaneModeKeySuffix, StringComparison.Ordinal)) + return ToolApprovalMode.Approval; + } return ToolApprovalMode.Auto; } + private IToolApprovalMatcher SelectMatcherForTool(string toolName) + { + if (string.Equals(toolName, FileWriteTool.ToolName, StringComparison.Ordinal) + || string.Equals(toolName, FileEditTool.ToolName, StringComparison.Ordinal)) + { + return _fileApprovalMatcher; + } + + return DefaultApprovalMatcher.Instance; + } + private ShellExecutionMode ResolveShellMode() => _toolConfig.ShellMode ?? _defaults.ShellExecutionMode; diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 46a75713d..63e895d02 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -551,14 +551,49 @@ static void ConfigureDaemonServices( .Get() ?? new SearchConfig(); var searchBackend = CreateSearchBackend(searchConfig); - // Tool path deny-list: prevent agent tools from accessing secrets - var toolPathPolicy = new ToolPathPolicy([paths.SecretsPath, paths.WebhooksDirectory, paths.KeysDirectory]); + // Tool path deny-list: prevent agent tools from touching Netclaw's control plane. + // Three independent surfaces so the write-deny set can be broad (covers the + // files whose edits would corrupt or destabilize the daemon) without letting + // over-broad directory entries bleed into the shell indicator substring scan, + // which would otherwise block legitimate diagnostic commands like + // `ls ~/.netclaw/config`. See ToolPathPolicy remarks. + var writeDenyList = new[] + { + paths.SecretsPath, // credentials + paths.KeysDirectory, // cryptographic keyring + paths.SqliteDbPath, // memory/session store — corruption is unrecoverable + paths.PidFilePath, // lifecycle files + paths.LockFilePath, + paths.RestartManifestPath, // cache/restart-manifest.json — agents writing this could spoof restart state + }; + var readDenyList = new[] + { + paths.SecretsPath, + paths.KeysDirectory, + paths.WebhooksDirectory, // webhook configs embed inline secrets + }; + var shellIndicatorList = new[] + { + paths.SecretsPath, + paths.WebhooksDirectory, + paths.KeysDirectory, + }; + var toolPathPolicy = new ToolPathPolicy(writeDenyList, readDenyList, shellIndicatorList); services.AddSingleton(toolPathPolicy); var shellCommandPolicy = new ShellCommandPolicy(toolConfig.HardDenyPatterns); services.AddSingleton(shellCommandPolicy); - var toolAccessPolicy = new ToolAccessPolicy(toolConfig, effectivePolicyDefaults, shellCommandPolicy); + // Argument-aware matcher: routes file_write/file_edit into a control-plane + // approval bucket when the target is under ~/.netclaw/config/ so those + // writes fail-closed to interactive approval even when no explicit policy + // override is set. + var fileApprovalMatcher = new FilePathApprovalMatcher(paths.ConfigDirectory); + var toolAccessPolicy = new ToolAccessPolicy( + toolConfig, + effectivePolicyDefaults, + shellCommandPolicy, + fileApprovalMatcher); services.AddSingleton(toolAccessPolicy); var toolApprovalStore = new ToolApprovalStore(paths.ToolApprovalsPath); diff --git a/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs b/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs index a43b22847..adebbe8bd 100644 --- a/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs +++ b/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs @@ -142,4 +142,151 @@ public void CommandReferencesDeniedPath_detects_high_risk_archive_of_config_dire Assert.True(policy.CommandReferencesDeniedPath("tar czf /tmp/netclaw-config.tgz ~/.netclaw/config")); } + + // ── Three-list split tests (production Tier 1 hard-deny) ───────────── + + private static ToolPathPolicy CreateProductionPolicy() + { + var writeDeny = new[] + { + "/home/user/.netclaw/config/secrets.json", + "/home/user/.netclaw/keys", + "/home/user/.netclaw/netclaw.db", + "/home/user/.netclaw/netclaw.pid", + "/home/user/.netclaw/netclaw.lock", + "/home/user/.netclaw/cache/restart-manifest.json", + }; + var readDeny = new[] + { + "/home/user/.netclaw/config/secrets.json", + "/home/user/.netclaw/keys", + "/home/user/.netclaw/config/webhooks", + }; + var shellIndicators = new[] + { + "/home/user/.netclaw/config/secrets.json", + "/home/user/.netclaw/config/webhooks", + "/home/user/.netclaw/keys", + }; + return new ToolPathPolicy(writeDeny, readDeny, shellIndicators); + } + + [Fact] + public void IsDenied_blocks_sqlite_db_when_listed() + { + var policy = CreateProductionPolicy(); + Assert.True(policy.IsDenied("/home/user/.netclaw/netclaw.db")); + } + + [Fact] + public void IsDenied_blocks_pid_and_lock_files() + { + var policy = CreateProductionPolicy(); + Assert.True(policy.IsDenied("/home/user/.netclaw/netclaw.pid")); + Assert.True(policy.IsDenied("/home/user/.netclaw/netclaw.lock")); + } + + [Fact] + public void IsDenied_blocks_restart_manifest() + { + var policy = CreateProductionPolicy(); + Assert.True(policy.IsDenied("/home/user/.netclaw/cache/restart-manifest.json")); + } + + [Fact] + public void IsDenied_allows_writes_to_netclaw_config_json_so_approval_gate_can_fire() + { + // Tier 2: netclaw.json is NOT in the hard-deny set so it reaches the + // approval gate. The incident fix relies on this split — hard-deny + + // approval-gate, not hard-deny everywhere. + var policy = CreateProductionPolicy(); + Assert.False(policy.IsDenied("/home/user/.netclaw/config/netclaw.json")); + Assert.False(policy.IsDenied("/home/user/.netclaw/config/devices.json")); + Assert.False(policy.IsDenied("/home/user/.netclaw/config/tool-approvals.json")); + Assert.False(policy.IsDenied("/home/user/.netclaw/config/mcp-oauth-metadata.json")); + } + + [Fact] + public void IsDenied_allows_writes_to_identity_directory() + { + var policy = CreateProductionPolicy(); + Assert.False(policy.IsDenied("/home/user/.netclaw/identity/SOUL.md")); + Assert.False(policy.IsDenied("/home/user/.netclaw/identity/AGENTS.md")); + } + + [Fact] + public void IsDenied_allows_writes_to_skills_directory() + { + var policy = CreateProductionPolicy(); + Assert.False(policy.IsDenied("/home/user/.netclaw/skills/my-skill/SKILL.md")); + } + + [Fact] + public void IsDenied_allows_writes_to_arbitrary_user_paths() + { + var policy = CreateProductionPolicy(); + Assert.False(policy.IsDenied("/tmp/foo.json")); + Assert.False(policy.IsDenied("/home/user/Documents/notes.txt")); + } + + [Fact] + public void IsReadDenied_blocks_secrets_json() + { + var policy = CreateProductionPolicy(); + Assert.True(policy.IsReadDenied("/home/user/.netclaw/config/secrets.json")); + } + + [Fact] + public void IsReadDenied_blocks_keys_directory_children() + { + var policy = CreateProductionPolicy(); + Assert.True(policy.IsReadDenied("/home/user/.netclaw/keys/keyring.xml")); + } + + [Fact] + public void IsReadDenied_blocks_webhook_configs() + { + // Webhook configs embed inline secrets, so read-deny applies. + var policy = CreateProductionPolicy(); + Assert.True(policy.IsReadDenied("/home/user/.netclaw/config/webhooks/github-issues.json")); + } + + [Fact] + public void IsReadDenied_allows_netclaw_json_even_though_write_is_denied_elsewhere() + { + // The asymmetry is the point: netclaw.json may be read for diagnostics + // (the agent can show it to the user) but must not be written silently. + // netclaw.json is not in writeDeny either — it's gated by approval. + var policy = CreateProductionPolicy(); + Assert.False(policy.IsReadDenied("/home/user/.netclaw/config/netclaw.json")); + } + + [Fact] + public void IsReadDenied_allows_netclaw_db_even_though_write_is_hard_denied() + { + // SQLite db: write-denied (corruption is unrecoverable), but reading + // the raw file is not a credential leak — it's binary SQLite anyway. + var policy = CreateProductionPolicy(); + Assert.False(policy.IsReadDenied("/home/user/.netclaw/netclaw.db")); + } + + [Fact] + public void CommandReferencesDeniedPath_still_allows_ls_of_config_directory() + { + // Regression guard for the ShellTool indicator footgun: if the split + // lists ever collapse back into one and ConfigDirectory ends up as a + // substring indicator, every shell command containing ".netclaw/config" + // would be blocked — including legit diagnostics the netclaw-operations + // skill tells agents to run. + var policy = CreateProductionPolicy(); + Assert.False(policy.CommandReferencesDeniedPath("ls ~/.netclaw/config")); + Assert.False(policy.CommandReferencesDeniedPath("stat ~/.netclaw/config")); + } + + [Fact] + public void CommandReferencesDeniedPath_still_blocks_cat_of_secrets_json() + { + var policy = CreateProductionPolicy(); + Assert.True(policy.CommandReferencesDeniedPath("cat ~/.netclaw/config/secrets.json")); + } } diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 2df2dcb25..85394c4f8 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -7,6 +7,16 @@ namespace Netclaw.Security; /// public interface IToolApprovalMatcher { + /// + /// Returns the key used to look up this invocation's approval mode in + /// ToolApprovalConfig.ToolOverrides. Most matchers return the tool + /// name unchanged; argument-aware matchers may return a context-specific + /// key so different invocations of the same tool (e.g., a write to a + /// control-plane file vs. a write to a user file) can be gated + /// independently. + /// + string GetApprovalModeKey(string toolName, IDictionary? arguments); + /// /// Extracts the intent-level pattern from a tool call's arguments. /// For shell: verb-chain prefix (e.g., "git push" from "git push origin main"). @@ -33,6 +43,9 @@ public sealed class ShellApprovalMatcher : IToolApprovalMatcher { public static readonly ShellApprovalMatcher Instance = new(); + public string GetApprovalModeKey(string toolName, IDictionary? arguments) + => toolName; + public IReadOnlyList ExtractPatterns(string toolName, IDictionary? arguments) { var command = GetCommand(arguments); @@ -108,6 +121,9 @@ public sealed class DefaultApprovalMatcher : IToolApprovalMatcher { public static readonly DefaultApprovalMatcher Instance = new(); + public string GetApprovalModeKey(string toolName, IDictionary? arguments) + => toolName; + public IReadOnlyList ExtractPatterns(string toolName, IDictionary? arguments) { return [toolName]; diff --git a/src/Netclaw.Security/ToolPathPolicy.cs b/src/Netclaw.Security/ToolPathPolicy.cs index 7a070ff4c..b524d4fce 100644 --- a/src/Netclaw.Security/ToolPathPolicy.cs +++ b/src/Netclaw.Security/ToolPathPolicy.cs @@ -2,11 +2,32 @@ namespace Netclaw.Security; /// /// Evaluates whether a file path is denied for agent tool access. -/// Used to prevent the LLM from reading/writing sensitive files like secrets.json. +/// Used to prevent the LLM from reading/writing sensitive or control-plane files. /// +/// +/// The policy holds three independent deny lists because the three callers have +/// different risk profiles: +/// +/// writeDeniedPaths — checked by FileWriteTool and +/// FileEditTool via . Intended for control-plane +/// files that must never be mutated by an agent (secrets, keys, SQLite DB, +/// pid/lock files, etc.). +/// readDeniedPaths — checked by FileReadTool via +/// . Narrower: only files that leak credentials +/// (secrets, keys, webhook configs with inline secrets). Read access to +/// non-credential control-plane files like netclaw.json is allowed. +/// shellIndicatorPaths — drives the substring scan in +/// . Must stay narrow (file-level, +/// not directory-level) because the scan does a raw Contains on the +/// command string and over-broad indicators would block legitimate +/// diagnostics like ls ~/.netclaw/config. +/// +/// public sealed class ToolPathPolicy { - private readonly HashSet _deniedPaths; + private readonly HashSet _writeDeniedPaths; + private readonly HashSet _readDeniedPaths; + private readonly HashSet _shellDeniedPaths; private readonly HashSet _commandIndicators; private static readonly HashSet HighRiskVerbs = new(StringComparer.OrdinalIgnoreCase) { @@ -15,43 +36,90 @@ public sealed class ToolPathPolicy "python", "python3", "node", "ruby", "perl", "php" }; + /// + /// Backward-compat single-list constructor. Forwards the same list to all + /// three enforcement surfaces. Existing call sites (and tests) that treat + /// the policy as a flat deny list continue to work. + /// public ToolPathPolicy(IEnumerable deniedPaths) { - var paths = deniedPaths.ToList(); - var normalizedPaths = paths.Select(NormalizePath).ToList(); + var materialized = deniedPaths.ToList(); + _writeDeniedPaths = BuildNormalizedSet(materialized); + _readDeniedPaths = _writeDeniedPaths; + _shellDeniedPaths = _writeDeniedPaths; + _commandIndicators = BuildCommandIndicators(materialized); + } - _deniedPaths = new HashSet(normalizedPaths, StringComparer.OrdinalIgnoreCase); - _commandIndicators = new HashSet(StringComparer.OrdinalIgnoreCase); + /// + /// Three-list constructor. Use when write, read, and shell-indicator deny + /// surfaces need independent scopes — the production path does this because + /// the write-deny list is broader than the others. + /// + public ToolPathPolicy( + IEnumerable writeDeniedPaths, + IEnumerable readDeniedPaths, + IEnumerable shellIndicatorPaths) + { + _writeDeniedPaths = BuildNormalizedSet(writeDeniedPaths); + _readDeniedPaths = BuildNormalizedSet(readDeniedPaths); + var shellList = shellIndicatorPaths.ToList(); + _shellDeniedPaths = BuildNormalizedSet(shellList); + _commandIndicators = BuildCommandIndicators(shellList); + } - foreach (var path in paths.Concat(normalizedPaths)) + private static HashSet BuildNormalizedSet(IEnumerable paths) + { + var normalized = paths.Select(NormalizePath); + return new HashSet(normalized, StringComparer.OrdinalIgnoreCase); + } + + private static HashSet BuildCommandIndicators(IEnumerable paths) + { + var materialized = paths.ToList(); + var normalizedPaths = materialized.Select(NormalizePath).ToList(); + var indicators = new HashSet(StringComparer.OrdinalIgnoreCase); + + foreach (var path in materialized.Concat(normalizedPaths)) { var slashPath = path.Replace('\\', '/'); - _commandIndicators.Add(slashPath); + indicators.Add(slashPath); var netclawSegmentIdx = slashPath.IndexOf("/.netclaw/", StringComparison.OrdinalIgnoreCase); if (netclawSegmentIdx >= 0) - _commandIndicators.Add(slashPath[(netclawSegmentIdx + 1)..]); + indicators.Add(slashPath[(netclawSegmentIdx + 1)..]); var fileName = Path.GetFileName(path); if (!string.IsNullOrWhiteSpace(fileName) && fileName.Contains('.', StringComparison.Ordinal)) - _commandIndicators.Add(fileName); + indicators.Add(fileName); } + + return indicators; } /// - /// Returns true if the given path is denied by policy. + /// Returns true if the given path is denied for write by policy. /// Normalizes the path (resolves "..", removes trailing separators) before checking. /// public bool IsDenied(string path) + => IsDeniedAgainst(path, _writeDeniedPaths); + + /// + /// Returns true if the given path is denied for read by policy. Narrower + /// than : only covers files that leak credentials. + /// + public bool IsReadDenied(string path) + => IsDeniedAgainst(path, _readDeniedPaths); + + private static bool IsDeniedAgainst(string path, HashSet deniedSet) { if (string.IsNullOrWhiteSpace(path)) return false; - if (TryNormalizePath(path, null, out var normalized) && IsDeniedNormalized(normalized)) + if (TryNormalizePath(path, null, out var normalized) && IsDeniedNormalized(normalized, deniedSet)) return true; return TryResolveSymlinkTarget(path, out var resolvedTarget) - && IsDeniedNormalized(resolvedTarget); + && IsDeniedNormalized(resolvedTarget, deniedSet); } /// @@ -79,7 +147,7 @@ public bool CommandReferencesDeniedPath(string command, string? workingDirectory var expanded = ExpandHomeAndEnv(token); if (TryNormalizePath(expanded, workingDirectory, out var normalized) - && IsDeniedNormalized(normalized)) + && IsDeniedNormalized(normalized, _shellDeniedPaths)) { return true; } @@ -117,9 +185,9 @@ private static bool ContainsHighRiskVerb(IEnumerable tokens) return false; } - private bool IsDeniedNormalized(string candidate) + private static bool IsDeniedNormalized(string candidate, HashSet deniedSet) { - foreach (var denied in _deniedPaths) + foreach (var denied in deniedSet) { if (IsSamePathOrChild(candidate, denied)) return true; From 90ff752c26c0ae4bce8557dd5ac3e60ec05f41d2 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 12 Apr 2026 21:44:13 +0000 Subject: [PATCH 2/3] refactor(security): dedupe file tool errors and matcher fail-closed logic Review followups on the control-plane write fix: - Extract duplicated control-plane deny message from FileWriteTool and FileEditTool into FileToolErrors so the copy can't drift. - Move the "does this invocation require fail-closed approval?" decision onto IToolApprovalMatcher (new IsFailClosedOnPersonal method) instead of inspecting approval-mode-key string suffixes from ToolAccessPolicy. The shell fail-closed default now lives in ShellApprovalMatcher where it belongs; FilePathApprovalMatcher answers the same question for control-plane paths without ToolAccessPolicy needing to know its key format. - Drop unused ControlPlanePatternPrefix const and narrative/incident comments that referenced the originating fix rather than the invariant. --- .../Tools/ToolApprovalGateTests.cs | 8 ----- src/Netclaw.Actors/Tools/FileEditTool.cs | 5 +-- .../Tools/FilePathApprovalMatcher.cs | 19 ++++------- src/Netclaw.Actors/Tools/FileReadTool.cs | 3 +- src/Netclaw.Actors/Tools/FileToolErrors.cs | 14 ++++++++ src/Netclaw.Actors/Tools/FileWriteTool.cs | 5 +-- src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 25 +++++--------- src/Netclaw.Daemon/Program.cs | 22 ++++-------- .../ToolPathPolicyTests.cs | 23 +++---------- src/Netclaw.Security/IToolApprovalMatcher.cs | 14 ++++++++ src/Netclaw.Security/ToolPathPolicy.cs | 34 ++++--------------- 11 files changed, 62 insertions(+), 110 deletions(-) create mode 100644 src/Netclaw.Actors/Tools/FileToolErrors.cs diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs index 351d94dbd..a0839d3e4 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs @@ -156,8 +156,6 @@ public void Hard_denied_shell_command_is_blocked_before_approval() Assert.Equal("hard_deny_self_destructive", decision.DenyReason); } - // ── FilePathApprovalMatcher / control-plane approval tests ──────────── - private const string ControlPlaneRoot = "/home/user/.netclaw/config"; private static ToolAccessPolicy CreateFileWritePolicy(ToolApprovalConfig? approvalPolicy = null) @@ -180,8 +178,6 @@ private static ToolAccessPolicy CreateFileWritePolicy(ToolApprovalConfig? approv [Fact] public void file_write_to_netclaw_json_requires_approval_under_fail_closed_default() { - // ApprovalPolicy is null → GetMissingApprovalPolicyDefaultMode fires. - // For Personal + control-plane key, fail-closed default is Approval. var policy = CreateFileWritePolicy(approvalPolicy: null); var args = new Dictionary { @@ -236,8 +232,6 @@ public void file_edit_of_netclaw_json_requires_approval() [Fact] public void file_write_emits_distinct_per_path_patterns() { - // Per-path patterns so approving netclaw.json does not implicitly - // approve tool-approvals.json or devices.json. var matcher = new FilePathApprovalMatcher(ControlPlaneRoot); var netclawJson = matcher.ExtractPatterns("file_write", new Dictionary { ["Path"] = ControlPlaneRoot + "/netclaw.json" }); @@ -254,8 +248,6 @@ public void file_write_emits_distinct_per_path_patterns() [Fact] public void file_write_control_plane_approval_honors_explicit_auto_override() { - // Escape hatch: operator who knows what they're doing can downgrade the - // control-plane key to Auto via ApprovalPolicy.ToolOverrides. var approvalPolicy = new ToolApprovalConfig { ToolOverrides = new Dictionary(StringComparer.Ordinal) diff --git a/src/Netclaw.Actors/Tools/FileEditTool.cs b/src/Netclaw.Actors/Tools/FileEditTool.cs index ff63b5819..5cc07320a 100644 --- a/src/Netclaw.Actors/Tools/FileEditTool.cs +++ b/src/Netclaw.Actors/Tools/FileEditTool.cs @@ -56,10 +56,7 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon return accessError; if (_pathPolicy?.IsDenied(authorizedPath) == true) - return $"Error: Access denied: '{authorizedPath}' is part of Netclaw's control plane " - + "(secrets, keys, database, or lifecycle files) and cannot be modified by agent tools, " - + "even with approval. If the user wants this change, ask them to run a dedicated command " - + "(e.g. `netclaw doctor --fix`, `netclaw secrets set`) or edit the file directly."; + return FileToolErrors.ControlPlaneWriteDenied(authorizedPath); if (!File.Exists(authorizedPath)) return $"Error: File not found: {authorizedPath}"; diff --git a/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs b/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs index d181dbbd4..30c855f2a 100644 --- a/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs +++ b/src/Netclaw.Actors/Tools/FilePathApprovalMatcher.cs @@ -4,23 +4,13 @@ namespace Netclaw.Actors.Tools; /// /// Argument-aware approval matcher for the file_write and file_edit -/// tools. Inspects the target path and, when it lands inside Netclaw's control -/// plane (~/.netclaw/config/), routes the invocation to a distinct -/// approval-mode key so operators can gate control-plane writes without -/// requiring approval for every ordinary file write. +/// tools. Routes writes under a configured control-plane root to a distinct +/// approval-mode key so those invocations can be gated without requiring +/// approval for every ordinary file write. /// -/// -/// Tier split: Tier 1 files (secrets, keys, SQLite DB, pid/lock, restart -/// manifest) are hard-denied earlier by -/// and never reach this matcher. Tier 2 files — the rest of -/// ~/.netclaw/config/ — are routed here and surfaced through the -/// standard approval flow with per-path patterns so approving -/// netclaw.json does not implicitly approve tool-approvals.json. -/// public sealed class FilePathApprovalMatcher : IToolApprovalMatcher { public const string ControlPlaneModeKeySuffix = ":control-plane"; - public const string ControlPlanePatternPrefix = "control-plane:"; private readonly string _controlPlaneRoot; @@ -36,6 +26,9 @@ public string GetApprovalModeKey(string toolName, IDictionary? : toolName; } + public bool IsFailClosedOnPersonal(string toolName, IDictionary? arguments) + => TryGetControlPlaneRelativePath(arguments, out _); + public IReadOnlyList ExtractPatterns(string toolName, IDictionary? arguments) { if (TryGetControlPlaneRelativePath(arguments, out var relativePath)) diff --git a/src/Netclaw.Actors/Tools/FileReadTool.cs b/src/Netclaw.Actors/Tools/FileReadTool.cs index 72aca11b7..f64d905f9 100644 --- a/src/Netclaw.Actors/Tools/FileReadTool.cs +++ b/src/Netclaw.Actors/Tools/FileReadTool.cs @@ -44,8 +44,7 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon return accessError; if (_pathPolicy?.IsReadDenied(authorizedPath) == true) - return $"Error: Access denied: '{authorizedPath}' contains credentials or keys " - + "and cannot be read by agent tools."; + return FileToolErrors.CredentialReadDenied(authorizedPath); if (!File.Exists(authorizedPath)) return $"Error: File not found: {authorizedPath}"; diff --git a/src/Netclaw.Actors/Tools/FileToolErrors.cs b/src/Netclaw.Actors/Tools/FileToolErrors.cs new file mode 100644 index 000000000..fefd8f448 --- /dev/null +++ b/src/Netclaw.Actors/Tools/FileToolErrors.cs @@ -0,0 +1,14 @@ +namespace Netclaw.Actors.Tools; + +internal static class FileToolErrors +{ + public static string ControlPlaneWriteDenied(string path) + => $"Error: Access denied: '{path}' is part of Netclaw's control plane " + + "(secrets, keys, database, or lifecycle files) and cannot be modified by agent tools, " + + "even with approval. If the user wants this change, ask them to run a dedicated command " + + "(e.g. `netclaw doctor --fix`, `netclaw secrets set`) or edit the file directly."; + + public static string CredentialReadDenied(string path) + => $"Error: Access denied: '{path}' contains credentials or keys " + + "and cannot be read by agent tools."; +} diff --git a/src/Netclaw.Actors/Tools/FileWriteTool.cs b/src/Netclaw.Actors/Tools/FileWriteTool.cs index ff6387d5c..4c46cd39b 100644 --- a/src/Netclaw.Actors/Tools/FileWriteTool.cs +++ b/src/Netclaw.Actors/Tools/FileWriteTool.cs @@ -47,10 +47,7 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon return accessError; if (_pathPolicy?.IsDenied(authorizedPath) == true) - return $"Error: Access denied: '{authorizedPath}' is part of Netclaw's control plane " - + "(secrets, keys, database, or lifecycle files) and cannot be modified by agent tools, " - + "even with approval. If the user wants this change, ask them to run a dedicated command " - + "(e.g. `netclaw doctor --fix`, `netclaw secrets set`) or edit the file directly."; + return FileToolErrors.ControlPlaneWriteDenied(authorizedPath); try { diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 5034a8b26..a2a430c67 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -142,7 +142,7 @@ private ToolAccessDecision CheckApprovalGate( var approvalPolicy = profile.ApprovalPolicy; var approvalModeKey = matcher.GetApprovalModeKey(toolName, arguments); var mode = approvalPolicy?.GetEffectiveMode(approvalModeKey) - ?? GetMissingApprovalPolicyDefaultMode(approvalModeKey, audience); + ?? GetMissingApprovalPolicyDefaultMode(toolName, arguments, audience, matcher); if (mode == ToolApprovalMode.Deny) return ToolAccessDecision.Deny("tool_denied_by_approval_policy"); @@ -169,23 +169,14 @@ private ToolAccessDecision CheckApprovalGate( return ToolAccessDecision.RequiresApproval(approvalContext); } - private static ToolApprovalMode GetMissingApprovalPolicyDefaultMode(string approvalModeKey, TrustAudience audience) + private static ToolApprovalMode GetMissingApprovalPolicyDefaultMode( + string toolName, + IDictionary? arguments, + TrustAudience audience, + IToolApprovalMatcher matcher) { - // Fail-closed for personal shell: if the approval policy was omitted, - // still require interactive approval rather than silently auto-approving. - if (audience == TrustAudience.Personal) - { - if (string.Equals(approvalModeKey, ShellTool.ToolName, StringComparison.Ordinal)) - return ToolApprovalMode.Approval; - - // Fail-closed for control-plane file writes: even if the operator did - // not explicitly configure an approval policy, writes into - // ~/.netclaw/config/** require interactive approval. Without this the - // agent can silently edit netclaw.json and trigger a daemon restart - // that drops the current session (see the session-file-blown incident). - if (approvalModeKey.EndsWith(FilePathApprovalMatcher.ControlPlaneModeKeySuffix, StringComparison.Ordinal)) - return ToolApprovalMode.Approval; - } + if (audience == TrustAudience.Personal && matcher.IsFailClosedOnPersonal(toolName, arguments)) + return ToolApprovalMode.Approval; return ToolApprovalMode.Auto; } diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 63e895d02..8d348f9b7 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -551,26 +551,20 @@ static void ConfigureDaemonServices( .Get() ?? new SearchConfig(); var searchBackend = CreateSearchBackend(searchConfig); - // Tool path deny-list: prevent agent tools from touching Netclaw's control plane. - // Three independent surfaces so the write-deny set can be broad (covers the - // files whose edits would corrupt or destabilize the daemon) without letting - // over-broad directory entries bleed into the shell indicator substring scan, - // which would otherwise block legitimate diagnostic commands like - // `ls ~/.netclaw/config`. See ToolPathPolicy remarks. var writeDenyList = new[] { - paths.SecretsPath, // credentials - paths.KeysDirectory, // cryptographic keyring - paths.SqliteDbPath, // memory/session store — corruption is unrecoverable - paths.PidFilePath, // lifecycle files + paths.SecretsPath, + paths.KeysDirectory, + paths.SqliteDbPath, + paths.PidFilePath, paths.LockFilePath, - paths.RestartManifestPath, // cache/restart-manifest.json — agents writing this could spoof restart state + paths.RestartManifestPath, }; var readDenyList = new[] { paths.SecretsPath, paths.KeysDirectory, - paths.WebhooksDirectory, // webhook configs embed inline secrets + paths.WebhooksDirectory, }; var shellIndicatorList = new[] { @@ -584,10 +578,6 @@ static void ConfigureDaemonServices( var shellCommandPolicy = new ShellCommandPolicy(toolConfig.HardDenyPatterns); services.AddSingleton(shellCommandPolicy); - // Argument-aware matcher: routes file_write/file_edit into a control-plane - // approval bucket when the target is under ~/.netclaw/config/ so those - // writes fail-closed to interactive approval even when no explicit policy - // override is set. var fileApprovalMatcher = new FilePathApprovalMatcher(paths.ConfigDirectory); var toolAccessPolicy = new ToolAccessPolicy( toolConfig, diff --git a/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs b/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs index adebbe8bd..5c84a0dd4 100644 --- a/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs +++ b/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs @@ -143,8 +143,6 @@ public void CommandReferencesDeniedPath_detects_high_risk_archive_of_config_dire Assert.True(policy.CommandReferencesDeniedPath("tar czf /tmp/netclaw-config.tgz ~/.netclaw/config")); } - // ── Three-list split tests (production Tier 1 hard-deny) ───────────── - private static ToolPathPolicy CreateProductionPolicy() { var writeDeny = new[] @@ -196,9 +194,6 @@ public void IsDenied_blocks_restart_manifest() [Fact] public void IsDenied_allows_writes_to_netclaw_config_json_so_approval_gate_can_fire() { - // Tier 2: netclaw.json is NOT in the hard-deny set so it reaches the - // approval gate. The incident fix relies on this split — hard-deny + - // approval-gate, not hard-deny everywhere. var policy = CreateProductionPolicy(); Assert.False(policy.IsDenied("/home/user/.netclaw/config/netclaw.json")); Assert.False(policy.IsDenied("/home/user/.netclaw/config/devices.json")); @@ -246,26 +241,20 @@ public void IsReadDenied_blocks_keys_directory_children() [Fact] public void IsReadDenied_blocks_webhook_configs() { - // Webhook configs embed inline secrets, so read-deny applies. var policy = CreateProductionPolicy(); Assert.True(policy.IsReadDenied("/home/user/.netclaw/config/webhooks/github-issues.json")); } [Fact] - public void IsReadDenied_allows_netclaw_json_even_though_write_is_denied_elsewhere() + public void IsReadDenied_allows_netclaw_json() { - // The asymmetry is the point: netclaw.json may be read for diagnostics - // (the agent can show it to the user) but must not be written silently. - // netclaw.json is not in writeDeny either — it's gated by approval. var policy = CreateProductionPolicy(); Assert.False(policy.IsReadDenied("/home/user/.netclaw/config/netclaw.json")); } [Fact] - public void IsReadDenied_allows_netclaw_db_even_though_write_is_hard_denied() + public void IsReadDenied_allows_netclaw_db() { - // SQLite db: write-denied (corruption is unrecoverable), but reading - // the raw file is not a credential leak — it's binary SQLite anyway. var policy = CreateProductionPolicy(); Assert.False(policy.IsReadDenied("/home/user/.netclaw/netclaw.db")); } @@ -273,11 +262,9 @@ public void IsReadDenied_allows_netclaw_db_even_though_write_is_hard_denied() [Fact] public void CommandReferencesDeniedPath_still_allows_ls_of_config_directory() { - // Regression guard for the ShellTool indicator footgun: if the split - // lists ever collapse back into one and ConfigDirectory ends up as a - // substring indicator, every shell command containing ".netclaw/config" - // would be blocked — including legit diagnostics the netclaw-operations - // skill tells agents to run. + // Regression guard: directory-scoped writeDeny entries must not bleed + // into the shell substring indicator set, otherwise every shell command + // whose text contains ".netclaw/config" would be rejected. var policy = CreateProductionPolicy(); Assert.False(policy.CommandReferencesDeniedPath("ls ~/.netclaw/config")); Assert.False(policy.CommandReferencesDeniedPath("stat ~/.netclaw/config")); diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 85394c4f8..17de58c5b 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -17,6 +17,14 @@ public interface IToolApprovalMatcher /// string GetApprovalModeKey(string toolName, IDictionary? arguments); + /// + /// Returns true if this invocation must require interactive approval on + /// the Personal audience when no explicit approval policy is configured. + /// Encapsulates the fail-closed decision so callers do not have to inspect + /// tool names or approval-key string formats. + /// + bool IsFailClosedOnPersonal(string toolName, IDictionary? arguments); + /// /// Extracts the intent-level pattern from a tool call's arguments. /// For shell: verb-chain prefix (e.g., "git push" from "git push origin main"). @@ -46,6 +54,9 @@ public sealed class ShellApprovalMatcher : IToolApprovalMatcher public string GetApprovalModeKey(string toolName, IDictionary? arguments) => toolName; + public bool IsFailClosedOnPersonal(string toolName, IDictionary? arguments) + => true; + public IReadOnlyList ExtractPatterns(string toolName, IDictionary? arguments) { var command = GetCommand(arguments); @@ -124,6 +135,9 @@ public sealed class DefaultApprovalMatcher : IToolApprovalMatcher public string GetApprovalModeKey(string toolName, IDictionary? arguments) => toolName; + public bool IsFailClosedOnPersonal(string toolName, IDictionary? arguments) + => false; + public IReadOnlyList ExtractPatterns(string toolName, IDictionary? arguments) { return [toolName]; diff --git a/src/Netclaw.Security/ToolPathPolicy.cs b/src/Netclaw.Security/ToolPathPolicy.cs index b524d4fce..bbcc780d0 100644 --- a/src/Netclaw.Security/ToolPathPolicy.cs +++ b/src/Netclaw.Security/ToolPathPolicy.cs @@ -2,26 +2,14 @@ namespace Netclaw.Security; /// /// Evaluates whether a file path is denied for agent tool access. -/// Used to prevent the LLM from reading/writing sensitive or control-plane files. /// /// -/// The policy holds three independent deny lists because the three callers have -/// different risk profiles: -/// -/// writeDeniedPaths — checked by FileWriteTool and -/// FileEditTool via . Intended for control-plane -/// files that must never be mutated by an agent (secrets, keys, SQLite DB, -/// pid/lock files, etc.). -/// readDeniedPaths — checked by FileReadTool via -/// . Narrower: only files that leak credentials -/// (secrets, keys, webhook configs with inline secrets). Read access to -/// non-credential control-plane files like netclaw.json is allowed. -/// shellIndicatorPaths — drives the substring scan in -/// . Must stay narrow (file-level, -/// not directory-level) because the scan does a raw Contains on the -/// command string and over-broad indicators would block legitimate -/// diagnostics like ls ~/.netclaw/config. -/// +/// Three independent deny surfaces: write (), read +/// (), and shell indicators +/// (). The shell indicator list must +/// stay narrow — file-level only — because that path does a raw substring scan +/// against the command text, so directory-scoped entries would block legitimate +/// commands whose arguments happen to contain the directory name. /// public sealed class ToolPathPolicy { @@ -36,11 +24,6 @@ public sealed class ToolPathPolicy "python", "python3", "node", "ruby", "perl", "php" }; - /// - /// Backward-compat single-list constructor. Forwards the same list to all - /// three enforcement surfaces. Existing call sites (and tests) that treat - /// the policy as a flat deny list continue to work. - /// public ToolPathPolicy(IEnumerable deniedPaths) { var materialized = deniedPaths.ToList(); @@ -50,11 +33,6 @@ public ToolPathPolicy(IEnumerable deniedPaths) _commandIndicators = BuildCommandIndicators(materialized); } - /// - /// Three-list constructor. Use when write, read, and shell-indicator deny - /// surfaces need independent scopes — the production path does this because - /// the write-deny list is broader than the others. - /// public ToolPathPolicy( IEnumerable writeDeniedPaths, IEnumerable readDeniedPaths, From 7d09c0d54ead28f3a83cd03eb3ddff9e026111ed Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 12 Apr 2026 23:22:19 +0000 Subject: [PATCH 3/3] fix(security): align tool approval composition for control-plane writes Ensure matcher and base-key precedence plus approve-once retries use the same filtered pattern set so approval prompts stay deterministic. Harden shell resource deny coverage and sync/archive the related OpenSpec updates. --- docs/runbooks/tool-approval-gates.md | 30 ++- .../.openspec.yaml | 2 + .../design.md | 119 ++++++++++++ .../proposal.md | 50 +++++ .../specs/tool-approval-gates/spec.md | 182 ++++++++++++++++++ .../tasks.md | 24 +++ openspec/specs/tool-approval-gates/spec.md | 96 +++++++-- .../Tools/DispatchingToolExecutorTests.cs | 164 ++++++++++++++++ .../Tools/ShellToolTests.cs | 19 ++ .../Tools/ToolApprovalGateTests.cs | 74 +++++++ .../Tools/DispatchingToolExecutor.cs | 62 +++--- src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 35 +++- .../Schemas/netclaw-config.v1.schema.json | 2 +- src/Netclaw.Daemon/Program.cs | 4 + .../ToolPathPolicyTests.cs | 15 ++ 15 files changed, 812 insertions(+), 66 deletions(-) create mode 100644 openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/.openspec.yaml create mode 100644 openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/design.md create mode 100644 openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/proposal.md create mode 100644 openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/specs/tool-approval-gates/spec.md create mode 100644 openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/tasks.md diff --git a/docs/runbooks/tool-approval-gates.md b/docs/runbooks/tool-approval-gates.md index 9abea1028..c6cf0b98e 100644 --- a/docs/runbooks/tool-approval-gates.md +++ b/docs/runbooks/tool-approval-gates.md @@ -7,13 +7,16 @@ channel. ## Overview -Tool invocations pass through three layers: - -1. **Hard deny** — commands that are always blocked (e.g., `netclaw daemon stop`, - `rm -rf /`). Never approvable. Checked first. -2. **Tool access** — per-audience allowlists (`AllowedTools`, +Tool invocations pass through four layers: + +1. **Operation hard deny** — shell commands that are always blocked + (e.g., `netclaw daemon stop`, `rm -rf /`). Never approvable. Checked first. +2. **Resource hard deny** — protected files and directories (secrets, keys, + lifecycle/control-plane files) that are blocked for file tools and shell + path references. Never approvable. +3. **Tool access** — per-audience allowlists (`AllowedTools`, `AllowedMcpServers`). Binary: the tool is available or it isn't. -3. **Approval gate** — for tools that pass layers 1 and 2, does this specific +4. **Approval gate** — for tools that pass layers 1-3, does this specific invocation need user sign-off? The approval gate is transparent to the LLM — it never knows approval is @@ -132,8 +135,14 @@ For **compound commands** (`&&`, `||`, `;`, `|`), each segment is checked independently. If any segment is unapproved, all unapproved patterns are batched into one prompt. -For **non-shell tools** (MCP tools, `file_write`, etc.), approval is at the -tool-name level — either the tool is approved or it isn't. +For most **non-shell tools** (MCP tools, `file_read`, etc.), approval is at the +tool-name level. + +For `file_write` and `file_edit`, approval is path-aware for Netclaw +control-plane targets. Writes under the control-plane root use mode keys like +`file_write:control-plane` / `file_edit:control-plane` and persist approvals as +path-scoped patterns (for example, +`file_write:control-plane:netclaw.json`). ### Persistent approvals @@ -165,6 +174,11 @@ mode: The hard deny check runs even in `Auto` mode (no approval configured). +In addition to command hard deny, Netclaw enforces path hard deny for protected +resources (for example `secrets.json`, key material, webhook secrets, and +control-plane lifecycle files). Those accesses are blocked for file tools and +for shell commands that reference those paths. + ### Custom hard deny patterns Add patterns via `HardDenyPatterns` in `netclaw.json`: diff --git a/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/.openspec.yaml b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/.openspec.yaml new file mode 100644 index 000000000..33d01f784 --- /dev/null +++ b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-04-12 diff --git a/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/design.md b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/design.md new file mode 100644 index 000000000..8bd4b289c --- /dev/null +++ b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/design.md @@ -0,0 +1,119 @@ +## Context + +`tool-approval-gates` added approval interception across `ToolAccessPolicy`, +`DispatchingToolExecutor`, and session approval retry handling. Follow-on +testing exposed composition edge cases: + +- File mutation gating now uses argument-aware matcher keys for control-plane + paths, but key resolution needs explicit precedence when both path-specific + and base tool overrides exist. +- Approve-once currently depends on one-time context state and matcher patterns; + retry matching must align with the filtered unapproved set returned by + `IToolApprovalService`, not the pre-filter candidate set. +- Shell deny checks are split between operation-level command hard deny and + resource-level path denial; precedence and user-visible deny semantics need a + single contract. + +The implementation spans `Netclaw.Actors.Tools`, `Netclaw.Security`, and +session retry flow state. Actor boundary remains unchanged: session actor owns +approval decisions and temporary one-time retry grants. + +## Goals / Non-Goals + +**Goals:** + +- Make approval mode resolution deterministic when matcher-derived keys and base + tool keys both exist. +- Ensure approve-once retry acceptance checks use the same filtered unapproved + pattern set shown in the interaction prompt. +- Define shell hard-deny composition semantics between operation hard-deny and + resource hard-deny, including precedence for deny reasons. +- Add regression scenarios to the capability spec so future refactors preserve + these compositions. + +**Non-Goals:** + +- Redesign of approval UI/options or interaction protocol. +- New persistence model for approvals. +- Changes to trust audiences, grant categories, or Slack channel UX. + +## Decisions + +### Decision 1: Approval mode key precedence uses most-specific to least-specific + +**Choice:** Resolve approval mode in this order: + +1. Matcher-derived key override (for example `file_write:control-plane`) +2. Base tool key override (`file_write`) +3. Matcher fail-closed behavior for Personal audience +4. Audience `DefaultMode` + +**Alternatives considered:** + +- Matcher key only with no base fallback: rejected because adding path-specific + matchers unintentionally bypasses existing tool-level policy intent. +- Base key first: rejected because it prevents finer-grained overrides from + taking effect. + +**Rationale:** This preserves backward compatibility for existing overrides while +letting operators tighten high-risk subsets without broadening unrelated calls. + +### Decision 2: Approve-once matching is evaluated after unapproved filtering + +**Choice:** For approval-gated calls, first compute unapproved patterns via +`IToolApprovalService`; then evaluate one-time retry bypass against that filtered +set. + +**Alternatives considered:** + +- Check one-time bypass against pre-filter matcher patterns: rejected because it + can reprompt even when the user just approved the exact prompt set. +- Persist approve-once to shared approval service: rejected because it breaks + one-shot scope guarantees. + +**Rationale:** Prompt set and retry set must be identical to avoid UX/security +drift. One-time state remains in-memory and call-retry scoped. + +### Decision 3: Shell deny composition is fail-closed with operation precedence + +**Choice:** Shell invocation remains denied if either operation hard-deny +(`ShellCommandPolicy`) or resource hard-deny (`ToolPathPolicy`) matches. +Operation hard-deny is evaluated first; if it matches, resource checks are not +consulted for the result reason. + +**Alternatives considered:** + +- Resource deny first: rejected because known self-destructive operations should + short-circuit early and return stable hard-deny categorization. +- Merge both policies into one matcher: rejected for now to keep policy modules + independently testable. + +**Rationale:** This keeps a strict deny floor while preserving diagnosable, +deterministic denial reasons. + +## Risks / Trade-offs + +- **[Risk] Key fallback could broaden approval-gated scope unexpectedly** -> + Mitigation: explicit spec scenarios for matcher-key override and base-key + fallback; add policy tests for both branches. +- **[Risk] Retry-path reordering may miss existing one-time checks** -> + Mitigation: add executor and pipeline tests that assert no reprompt on the + immediate retry but prompt on later calls. +- **[Risk] Deny precedence can hide secondary violations** -> Mitigation: + preserve first-deny reason in user result and audit log while keeping + independent tests for both operation and resource deny paths. + +## Migration Plan + +1. Update approval-mode resolution logic and unit tests. +2. Update executor retry matching order to use filtered unapproved patterns. +3. Update shell policy composition checks and denial reason assertions. +4. Update OpenSpec delta scenarios and run targeted test suites. + +Rollback is straightforward: revert this change set to restore prior composition +behavior. + +## Open Questions + +- None for this scope; behavior is constrained to composition clarifications and + regression coverage. diff --git a/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/proposal.md b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/proposal.md new file mode 100644 index 000000000..048c527f8 --- /dev/null +++ b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/proposal.md @@ -0,0 +1,50 @@ +## Why + +Tool approval behavior in the shipped pipeline has three composition gaps that can +produce surprising security outcomes: control-plane file mutations can miss +intended approval overrides, approve-once retries can reprompt because matching +uses pre-filter patterns, and shell policy layering between operation-level and +resource-level hard denies is under-specified. This follow-up closes those gaps +to keep approval gates deterministic and auditable under PRD-002 security +constraints. + +## What Changes + +- Define deterministic approval-mode key precedence for matcher-derived keys + (for example `file_write:control-plane`) versus base tool keys (`file_write`) + and default mode fallback. +- Align approve-once retry matching with the same filtered unapproved pattern + set that was shown to the user in the prompt, including path-aware matcher + patterns for control-plane file mutations. +- Clarify shell policy composition so operation hard-deny and resource hard-deny + are both enforced with explicit precedence and denial reasons. +- Add targeted behavior scenarios in the capability spec for key precedence, + approve-once retry behavior, and shell deny composition. +- In scope: tool approval composition and requirement/test updates in the + existing approval capability. +- Out of scope: new approval UX options, non-tool interaction types, sandbox + shell implementation, and broad ACL model redesign. + +## Capabilities + +### New Capabilities + +- None. + +### Modified Capabilities + +- `tool-approval-gates`: Refine approval key resolution precedence, + approve-once retry matching semantics, and shell hard-deny composition + semantics; extend normative scenarios for these behaviors. + +## Impact + +- **Security / policy surface**: `ToolAccessPolicy`, `DispatchingToolExecutor`, + matcher implementations, and shell deny checks gain explicit composition + rules (PRD-002: SEC-003, SEC-006, SEC-009). +- **Behavioral consistency**: Approval prompts and immediate retries use the + same pattern identity set, reducing false reprompts. +- **Operational clarity**: Deny reason precedence is documented for actor logs, + tool audit entries, and troubleshooting. +- **Validation impact**: Update capability scenarios and matching tests in + approval gate and executor suites; no config schema changes expected. diff --git a/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/specs/tool-approval-gates/spec.md b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/specs/tool-approval-gates/spec.md new file mode 100644 index 000000000..13573677a --- /dev/null +++ b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/specs/tool-approval-gates/spec.md @@ -0,0 +1,182 @@ +## MODIFIED Requirements + +### Requirement: Tool approval configuration per audience + +The system SHALL support per-audience tool approval configuration via +`ToolApprovalConfig` on `ToolAudienceProfile`. Each audience profile SHALL +independently specify a `DefaultMode` (Auto, Approval, Deny) and per-tool +overrides in `ToolOverrides`. + +Approval mode resolution SHALL use deterministic precedence: + +1. Matcher-derived approval-mode key override (for example + `file_write:control-plane`) +2. Base tool key override (for example `file_write`) +3. Matcher fail-closed behavior for Personal audience +4. Audience `DefaultMode` + +Runtime audience defaults SHALL NOT implicitly place `shell_execute` in +`Approval` mode. Instead, the init-generated Personal config SHALL explicitly +write `ApprovalPolicy.ToolOverrides.shell_execute = Approval` as the +recommended shell-safe default. + +#### Scenario: Shell requires approval in init-generated Personal config + +- **GIVEN** a Personal audience session whose generated config explicitly sets + `ApprovalPolicy.ToolOverrides.shell_execute` to `Approval` +- **WHEN** the agent invokes `shell_execute` +- **THEN** the system checks the approval cache before execution +- **AND** if the command pattern is not approved, an approval prompt is emitted + +#### Scenario: Tool in Auto mode executes without approval + +- **GIVEN** a tool whose approval mode is `Auto` for the session's audience +- **WHEN** the agent invokes the tool +- **THEN** the tool executes immediately without an approval prompt + +#### Scenario: Tool in Deny mode is always blocked + +- **GIVEN** a tool whose approval mode is `Deny` for the session's audience +- **WHEN** the agent invokes the tool +- **THEN** the tool is denied with reason `tool_denied_by_approval_policy` +- **AND** no approval prompt is offered + +#### Scenario: Per-audience independence + +- **GIVEN** Personal sets `shell_execute` to `Approval` and Team sets it to `Deny` +- **WHEN** a Personal session invokes `shell_execute` +- **THEN** the system checks approval cache and may prompt +- **AND** when a Team session invokes `shell_execute` +- **THEN** the system denies immediately without prompting + +#### Scenario: Matcher-specific override key takes precedence over base tool key + +- **GIVEN** `ApprovalPolicy.ToolOverrides.file_write = Auto` +- **AND** `ApprovalPolicy.ToolOverrides.file_write:control-plane = Approval` +- **WHEN** the agent invokes `file_write` targeting a control-plane path +- **THEN** the resolved mode is `Approval` +- **AND** the call is approval-gated unless already approved for that path pattern + +#### Scenario: Base tool key applies when matcher-specific key is absent + +- **GIVEN** `ApprovalPolicy.ToolOverrides.file_write = Approval` +- **AND** no override exists for `file_write:control-plane` +- **WHEN** the agent invokes `file_write` targeting a control-plane path +- **THEN** the resolved mode is `Approval` +- **AND** mode resolution does NOT fall directly to `DefaultMode` + +### Requirement: Configurable hard deny list + +The system SHALL enforce shell hard-deny composition across both +operation-level hard deny and resource-level hard deny: + +- **Operation hard-deny**: command intent patterns evaluated by + `ShellCommandPolicy` (for example self-destructive/system-destructive verbs) +- **Resource hard-deny**: protected-path checks evaluated by `ToolPathPolicy` + +Shell execution SHALL be denied when either hard-deny layer matches. Operation +hard-deny SHALL be evaluated first and SHALL short-circuit result reason when it +matches. Denied commands SHALL never be approvable. The system SHALL ship with +sensible defaults: commands that kill the Netclaw daemon process, `rm -rf /`, +`rm -rf ~/`, and fork bombs. Operators SHALL be able to add or remove operation +hard-deny patterns via configuration. + +#### Scenario: Hard-denied command blocked before approval + +- **GIVEN** a command matching the hard deny list (e.g., `netclaw daemon stop`) +- **WHEN** the agent invokes `shell_execute` with that command +- **THEN** the command is denied with reason `hard_deny_self_destructive` +- **AND** no approval prompt is offered +- **AND** the denial is logged + +#### Scenario: Hard deny enforced even in HostAllowed mode + +- **GIVEN** `ShellMode` is `HostAllowed` (no approval config) +- **WHEN** the agent runs a hard-denied command +- **THEN** the command is still blocked + +#### Scenario: Operator adds custom hard deny pattern + +- **GIVEN** the operator adds `docker rm` to the hard deny list in config +- **WHEN** the agent runs `docker rm my-container` +- **THEN** the command is denied + +#### Scenario: Compound command with hard-denied segment + +- **GIVEN** a compound command `git add . && netclaw daemon stop` +- **WHEN** the agent invokes `shell_execute` +- **THEN** the entire command is denied because one segment matches hard deny + +#### Scenario: Operation hard-deny reason takes precedence over resource deny + +- **GIVEN** a shell command matches both operation and resource deny checks +- **WHEN** the command is evaluated +- **THEN** operation hard-deny is applied first +- **AND** the surfaced deny reason is the operation hard-deny reason + +#### Scenario: Resource hard-deny blocks when operation hard-deny does not match + +- **GIVEN** a shell command does not match operation hard-deny patterns +- **AND** the command references a protected file path +- **WHEN** the command is executed +- **THEN** execution is denied by resource hard-deny + +### Requirement: Persistent approval storage + +The system SHALL store persistent approvals ("Approve Always" decisions) in +`~/.netclaw/config/tool-approvals.json`, separate from `netclaw.json`. The file +SHALL NOT be monitored by `ConfigWatcherService`. The file SHALL contain +per-audience sections with per-tool approval lists. For shell, the lists SHALL +contain command patterns. For other tools, approval SHALL be tool-level or +matcher-pattern-level as defined by that matcher. The file SHALL be read at +startup and written immediately on "Approve Always" decisions. + +The retry path for "Approve Once" SHALL match against the filtered unapproved +pattern set presented in the approval prompt, not against pre-filter pattern +candidates. + +#### Scenario: Approve Always persists to file + +- **GIVEN** the user clicks "Approve Always" for pattern `git push` +- **WHEN** the approval is processed +- **THEN** `git push` is added to the Personal shell_execute list in + `tool-approvals.json` +- **AND** the daemon does NOT restart + +#### Scenario: Persistent approvals loaded at startup + +- **GIVEN** `tool-approvals.json` contains `{"personal":{"shell_execute":["git push"]}}` +- **WHEN** the daemon starts +- **THEN** `git push` is pre-approved for Personal audience shell commands + +#### Scenario: Approve Once is retry-scoped only + +- **GIVEN** the user clicks "Approve Once" for pattern `docker build` +- **WHEN** the approval is processed +- **THEN** the blocked `docker build` call is retried immediately +- **AND** a later `docker build` call in the same session prompts again +- **AND** `tool-approvals.json` is NOT modified + +#### Scenario: Approve Once retry uses filtered unapproved patterns + +- **GIVEN** a command yields candidate patterns where some are already approved +- **AND** the prompt shows only filtered unapproved patterns +- **WHEN** the user selects "Approve Once" +- **THEN** the immediate retry succeeds without a second prompt for that call +- **AND** the one-time bypass checks only the filtered unapproved set + +#### Scenario: Approve Once for control-plane file path is path-scoped + +- **GIVEN** `file_write` on `.netclaw/tooling/AGENTS.md` prompts with matcher + pattern `file_write:control-plane:.netclaw/tooling/AGENTS.md` +- **WHEN** the user selects "Approve Once" +- **THEN** the blocked retry for that same path proceeds without reprompt +- **AND** a subsequent control-plane write to a different path prompts again +- **AND** no persistent approval file entry is written + +#### Scenario: Approve For This Chat is session-scoped only + +- **GIVEN** the user clicks "Approve For This Chat" for pattern `docker build` +- **WHEN** the approval is processed +- **THEN** `docker build` is approved for the current session only +- **AND** `tool-approvals.json` is NOT modified diff --git a/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/tasks.md b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/tasks.md new file mode 100644 index 000000000..d3b2bcb94 --- /dev/null +++ b/openspec/changes/archive/2026-04-12-tool-approval-composition-fixes/tasks.md @@ -0,0 +1,24 @@ +## 1. Approval Mode Resolution Precedence + +- [x] 1.1 Update `ToolAccessPolicy.ResolveApprovalMode` to resolve matcher-derived override key before base tool key and then fall back to fail-closed/default mode +- [x] 1.2 Add policy tests for control-plane file mutation precedence (`file_write:control-plane` override beats `file_write`) +- [x] 1.3 Add policy tests for base-key fallback when matcher-specific override is absent + +## 2. Approve-Once Retry Matching Alignment + +- [x] 2.1 Update `DispatchingToolExecutor` approval flow so one-time bypass checks run against the filtered unapproved pattern set returned by `IToolApprovalService` +- [x] 2.2 Preserve approve-once scope to immediate retry only (no persistent writes, no broader session cache) +- [x] 2.3 Add executor and pipeline tests verifying approve-once does not reprompt on immediate retry but prompts on a later invocation +- [x] 2.4 Add path-aware tests for control-plane file mutation approve-once matching (same path bypass, different path reprompt) + +## 3. Shell Hard-Deny Composition + +- [x] 3.1 Codify shell deny composition order (operation hard-deny before resource hard-deny) with explicit deny reason precedence +- [x] 3.2 Add tests for operation-first precedence when both deny categories match +- [x] 3.3 Add tests for resource hard-deny when operation hard-deny does not match + +## 4. Spec and Validation Sync + +- [x] 4.1 Update `openspec/changes/tool-approval-composition-fixes/specs/tool-approval-gates/spec.md` scenarios as implemented +- [x] 4.2 Run targeted test suites for tool approval policy/executor/pipeline and shell deny behavior +- [x] 4.3 Run `dotnet slopwatch analyze` and address any new violations diff --git a/openspec/specs/tool-approval-gates/spec.md b/openspec/specs/tool-approval-gates/spec.md index fc19c9f3c..2c9f26f9f 100644 --- a/openspec/specs/tool-approval-gates/spec.md +++ b/openspec/specs/tool-approval-gates/spec.md @@ -16,12 +16,20 @@ approval capability. The system SHALL support per-audience tool approval configuration via `ToolApprovalConfig` on `ToolAudienceProfile`. Each audience profile SHALL independently specify a `DefaultMode` (Auto, Approval, Deny) and per-tool -overrides in `ToolOverrides`. The default `DefaultMode` SHALL be `Auto` (no -approval required). Runtime audience defaults SHALL NOT implicitly place -`shell_execute` in `Approval` mode. Instead, the init-generated Personal config -SHALL explicitly write -`ApprovalPolicy.ToolOverrides.shell_execute = Approval` as the recommended -shell-safe default. +overrides in `ToolOverrides`. + +Approval mode resolution SHALL use deterministic precedence: + +1. Matcher-derived approval-mode key override (for example + `file_write:control-plane`) +2. Base tool key override (for example `file_write`) +3. Matcher fail-closed behavior for Personal audience +4. Audience `DefaultMode` + +Runtime audience defaults SHALL NOT implicitly place `shell_execute` in +`Approval` mode. Instead, the init-generated Personal config SHALL explicitly +write `ApprovalPolicy.ToolOverrides.shell_execute = Approval` as the +recommended shell-safe default. #### Scenario: Shell requires approval in init-generated Personal config @@ -52,13 +60,37 @@ shell-safe default. - **AND** when a Team session invokes `shell_execute` - **THEN** the system denies immediately without prompting +#### Scenario: Matcher-specific override key takes precedence over base tool key + +- **GIVEN** `ApprovalPolicy.ToolOverrides.file_write = Auto` +- **AND** `ApprovalPolicy.ToolOverrides.file_write:control-plane = Approval` +- **WHEN** the agent invokes `file_write` targeting a control-plane path +- **THEN** the resolved mode is `Approval` +- **AND** the call is approval-gated unless already approved for that path pattern + +#### Scenario: Base tool key applies when matcher-specific key is absent + +- **GIVEN** `ApprovalPolicy.ToolOverrides.file_write = Approval` +- **AND** no override exists for `file_write:control-plane` +- **WHEN** the agent invokes `file_write` targeting a control-plane path +- **THEN** the resolved mode is `Approval` +- **AND** mode resolution does NOT fall directly to `DefaultMode` + ### Requirement: Configurable hard deny list -The system SHALL enforce a configurable hard deny list of command patterns that -are blocked before the approval gate is consulted. Denied commands SHALL never -be approvable. The system SHALL ship with sensible defaults: commands that kill -the Netclaw daemon process, `rm -rf /`, `rm -rf ~/`, and fork bombs. Operators -SHALL be able to add or remove patterns via configuration. +The system SHALL enforce shell hard-deny composition across both +operation-level hard deny and resource-level hard deny: + +- **Operation hard-deny**: command intent patterns evaluated by + `ShellCommandPolicy` (for example self-destructive/system-destructive verbs) +- **Resource hard-deny**: protected-path checks evaluated by `ToolPathPolicy` + +Shell execution SHALL be denied when either hard-deny layer matches. Operation +hard-deny SHALL be evaluated first and SHALL short-circuit result reason when it +matches. Denied commands SHALL never be approvable. The system SHALL ship with +sensible defaults: commands that kill the Netclaw daemon process, `rm -rf /`, +`rm -rf ~/`, and fork bombs. Operators SHALL be able to add or remove operation +hard-deny patterns via configuration. #### Scenario: Hard-denied command blocked before approval @@ -86,6 +118,20 @@ SHALL be able to add or remove patterns via configuration. - **WHEN** the agent invokes `shell_execute` - **THEN** the entire command is denied because one segment matches hard deny +#### Scenario: Operation hard-deny reason takes precedence over resource deny + +- **GIVEN** a shell command matches both operation and resource deny checks +- **WHEN** the command is evaluated +- **THEN** operation hard-deny is applied first +- **AND** the surfaced deny reason is the operation hard-deny reason + +#### Scenario: Resource hard-deny blocks when operation hard-deny does not match + +- **GIVEN** a shell command does not match operation hard-deny patterns +- **AND** the command references a protected file path +- **WHEN** the command is executed +- **THEN** execution is denied by resource hard-deny + ### Requirement: Shell command pattern matching The system SHALL extract verb-chain prefix patterns from shell commands using @@ -226,9 +272,13 @@ The system SHALL store persistent approvals ("Approve Always" decisions) in `~/.netclaw/config/tool-approvals.json`, separate from `netclaw.json`. The file SHALL NOT be monitored by `ConfigWatcherService`. The file SHALL contain per-audience sections with per-tool approval lists. For shell, the lists SHALL -contain command patterns. For other tools, approval SHALL be tool-level -(`true`). The file SHALL be read at startup and written immediately on "Approve -Always" decisions. +contain command patterns. For other tools, approval SHALL be tool-level or +matcher-pattern-level as defined by that matcher. The file SHALL be read at +startup and written immediately on "Approve Always" decisions. + +The retry path for "Approve Once" SHALL match against the filtered unapproved +pattern set presented in the approval prompt, not against pre-filter pattern +candidates. #### Scenario: Approve Always persists to file @@ -252,13 +302,29 @@ Always" decisions. - **AND** a later `docker build` call in the same session prompts again - **AND** `tool-approvals.json` is NOT modified +#### Scenario: Approve Once retry uses filtered unapproved patterns + +- **GIVEN** a command yields candidate patterns where some are already approved +- **AND** the prompt shows only filtered unapproved patterns +- **WHEN** the user selects "Approve Once" +- **THEN** the immediate retry succeeds without a second prompt for that call +- **AND** the one-time bypass checks only the filtered unapproved set + +#### Scenario: Approve Once for control-plane file path is path-scoped + +- **GIVEN** `file_write` on `.netclaw/tooling/AGENTS.md` prompts with matcher + pattern `file_write:control-plane:.netclaw/tooling/AGENTS.md` +- **WHEN** the user selects "Approve Once" +- **THEN** the blocked retry for that same path proceeds without reprompt +- **AND** a subsequent control-plane write to a different path prompts again +- **AND** no persistent approval file entry is written + #### Scenario: Approve For This Chat is session-scoped only - **GIVEN** the user clicks "Approve For This Chat" for pattern `docker build` - **WHEN** the approval is processed - **THEN** `docker build` is approved for the current session only - **AND** `tool-approvals.json` is NOT modified -- **AND** a new session will prompt for `docker build` again ### Requirement: Channel approval capability diff --git a/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs b/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs index 28cc91811..a75cdb782 100644 --- a/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs @@ -501,6 +501,170 @@ public async Task One_time_approval_bypasses_policy_for_matching_shell_patterns( Assert.Contains("bypass", retryResult, StringComparison.OrdinalIgnoreCase); } + [Fact] + public async Task One_time_approval_bypasses_policy_for_path_aware_file_patterns() + { + var controlPlaneRoot = Path.Combine(Path.GetTempPath(), $"netclaw-control-plane-{Guid.NewGuid():N}"); + var targetPath = Path.Combine(controlPlaneRoot, "netclaw.json"); + var secondPath = Path.Combine(controlPlaneRoot, "devices.json"); + Directory.CreateDirectory(controlPlaneRoot); + + try + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + + var registry = new ToolRegistry(); + registry.WithFirstPartyTools(config); + + var executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + fileApprovalMatcher: new FilePathApprovalMatcher(controlPlaneRoot))); + + var toolCall = new FunctionCallContent( + "call-file-approve-once-bypass", + "file_write", + new Dictionary + { + ["Path"] = targetPath, + ["Content"] = "approved once" + }); + + var context = new Netclaw.Tools.ToolExecutionContext("signalr/thread-1", null) + { + Audience = TrustAudience.Personal.ToWireValue(), + Boundary = SecurityPolicyDefaults.TrustedInstanceBoundary, + ChannelType = "signalr", + SupportsInteractiveApproval = true + }; + + var firstAttempt = await Assert.ThrowsAsync(() => + executor.ExecuteAsync(toolCall, context, TestContext.Current.CancellationToken)); + + context.OneTimeApprovedToolName = toolCall.Name; + context.SetOneTimeApprovedPatterns(firstAttempt.ApprovalContext.UnapprovedPatterns); + + var retryResult = await executor.ExecuteAsync(toolCall, context, TestContext.Current.CancellationToken); + Assert.Contains("Successfully wrote", retryResult, StringComparison.Ordinal); + Assert.True(File.Exists(targetPath)); + + var secondCall = new FunctionCallContent( + "call-file-approve-once-bypass-second", + "file_write", + new Dictionary + { + ["Path"] = secondPath, + ["Content"] = "different path" + }); + + await Assert.ThrowsAsync(() => + executor.ExecuteAsync(secondCall, context, TestContext.Current.CancellationToken)); + + context.OneTimeApprovedToolName = null; + context.SetOneTimeApprovedPatterns([]); + + await Assert.ThrowsAsync(() => + executor.ExecuteAsync(toolCall, context, TestContext.Current.CancellationToken)); + } + finally + { + if (Directory.Exists(controlPlaneRoot)) + Directory.Delete(controlPlaneRoot, recursive: true); + } + } + + [Fact] + public async Task One_time_approval_uses_filtered_unapproved_patterns_on_retry() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + + var registry = new ToolRegistry(); + registry.WithFirstPartyTools(config); + + var system = ActorSystem.Create($"tool-approval-filtered-once-{Guid.NewGuid():N}"); + try + { + var approvalActor = system.ActorOf(ToolApprovalActor.CreateProps(), "tool-approval"); + var approvalService = new AkkaToolApprovalService(new StubRequiredActor(approvalActor)); + var executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false)), + approvalService); + + var context = new Netclaw.Tools.ToolExecutionContext("signalr/thread-filtered", null) + { + Audience = TrustAudience.Personal.ToWireValue(), + Boundary = SecurityPolicyDefaults.TrustedInstanceBoundary, + ChannelType = "signalr", + SupportsInteractiveApproval = true + }; + + await approvalService.RecordApprovalAsync( + "signalr/thread-filtered", + TrustAudience.Personal, + "shell_execute", + ["pwd"], + persistent: false, + TestContext.Current.CancellationToken); + + var call = new FunctionCallContent( + "call-filtered-once", + "shell_execute", + new Dictionary + { + ["Command"] = "pwd && ls" + }); + + var firstAttempt = await Assert.ThrowsAsync(() => + executor.ExecuteAsync(call, context, TestContext.Current.CancellationToken)); + + Assert.DoesNotContain("pwd", firstAttempt.ApprovalContext.UnapprovedPatterns); + Assert.Contains("ls", firstAttempt.ApprovalContext.UnapprovedPatterns); + + context.OneTimeApprovedToolName = call.Name; + context.SetOneTimeApprovedPatterns(firstAttempt.ApprovalContext.UnapprovedPatterns); + + var retryResult = await executor.ExecuteAsync(call, context, TestContext.Current.CancellationToken); + Assert.Contains("Exit code: 0", retryResult, StringComparison.Ordinal); + + context.OneTimeApprovedToolName = null; + context.SetOneTimeApprovedPatterns([]); + + await Assert.ThrowsAsync(() => + executor.ExecuteAsync(call, context, TestContext.Current.CancellationToken)); + } + finally + { + await system.Terminate(); + } + } + [Fact] public async Task Session_approval_allows_same_session_but_not_different_session() { diff --git a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs index aff1ce1ae..ac8ca1ca3 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs @@ -203,4 +203,23 @@ public async Task Hard_deny_checked_before_path_policy() Assert.Contains("hard deny policy", result); Assert.DoesNotContain("protected file path", result); } + + [Fact] + public async Task Path_policy_still_blocks_sensitive_paths_when_command_is_not_hard_denied() + { + var commandPolicy = new ShellCommandPolicy(); + var pathPolicy = new ToolPathPolicy(["/home/user/.netclaw/config/secrets.json"]); + var tool = new ShellTool(new ToolConfig(), pathPolicy, commandPolicy); + + var args = new Dictionary + { + ["Command"] = "cat /home/user/.netclaw/config/secrets.json" + }; + + var result = await tool.ExecuteAsync(args, CancellationToken.None); + + Assert.DoesNotContain("hard deny policy", result); + Assert.Contains("protected file path", result); + Assert.Contains("Access denied", result); + } } diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs index a0839d3e4..377678c03 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs @@ -194,6 +194,32 @@ public void file_write_to_netclaw_json_requires_approval_under_fail_closed_defau p => p.StartsWith("file_write:control-plane:", StringComparison.Ordinal)); } + [Fact] + public void file_write_to_control_plane_still_requires_approval_when_policy_exists_without_override() + { + var approvalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + var policy = CreateFileWritePolicy(approvalPolicy); + var args = new Dictionary + { + ["Path"] = ControlPlaneRoot + "/netclaw.json", + ["Content"] = "{}" + }; + + var decision = policy.AuthorizeInvocation(FileWriteToolInstance(), PersonalContext(), args); + + Assert.True(decision.NeedsApproval); + Assert.NotNull(decision.ApprovalContext); + Assert.Contains( + decision.ApprovalContext!.UnapprovedPatterns, + p => p.StartsWith("file_write:control-plane:", StringComparison.Ordinal)); + } + [Fact] public void file_write_to_non_control_plane_path_auto_approves_under_null_policy() { @@ -267,4 +293,52 @@ public void file_write_control_plane_approval_honors_explicit_auto_override() Assert.True(decision.Allowed); Assert.False(decision.NeedsApproval); } + + [Fact] + public void file_write_control_plane_override_takes_precedence_over_base_tool_override() + { + var approvalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["file_write"] = ToolApprovalMode.Auto, + ["file_write:control-plane"] = ToolApprovalMode.Approval + } + }; + var policy = CreateFileWritePolicy(approvalPolicy); + var args = new Dictionary + { + ["Path"] = ControlPlaneRoot + "/netclaw.json", + ["Content"] = "{}" + }; + + var decision = policy.AuthorizeInvocation(FileWriteToolInstance(), PersonalContext(), args); + + Assert.True(decision.NeedsApproval); + Assert.NotNull(decision.ApprovalContext); + } + + [Fact] + public void file_write_control_plane_falls_back_to_base_tool_override_when_specific_override_missing() + { + var approvalPolicy = new ToolApprovalConfig + { + DefaultMode = ToolApprovalMode.Approval, + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["file_write"] = ToolApprovalMode.Auto + } + }; + var policy = CreateFileWritePolicy(approvalPolicy); + var args = new Dictionary + { + ["Path"] = ControlPlaneRoot + "/netclaw.json", + ["Content"] = "{}" + }; + + var decision = policy.AuthorizeInvocation(FileWriteToolInstance(), PersonalContext(), args); + + Assert.True(decision.Allowed); + Assert.False(decision.NeedsApproval); + } } diff --git a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs index 4a0d56d21..248e50c78 100644 --- a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs +++ b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs @@ -53,39 +53,6 @@ public async Task ExecuteAsync(FunctionCallContent toolCall, ToolExecuti return $"Unknown tool: {toolCall.Name}"; } - if (context is not null - && string.Equals(context.OneTimeApprovedToolName, toolCall.Name, StringComparison.Ordinal) - && IsOneTimeApprovalSatisfied(context, toolCall)) - { - _logger.LogInformation( - "Applying one-time approval bypass for tool {ToolName} in session {SessionId}", - toolCall.Name, - context.SessionId ?? "unknown"); - - var swBypass = Stopwatch.StartNew(); - try - { - var bypassResult = await tool.ExecuteAsync(toolCall.Arguments, context, ct); - swBypass.Stop(); - _logger.LogInformation( - "Tool executed via one-time approval bypass: {ToolName} ({Duration}ms, {ResultLength} chars)", - toolCall.Name, - swBypass.ElapsedMilliseconds, - bypassResult.Length); - return bypassResult; - } - catch (Exception ex) - { - swBypass.Stop(); - _logger.LogError( - ex, - "Tool execution failed via one-time approval bypass: {ToolName} ({Duration}ms)", - toolCall.Name, - swBypass.ElapsedMilliseconds); - throw; - } - } - var accessDecision = _policy.AuthorizeInvocation(tool, context, toolCall.Arguments); if (accessDecision.NeedsApproval && _approvalService is not null) @@ -116,6 +83,17 @@ public async Task ExecuteAsync(FunctionCallContent toolCall, ToolExecuti } } + if (accessDecision.NeedsApproval + && context is not null + && IsOneTimeApprovalSatisfied(context, toolCall, accessDecision.ApprovalContext)) + { + _logger.LogInformation( + "Applying one-time approval bypass for tool {ToolName} in session {SessionId}", + toolCall.Name, + context.SessionId ?? "unknown"); + accessDecision = ToolAccessDecision.Allow(); + } + if (accessDecision.NeedsApproval) { _logger.LogInformation("Tool requires approval: {ToolName}", toolCall.Name); @@ -152,19 +130,23 @@ public async Task ExecuteAsync(FunctionCallContent toolCall, ToolExecuti } } - private static bool IsOneTimeApprovalSatisfied(ToolExecutionContext context, FunctionCallContent toolCall) + private static bool IsOneTimeApprovalSatisfied( + ToolExecutionContext context, + FunctionCallContent toolCall, + ToolApprovalContext? approvalContext) { + if (approvalContext is null) + return false; + if (context.OneTimeApprovedPatterns.Count == 0) return false; - if (!string.Equals(toolCall.Name, "shell_execute", StringComparison.Ordinal)) - return context.OneTimeApprovedPatterns.Contains(toolCall.Name); + if (approvalContext.UnapprovedPatterns.Count == 0) + return false; - var matcher = ShellApprovalMatcher.Instance; - var commandPatterns = matcher.ExtractPatterns(toolCall.Name, toolCall.Arguments); - if (commandPatterns.Count == 0) + if (!string.Equals(context.OneTimeApprovedToolName, toolCall.Name, StringComparison.Ordinal)) return false; - return commandPatterns.All(pattern => context.OneTimeApprovedPatterns.Contains(pattern)); + return approvalContext.UnapprovedPatterns.All(pattern => context.OneTimeApprovedPatterns.Contains(pattern)); } } diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index a2a430c67..19cb9c852 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -141,8 +141,13 @@ private ToolAccessDecision CheckApprovalGate( var profile = ToolAudienceProfileDefaults.GetResolvedProfile(_toolConfig.AudienceProfiles, audience); var approvalPolicy = profile.ApprovalPolicy; var approvalModeKey = matcher.GetApprovalModeKey(toolName, arguments); - var mode = approvalPolicy?.GetEffectiveMode(approvalModeKey) - ?? GetMissingApprovalPolicyDefaultMode(toolName, arguments, audience, matcher); + var mode = ResolveApprovalMode( + approvalPolicy, + approvalModeKey, + toolName, + arguments, + audience, + matcher); if (mode == ToolApprovalMode.Deny) return ToolAccessDecision.Deny("tool_denied_by_approval_policy"); @@ -181,6 +186,32 @@ private static ToolApprovalMode GetMissingApprovalPolicyDefaultMode( return ToolApprovalMode.Auto; } + private static ToolApprovalMode ResolveApprovalMode( + ToolApprovalConfig? approvalPolicy, + string approvalModeKey, + string toolName, + IDictionary? arguments, + TrustAudience audience, + IToolApprovalMatcher matcher) + { + if (approvalPolicy is null) + return GetMissingApprovalPolicyDefaultMode(toolName, arguments, audience, matcher); + + if (approvalPolicy.ToolOverrides.TryGetValue(approvalModeKey, out var mode)) + return mode; + + if (!string.Equals(approvalModeKey, toolName, StringComparison.Ordinal) + && approvalPolicy.ToolOverrides.TryGetValue(toolName, out mode)) + { + return mode; + } + + if (audience == TrustAudience.Personal && matcher.IsFailClosedOnPersonal(toolName, arguments)) + return ToolApprovalMode.Approval; + + return approvalPolicy.DefaultMode; + } + private IToolApprovalMatcher SelectMatcherForTool(string toolName) { if (string.Equals(toolName, FileWriteTool.ToolName, StringComparison.Ordinal) diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index c9a4950fd..09c151a57 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -608,7 +608,7 @@ }, "ToolOverrides": { "type": "object", - "description": "Per-tool approval mode overrides. Keys are tool names (e.g., shell_execute, mcp:server:tool).", + "description": "Per-tool approval mode overrides. Keys are tool names (e.g., shell_execute, mcp:server:tool) or matcher-specific keys such as file_write:control-plane and file_edit:control-plane.", "additionalProperties": { "type": "string", "enum": ["Auto", "Approval", "Deny"] diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 8d348f9b7..ade69c6ce 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -571,6 +571,10 @@ static void ConfigureDaemonServices( paths.SecretsPath, paths.WebhooksDirectory, paths.KeysDirectory, + paths.SqliteDbPath, + paths.PidFilePath, + paths.LockFilePath, + paths.RestartManifestPath, }; var toolPathPolicy = new ToolPathPolicy(writeDenyList, readDenyList, shellIndicatorList); services.AddSingleton(toolPathPolicy); diff --git a/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs b/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs index 5c84a0dd4..975843a7e 100644 --- a/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs +++ b/src/Netclaw.Security.Tests/ToolPathPolicyTests.cs @@ -165,6 +165,10 @@ private static ToolPathPolicy CreateProductionPolicy() "/home/user/.netclaw/config/secrets.json", "/home/user/.netclaw/config/webhooks", "/home/user/.netclaw/keys", + "/home/user/.netclaw/netclaw.db", + "/home/user/.netclaw/netclaw.pid", + "/home/user/.netclaw/netclaw.lock", + "/home/user/.netclaw/cache/restart-manifest.json", }; return new ToolPathPolicy(writeDeny, readDeny, shellIndicators); } @@ -276,4 +280,15 @@ public void CommandReferencesDeniedPath_still_blocks_cat_of_secrets_json() var policy = CreateProductionPolicy(); Assert.True(policy.CommandReferencesDeniedPath("cat ~/.netclaw/config/secrets.json")); } + + [Fact] + public void CommandReferencesDeniedPath_blocks_control_plane_lifecycle_files() + { + var policy = CreateProductionPolicy(); + + Assert.True(policy.CommandReferencesDeniedPath("cat ~/.netclaw/netclaw.db")); + Assert.True(policy.CommandReferencesDeniedPath("cat ~/.netclaw/netclaw.pid")); + Assert.True(policy.CommandReferencesDeniedPath("cat ~/.netclaw/netclaw.lock")); + Assert.True(policy.CommandReferencesDeniedPath("cat ~/.netclaw/cache/restart-manifest.json")); + } }