Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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(Path.Join("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