Python: Improve regex validation for shell and file access

dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs

@@ -141,6 +141,14 @@ public bool Equals(ShellPolicyOutcome other) =>
 /// </remarks>
 public sealed class ShellPolicy
 {
+    /// <summary>
+    /// Ceiling on any single pattern match. Policy patterns are operator-authored, but the
+    /// commands they filter are model-generated, so a pattern that backtracks catastrophically
+    /// would stall the authorization path itself. The timeout makes that failure bounded and
+    /// recoverable rather than a hang.
+    /// </summary>
+    private static readonly TimeSpan s_patternMatchTimeout = TimeSpan.FromSeconds(1);
+
     private readonly IReadOnlyList<Regex> _denyList;
     private readonly IReadOnlyList<Regex>? _allowList;
     private readonly Func<ShellRequest, ShellPolicyOutcome?>? _custom;
@@ -170,11 +178,11 @@ public ShellPolicy(
         Func<ShellRequest, ShellPolicyOutcome?>? custom = null)
     {
         this._denyList = denyList?
-            .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase))
+            .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, s_patternMatchTimeout))
             .ToArray() ?? Array.Empty<Regex>();
 
         this._allowList = allowList?
-            .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase))
+            .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, s_patternMatchTimeout))
             .ToArray();
 
         this._custom = custom;
@@ -200,15 +208,36 @@ public ShellPolicyOutcome Evaluate(ShellRequest request)
 
         foreach (var deny in this._denyList)
         {
-            if (deny.IsMatch(command))
+            // A deny pattern that cannot be evaluated in time fails closed: an
+            // unevaluated rule must never read as "this command is fine".
+            try
             {
-                return ShellPolicyOutcome.Deny($"matched deny pattern: {deny}");
+                if (deny.IsMatch(command))
+                {
+                    return ShellPolicyOutcome.Deny($"matched deny pattern: {deny}");
+                }
+            }
+            catch (RegexMatchTimeoutException)
+            {
+                return ShellPolicyOutcome.Deny($"deny pattern could not be evaluated in time: {deny}");
             }
         }
 
         if (this._allowList is not null)
         {
-            var matched = this._allowList.Any(allow => allow.IsMatch(command));
+            var matched = this._allowList.Any(allow =>
+            {
+                // A timed-out allow pattern is treated as a non-match, so it can
+                // never be the thing that grants permission.
+                try
+                {
+                    return allow.IsMatch(command);
+                }
+                catch (RegexMatchTimeoutException)
+                {
+                    return false;
+                }
+            });
             if (!matched)
             {
                 return ShellPolicyOutcome.Deny("command does not match allow list");

dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs

@@ -0,0 +1,73 @@
+// Copyright (c) Microsoft. All rights reserved.
+
+using System;
+using System.Diagnostics;
+
+namespace Microsoft.Agents.AI.Tools.Shell.UnitTests;
+
+/// <summary>
+/// Guards the fail-closed behavior of <see cref="ShellPolicy.Evaluate"/> when a policy
+/// pattern backtracks catastrophically. Patterns are operator-authored, but the commands
+/// they filter are model-generated, so an unbounded match would let injected input stall
+/// the authorization path itself.
+/// </summary>
+public sealed class ShellPolicyTests
+{
+    /// <summary>
+    /// A pattern that backtracks exponentially, and a command it cannot match.
+    /// </summary>
+    /// <remarks>
+    /// Deliberately not <c>(a|a)*$</c>, which is the equivalent used by the Python tests.
+    /// .NET matches that one instantly: <c>(a|a)*</c> can match zero times, so the engine
+    /// finds an empty match at the end anchor and never backtracks. <c>(a+)+$</c> requires
+    /// at least one character per iteration, which forces the exponential search.
+    /// </remarks>
+    private const string RedosPattern = "(a+)+$";
+    private static readonly string s_redosCommand = new string('a', 30) + "!";
+
+    [Fact]
+    public void Evaluate_DenyPatternTimesOut_FailsClosed()
+    {
+        // Arrange
+        var policy = new ShellPolicy(denyList: [RedosPattern]);
+
+        // Act
+        var sw = Stopwatch.StartNew();
+        var outcome = policy.Evaluate(new ShellRequest(s_redosCommand));
+        sw.Stop();
+
+        // Assert
+        Assert.False(outcome.Allowed);
+        Assert.Contains("could not be evaluated in time", outcome.Reason ?? string.Empty, StringComparison.Ordinal);
+        Assert.True(sw.Elapsed < TimeSpan.FromSeconds(10), $"policy evaluation overran: {sw.Elapsed}");
+    }
+
+    [Fact]
+    public void Evaluate_AllowPatternTimesOut_DoesNotGrantAccess()
+    {
+        // Arrange
+        var policy = new ShellPolicy(allowList: [RedosPattern]);
+
+        // Act
+        var sw = Stopwatch.StartNew();
+        var outcome = policy.Evaluate(new ShellRequest(s_redosCommand));
+        sw.Stop();
+
+        // Assert
+        Assert.False(outcome.Allowed);
+        Assert.Contains("does not match allow list", outcome.Reason ?? string.Empty, StringComparison.Ordinal);
+        Assert.True(sw.Elapsed < TimeSpan.FromSeconds(10), $"policy evaluation overran: {sw.Elapsed}");
+    }
+
+    [Fact]
+    public void Evaluate_NormalPatterns_StillDecideAsBefore()
+    {
+        // Arrange
+        var policy = new ShellPolicy(denyList: ["^ssh\\b"], allowList: ["^ls\\b", "^ssh\\b"]);
+
+        // Act & Assert
+        Assert.False(policy.Evaluate(new ShellRequest("ssh host")).Allowed);
+        Assert.True(policy.Evaluate(new ShellRequest("ls -la")).Allowed);
+        Assert.False(policy.Evaluate(new ShellRequest("cat /etc/passwd")).Allowed);
+    }
+}

python/packages/core/AGENTS.md

@@ -200,7 +200,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID
 
 ### File Access Harness (`_harness/_file_access.py`)
 
-- **`AgentFileStore`** - Abstract async store backing the file-access harness. Implementations expose `write`, `read`, `delete`, `list_children`, `file_exists`, `search`, and `create_directory` over forward-slash relative paths. `list_children` returns the direct children (files and subdirectories, subdirectories first) as `FileStoreEntry` instances; `search` accepts a keyword-only `recursive` flag (default `False`) and, when `recursive=True`, walks all descendants and returns `file_name` values relative to the search directory. The line-numbering contract lives on the base class: `split_lines` publishes the `\n`-only keepends split that every `line_number` addresses, `scan_content` is the numbering primitive both shipped stores report through, and `search` is now **concrete** — it asks the overridable `find_matching_files` hook which files to consider (superset semantics; a backend with a native index overrides it and prunes server-side) and then reads and numbers them itself. A store may still override `search` outright, but then it owns numbering: it must report `line_number` as a 1-based coordinate into `split_lines` of the content `read` returns. Nothing checks that at run time, so a store that numbers differently makes a later edit land on the wrong line silently.
+- **`AgentFileStore`** - Abstract async store backing the file-access harness. Implementations expose `write`, `read`, `delete`, `list_children`, `file_exists`, `search`, and `create_directory` over forward-slash relative paths. `list_children` returns the direct children (files and subdirectories, subdirectories first) as `FileStoreEntry` instances; `search` accepts a keyword-only `recursive` flag (default `False`) and, when `recursive=True`, walks all descendants and returns `file_name` values relative to the search directory. The line-numbering contract lives on the base class: `split_lines` publishes the `\n`-only keepends split that every `line_number` addresses, `scan_content` is the numbering primitive both shipped stores report through, and `search` is now **concrete** — it asks the overridable `find_matching_files` hook which files to consider (superset semantics; a backend with a native index overrides it and prunes server-side) and then reads and numbers them itself. A store may still override `search` outright, but then it owns numbering: it must report `line_number` as a 1-based coordinate into `split_lines` of the content `read` returns. Nothing checks that at run time, so a store that numbers differently makes a later edit land on the wrong line silently. The same applies to ReDoS: the grep pattern is model-supplied, so `search` compiles it through the `regex` module against a single monotonic deadline that bounds the whole scan (CPython's `re` holds the GIL for an entire match, so `asyncio.wait_for` around it bounds nothing). A store overriding `search` with bare `re` silently opts itself back out of that guarantee.
 - **`InMemoryAgentFileStore`** - Dict-backed store suitable for tests and lightweight scenarios.
 - **`FileSystemAgentFileStore`** - Disk-backed store rooted under a configurable directory. Enforces relative-path normalization, root containment, and rejects symlink/reparse-point segments to prevent escape.
 - **`FileSearchResult`** / **`FileSearchMatch`** - `SerializationMixin` DTOs returned by `search`, carrying the matching file name, a context snippet, and the matching lines with 1-based line numbers. Implementers should report each matching line verbatim, including its own terminator, so it can be reused as a `file_access_replace_lines` `new_line`; the pattern itself is matched against the line with its whole terminator removed, so `^`/`$` anchor to the line's text on a CRLF file as they already did on an LF one. A custom store populates these DTOs from its own `search`; the verbatim text is a recommendation, but the line number is not — it must address `split_lines`.