diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs index 02a0b5b4f..7950ed067 100644 --- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs +++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Text.RegularExpressions; namespace Microsoft.Agents.AI.Tools.Shell; @@ -118,24 +119,31 @@ public readonly struct ShellPolicyOutcome : IEquatable /// /// /// No default patterns. A constructed -/// with no arguments has an empty deny list and an empty allow list — -/// it will allow any non-empty command. Operators who want pre-execution -/// rejection of specific shapes must supply their own -/// denyList. +/// with no arguments has an empty deny list and no allow list (allow list +/// disabled) — it will allow any non-empty command. Operators who want +/// pre-execution rejection of specific shapes must supply their own +/// denyList, or an allowList to +/// deny everything except the explicitly allowed commands. /// /// -/// Evaluation order — allow short-circuits deny. Allow patterns are -/// checked first; a match returns immediately without consulting the deny -/// list. Use allow patterns sparingly (and prefer narrowly anchored regexes -/// like ^git\s+status$ rather than substring matches), because an -/// over-broad allow pattern can re-enable a command that the deny list was -/// supposed to block. +/// Evaluation order — deny-first, the allow list is exclusive. Deny +/// patterns are checked first and a match wins immediately. If an allow list +/// is supplied, it is treated as exclusive: any command that matches +/// none of the allow patterns is denied. Supplying an empty allow +/// list therefore denies every command; leaving the allow list +/// disables the allow list entirely. An optional +/// custom callback runs last — after the deny and allow lists have +/// passed — and may override the default allow (for example, turning it into +/// a deny); it cannot re-enable a command already rejected by the deny list +/// or the allow list. Prefer narrowly anchored regexes (like +/// ^git\s+status$) over substring matches when building an allow list. /// /// public sealed class ShellPolicy { - private readonly IReadOnlyList _denies; - private readonly IReadOnlyList _allows; + private readonly IReadOnlyList _denyList; + private readonly IReadOnlyList? _allowList; + private readonly Func? _custom; /// /// Initializes a new instance of the class. @@ -145,39 +153,40 @@ public sealed class ShellPolicy /// empty collection disables the deny list entirely. /// /// - /// Optional explicit-allow patterns. A match here short-circuits the - /// deny list and is useful when the caller knows the command is safe. + /// Optional allow-list patterns. When the allow + /// list is disabled. When supplied (including as an empty collection) any + /// command matching none of the patterns is denied — an empty collection + /// therefore denies every command. /// - public ShellPolicy(IEnumerable? denyList = null, IEnumerable? allowList = null) + /// + /// Optional callback that gets the final say. It runs after the deny and + /// allow lists have passed; returning a non- outcome + /// overrides the default allow, while leaves the + /// default in place. + /// + public ShellPolicy( + IEnumerable? denyList = null, + IEnumerable? allowList = null, + Func? custom = null) { - var deny = new List(); - if (denyList is not null) - { - foreach (var pattern in denyList) - { - deny.Add(new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase)); - } - } - this._denies = deny; + this._denyList = denyList? + .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase)) + .ToArray() ?? Array.Empty(); - var allow = new List(); - if (allowList is not null) - { - foreach (var pattern in allowList) - { - allow.Add(new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase)); - } - } - this._allows = allow; + this._allowList = allowList? + .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase)) + .ToArray(); + + this._custom = custom; } /// /// Evaluate and return an outcome. /// /// - /// Order of operations: empty-command guard → explicit allow patterns - /// (a match short-circuits with ) - /// → deny patterns (first match wins) → default allow. + /// Order of operations (first hit wins): empty-command guard → deny + /// patterns → allow list (deny when supplied and unmatched) → + /// custom callback override → default allow. /// /// The request to evaluate. /// An allow or deny outcome. @@ -189,15 +198,7 @@ public sealed class ShellPolicy return ShellPolicyOutcome.Deny("empty command"); } - foreach (var allow in this._allows) - { - if (allow.IsMatch(command)) - { - return new ShellPolicyOutcome(true, "matched allow pattern"); - } - } - - foreach (var deny in this._denies) + foreach (var deny in this._denyList) { if (deny.IsMatch(command)) { @@ -205,6 +206,24 @@ public sealed class ShellPolicy } } + if (this._allowList is not null) + { + var matched = this._allowList.Any(allow => allow.IsMatch(command)); + if (!matched) + { + return ShellPolicyOutcome.Deny("command does not match allow list"); + } + } + + if (this._custom is not null) + { + var overrideOutcome = this._custom(request); + if (overrideOutcome is { } outcome) + { + return outcome; + } + } + return ShellPolicyOutcome.Allow; } } diff --git a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/LocalShellExecutorTests.cs b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/LocalShellExecutorTests.cs index c0706b566..247033fa1 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/LocalShellExecutorTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/LocalShellExecutorTests.cs @@ -37,13 +37,103 @@ public sealed class LocalShellExecutorTests } [Fact] - public void Policy_AllowList_OverridesDeny() + public void Policy_DenyList_TakesPrecedenceOverAllowList() { + // Under deny-first semantics an allow-list match does NOT override a + // deny-list match; deny wins. var policy = new ShellPolicy( allowList: ["^echo "], denyList: ["echo"]); var decision = policy.Evaluate(new ShellRequest("echo hello")); - Assert.True(decision.Allowed); + Assert.False(decision.Allowed); + Assert.Contains("deny pattern", decision.Reason ?? string.Empty, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void Policy_BroadDenyList_OverridesSpecificAllowList() + { + // A broad deny pattern wins even when a more specific allow pattern + // would otherwise permit the command. + var policy = new ShellPolicy( + allowList: ["^git push origin main$"], + denyList: ["git push"]); + var decision = policy.Evaluate(new ShellRequest("git push origin main")); + Assert.False(decision.Allowed); + Assert.Contains("deny pattern", decision.Reason ?? string.Empty, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void Policy_AllowList_AllowsMatch() + { + var policy = new ShellPolicy(allowList: ["^echo "]); + Assert.True(policy.Evaluate(new ShellRequest("echo hello")).Allowed); + } + + [Fact] + public void Policy_AllowList_DeniesNonMatch() + { + var policy = new ShellPolicy(allowList: ["^echo "]); + var decision = policy.Evaluate(new ShellRequest("ls -la")); + Assert.False(decision.Allowed); + Assert.Contains("allow list", decision.Reason ?? string.Empty, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void Policy_EmptyAllowList_DeniesEverything() + { + // A supplied-but-empty allow list matches nothing, so it denies all. + var policy = new ShellPolicy(allowList: []); + Assert.False(policy.Evaluate(new ShellRequest("echo hello")).Allowed); + } + + [Fact] + public void Policy_NullAllowList_DisablesAllowList() + { + var policy = new ShellPolicy(allowList: null); + Assert.True(policy.Evaluate(new ShellRequest("echo hello")).Allowed); + } + + [Fact] + public void Policy_Custom_CanOverrideDefaultAllowToDeny() + { + var policy = new ShellPolicy( + custom: req => req.Command.Contains("secret", StringComparison.OrdinalIgnoreCase) + ? ShellPolicyOutcome.Deny("custom blocked") + : null); + Assert.True(policy.Evaluate(new ShellRequest("echo hello")).Allowed); + var decision = policy.Evaluate(new ShellRequest("cat secret.txt")); + Assert.False(decision.Allowed); + Assert.Equal("custom blocked", decision.Reason); + } + + [Fact] + public void Policy_Custom_NullReturn_LeavesDefaultAllow() + { + var policy = new ShellPolicy(custom: _ => null); + Assert.True(policy.Evaluate(new ShellRequest("echo hello")).Allowed); + } + + [Fact] + public void Policy_Custom_DoesNotRunWhenDenyListMatches() + { + // Deny-list match short-circuits before the custom callback runs, so a + // permissive custom callback cannot re-enable a denied command. + var policy = new ShellPolicy( + denyList: ["echo"], + custom: _ => ShellPolicyOutcome.Allow); + Assert.False(policy.Evaluate(new ShellRequest("echo hello")).Allowed); + } + + [Fact] + public void Policy_Custom_DoesNotOverrideAllowListDenial() + { + // An allow-list denial short-circuits before the custom callback runs, + // so a permissive custom callback cannot re-enable a command that is + // outside the allow list. + var policy = new ShellPolicy( + allowList: ["^echo "], + custom: _ => ShellPolicyOutcome.Allow); + Assert.False(policy.Evaluate(new ShellRequest("ls -la")).Allowed); } [Fact]