Skip to content
Merged
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
87 changes: 87 additions & 0 deletions src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,93 @@ public void ExtractPatterns_carriage_return_arg_terminates_pattern_at_flag()
Assert.Equal("freshdesk ticket reply --message", patterns[0]);
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void ExtractPatterns_single_line_quoted_free_text_terminates_pattern_at_flag()
{
// Issue #1406: a single-line quoted commit message is call-specific
// free text, not approvable intent. The stored pattern stops at the
// flag so a later commit with a different message still matches.
var patterns = _matcher.ExtractPatterns(new ToolName("shell_execute"),
Args("git commit -m \"fix the bug\""));

Assert.Single(patterns);
Assert.Equal("git commit -m", patterns[0]);
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void ExtractPatterns_single_line_quoted_body_drops_from_pattern()
{
// Issue #1406: the ticket body is a single-line quoted operand with
// internal whitespace, so it drops before it inflates the pattern.
var patterns = _matcher.ExtractPatterns(new ToolName("shell_execute"),
Args("freshdesk ticket reply --message \"Single line body\""));

Assert.Single(patterns);
Assert.Equal("freshdesk ticket reply --message", patterns[0]);
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void ExtractPatterns_single_word_quoted_arg_is_kept()
{
// A single-word quoted arg has no internal whitespace, so it stays in
// the pattern and normalizes the same as its unquoted form — the drop
// rule targets only multi-word quoted free text.
var patterns = _matcher.ExtractPatterns(new ToolName("shell_execute"),
Args("git commit -m \"fix\""));

Assert.Single(patterns);
Assert.Equal("git commit -m fix", patterns[0]);
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void ExtractPatterns_quoted_glob_without_internal_whitespace_is_kept()
{
// `"*.cs"` is quoted but has no internal whitespace, so the drop rule
// leaves it in the pattern — only whitespace-bearing free text drops.
var patterns = _matcher.ExtractPatterns(new ToolName("shell_execute"),
Args("find . -name \"*.cs\"", "/srv/project"));

Assert.Single(patterns);
Assert.Contains("-name", patterns[0]);
Assert.Contains("*.cs", patterns[0]);
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void ExtractCandidates_quoted_path_with_space_keeps_directory_scope()
{
// Security: the drop rule shapes only the stored pattern. A quoted
// path with a space is authorization state — ExtractCandidates still
// scopes the candidate to the file's parent directory.
var candidates = _matcher.ExtractCandidates(new ToolName("shell_execute"),
Args("cat \"my file.txt\"", "/srv/project"));

Assert.Contains(candidates, c => c.Verb == "cat" && c.Directory == "/srv/project");
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void ExtractCandidates_quoted_free_text_before_path_keeps_path_scope()
{
// The quoted search pattern `"foo bar"` is free text and never becomes
// a scope, while the trailing path operand still scopes the candidate.
var candidates = _matcher.ExtractCandidates(new ToolName("shell_execute"),
Args("grep \"foo bar\" ./notes.txt", "/srv/project"));

Assert.Contains(candidates, c => c.Verb == "grep" && c.Directory == "/srv/project");
Assert.DoesNotContain(candidates, c => c.Directory is not null && c.Directory.Contains("foo"));
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void FormatForDisplay_single_line_quoted_free_text_shows_full_command()
{
// The drop rule is pattern-only: a single-line command has no line
// break, so the operator still sees the full message verbatim in the
// approval prompt. Only the stored pattern omits the body.
var display = _matcher.FormatForDisplay(new ToolName("shell_execute"),
Args("git commit -m \"fix the bug\""));

Assert.Equal("git commit -m \"fix the bug\"", display);
}

[Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only — matcher routes through BashParser on POSIX")]
public void FormatForDisplay_carriage_return_arg_is_summarized()
{
Expand Down
76 changes: 70 additions & 6 deletions src/Netclaw.Security/IToolApprovalMatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -511,12 +511,14 @@ private static IReadOnlyList<string> ExtractApprovalUnitsViaBashAnalysis(
/// Rebuilds one clause's user-facing text from its parsed parts: verb
/// chain, positional/flag args, and redirects. Synthetic cd-attribution
/// args are dropped — they carry an inherited cwd, not a token the user
/// typed. Call-specific value arguments (issue #1331, generalized to
/// digit-bearing tokens — see <see cref="IsCallSpecificValueToken"/>)
/// are also excluded since they vary between invocations of the same
/// verb chain. Once a value token is encountered, the greedy walk
/// terminates — subsequent args (wrapped subcommands like <c>curl</c>
/// after <c>timeout 30</c>) are outside the approval intent.
/// typed. Call-specific value arguments are also excluded since they vary
/// between invocations of the same verb chain: digit-bearing tokens
/// (issue #1331, see <see cref="IsCallSpecificValueToken"/>), multi-line
/// quoted strings (issue #1402, see <see cref="ContainsLineBreak"/>), and
/// single-line quoted free text (issue #1406, see
/// <see cref="IsQuotedFreeTextArg"/>). Once such a token is encountered,
/// the greedy walk terminates — subsequent args (wrapped subcommands like
/// <c>curl</c> after <c>timeout 30</c>) are outside the approval intent.
/// The result is fed back through
/// <see cref="ShellTokenizer.NormalizeApprovalUnit"/> for path
/// normalization, so this only needs to emit a clean token sequence.
Expand Down Expand Up @@ -549,6 +551,16 @@ private static string ReconstructClauseText(ShellSyntaxTree.Clause clause)
if (ContainsLineBreak(arg.Raw))
break;

// Issue #1406: a single-line quoted operand whose text holds
// internal whitespace (a commit message, a ticket body, an inline
// note) is call-specific free text. Every unique value would
// otherwise become a new stored pattern that re-prompts. Same
// termination mechanism as the digit (#1331) and multi-line
// (#1402) rules. A path arg is exempt so a quoted path with a
// space keeps its directory scope (see IsQuotedFreeTextArg).
if (IsQuotedFreeTextArg(arg))
break;

if (sb.Length > 0)
sb.Append(' ');
sb.Append(arg.Raw);
Expand Down Expand Up @@ -622,6 +634,58 @@ private static bool IsCallSpecificValueToken(string token)
return false;
}

/// <summary>
/// True when <paramref name="arg"/> is a quote-wrapped operand whose text
/// holds internal whitespace and is not a path (issue #1406). Such an
/// operand is call-specific free text — a commit message, a ticket body,
/// an inline note — that varies between invocations of the same verb
/// chain, so it must not enter the stored approval pattern. Like the
/// digit-bearing (#1331) and multi-line (#1402) rules, hitting one
/// terminates the reconstruction walk.
/// <para>
/// The rule is deliberately narrow so it never widens a grant:
/// </para>
/// <list type="bullet">
/// <item>Only a single matching quote pair qualifies. An unquoted token
/// cannot hold internal whitespace — the shell splits it into separate
/// args — so a single-word quoted arg (<c>git commit -m "fix"</c>) has no
/// internal whitespace, stays in the pattern, and normalizes the same as
/// its unquoted form.</item>
/// <item>A path arg is exempt. Its directory is authorization state that
/// <see cref="ExtractCandidatesViaBashAnalysis"/> resolves separately from
/// the same parsed <c>Arg</c>, so a quoted path with a space
/// (<c>cat "my file.txt"</c>) keeps its scope. Only a value operand, never
/// a path, drops here.</item>
/// </list>
/// This rule only shapes the stored/display pattern. It does not touch the
/// gate candidate or the live authorization decision, which re-parse each
/// command and scope every path arg through the zone gate.
/// </summary>
private static bool IsQuotedFreeTextArg(ShellSyntaxTree.Arg arg)
{
if (arg.IsPath)
return false;

var raw = arg.Raw;

// A single matching quote pair around at least one inner character.
// The shortest droppable form is quote + whitespace + quote (length 3).
if (raw.Length < 3)
return false;

var quote = raw[0];
if (quote is not ('"' or '\'') || raw[^1] != quote)
return false;

for (var i = 1; i < raw.Length - 1; i++)
{
if (char.IsWhiteSpace(raw[i]))
return true;
}

return false;
}

/// <summary>
/// Drops trailing call-specific value tokens from a parsed verb chain,
/// always retaining at least the command word. See
Expand Down
Loading