From d8d4b33fd25b269d3deb98f0741f1ad6ddc663dc Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 6 Aug 2026 02:53:48 +0000 Subject: [PATCH 1/2] fix(approvals): stop treating fd-dup redirects (2>&1) as dynamic syntax (#1780) fd-dup targets (&1, &2, &-) are static file descriptors, not dynamic tokens. ShellSyntaxTree marks them IsDynamicSkip to mean 'do not path-resolve', but HasDynamicSyntax read that as unresolved syntax and failed the whole command closed to an approval prompt. Exclude &-prefixed redirect targets from the dynamic check. Adds regression cases: safe 2>&1 verbs auto-approve, mutating 2>&1 prompts without messy classification, and dynamic redirects (> "$OUTPUT") stay fail-closed. --- .../Tools/ShellApprovalCaseCatalog.cs | 15 +++++++++++++++ ..._approval_cases_match_review_table.verified.md | 5 ++++- src/Netclaw.Security/ShellCommandAnalysis.cs | 9 ++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index bc4c10a06..d0083b20f 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -485,6 +485,21 @@ public static class ShellApprovalCases Bash("git status > \"$OUTPUT\""), Approvals.None, ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), + Case( + "fd-dup-redirect-safe-verb-allows", + Bash("git status 2>&1"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "fd-dup-redirect-safe-pipeline-allows", + Bash("git log --oneline -5 2>&1 | tail -20"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "fd-dup-redirect-mutating-no-grant-prompts-not-messy", + Bash("git push origin dev 2>&1 | tail -2"), + Approvals.None, + ExpectedApproval.Require(["git push origin dev", "tail"], isMessy: false)), Case( "background-list-prompts-for-mutating-tail", Bash("git status & git push"), 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 9eb148f93..f5e424ed5 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 @@ -1,4 +1,4 @@ -# Fresh Personal approval matrix +# Fresh Personal approval matrix `Tools.ShellMode`: `HostAllowed` @@ -62,6 +62,9 @@ | command-substitution-fails-closed | Personal | Project | Interactive | echo $(git push) | none | RequiresApproval | approval required | none | Yes | | dynamic-path-fails-closed | Personal | Project | Interactive | cat "$FILE" | none | RequiresApproval | approval required | none | Yes | | dynamic-redirect-fails-closed | Personal | Project | Interactive | git status > "$OUTPUT" | none | RequiresApproval | approval required | none | Yes | +| fd-dup-redirect-safe-verb-allows | Personal | Project | Interactive | git status 2>&1 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| fd-dup-redirect-safe-pipeline-allows | Personal | Project | Interactive | git log --oneline -5 2>&1 \| tail -20 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| fd-dup-redirect-mutating-no-grant-prompts-not-messy | Personal | Project | Interactive | git push origin dev 2>&1 \| tail -2 | none | RequiresApproval | approval required | git push origin dev, tail | No | | background-list-prompts-for-mutating-tail | Personal | Project | Interactive | git status & git push | none | RequiresApproval | approval required | none | Yes | | unbalanced-quote-fails-closed | Personal | Project | Interactive | git push "unterminated | none | RequiresApproval | approval required | none | Yes | | multiline-argument-prompts | Personal | Project | Interactive | gh issue comment 123 --body "first line\nsecond line" | none | RequiresApproval | approval required | gh issue comment | No | diff --git a/src/Netclaw.Security/ShellCommandAnalysis.cs b/src/Netclaw.Security/ShellCommandAnalysis.cs index 704295c72..b1b1c8082 100644 --- a/src/Netclaw.Security/ShellCommandAnalysis.cs +++ b/src/Netclaw.Security/ShellCommandAnalysis.cs @@ -183,5 +183,12 @@ internal sealed record ShellCommandAnalysis( // A glob in a directory segment can hide traversal or a symlink. // Only a leaf glob has a fixed directory scope. || clause.Args.Any(ShellGlobPath.HasUnresolvedDescendantScope) - || clause.Redirects.Any(static redirect => redirect.IsDynamicSkip)); + // An fd-dup target (&1, &2, &-) is a static file-descriptor number, + // not a dynamic token: ShellSyntaxTree marks it IsDynamicSkip to mean + // "do not path-resolve", but it carries no unresolved syntax and no + // filesystem scope (ResolveRedirectDirectory skips &-prefixed targets). + // Treating it as dynamic fails the whole command closed to an approval + // prompt for every `2>&1`-shaped command, even fully safe ones. + || clause.Redirects.Any(static redirect => + redirect.IsDynamicSkip && !redirect.Target.StartsWith('&'))); } From af12b604ad90b47043edf11c4d5fd544142932f6 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Thu, 6 Aug 2026 03:08:11 +0000 Subject: [PATCH 2/2] fix: restrict fd redirect exemption to static targets --- .../Tools/ShellApprovalCaseCatalog.cs | 15 +++++++++++ ...roval_cases_match_review_table.verified.md | 3 +++ .../ShellCommandAnalysisTests.cs | 25 ++++++++++++++++++ src/Netclaw.Security/ShellCommandAnalysis.cs | 26 ++++++++++++++++++- 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index d0083b20f..304ddd8d3 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -495,11 +495,26 @@ public static class ShellApprovalCases Bash("git log --oneline -5 2>&1 | tail -20"), Approvals.None, ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "fd-close-redirect-safe-verb-allows", + Bash("git status 2>&-"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), + Case( + "fd-move-redirect-safe-verb-allows", + Bash("git status 2>&1-"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)), Case( "fd-dup-redirect-mutating-no-grant-prompts-not-messy", Bash("git push origin dev 2>&1 | tail -2"), Approvals.None, ExpectedApproval.Require(["git push origin dev", "tail"], isMessy: false)), + Case( + "dynamic-fd-redirect-fails-closed", + Bash("git status 2>&$FD"), + Approvals.None, + ExpectedApproval.Require([], isMessy: true, approvalChecks: 0)), Case( "background-list-prompts-for-mutating-tail", Bash("git status & git push"), 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 f5e424ed5..d2ffae79a 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 @@ -64,7 +64,10 @@ | dynamic-redirect-fails-closed | Personal | Project | Interactive | git status > "$OUTPUT" | none | RequiresApproval | approval required | none | Yes | | fd-dup-redirect-safe-verb-allows | Personal | Project | Interactive | git status 2>&1 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | fd-dup-redirect-safe-pipeline-allows | Personal | Project | Interactive | git log --oneline -5 2>&1 \| tail -20 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| fd-close-redirect-safe-verb-allows | Personal | Project | Interactive | git status 2>&- | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | +| fd-move-redirect-safe-verb-allows | Personal | Project | Interactive | git status 2>&1- | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | fd-dup-redirect-mutating-no-grant-prompts-not-messy | Personal | Project | Interactive | git push origin dev 2>&1 \| tail -2 | none | RequiresApproval | approval required | git push origin dev, tail | No | +| dynamic-fd-redirect-fails-closed | Personal | Project | Interactive | git status 2>&$FD | none | RequiresApproval | approval required | none | Yes | | background-list-prompts-for-mutating-tail | Personal | Project | Interactive | git status & git push | none | RequiresApproval | approval required | none | Yes | | unbalanced-quote-fails-closed | Personal | Project | Interactive | git push "unterminated | none | RequiresApproval | approval required | none | Yes | | multiline-argument-prompts | Personal | Project | Interactive | gh issue comment 123 --body "first line\nsecond line" | none | RequiresApproval | approval required | gh issue comment | No | diff --git a/src/Netclaw.Security.Tests/ShellCommandAnalysisTests.cs b/src/Netclaw.Security.Tests/ShellCommandAnalysisTests.cs index dbb216951..18f07bb3c 100644 --- a/src/Netclaw.Security.Tests/ShellCommandAnalysisTests.cs +++ b/src/Netclaw.Security.Tests/ShellCommandAnalysisTests.cs @@ -36,6 +36,31 @@ public void Dynamic_command_syntax_is_explicit(string command) Assert.True(analysis.HasDynamicSyntax); } + [Theory] + [InlineData("git status 2>&1")] + [InlineData("git status 2>&123")] + [InlineData("git status 2>&1-")] + [InlineData("git status 2>&-")] + public void Static_file_descriptor_redirect_is_not_dynamic(string command) + { + var analysis = _analyzer.Analyze(command); + + Assert.Equal(ShellAnalysisFailure.None, analysis.Failure); + Assert.False(analysis.HasDynamicSyntax); + } + + [Theory] + [InlineData("git status 2>&$FD")] + [InlineData("git status >&$FD")] + [InlineData("git status 2>&${FD}")] + public void Dynamic_file_descriptor_redirect_stays_dynamic(string command) + { + var analysis = _analyzer.Analyze(command); + + Assert.Equal(ShellAnalysisFailure.None, analysis.Failure); + Assert.True(analysis.HasDynamicSyntax); + } + [Fact] public void Background_list_fails_closed_when_parser_omits_its_tail() { diff --git a/src/Netclaw.Security/ShellCommandAnalysis.cs b/src/Netclaw.Security/ShellCommandAnalysis.cs index b1b1c8082..38e2a4092 100644 --- a/src/Netclaw.Security/ShellCommandAnalysis.cs +++ b/src/Netclaw.Security/ShellCommandAnalysis.cs @@ -190,5 +190,29 @@ internal sealed record ShellCommandAnalysis( // Treating it as dynamic fails the whole command closed to an approval // prompt for every `2>&1`-shaped command, even fully safe ones. || clause.Redirects.Any(static redirect => - redirect.IsDynamicSkip && !redirect.Target.StartsWith('&'))); + redirect.IsDynamicSkip + && !IsStaticFileDescriptor(redirect.Target))); + + /// + /// Returns true only for the static file-descriptor targets that + /// ShellSyntaxTree 0.2 recognizes: &N, &N-, and &-. + /// + private static bool IsStaticFileDescriptor(string target) + { + if (target == "&-") + return true; + + if (target.Length < 2 || target[0] != '&') + return false; + + var index = 1; + while (index < target.Length && char.IsAsciiDigit(target[index])) + index++; + + if (index == 1) + return false; + + return index == target.Length + || index == target.Length - 1 && target[index] == '-'; + } }