Skip to content
Merged
30 changes: 29 additions & 1 deletion src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
105 changes: 105 additions & 0 deletions src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,35 @@ public sealed class ShellApprovalMatcherPathExtractionTests
{ "cat artifacts/\\.*", ".leak" }
};

/// <summary>
/// Directory-listing globs: a trailing slash restricts the wildcard to
/// directories (<c>foo/*/</c>) but adds no descendant path segment — every
/// match is still a direct child of the covering directory <c>foo</c>. These
/// MUST resolve to that covering directory and stay persistable, exactly like
/// the leaf glob <c>foo/*</c>. Regression for the 0.25.3 change that swept the
/// directory-listing idiom into the one-shot-only "complex command" bucket
/// (the <c>ls -d .../immovlan/*/ | xargs -n1 basename</c> report).
/// </summary>
public static TheoryData<string, string> DirectoryOnlyTrailingSlashGlobCases => new()
{
{ "ls -d artifacts/*/", "artifacts" },
{ "ls artifacts/*/", "artifacts" },
{ "ls -d workspaces/immovlan/*/", "workspaces/immovlan" }
};

/// <summary>
/// A trailing slash relaxes ONLY the directory-listing case (<c>foo/*/</c>).
/// 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.
/// </summary>
public static TheoryData<string> TrailingSlashWithRealSegmentStaysMessyCases => new()
{
{ "cat artifacts/*/deeper/" },
{ "ls artifacts/*/*/" }
};

/// <summary>
/// xunit.v3 <c>SkipUnless</c> hook for POSIX-only tests. The v2
/// matcher falls through to the legacy <c>ShellTokenizer</c> path
Expand Down Expand Up @@ -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()
Expand Down
12 changes: 10 additions & 2 deletions src/Netclaw.Security/ShellCommandAnalysis.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
Loading