From c646a5033081c8c11d771ce5beb9d4b58531f80f Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 7 Aug 2026 18:09:25 +0000 Subject: [PATCH 1/3] test(approvals): reproduce phantom approval directory scopes (#1795) Two regression tests that fail on current dev and demonstrate #1795: - echo text containing a URL query '?' is classified as a glob token and gains a covering directory derived from its static prefix (cwd/---try) - a bare numeric operand (head -c 2000) is treated as a path arg and resolves relative to cwd (cwd/2000) Both cases assert Directory == null, matching how the same verbs behave with ordinary operands. No production fix in this change. --- .../ShellApprovalMatcherTests.cs | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 28db0f9e1..fa628440f 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -1141,6 +1141,52 @@ public void ExtractCandidates_uses_redirect_target_and_invocation_working_direct candidate.Verb == "echo" && candidate.Directory == workingDirectory); } + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void ExtractCandidates_echo_text_question_mark_is_not_a_glob_scope() + { + // Regression for #1795: `echo "---try /stats?format=json---"` contains + // a `?`, which the parser classifies as a Glob token. The matcher then + // derives a covering directory from the static prefix (`---try`) — + // but the `?` is URL query syntax inside echo text, not a glob + // pattern, and `---try` is not a real directory. echo is a + // stdout-only side-effect verb, so the candidate must carry + // Directory == null (matching `echo "done"`). The phantom scope is + // what inflated the approval header to "Approve in 2 directories?". + var candidates = _matcher.ExtractCandidates( + new ToolName("shell_execute"), + new Dictionary + { + ["Command"] = "echo \"---try /stats?format=json---\"", + ["WorkingDirectory"] = "/home/user/repos/demo" + }); + + var echoCandidate = Assert.Single(candidates); + Assert.Equal("echo", echoCandidate.Verb); + Assert.Null(echoCandidate.Directory); + } + + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void ExtractCandidates_bare_numeric_operand_is_not_a_path_scope() + { + // Regression for #1795: `head -c 2000` treats the bare numeric + // operand `2000` as a path arg (slash-free token branch of + // IsAuthorizationPathArg) and resolves it relative to cwd, producing + // the phantom scope `/cwd/2000`. `head -c 2000` touches no filesystem + // path; head is a read-only verb with no path argument, so its + // candidate must carry Directory == null (like `git status`). + var candidates = _matcher.ExtractCandidates( + new ToolName("shell_execute"), + new Dictionary + { + ["Command"] = "head -c 2000", + ["WorkingDirectory"] = "/home/user/repos/demo" + }); + + var headCandidate = Assert.Single(candidates); + Assert.Equal("head", headCandidate.Verb); + Assert.Null(headCandidate.Directory); + } + [Fact] public void IsApproved_treats_side_effect_candidates_as_authorized() { From aaf5f7cb995a154ba36477eb8c9ae452af7abf41 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 00:29:03 +0000 Subject: [PATCH 2/3] fix(approvals): stop fabricating directory scopes for side-effect verbs and numeric operands (#1795) The shell approval matcher made phantom directory scopes from two token shapes. Each phantom scope inflated the approval header to "Approve in N directories?" and forced a re-prompt. The root cause is a ShellSyntaxTree 0.2 misclassification: - `echo "---try /stats?format=json---"` holds a `?`. The parser tags the text operand as a glob arg. The matcher then makes a covering directory from the static prefix (`---try`). - `head -c 2000` holds a bare number. The parser tags `2000` as a path arg. The matcher then resolves it to `/cwd/2000`. This change adds two narrow guards in `IToolApprovalMatcher`: - A pure side-effect verb (echo, printf, :, true, false) derives no arg-based directory scope. It writes only to stdout. A redirect still gives a real scope, so `echo x > dir/file` keeps `dir`. - A token of only ASCII digits with no `/` is a numeric value, not a path arg. The guard stays narrow: `2000.txt`, `file1234`, and `1234/x` pass. Both guards only remove a false scope. Neither grants, widens, or hides authorization. ShellSyntaxTree v0.3.0 fixes the misclassification in the parser. A comment on each guard marks it for removal then. Adds five ExtractCandidates coverage cases for the two shapes. --- .../ShellApprovalMatcherTests.cs | 57 +++++++++++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 54 ++++++++++++------ 2 files changed, 95 insertions(+), 16 deletions(-) diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index fa628440f..7f1560746 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -1187,6 +1187,63 @@ public void ExtractCandidates_bare_numeric_operand_is_not_a_path_scope() Assert.Null(headCandidate.Directory); } + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void ExtractCandidates_printf_format_operand_is_not_a_path_scope() + { + // Regression for #1795: printf is a stdout-only side-effect verb. Its + // format string and value operands are literal text, not paths. The + // candidate must carry Directory == null. + var candidates = _matcher.ExtractCandidates( + new ToolName("shell_execute"), + new Dictionary + { + ["Command"] = "printf \"%d\" 5", + ["WorkingDirectory"] = "/home/user/repos/demo" + }); + + var printfCandidate = Assert.Single(candidates); + Assert.Equal("printf", printfCandidate.Verb); + Assert.Null(printfCandidate.Directory); + } + + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void ExtractCandidates_echo_glob_char_text_is_not_a_scope() + { + // Regression for #1795: `echo "a?b"` contains a `?`, which the parser + // classifies as a Glob token. echo is a side-effect verb, so no + // arg-derived scope forms. The candidate must carry Directory == null. + var candidates = _matcher.ExtractCandidates( + new ToolName("shell_execute"), + new Dictionary + { + ["Command"] = "echo \"a?b\"", + ["WorkingDirectory"] = "/home/user/repos/demo" + }); + + var echoCandidate = Assert.Single(candidates); + Assert.Equal("echo", echoCandidate.Verb); + Assert.Null(echoCandidate.Directory); + } + + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void ExtractCandidates_bare_numeric_flag_value_is_not_a_path_scope() + { + // Regression for #1795: `head -n 20` has a bare numeric operand `20`. + // A number is not a path, so no scope forms. The candidate must carry + // Directory == null. + var candidates = _matcher.ExtractCandidates( + new ToolName("shell_execute"), + new Dictionary + { + ["Command"] = "head -n 20", + ["WorkingDirectory"] = "/home/user/repos/demo" + }); + + var headCandidate = Assert.Single(candidates); + Assert.Equal("head", headCandidate.Verb); + Assert.Null(headCandidate.Directory); + } + [Fact] public void IsApproved_treats_side_effect_candidates_as_authorized() { diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index e1490adff..89998edfd 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -269,27 +269,39 @@ private static IReadOnlyList ExtractCandidatesViaBashAnalysis // 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) + // + // Temporary containment for #1795. ShellSyntaxTree 0.2 tags an echo or + // printf text operand as a path arg or a glob arg. The matcher then + // makes a phantom directory scope from literal text. A pure side-effect + // verb writes only to stdout. Its operands are never filesystem paths. + // So this verb derives no arg-based scope. A redirect still gives a real + // scope below. This code only removes a false scope. It never grants a + // new one. ShellSyntaxTree v0.3.0 fixes the misclassification in the + // parser. Remove this guard then. + if (!isSideEffectVerb) { - if (arg.IsCwdAttribution || !IsAuthorizationPathArg(arg, clauseWorkingDirectory)) - continue; - - if (arg.Kind == ShellSyntaxTree.ArgKind.Glob) + foreach (var arg in clause.Args) { - var coveringDirectory = ResolveGlobCoveringDirectory(arg, clauseWorkingDirectory); - if (coveringDirectory is null) - return null; + if (arg.IsCwdAttribution || !IsAuthorizationPathArg(arg, clauseWorkingDirectory)) + continue; - directories.Add(coveringDirectory); - continue; - } + if (arg.Kind == ShellSyntaxTree.ArgKind.Glob) + { + var coveringDirectory = ResolveGlobCoveringDirectory(arg, clauseWorkingDirectory); + if (coveringDirectory is null) + return null; - // 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; + 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)) + return null; - directories.Add(ShellTokenizer.ApplyFileParentRule(arg.Resolved)); + directories.Add(ShellTokenizer.ApplyFileParentRule(arg.Resolved)); + } } foreach (var redirect in clause.Redirects) @@ -395,6 +407,16 @@ private static bool IsAuthorizationPathArg( if (!arg.IsPath) return false; + // Temporary containment for #1795. ShellSyntaxTree 0.2 tags a bare + // numeric operand such as `head -c 2000` as a path arg. The matcher + // then makes a phantom scope `/cwd/2000`. A token of only ASCII digits + // with no `/` is a numeric value, not a path. Reject it. This guard is + // narrow: `2000.txt`, `file1234`, and `1234/x` still pass. This code + // only removes a false scope. It never grants a new one. ShellSyntaxTree + // v0.3.0 classifies the operand in the parser. Remove this guard then. + if (arg.Raw.Length > 0 && arg.Raw.All(char.IsAsciiDigit)) + return false; + if (ShellTokenizer.IsPathToken(arg.Raw) || !arg.Raw.Contains('/', StringComparison.Ordinal)) return true; From 8afdb500fc8347fe7cfad9a4efb27cd872722bb0 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 00:48:28 +0000 Subject: [PATCH 3/3] fix(approvals): gate the numeric operand guard on filesystem existence (#1795) The first #1795 numeric guard was purely syntactic. It dropped any all-digit token as a numeric value. It could not tell the value in `head -c 2000` from a real filesystem object named `2000`. This let an attacker escalate a prompt into an auto-approve. `cat 2000` where `2000` is a symlink out of the granted tree, plus a folder grant `(cat, )`, dropped the path scope. The candidate collapsed to the cwd. The symlink-segment check in MatchesShellApproval never ran. The read auto-approved with no prompt. The fix gates the guard on filesystem existence: - Drop the all-digit token ONLY when no entry exists at its resolved path. - A real file, directory, or symlink named `2000` stays a path arg, so its scope and the symlink-segment check survive. - SAFE-FAIL: if the path is unknown or the probe throws, keep the token as a path. Never fail toward auto-approve. `head -c 2000` with no file named `2000` still drops to Directory == null, so the prompt-spam fix holds. `cat 2000` on a symlink keeps its scope, so the read prompts again. Adds two regression tests: the escaping-symlink attack asserts IsApproved is false, and a real in-tree directory named `2000` asserts the folder grant still covers it. --- .../ShellApprovalMatcherTests.cs | 66 +++++++++++++++++++ src/Netclaw.Security/IToolApprovalMatcher.cs | 65 ++++++++++++++++-- 2 files changed, 127 insertions(+), 4 deletions(-) diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 7f1560746..6bb9c73a4 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -1244,6 +1244,72 @@ public void ExtractCandidates_bare_numeric_flag_value_is_not_a_path_scope() Assert.Null(headCandidate.Directory); } + [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 IsApproved_numeric_token_that_names_an_escaping_symlink_still_prompts() + { + // Security regression for the #1795 numeric guard. `cat 2000` where + // `2000` is a symlink out of the granted tree must NOT auto-approve. + // The guard drops a numeric operand only when no filesystem entry + // exists at its path. A real symlink named `2000` stays a path arg, so + // its scope survives and the symlink-segment check in + // MatchesShellApproval refuses the folder grant. A purely syntactic + // guard would drop `2000`, collapse the scope to the cwd, skip the + // symlink check, and auto-approve a read outside the tree. + var root = Path.Combine(Path.GetTempPath(), $"netclaw-numeric-symlink-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var externalDirectory = Path.Combine(root, "external"); + var externalSecret = Path.Combine(externalDirectory, "secret.txt"); + var link = Path.Combine(projectDirectory, "2000"); + Directory.CreateDirectory(projectDirectory); + Directory.CreateDirectory(externalDirectory); + File.WriteAllText(externalSecret, "secret"); + File.CreateSymbolicLink(link, externalSecret); + + try + { + var approved = new[] { new ApprovalEntry("cat") { Directory = projectDirectory } }; + Assert.False(_matcher.IsApproved( + new ToolName("shell_execute"), + Args("cat 2000", projectDirectory), + approved, + cwd: projectDirectory)); + } + finally + { + File.Delete(link); + Directory.Delete(root, recursive: true); + } + } + + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only path semantics")] + public void IsApproved_numeric_token_that_names_a_real_in_tree_directory_is_covered_by_grant() + { + // Complement to the symlink case. `cat 2000` where `2000` is a real + // directory inside the granted tree keeps its scope and is covered by + // the folder grant. This proves the existence gate does not over-block + // a legitimate in-tree entry, and that the numeric token stays a path + // when a real object exists. + var root = Path.Combine(Path.GetTempPath(), $"netclaw-numeric-dir-{Guid.NewGuid():N}"); + var projectDirectory = Path.Combine(root, "project"); + var numericDirectory = Path.Combine(projectDirectory, "2000"); + Directory.CreateDirectory(numericDirectory); + + try + { + var approved = new[] { new ApprovalEntry("cat") { Directory = projectDirectory } }; + Assert.True(_matcher.IsApproved( + new ToolName("shell_execute"), + Args("cat 2000", projectDirectory), + approved, + cwd: projectDirectory)); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + [Fact] public void IsApproved_treats_side_effect_candidates_as_authorized() { diff --git a/src/Netclaw.Security/IToolApprovalMatcher.cs b/src/Netclaw.Security/IToolApprovalMatcher.cs index 89998edfd..a2894a6ca 100644 --- a/src/Netclaw.Security/IToolApprovalMatcher.cs +++ b/src/Netclaw.Security/IToolApprovalMatcher.cs @@ -410,12 +410,24 @@ private static bool IsAuthorizationPathArg( // Temporary containment for #1795. ShellSyntaxTree 0.2 tags a bare // numeric operand such as `head -c 2000` as a path arg. The matcher // then makes a phantom scope `/cwd/2000`. A token of only ASCII digits - // with no `/` is a numeric value, not a path. Reject it. This guard is - // narrow: `2000.txt`, `file1234`, and `1234/x` still pass. This code - // only removes a false scope. It never grants a new one. ShellSyntaxTree + // is normally a numeric value, not a path. + // + // Drop the token ONLY when no filesystem entry exists at its resolved + // path. A real file, directory, or symlink named `2000` stays a path + // arg, so its directory scope and the later symlink-segment check + // survive. Without this existence gate, `cat 2000` where `2000` is a + // symlink out of the granted tree would collapse to the cwd and skip + // the symlink check — a prompt-to-auto-approve escalation on the ACL + // perimeter. ShouldDropNumericToken safe-fails: it keeps the arg as a + // path on any doubt, so the gate prompts. This guard stays narrow: + // `2000.txt`, `file1234`, and `1234/x` still pass. This code only + // removes a false scope. It never grants a new one. ShellSyntaxTree // v0.3.0 classifies the operand in the parser. Remove this guard then. - if (arg.Raw.Length > 0 && arg.Raw.All(char.IsAsciiDigit)) + if (arg.Raw.Length > 0 && arg.Raw.All(char.IsAsciiDigit) + && ShouldDropNumericToken(arg, workingDirectory)) + { return false; + } if (ShellTokenizer.IsPathToken(arg.Raw) || !arg.Raw.Contains('/', StringComparison.Ordinal)) return true; @@ -450,6 +462,51 @@ or NotSupportedException } } + /// + /// Decides if an all-digit operand is a plain numeric value and not a real + /// filesystem object. This is temporary containment for #1795. It returns + /// true only when the matcher can prove no entry exists at the + /// resolved path. On any doubt it returns false, so the caller keeps + /// the token as a path and the gate prompts. It never fails toward an + /// automatic grant. + /// + private static bool ShouldDropNumericToken(ShellSyntaxTree.Arg arg, string? workingDirectory) + { + try + { + var token = string.IsNullOrWhiteSpace(arg.Resolved) ? arg.Raw : arg.Resolved; + + string path; + if (Path.IsPathRooted(token)) + { + path = token; + } + else if (!string.IsNullOrWhiteSpace(workingDirectory)) + { + path = Path.Combine(workingDirectory, token); + } + else + { + // No absolute path to probe. Keep the token as a path. + return false; + } + + // File.Exists and Directory.Exists follow a symlink to its target. + // A real symlink to an outside path returns true here, so the token + // stays a path and keeps its symlink-segment check downstream. + return !File.Exists(path) && !Directory.Exists(path); + } + catch (Exception ex) when (ex is ArgumentException + or IOException + or NotSupportedException + or UnauthorizedAccessException + or System.Security.SecurityException) + { + // The probe failed. Keep the token as a path so the gate prompts. + return false; + } + } + private static string? ResolveRedirectDirectory(ShellSyntaxTree.Redirect redirect) { if (string.IsNullOrWhiteSpace(redirect.Target)