diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index 304ddd8d3..552b11939 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -390,8 +390,12 @@ public static class ShellApprovalCases Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), ExpectedApproval.Allow(ToolAllowReason.StoredApproval, 1, "persistent:rm")), Case( + // Use an isolated temp subdirectory as the covering directory, not + // the shared system temp root: a symlink child there (e.g. an IDE + // socket) trips ContainsSymlinkEntry and fails the glob closed, + // which is correct behavior but not what this case exercises. "external-glob-does-not-reuse-project-grant", - Bash($"rm {TemporaryFile("*.bak")}"), + Bash($"rm {TemporaryFile("netclaw-ext-glob/*.bak")}"), Approvals.PersistentHere(ApprovalDirectoryShape.Project, "rm"), ExpectedApproval.Require(["rm"])), Case( @@ -404,6 +408,30 @@ public static class ShellApprovalCases Bash("cat artifacts/*/secret.txt"), Approvals.PersistentAnywhere("cat"), ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), + // Directory-listing idiom `foo/*/`: a trailing slash filters the glob to + // directories but stays a direct-child scope, so it is NOT a "complex + // command". Inside the trusted tree a read-only safe verb auto-allows + // (silent, no prompt) exactly like the leaf glob `ls *.txt`. + Case( + "directory-listing-glob-in-project-auto-allows", + Bash("ls -d subdirs/*/"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + // Outside the trusted tree the same command prompts — but now with a + // persistent grant scoped to the covering directory, not one-shot only. + // This is the reported regression (0.25.3 flipped it to complex-command). + Case( + "directory-listing-glob-external-offers-persistent-grant", + Bash("ls -d subdirs/*/", ApprovalDirectoryShape.External), + Approvals.None, + ExpectedApproval.Require(["ls"], isMessy: false)), + // The exact reported command: the pipe folds into one approval unit and + // the directory glob no longer forces the whole pipeline one-shot. + Case( + "directory-listing-glob-pipeline-offers-persistent-grant", + Bash("ls -d subdirs/*/ | xargs -n1 basename", ApprovalDirectoryShape.External), + Approvals.None, + ExpectedApproval.Require(["ls", "xargs"], isMessy: false)), 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 d2ffae79a..67552a9df 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 @@ -43,9 +43,12 @@ | 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 *.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 | +| external-glob-does-not-reuse-project-grant | Personal | Project | Interactive | rm {TempPath}netclaw-ext-glob/*.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 | +| directory-listing-glob-in-project-auto-allows | Personal | Project | Interactive | ls -d subdirs/*/ | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| directory-listing-glob-external-offers-persistent-grant | Personal | External | Interactive | ls -d subdirs/*/ | none | RequiresApproval | approval required | ls | No | +| directory-listing-glob-pipeline-offers-persistent-grant | Personal | External | Interactive | ls -d subdirs/*/ \| xargs -n1 basename | none | RequiresApproval | approval required | ls, xargs | 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 28db0f9e1..5d672be33 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -606,6 +606,35 @@ public sealed class ShellApprovalMatcherPathExtractionTests { "cat artifacts/\\.*", ".leak" } }; + /// + /// Directory-listing globs: a trailing slash restricts the wildcard to + /// directories (foo/*/) but adds no descendant path segment — every + /// match is still a direct child of the covering directory foo. These + /// MUST resolve to that covering directory and stay persistable, exactly like + /// the leaf glob foo/*. Regression for the 0.25.3 change that swept the + /// directory-listing idiom into the one-shot-only "complex command" bucket + /// (the ls -d .../immovlan/*/ | xargs -n1 basename report). + /// + public static TheoryData DirectoryOnlyTrailingSlashGlobCases => new() + { + { "ls -d artifacts/*/", "artifacts" }, + { "ls artifacts/*/", "artifacts" }, + { "ls -d workspaces/immovlan/*/", "workspaces/immovlan" } + }; + + /// + /// A trailing slash relaxes ONLY the directory-listing case (foo/*/). + /// A glob with a real path segment after the wildcard still hides the matched + /// segment's identity — a symlink or traversal the covering directory cannot + /// bound — so it MUST stay one-shot even when it also ends in a slash. Guards + /// the fix against over-reaching past a single trailing slash. + /// + public static TheoryData TrailingSlashWithRealSegmentStaysMessyCases => new() + { + { "cat artifacts/*/deeper/" }, + { "ls artifacts/*/*/" } + }; + /// /// xunit.v3 SkipUnless hook for POSIX-only tests. The v2 /// matcher falls through to the legacy ShellTokenizer path @@ -723,6 +752,82 @@ public void Leaf_glob_in_directory_with_symlink_fails_closed(string command, str } } + [SlopwatchSuppress("SW001", "This theory verifies Bash directory-glob scopes, which do not apply to the Windows shell parser.")] + [Theory(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + [MemberData(nameof(DirectoryOnlyTrailingSlashGlobCases))] + public void ExtractCandidates_trailing_slash_directory_glob_resolves_covering_directory( + string command, + string expectedRelativeScope) + { + // The directory-listing idiom `foo/*/` must scope to the covering + // directory `foo` and stay persistable — not degrade to a one-shot + // "complex command". Currently fails (the trailing slash trips the + // descendant-scope guard); passes once `foo/*/` normalizes to `foo/*`. + var projectDirectory = Path.Combine( + Path.GetTempPath(), + $"netclaw-trailing-slash-glob-{Guid.NewGuid():N}"); + var expectedDirectory = expectedRelativeScope + .Split('/') + .Aggregate(projectDirectory, Path.Combine); + var arguments = Args(command, projectDirectory); + + var candidate = Assert.Single( + _matcher.ExtractCandidates(new ToolName("shell_execute"), arguments)); + + Assert.Equal("ls", candidate.Verb); + Assert.Equal(expectedDirectory, candidate.Directory); + Assert.False(_matcher.IsMessy(new ToolName("shell_execute"), arguments)); + } + + [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(TrailingSlashWithRealSegmentStaysMessyCases))] + public void Trailing_slash_does_not_rescue_real_descendant_segment(string command) + { + // A trailing slash after a real intermediate segment (`foo/*/deeper/`) + // or a second wildcard (`foo/*/*/`) must NOT be mistaken for the benign + // directory-listing case — the matched segment is still unbounded, so + // these stay one-shot both before and after the fix. + var projectDirectory = Path.Combine( + Path.GetTempPath(), + $"netclaw-trailing-descendant-{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 glob behavior, which does not apply to the Windows shell parser.")] + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void Trailing_slash_directory_glob_with_symlink_child_fails_closed() + { + // `foo/*/` reduces to the covering directory `foo`. The symlink scan of + // `foo` must still fail the command closed — the trailing-slash + // relaxation must not remove the symlink protection a leaf glob already + // enforces. A fix that skips the covering-directory scan for `foo/*/` + // would surface a candidate here and flip IsMessy to false. + var root = Path.Combine(Path.GetTempPath(), $"netclaw-trailing-symlink-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var artifactsDirectory = Path.Combine(projectDirectory, "artifacts"); + var externalDirectory = Path.Combine(root, "external"); + var link = Path.Combine(artifactsDirectory, "escape"); + Directory.CreateDirectory(artifactsDirectory); + Directory.CreateDirectory(externalDirectory); + Directory.CreateSymbolicLink(link, externalDirectory); + + try + { + var arguments = Args("ls -d artifacts/*/", 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/ShellCommandAnalysis.cs b/src/Netclaw.Security/ShellCommandAnalysis.cs index 38e2a4092..7ca2fdd17 100644 --- a/src/Netclaw.Security/ShellCommandAnalysis.cs +++ b/src/Netclaw.Security/ShellCommandAnalysis.cs @@ -163,9 +163,17 @@ public static bool HasUnresolvedDescendantScope(Arg arg) if (!arg.IsPath || arg.Kind != ArgKind.Glob) return false; - var firstGlob = arg.Raw.IndexOfAny(['*', '?', '[']); + // A trailing slash is a directory-only type filter (foo/*/), not a + // descendant path segment: every match is still a direct child of the + // covering directory, exactly like the leaf glob foo/*. Strip it before + // the scan so the directory-listing idiom keeps a fixed, persistable + // scope instead of degrading to a one-shot "complex command". A real + // segment after the wildcard (foo/*/x, foo/*/*) keeps its separator and + // stays unresolved. + var scope = arg.Raw.TrimEnd('/'); + var firstGlob = scope.IndexOfAny(['*', '?', '[']); return firstGlob >= 0 - && arg.Raw.IndexOf('/', firstGlob + 1) >= 0; + && scope.IndexOf('/', firstGlob + 1) >= 0; } }