diff --git a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs index 90b069744..387a490a7 100644 --- a/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs +++ b/src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs @@ -1333,6 +1333,175 @@ 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(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); + } + + [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 2d33d7e1b..02b36245e 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,28 @@ 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 + // 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) + && ShouldDropNumericToken(arg, workingDirectory)) + { + return false; + } + if (ShellTokenizer.IsPathToken(arg.Raw) || !arg.Raw.Contains('/', StringComparison.Ordinal)) return true; @@ -428,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)