From b2ee4b2de3f9cb094d62b94b02d6526ee2e24b8f Mon Sep 17 00:00:00 2001
From: westey <164392973+westey-m@users.noreply.github.com>
Date: Fri, 18 Sep 2026 11:17:56 +0000
Subject: [PATCH 1/4] Improve regex validation for shell and file access
---
.../ShellPolicy.cs | 39 ++++-
.../ShellPolicyTests.cs | 56 ++++++
python/packages/core/AGENTS.md | 2 +-
.../agent_framework/_harness/_file_access.py | 162 ++++++++++++++----
.../packages/core/agent_framework/_tools.py | 4 +-
.../_workflows/_agent_executor.py | 4 +-
python/packages/core/pyproject.toml | 1 +
.../core/test_function_invocation_logic.py | 7 +-
.../tests/core/test_harness_file_access.py | 95 ++++++++++
.../tests/core/test_harness_tool_approval.py | 6 +-
.../test_agent_executor_tool_calls.py | 4 +-
.../agent_framework_tools/shell/_policy.py | 86 +++++++++-
python/packages/tools/pyproject.toml | 1 +
python/packages/tools/tests/test_policy.py | 43 +++++
python/uv.lock | 4 +
15 files changed, 454 insertions(+), 60 deletions(-)
create mode 100644 dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs
index 7950ed0674d..3a0be56f2a1 100644
--- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs
+++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs
@@ -141,6 +141,14 @@ public bool Equals(ShellPolicyOutcome other) =>
///
public sealed class ShellPolicy
{
+ ///
+ /// 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.
+ ///
+ private static readonly TimeSpan PatternMatchTimeout = TimeSpan.FromSeconds(1);
+
private readonly IReadOnlyList _denyList;
private readonly IReadOnlyList? _allowList;
private readonly Func? _custom;
@@ -170,11 +178,11 @@ public ShellPolicy(
Func? custom = null)
{
this._denyList = denyList?
- .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase))
+ .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, PatternMatchTimeout))
.ToArray() ?? Array.Empty();
this._allowList = allowList?
- .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase))
+ .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, 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");
diff --git a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
new file mode 100644
index 00000000000..2425aa5c160
--- /dev/null
+++ b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
@@ -0,0 +1,56 @@
+// Copyright (c) Microsoft. All rights reserved.
+
+using System.Diagnostics;
+
+namespace Microsoft.Agents.AI.Tools.Shell.UnitTests;
+
+///
+/// Guards the fail-closed behavior of 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.
+///
+public sealed class ShellPolicyTests
+{
+ /// A pattern that backtracks exponentially, and a command it cannot match.
+ private const string RedosPattern = "(a|a)*$";
+ private static readonly string s_redosCommand = new string('a', 30) + "!";
+
+ [Fact]
+ public void Evaluate_DenyPatternTimesOut_FailsClosed()
+ {
+ var policy = new ShellPolicy(denyList: [RedosPattern]);
+
+ var sw = Stopwatch.StartNew();
+ var outcome = policy.Evaluate(new ShellRequest(s_redosCommand));
+ sw.Stop();
+
+ 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()
+ {
+ var policy = new ShellPolicy(allowList: [RedosPattern]);
+
+ var sw = Stopwatch.StartNew();
+ var outcome = policy.Evaluate(new ShellRequest(s_redosCommand));
+ sw.Stop();
+
+ 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()
+ {
+ var policy = new ShellPolicy(denyList: ["^ssh\\b"], allowList: ["^ls\\b", "^ssh\\b"]);
+
+ 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);
+ }
+}
diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md
index d32843f4204..43106150796 100644
--- a/python/packages/core/AGENTS.md
+++ b/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`.
diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py
index 7dc95779ebc..f4c935e2aab 100644
--- a/python/packages/core/agent_framework/_harness/_file_access.py
+++ b/python/packages/core/agent_framework/_harness/_file_access.py
@@ -27,11 +27,13 @@
import logging
import os
import re
+import time
from abc import ABC, abstractmethod
from collections.abc import Awaitable, Mapping, MutableMapping
from pathlib import Path
-from typing import Annotated, Any, ClassVar, cast
+from typing import Annotated, Any, ClassVar, Protocol, cast
+import regex as regex_module
from pydantic import BaseModel, Field
from .._feature_stage import ExperimentalFeature, experimental
@@ -68,15 +70,15 @@
# regex match when building a result snippet.
_SEARCH_SNIPPET_RADIUS = 50
-# Hard cap on the length of a user-supplied search regex. Python's ``re`` module
-# has no built-in timeout, so a catastrophic-backtracking pattern (such as
-# ``(a+)+$``) submitted by the model could spin the CPU indefinitely. The cap
-# alone does not stop short pathological patterns, so :meth:`search`
-# additionally executes the regex scan in a worker thread and bounds the wall
-# clock with :data:`_SEARCH_TIMEOUT_SECONDS`. The thread itself cannot be
-# safely interrupted from Python, so a runaway scan continues until the
-# regex engine returns, but the caller and event loop stay responsive.
+# Hard cap on the length of a user-supplied search regex. The cap is a coarse bound on
+# how much work a single pattern can describe; it does not stop a short pathological
+# pattern, which is what :class:`_BoundedSearchPattern` is for.
_MAX_SEARCH_PATTERN_LENGTH = 256
+
+# Wall-clock budget for one search, covering the whole call: every file read and every
+# line matched. Enforced twice over, by :class:`_BoundedSearchPattern` inside the match
+# and by :func:`_run_search_with_timeout` around the call. See
+# :func:`_compile_search_regex` for why the inner bound is the one that matters.
_SEARCH_TIMEOUT_SECONDS = 10.0
# How much file content :meth:`AgentFileStore.search` accumulates before handing a batch
@@ -97,12 +99,89 @@
_ELOOP = errno.ELOOP
-def _compile_search_regex(pattern: str) -> re.Pattern[str]:
- """Compile a case-insensitive search regex, enforcing the length cap.
+class _SearchMatch(Protocol):
+ """The part of a match object :func:`_search_file_content` uses."""
+
+ def start(self) -> int: ...
+
+ def end(self) -> int: ...
+
+
+class _SearchPattern(Protocol):
+ """The part of a compiled pattern the scan pipeline uses.
+
+ Both :class:`re.Pattern` and :class:`_BoundedSearchPattern` satisfy this, which is
+ what lets :meth:`AgentFileStore.scan_content` keep accepting a plain ``re.Pattern``
+ from a third-party store while the built-in stores pass a deadline-bounded one.
+ """
+
+ @property
+ def pattern(self) -> str: ...
+
+ def search(self, string: str) -> _SearchMatch | None: ...
+
+
+class _SearchTimeout(Exception):
+ """Raised when a scan exhausts its deadline.
+
+ Deliberately not an :class:`OSError` subclass. ``regex`` signals its own timeout with
+ the builtin :class:`TimeoutError`, which *is* an ``OSError``, and the grep tools catch
+ ``OSError`` around the search to report unreadable files -- so letting that escape as-is
+ would have the timeout silently reported as a file error. This is converted to the
+ documented :class:`ValueError` at the boundary by :func:`_run_search_with_timeout`.
+ """
+
+
+class _BoundedSearchPattern:
+ """A compiled pattern that enforces one deadline across every match it performs.
+
+ The deadline is shared rather than per-call: a pattern is matched once per line of
+ every searched file, so a per-match timeout would reset thousands of times over and
+ bound nothing in aggregate.
+ """
+
+ def __init__(self, compiled: Any, pattern: str, deadline: float) -> None:
+ self._compiled = compiled
+ self._pattern = pattern
+ self._deadline = deadline
+
+ @property
+ def pattern(self) -> str:
+ """The pattern string this was compiled from."""
+ return self._pattern
+
+ def search(self, string: str) -> Any:
+ """Search ``string``, charging the elapsed time against the shared deadline.
+
+ Raises:
+ _SearchTimeout: When the deadline has passed, or passes mid-match.
+ """
+ remaining = self._deadline - time.monotonic()
+ if remaining <= 0.0:
+ raise _SearchTimeout
+ try:
+ return self._compiled.search(string, timeout=remaining)
+ except TimeoutError as exc:
+ raise _SearchTimeout from exc
+
- An invalid ``pattern`` raises :class:`re.error` unchanged so the search
- tools surface it to the calling model, which can correct the pattern and
- retry.
+def _compile_search_regex(pattern: str) -> _BoundedSearchPattern:
+ """Compile a case-insensitive search regex, enforcing the length cap and a deadline.
+
+ Compiled with ``regex`` rather than the standard library's ``re``. A search pattern
+ comes from the model, so it is attacker-influenced: a request can carry an indirect
+ prompt injection that asks for a catastrophically backtracking pattern such as
+ ``(a|a)*$``. ``re`` offers no way to bound a match -- it holds the GIL for the whole
+ operation and has no interruption point -- so wrapping the scan in a worker thread and
+ an ``asyncio`` timeout bounds nothing: the timer cannot be scheduled, the thread cannot
+ be cancelled, and the host process stops servicing unrelated work until the match
+ finally returns. ``regex`` checks a deadline mid-match and releases the GIL while
+ matching, which is what makes the bound real and keeps the event loop responsive.
+
+ An invalid ``pattern`` raises :class:`re.error` unchanged so the search tools surface
+ it to the calling model, which can correct the pattern and retry. ``regex`` reports
+ syntax errors with its own ``regex.error``, which is *not* a subclass of ``re.error``,
+ so it is translated here to keep that contract.
Raises:
ValueError: When ``pattern`` exceeds ``_MAX_SEARCH_PATTERN_LENGTH``
@@ -114,7 +193,29 @@ def _compile_search_regex(pattern: str) -> re.Pattern[str]:
f"Regex pattern is too long ({len(pattern)} characters). "
f"Maximum supported length is {_MAX_SEARCH_PATTERN_LENGTH} characters."
)
- return re.compile(pattern, flags=re.IGNORECASE)
+ try:
+ # VERSION0 keeps ``regex`` in its ``re``-compatible dialect, so a pattern that
+ # worked against the standard library keeps the same meaning here.
+ compiled = regex_module.compile(pattern, flags=regex_module.IGNORECASE | regex_module.VERSION0)
+ except regex_module.error as exc:
+ raise re.error(str(exc)) from exc
+ # The deadline starts here, not at first match: the budget covers the whole search,
+ # including the file reads the scan is interleaved with.
+ return _BoundedSearchPattern(compiled, pattern, time.monotonic() + _SEARCH_TIMEOUT_SECONDS)
+
+
+def _search_timeout_message() -> str:
+ """Build the message for a search that ran out of budget.
+
+ Built on demand rather than stored as a constant so it reflects the current value of
+ :data:`_SEARCH_TIMEOUT_SECONDS`, which tests patch.
+ """
+ return (
+ f"Search did not complete within {_SEARCH_TIMEOUT_SECONDS:g} seconds. The bound covers "
+ "the whole search, so this is either a pathological pattern (avoid nested quantifiers "
+ "such as '(a+)+') or a store too slow to read this many files in time. Narrow the "
+ "pattern, or search a smaller directory."
+ )
async def _run_search_with_timeout(
@@ -122,11 +223,9 @@ async def _run_search_with_timeout(
) -> list[FileSearchResult]:
"""Await ``work`` under a bounded wall-clock timeout.
- The one bound covers both shapes of search: a whole scan offloaded with
- :func:`asyncio.to_thread` (what the stores in this package do) and the base
- :meth:`AgentFileStore.search` pipeline, which keeps store I/O on the event
- loop and offloads only the per-file regex work. In both cases the
- model-supplied pattern executes in a worker thread, never on the loop.
+ A backstop around the deadline :func:`_compile_search_regex` binds into the pattern
+ itself. The inner bound covers time spent matching; this one also covers a store too
+ slow to read its files, which no regex deadline would catch.
Raises:
ValueError: When the search does not complete within
@@ -134,17 +233,14 @@ async def _run_search_with_timeout(
"""
try:
return await asyncio.wait_for(work, timeout=_SEARCH_TIMEOUT_SECONDS)
+ except _SearchTimeout as exc:
+ raise ValueError(_search_timeout_message()) from exc
except asyncio.TimeoutError as exc:
# On Python 3.10 ``asyncio.wait_for`` raises ``asyncio.TimeoutError``
# which is distinct from the builtin ``TimeoutError`` (the two were
# unified in 3.11). Catching the asyncio alias works on every
# supported version.
- raise ValueError(
- f"Search did not complete within {_SEARCH_TIMEOUT_SECONDS:g} seconds. The bound covers "
- "the whole search, so this is either a pathological pattern (avoid nested quantifiers "
- "such as '(a+)+') or a store too slow to read this many files in time. Narrow the "
- "pattern, or search a smaller directory."
- ) from exc
+ raise ValueError(_search_timeout_message()) from exc
def _normalize_relative_path(path: str, *, is_directory: bool = False) -> str:
@@ -595,7 +691,7 @@ def __repr__(self) -> str:
return f"FileStoreEntry(name={self.name!r}, type={self.type!r})"
-def _search_file_content(file_name: str, content: str, regex: re.Pattern[str]) -> FileSearchResult | None:
+def _search_file_content(file_name: str, content: str, regex: _SearchPattern) -> FileSearchResult | None:
r"""Search one file's content and return a :class:`FileSearchResult` if any lines match.
Lines are split by :func:`_split_lines_keepends` and reported verbatim, terminator
@@ -729,7 +825,7 @@ def split_lines(content: str) -> list[str]:
return _split_lines_keepends(content)
@staticmethod
- def scan_content(file_name: str, content: str, regex: re.Pattern[str]) -> FileSearchResult | None:
+ def scan_content(file_name: str, content: str, regex: _SearchPattern) -> FileSearchResult | None:
"""Find every line of ``content`` matching ``regex``, numbered by :meth:`split_lines`.
This is the numbering primitive the base :meth:`search` uses, published so a
@@ -741,7 +837,11 @@ def scan_content(file_name: str, content: str, regex: re.Pattern[str]) -> FileSe
file_name: The name recorded on the result, relative to the searched directory.
content: The file's full text.
regex: A compiled pattern, normally from the same source string passed
- to :meth:`search`.
+ to :meth:`search`. Anything exposing ``pattern`` and ``search`` works,
+ including a plain :class:`re.Pattern`. Note that a store passing its own
+ ``re.Pattern`` here opts out of the deadline the built-in stores apply,
+ and so must bound a model-supplied pattern by other means -- see
+ :func:`_compile_search_regex` for why ``re`` cannot be interrupted.
Returns:
The match metadata, or ``None`` when no line matches.
@@ -866,7 +966,7 @@ async def search(
async def _scan_candidate_files(
self,
directory: str,
- regex: re.Pattern[str],
+ regex: _SearchPattern,
glob_pattern: str | None,
recursive: bool,
) -> list[FileSearchResult]:
@@ -1387,7 +1487,7 @@ def _enumerate_search_files(full_dir: Path, recursive: bool) -> list[tuple[str,
@staticmethod
def _search_files_sync(
- full_dir: Path, regex: re.Pattern[str], glob_pattern: str | None, recursive: bool
+ full_dir: Path, regex: _SearchPattern, glob_pattern: str | None, recursive: bool
) -> list[FileSearchResult]:
if not full_dir.is_dir():
return []
diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py
index ff2756cc60d..71bdfcf2646 100644
--- a/python/packages/core/agent_framework/_tools.py
+++ b/python/packages/core/agent_framework/_tools.py
@@ -3061,9 +3061,7 @@ def _stage_approval_batch_responses(
if any(request_id not in stored_responses for request_id in group_ids):
updated_group = dict(group)
updated_group[_APPROVAL_RESPONSES_KEY] = [
- stored_responses[request_id].to_dict()
- for request_id in group_ids
- if request_id in stored_responses
+ stored_responses[request_id].to_dict() for request_id in group_ids if request_id in stored_responses
]
remaining_groups.append(updated_group)
missing_request_ids = [request_id for request_id in group_ids if request_id not in stored_responses]
diff --git a/python/packages/core/agent_framework/_workflows/_agent_executor.py b/python/packages/core/agent_framework/_workflows/_agent_executor.py
index a56cf2d0331..2dd0367ac33 100644
--- a/python/packages/core/agent_framework/_workflows/_agent_executor.py
+++ b/python/packages/core/agent_framework/_workflows/_agent_executor.py
@@ -344,9 +344,7 @@ async def _cancel_pending_request(
"""Release an agent-owned user-input request after workflow cancellation."""
cancelled_request = self._pending_agent_requests.pop(request_id, None)
if cancelled_request is not None and cancelled_request.type == "function_approval_request":
- self._pending_responses_to_agent.append(
- cancelled_request.to_function_approval_response(approved=False)
- )
+ self._pending_responses_to_agent.append(cancelled_request.to_function_approval_response(approved=False))
elif (
cancelled_request is not None
and cancelled_request.type == "function_call"
diff --git a/python/packages/core/pyproject.toml b/python/packages/core/pyproject.toml
index c1ff2524d59..c78dfc34e70 100644
--- a/python/packages/core/pyproject.toml
+++ b/python/packages/core/pyproject.toml
@@ -29,6 +29,7 @@ dependencies = [
"python-dotenv>=1,<2",
"opentelemetry-api>=1.39.0,<2",
"pyyaml>=6.0,<7.0",
+ "regex>=2024.11.6",
]
[project.optional-dependencies]
diff --git a/python/packages/core/tests/core/test_function_invocation_logic.py b/python/packages/core/tests/core/test_function_invocation_logic.py
index f97e80ea0f8..f85ba431291 100644
--- a/python/packages/core/tests/core/test_function_invocation_logic.py
+++ b/python/packages/core/tests/core/test_function_invocation_logic.py
@@ -2327,9 +2327,10 @@ def second_write() -> str:
for content in message.contents
if content.type == "function_approval_request"
]
- assert [
- request.function_call.name for request in approval_requests if request.function_call is not None
- ] == ["first_write", "second_write"]
+ assert [request.function_call.name for request in approval_requests if request.function_call is not None] == [
+ "first_write",
+ "second_write",
+ ]
await chat_client_base.get_response(
[
diff --git a/python/packages/core/tests/core/test_harness_file_access.py b/python/packages/core/tests/core/test_harness_file_access.py
index da7b4255f40..f3199e9794c 100644
--- a/python/packages/core/tests/core/test_harness_file_access.py
+++ b/python/packages/core/tests/core/test_harness_file_access.py
@@ -929,6 +929,101 @@ async def slow_pipeline() -> list[FileSearchResult]:
await _run_search_with_timeout(slow_pipeline())
+# region ReDoS guard
+
+# A pattern that forces catastrophic backtracking, plus a subject it cannot match.
+#
+# ``(a+)+$`` is the textbook example and the one the .NET suite uses, but it is useless
+# here: the ``regex`` engine optimises it away and returns instantly, so it would pass
+# against an unguarded implementation and prove nothing. ``(a|a)*$`` still backtracks
+# exponentially under both engines, so it actually exercises the guard.
+#
+# The trap is sized so an unguarded ``re`` scan takes tens of seconds -- long enough to
+# fail the assertions below decisively, short enough that a regression cannot wedge CI
+# forever. Every doubling of the length doubles the unguarded runtime.
+_REDOS_PATTERN = r"(a|a)*$"
+_REDOS_TRAP = "a" * 26 + "!"
+
+# Deadline used by the guard tests. Short, because a working guard returns at the
+# deadline; only a broken one runs long.
+_REDOS_TIMEOUT_SECONDS = 0.3
+
+
+async def _assert_search_is_bounded(
+ store: AgentFileStore,
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """Assert a ReDoS pattern is refused promptly without stalling the event loop.
+
+ Two properties are checked, because either one alone can pass against a broken
+ implementation:
+
+ * The search returns (as a ``ValueError``) close to the deadline rather than
+ running the match to completion.
+ * A concurrent task keeps being scheduled *throughout* the search. This is the
+ property the original report turned on: offloading the scan to a worker thread
+ bounds nothing on its own, because CPython's ``re`` engine holds the GIL for the
+ duration of a single match, so the timeout cannot fire and unrelated work on the
+ same loop stops.
+ """
+ monkeypatch.setattr(_file_access_module, "_SEARCH_TIMEOUT_SECONDS", _REDOS_TIMEOUT_SECONDS)
+ await store.write("trap.txt", _REDOS_TRAP)
+
+ beats = 0
+ stop = asyncio.Event()
+
+ async def heartbeat() -> None:
+ nonlocal beats
+ while not stop.is_set():
+ await asyncio.sleep(_REDOS_TIMEOUT_SECONDS / 10)
+ beats += 1
+
+ beating = asyncio.create_task(heartbeat())
+ # Let the heartbeat reach its first await so the count reflects the search window only.
+ await asyncio.sleep(0)
+ started = time.monotonic()
+ try:
+ with pytest.raises(ValueError, match="did not complete"):
+ await store.search("", _REDOS_PATTERN)
+ elapsed = time.monotonic() - started
+ finally:
+ stop.set()
+ beating.cancel()
+
+ # Generous ceiling: the point is that the deadline is enforced at all, not that it is
+ # precise. An unguarded scan overshoots by orders of magnitude, not by a factor of ten.
+ assert elapsed < _REDOS_TIMEOUT_SECONDS * 10, f"search overran its deadline: {elapsed:.2f}s"
+ assert beats >= 2, f"event loop stalled during the search: {beats} heartbeat(s)"
+
+
+async def test_in_memory_store_search_bounds_catastrophic_backtracking(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A ReDoS pattern must not wedge the in-memory store's scan."""
+ await _assert_search_is_bounded(InMemoryAgentFileStore(), monkeypatch)
+
+
+async def test_filesystem_store_search_bounds_catastrophic_backtracking(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ """A ReDoS pattern must not wedge the filesystem store's scan."""
+ await _assert_search_is_bounded(FileSystemAgentFileStore(tmp_path), monkeypatch)
+
+
+async def test_base_search_pipeline_bounds_catastrophic_backtracking(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """The inherited ``search`` pipeline must be bounded too.
+
+ A store that implements only the mandatory members gets this pipeline for free, so
+ it is the shape most third-party stores will run.
+ """
+ await _assert_search_is_bounded(_ContentOnlyStore(), monkeypatch)
+
+
+# endregion
+
+
async def test_filesystem_store_symlink_probe_fails_closed_on_oserror(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
diff --git a/python/packages/core/tests/core/test_harness_tool_approval.py b/python/packages/core/tests/core/test_harness_tool_approval.py
index cf5e179153c..1a96bd97088 100644
--- a/python/packages/core/tests/core/test_harness_tool_approval.py
+++ b/python/packages/core/tests/core/test_harness_tool_approval.py
@@ -1658,9 +1658,9 @@ def second_read() -> str:
)
assert execution_order == []
- assert [
- _function_call(request).name for request in _approval_requests(partial_response.messages)
- ] == ["first_write"]
+ assert [_function_call(request).name for request in _approval_requests(partial_response.messages)] == [
+ "first_write"
+ ]
repeated_partial_response = await agent.run(
requests[1].to_function_approval_response(approved=True),
diff --git a/python/packages/core/tests/workflow/test_agent_executor_tool_calls.py b/python/packages/core/tests/workflow/test_agent_executor_tool_calls.py
index 46278205945..8c931260fe8 100644
--- a/python/packages/core/tests/workflow/test_agent_executor_tool_calls.py
+++ b/python/packages/core/tests/workflow/test_agent_executor_tool_calls.py
@@ -568,9 +568,7 @@ async def _get_response() -> ChatResponse:
def _create_response(self) -> ChatResponse:
if self._iteration == 0:
if self._mixed_request:
- response = ChatResponse(
- messages=Message("assistant", self._mixed_request_contents())
- )
+ response = ChatResponse(messages=Message("assistant", self._mixed_request_contents()))
elif self._parallel_request:
response = ChatResponse(
messages=Message(
diff --git a/python/packages/tools/agent_framework_tools/shell/_policy.py b/python/packages/tools/agent_framework_tools/shell/_policy.py
index cf5097acb40..584118acbda 100644
--- a/python/packages/tools/agent_framework_tools/shell/_policy.py
+++ b/python/packages/tools/agent_framework_tools/shell/_policy.py
@@ -36,13 +36,49 @@
from __future__ import annotations
+import logging
import re
from collections.abc import Callable, Sequence
from dataclasses import dataclass, field
-from typing import Literal, Union
+from typing import Any, Literal, Union
+
+import regex as regex_module
+
+logger = logging.getLogger(__name__)
PatternLike = Union[str, re.Pattern[str]]
+# Wall-clock bound on a single pattern match.
+#
+# Policy patterns are operator-authored, but the command they are matched against is
+# model-generated and therefore attacker-influenced. An ambiguous pattern can be pushed
+# into catastrophic backtracking by a crafted command, and ``evaluate`` is synchronous --
+# it runs on the caller's thread with no offload -- so an unbounded match stalls the whole
+# process. One second is far longer than any realistic policy match needs.
+_PATTERN_MATCH_TIMEOUT_SECONDS = 1.0
+
+
+class _PatternTimeout(Exception):
+ """Raised when a single policy pattern match exceeds its budget."""
+
+
+def _search(pattern: Any, command: str) -> bool:
+ """Return whether ``pattern`` matches ``command``.
+
+ Raises:
+ _PatternTimeout: When a bounded pattern exceeds
+ :data:`_PATTERN_MATCH_TIMEOUT_SECONDS`. Callers decide what a non-answer
+ means for them; both call sites in :meth:`ShellPolicy.evaluate` fail closed.
+ """
+ if isinstance(pattern, re.Pattern):
+ # An operator handed us a pre-compiled ``re`` pattern; its flags are its own and
+ # ``re`` cannot be interrupted, so this one is matched unbounded.
+ return pattern.search(command) is not None # pyright: ignore[reportUnknownMemberType]
+ try:
+ return pattern.search(command, timeout=_PATTERN_MATCH_TIMEOUT_SECONDS) is not None
+ except TimeoutError as exc:
+ raise _PatternTimeout from exc
+
@dataclass(frozen=True)
class ShellRequest:
@@ -60,10 +96,20 @@ class ShellDecision:
reason: str = ""
-def _compile_patterns(patterns: Sequence[PatternLike]) -> tuple[re.Pattern[str], ...]:
- compiled: list[re.Pattern[str]] = []
+def _compile_patterns(patterns: Sequence[PatternLike]) -> tuple[Any, ...]:
+ """Compile policy patterns, preferring the interruptible ``regex`` engine.
+
+ A pattern given as a string is compiled with ``regex`` in its ``re``-compatible
+ dialect, so the match can be bounded by :func:`_search`. A caller who hands over an
+ already-compiled :class:`re.Pattern` keeps it verbatim -- re-compiling would silently
+ reinterpret whichever flags they set -- and forgoes the bound.
+ """
+ compiled: list[Any] = []
for pat in patterns:
- compiled.append(pat if isinstance(pat, re.Pattern) else re.compile(pat, re.IGNORECASE))
+ if isinstance(pat, re.Pattern):
+ compiled.append(pat)
+ else:
+ compiled.append(regex_module.compile(pat, flags=regex_module.IGNORECASE | regex_module.VERSION0))
return tuple(compiled)
@@ -92,8 +138,8 @@ class ShellPolicy:
allowlist: Sequence[PatternLike] | None = None
custom: Callable[[ShellRequest], ShellDecision | None] | None = None
- _denies: tuple[re.Pattern[str], ...] = field(init=False, repr=False, compare=False)
- _allows: tuple[re.Pattern[str], ...] | None = field(init=False, repr=False, compare=False)
+ _denies: tuple[Any, ...] = field(init=False, repr=False, compare=False)
+ _allows: tuple[Any, ...] | None = field(init=False, repr=False, compare=False)
def __post_init__(self) -> None:
self._denies = _compile_patterns(self.denylist)
@@ -105,14 +151,23 @@ def evaluate(self, request: ShellRequest) -> ShellDecision:
Empty/whitespace-only commands are denied (there is nothing to
run). With default settings (no denylist, no allowlist) every
non-empty command is allowed.
+
+ A pattern that exceeds its match budget yields no answer, and both lists treat a
+ non-answer as a denial: an unproven denylist pattern is assumed to have matched,
+ and an unproven allowlist pattern is assumed not to have.
"""
command = request.command.strip()
if not command:
return ShellDecision("deny", "command is empty")
for pat in self._denies:
- if pat.search(command):
+ try:
+ hit = _search(pat, command)
+ except _PatternTimeout:
+ logger.warning("Denylist pattern timed out; denying command: %s", pat.pattern)
+ return ShellDecision("deny", f"denylist pattern could not be evaluated in time: {pat.pattern}")
+ if hit:
return ShellDecision("deny", f"matches denylist pattern: {pat.pattern}")
- if self._allows is not None and not any(pat.search(command) for pat in self._allows):
+ if self._allows is not None and not self._matches_allowlist(command):
return ShellDecision("deny", "command does not match allowlist")
if self.custom is not None:
override = self.custom(request)
@@ -120,6 +175,21 @@ def evaluate(self, request: ShellRequest) -> ShellDecision:
return override
return ShellDecision("allow")
+ def _matches_allowlist(self, command: str) -> bool:
+ """Return whether any allowlist pattern matches ``command``.
+
+ A pattern that times out is skipped rather than counted as a match: the allowlist
+ grants permission, so an unproven pattern must not be the thing that grants it.
+ """
+ assert self._allows is not None # nosec B101 - guarded by the caller
+ for pat in self._allows:
+ try:
+ if _search(pat, command):
+ return True
+ except _PatternTimeout:
+ logger.warning("Allowlist pattern timed out; not counting it as a match: %s", pat.pattern)
+ return False
+
def evaluate_command(self, command: str) -> ShellDecision:
"""Convenience: evaluate a bare command with no workdir context."""
return self.evaluate(ShellRequest(command=command))
diff --git a/python/packages/tools/pyproject.toml b/python/packages/tools/pyproject.toml
index e443c16e1ca..7b8c15cbf12 100644
--- a/python/packages/tools/pyproject.toml
+++ b/python/packages/tools/pyproject.toml
@@ -28,6 +28,7 @@ dependencies = [
# may survive timeout on Windows" and "they don't" — a security-relevant
# property, not an optional one.
"psutil>=5.9",
+ "regex>=2024.11.6",
]
[tool.uv]
diff --git a/python/packages/tools/tests/test_policy.py b/python/packages/tools/tests/test_policy.py
index 12cc65b3955..d23b2e81a3b 100644
--- a/python/packages/tools/tests/test_policy.py
+++ b/python/packages/tools/tests/test_policy.py
@@ -1,5 +1,8 @@
# Copyright (c) Microsoft. All rights reserved.
+import re
+import time
+
from agent_framework_tools.shell import ShellDecision, ShellPolicy, ShellRequest
# Representative destructive-rm patterns used to exercise the deny-list
@@ -77,3 +80,43 @@ def veto(req: ShellRequest) -> ShellDecision | None:
policy = ShellPolicy(custom=veto)
assert _decide(policy, "echo hello").decision == "allow"
assert _decide(policy, "cat my_secret.env").decision == "deny"
+
+
+# A pattern that backtracks catastrophically, plus a command it cannot match. An operator
+# could plausibly write something this shape while trying to match a spaced-out command
+# line; the model then only has to supply the subject to stall the match.
+_REDOS_PATTERN = r"(a|a)*$"
+_REDOS_COMMAND = "a" * 26 + "!"
+
+
+def test_denylist_pattern_timeout_denies() -> None:
+ """A denylist pattern that cannot be evaluated in time must fail closed."""
+ policy = ShellPolicy(denylist=[_REDOS_PATTERN])
+
+ started = time.monotonic()
+ decision = _decide(policy, _REDOS_COMMAND)
+ elapsed = time.monotonic() - started
+
+ assert decision.decision == "deny"
+ assert "could not be evaluated in time" in decision.reason
+ assert elapsed < 5.0, f"policy evaluation overran: {elapsed:.2f}s"
+
+
+def test_allowlist_pattern_timeout_does_not_grant_access() -> None:
+ """An allowlist pattern that times out must not be what grants permission."""
+ policy = ShellPolicy(allowlist=[_REDOS_PATTERN])
+
+ started = time.monotonic()
+ decision = _decide(policy, _REDOS_COMMAND)
+ elapsed = time.monotonic() - started
+
+ assert decision.decision == "deny"
+ assert "does not match allowlist" in decision.reason
+ assert elapsed < 5.0, f"policy evaluation overran: {elapsed:.2f}s"
+
+
+def test_precompiled_re_pattern_still_supported() -> None:
+ """Handing over an already-compiled ``re`` pattern keeps working."""
+ policy = ShellPolicy(denylist=[re.compile(r"^ssh\b", re.IGNORECASE)])
+ assert _decide(policy, "ssh host").decision == "deny"
+ assert _decide(policy, "ls").decision == "allow"
diff --git a/python/uv.lock b/python/uv.lock
index 27783a3ef46..e931333ce74 100644
--- a/python/uv.lock
+++ b/python/uv.lock
@@ -450,6 +450,7 @@ dependencies = [
{ name = "pydantic", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
{ name = "python-dotenv", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
{ name = "pyyaml", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
+ { name = "regex", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
{ name = "typing-extensions", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
]
@@ -532,6 +533,7 @@ requires-dist = [
{ name = "pydantic", specifier = ">=2,<3" },
{ name = "python-dotenv", specifier = ">=1,<2" },
{ name = "pyyaml", specifier = ">=6.0,<7.0" },
+ { name = "regex", specifier = ">=2024.11.6" },
{ name = "typing-extensions", specifier = ">=4.15.0,<5" },
]
provides-extras = ["all"]
@@ -1004,12 +1006,14 @@ source = { editable = "packages/tools" }
dependencies = [
{ name = "agent-framework-core", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
{ name = "psutil", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
+ { name = "regex", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
]
[package.metadata]
requires-dist = [
{ name = "agent-framework-core", editable = "packages/core" },
{ name = "psutil", specifier = ">=5.9" },
+ { name = "regex", specifier = ">=2024.11.6" },
]
[[package]]
From c76ae3663427cf0d6eb8a7a28a874180936cea9e Mon Sep 17 00:00:00 2001
From: westey <164392973+westey-m@users.noreply.github.com>
Date: Fri, 18 Sep 2026 13:51:30 +0000
Subject: [PATCH 2/4] Address PR comments
---
.../ShellPolicyTests.cs | 1 +
.../agent_framework/_harness/_file_access.py | 50 +++++++++----------
.../tests/core/test_harness_file_access.py | 6 ++-
.../tests/core/test_harness_file_memory.py | 4 +-
.../agent_framework_tools/shell/_policy.py | 33 +++++++++---
python/packages/tools/pyproject.toml | 1 -
python/packages/tools/tests/test_policy.py | 22 ++++++++
python/uv.lock | 2 -
8 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
index 2425aa5c160..9b8ef579512 100644
--- a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
+++ b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft. All rights reserved.
+using System;
using System.Diagnostics;
namespace Microsoft.Agents.AI.Tools.Shell.UnitTests;
diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py
index f4c935e2aab..439e780dfdf 100644
--- a/python/packages/core/agent_framework/_harness/_file_access.py
+++ b/python/packages/core/agent_framework/_harness/_file_access.py
@@ -26,14 +26,13 @@
import fnmatch
import logging
import os
-import re
import time
from abc import ABC, abstractmethod
from collections.abc import Awaitable, Mapping, MutableMapping
from pathlib import Path
from typing import Annotated, Any, ClassVar, Protocol, cast
-import regex as regex_module
+import regex
from pydantic import BaseModel, Field
from .._feature_stage import ExperimentalFeature, experimental
@@ -178,27 +177,24 @@ def _compile_search_regex(pattern: str) -> _BoundedSearchPattern:
finally returns. ``regex`` checks a deadline mid-match and releases the GIL while
matching, which is what makes the bound real and keeps the event loop responsive.
- An invalid ``pattern`` raises :class:`re.error` unchanged so the search tools surface
- it to the calling model, which can correct the pattern and retry. ``regex`` reports
- syntax errors with its own ``regex.error``, which is *not* a subclass of ``re.error``,
- so it is translated here to keep that contract.
+ An invalid ``pattern`` raises :class:`regex.error` unchanged so the search tools surface
+ it to the calling model, which can correct the pattern and retry. Note that
+ ``regex.error`` is *not* a subclass of the standard library's ``re.error``.
Raises:
ValueError: When ``pattern`` exceeds ``_MAX_SEARCH_PATTERN_LENGTH``
characters.
- re.error: When ``pattern`` is not a valid regular expression.
+ regex.error: When ``pattern`` is not a valid regular expression.
"""
if len(pattern) > _MAX_SEARCH_PATTERN_LENGTH:
raise ValueError(
f"Regex pattern is too long ({len(pattern)} characters). "
f"Maximum supported length is {_MAX_SEARCH_PATTERN_LENGTH} characters."
)
- try:
- # VERSION0 keeps ``regex`` in its ``re``-compatible dialect, so a pattern that
- # worked against the standard library keeps the same meaning here.
- compiled = regex_module.compile(pattern, flags=regex_module.IGNORECASE | regex_module.VERSION0)
- except regex_module.error as exc:
- raise re.error(str(exc)) from exc
+ # VERSION1 is selected explicitly rather than left to ``regex.DEFAULT_VERSION``, which is
+ # a mutable process global: any library in the process can flip it and silently change how
+ # these patterns parse.
+ compiled = regex.compile(pattern, flags=regex.IGNORECASE | regex.VERSION1)
# The deadline starts here, not at first match: the budget covers the whole search,
# including the file reads the scan is interleaved with.
return _BoundedSearchPattern(compiled, pattern, time.monotonic() + _SEARCH_TIMEOUT_SECONDS)
@@ -691,7 +687,7 @@ def __repr__(self) -> str:
return f"FileStoreEntry(name={self.name!r}, type={self.type!r})"
-def _search_file_content(file_name: str, content: str, regex: _SearchPattern) -> FileSearchResult | None:
+def _search_file_content(file_name: str, content: str, search_pattern: _SearchPattern) -> FileSearchResult | None:
r"""Search one file's content and return a :class:`FileSearchResult` if any lines match.
Lines are split by :func:`_split_lines_keepends` and reported verbatim, terminator
@@ -713,7 +709,7 @@ def _search_file_content(file_name: str, content: str, regex: _SearchPattern) ->
# Same rule as _strip_line_terminator: a lone \r is content, so only a whole
# \r\n comes off.
scanned = line[:-2] if line.endswith("\r\n") else line.removesuffix("\n")
- match = regex.search(scanned)
+ match = search_pattern.search(scanned)
if match is not None:
matching_lines.append(FileSearchMatch(line_number=line_number, line=line))
if first_snippet is None:
@@ -960,13 +956,15 @@ async def search(
ValueError: When the search does not complete within
:data:`_SEARCH_TIMEOUT_SECONDS` seconds.
"""
- regex = _compile_search_regex(regex_pattern)
- return await _run_search_with_timeout(self._scan_candidate_files(directory, regex, glob_pattern, recursive))
+ search_pattern = _compile_search_regex(regex_pattern)
+ return await _run_search_with_timeout(
+ self._scan_candidate_files(directory, search_pattern, glob_pattern, recursive)
+ )
async def _scan_candidate_files(
self,
directory: str,
- regex: _SearchPattern,
+ search_pattern: _SearchPattern,
glob_pattern: str | None,
recursive: bool,
) -> list[FileSearchResult]:
@@ -976,7 +974,7 @@ async def _scan_candidate_files(
over-returns (which :meth:`find_matching_files` explicitly permits)
cannot widen the caller's scope.
"""
- names = await self.find_matching_files(directory, regex.pattern, glob_pattern, recursive=recursive)
+ names = await self.find_matching_files(directory, search_pattern.pattern, glob_pattern, recursive=recursive)
results: list[FileSearchResult] = []
batch: list[tuple[str, str]] = []
batch_chars = 0
@@ -987,7 +985,7 @@ def scan_batch(pending: list[tuple[str, str]]) -> list[FileSearchResult]:
# Called on the class, not the instance: a store that overrides the public
# scan_content must not be able to skew the numbers while still counting as
# aligned by construction.
- result = AgentFileStore.scan_content(candidate_name, candidate_content, regex)
+ result = AgentFileStore.scan_content(candidate_name, candidate_content, search_pattern)
if result is not None:
found.append(result)
return found
@@ -1146,7 +1144,7 @@ async def search(
prefix = _normalize_relative_path(directory, is_directory=True).lower()
if prefix and not prefix.endswith("/"):
prefix += "/"
- regex = _compile_search_regex(regex_pattern)
+ search_pattern = _compile_search_regex(regex_pattern)
async with self._lock:
entries = [(key, display, content) for key, (display, content) in self._files.items()]
@@ -1162,7 +1160,7 @@ def scan() -> list[FileSearchResult]:
relative_display = display[len(prefix) :]
if not _matches_glob(relative_display, glob_pattern):
continue
- result = AgentFileStore.scan_content(relative_display, file_content, regex)
+ result = AgentFileStore.scan_content(relative_display, file_content, search_pattern)
if result is not None:
results.append(result)
return results
@@ -1450,9 +1448,9 @@ async def search(
children.
"""
full_dir = self._resolve_safe_directory_path(directory)
- regex = _compile_search_regex(regex_pattern)
+ search_pattern = _compile_search_regex(regex_pattern)
return await _run_search_with_timeout(
- asyncio.to_thread(self._search_files_sync, full_dir, regex, glob_pattern, recursive)
+ asyncio.to_thread(self._search_files_sync, full_dir, search_pattern, glob_pattern, recursive)
)
@staticmethod
@@ -1487,7 +1485,7 @@ def _enumerate_search_files(full_dir: Path, recursive: bool) -> list[tuple[str,
@staticmethod
def _search_files_sync(
- full_dir: Path, regex: _SearchPattern, glob_pattern: str | None, recursive: bool
+ full_dir: Path, search_pattern: _SearchPattern, glob_pattern: str | None, recursive: bool
) -> list[FileSearchResult]:
if not full_dir.is_dir():
return []
@@ -1518,7 +1516,7 @@ def _search_files_sync(
logger.warning("Skipping unreadable file during search: %s", entry)
skipped.append(relative_name)
continue
- result = AgentFileStore.scan_content(relative_name, file_content, regex)
+ result = AgentFileStore.scan_content(relative_name, file_content, search_pattern)
if result is not None:
results.append(result)
if skipped:
diff --git a/python/packages/core/tests/core/test_harness_file_access.py b/python/packages/core/tests/core/test_harness_file_access.py
index f3199e9794c..68ef1e775ce 100644
--- a/python/packages/core/tests/core/test_harness_file_access.py
+++ b/python/packages/core/tests/core/test_harness_file_access.py
@@ -12,6 +12,7 @@
from types import SimpleNamespace
import pytest
+import regex
from agent_framework import (
Agent,
@@ -254,7 +255,7 @@ async def test_in_memory_store_search_rejects_invalid_and_oversize_regex() -> No
store = InMemoryAgentFileStore()
await store.write("a.md", "hello")
- with pytest.raises(re.error):
+ with pytest.raises(regex.error):
await store.search("", "[unclosed")
with pytest.raises(ValueError, match="too long"):
@@ -989,6 +990,7 @@ async def heartbeat() -> None:
finally:
stop.set()
beating.cancel()
+ await asyncio.gather(beating, return_exceptions=True)
# Generous ceiling: the point is that the deadline is enforced at all, not that it is
# precise. An unguarded scan overshoots by orders of magnitude, not by a factor of ten.
@@ -1170,7 +1172,7 @@ async def test_file_access_tool_wrappers_surface_value_error_as_message(
# An invalid regex is surfaced to the caller (the model) as a raised error
# so it can correct the pattern and retry.
- with pytest.raises(re.error):
+ with pytest.raises(regex.error):
await search.invoke(arguments={"regex_pattern": "[unclosed"})
diff --git a/python/packages/core/tests/core/test_harness_file_memory.py b/python/packages/core/tests/core/test_harness_file_memory.py
index 1442b151890..4f2e810c858 100644
--- a/python/packages/core/tests/core/test_harness_file_memory.py
+++ b/python/packages/core/tests/core/test_harness_file_memory.py
@@ -3,9 +3,9 @@
from __future__ import annotations
import json
-import re
import pytest
+import regex
from agent_framework import (
AgentFileStore,
@@ -388,7 +388,7 @@ async def test_search_propagates_invalid_regex() -> None:
provider = FileMemoryProvider(store=InMemoryAgentFileStore())
_, tools = await _prepare(provider)
- with pytest.raises(re.error):
+ with pytest.raises(regex.error):
await tools["file_memory_grep"].invoke(arguments={"regex_pattern": "[unclosed"})
diff --git a/python/packages/tools/agent_framework_tools/shell/_policy.py b/python/packages/tools/agent_framework_tools/shell/_policy.py
index 584118acbda..eb515ba6f28 100644
--- a/python/packages/tools/agent_framework_tools/shell/_policy.py
+++ b/python/packages/tools/agent_framework_tools/shell/_policy.py
@@ -42,11 +42,11 @@
from dataclasses import dataclass, field
from typing import Any, Literal, Union
-import regex as regex_module
+import regex
logger = logging.getLogger(__name__)
-PatternLike = Union[str, re.Pattern[str]]
+PatternLike = Union[str, re.Pattern[str], regex.Pattern[str]]
# Wall-clock bound on a single pattern match.
#
@@ -99,17 +99,21 @@ class ShellDecision:
def _compile_patterns(patterns: Sequence[PatternLike]) -> tuple[Any, ...]:
"""Compile policy patterns, preferring the interruptible ``regex`` engine.
- A pattern given as a string is compiled with ``regex`` in its ``re``-compatible
- dialect, so the match can be bounded by :func:`_search`. A caller who hands over an
- already-compiled :class:`re.Pattern` keeps it verbatim -- re-compiling would silently
- reinterpret whichever flags they set -- and forgoes the bound.
+ A pattern given as a string is compiled with ``regex``, so the match can be bounded by
+ :func:`_search`. An already-compiled pattern is kept verbatim -- re-compiling would
+ silently reinterpret whichever flags the caller set. A pre-compiled :class:`regex.Pattern`
+ is still bounded; a pre-compiled :class:`re.Pattern` is not, because ``re`` offers no way
+ to interrupt a match.
"""
compiled: list[Any] = []
for pat in patterns:
- if isinstance(pat, re.Pattern):
+ if isinstance(pat, (re.Pattern, regex.Pattern)):
compiled.append(pat)
else:
- compiled.append(regex_module.compile(pat, flags=regex_module.IGNORECASE | regex_module.VERSION0))
+ # VERSION1 is selected explicitly rather than left to ``regex.DEFAULT_VERSION``,
+ # which is a mutable process global: any library in the process can flip it and
+ # silently change how these patterns parse.
+ compiled.append(regex.compile(pat, flags=regex.IGNORECASE | regex.VERSION1))
return tuple(compiled)
@@ -132,6 +136,19 @@ class ShellPolicy:
Supply ``denylist`` and/or ``allowlist`` explicitly to enable filtering.
See the module docstring for why the framework does not ship default
deny patterns.
+
+ .. warning::
+ Policy patterns are **developer-authored code**, not model or user input, so
+ testing them is the developer's responsibility. A pattern with nested or
+ ambiguous quantifiers (``(a|a)*``, ``(a+)+``) can be pushed into catastrophic
+ backtracking by a crafted command, and the command *is* model-generated.
+
+ Prefer plain ``str`` patterns: the framework compiles those on the ``regex``
+ engine and bounds every match at one second, failing closed on timeout. A
+ pre-compiled :class:`regex.Pattern` is bounded the same way. A pre-compiled
+ :class:`re.Pattern` is honoured verbatim and is **not** bounded -- the standard
+ library offers no way to interrupt a match -- so an expensive pattern supplied
+ that way can stall the calling thread indefinitely.
"""
denylist: Sequence[PatternLike] = field(default_factory=tuple)
diff --git a/python/packages/tools/pyproject.toml b/python/packages/tools/pyproject.toml
index 7b8c15cbf12..e443c16e1ca 100644
--- a/python/packages/tools/pyproject.toml
+++ b/python/packages/tools/pyproject.toml
@@ -28,7 +28,6 @@ dependencies = [
# may survive timeout on Windows" and "they don't" — a security-relevant
# property, not an optional one.
"psutil>=5.9",
- "regex>=2024.11.6",
]
[tool.uv]
diff --git a/python/packages/tools/tests/test_policy.py b/python/packages/tools/tests/test_policy.py
index d23b2e81a3b..b3acd66d732 100644
--- a/python/packages/tools/tests/test_policy.py
+++ b/python/packages/tools/tests/test_policy.py
@@ -3,6 +3,8 @@
import re
import time
+import regex
+
from agent_framework_tools.shell import ShellDecision, ShellPolicy, ShellRequest
# Representative destructive-rm patterns used to exercise the deny-list
@@ -120,3 +122,23 @@ def test_precompiled_re_pattern_still_supported() -> None:
policy = ShellPolicy(denylist=[re.compile(r"^ssh\b", re.IGNORECASE)])
assert _decide(policy, "ssh host").decision == "deny"
assert _decide(policy, "ls").decision == "allow"
+
+
+def test_precompiled_regex_pattern_is_supported_and_bounded() -> None:
+ """A pre-compiled ``regex`` pattern is accepted and still matched under the timeout.
+
+ This is the pattern type the class docstring recommends for callers who want to compile
+ ahead of time without giving up the match bound, so it has to work end to end.
+ """
+ policy = ShellPolicy(denylist=[regex.compile(r"^ssh\b", regex.IGNORECASE)])
+ assert _decide(policy, "ssh host").decision == "deny"
+ assert _decide(policy, "ls").decision == "allow"
+
+ bounded = ShellPolicy(denylist=[regex.compile(_REDOS_PATTERN)])
+ started = time.monotonic()
+ decision = _decide(bounded, _REDOS_COMMAND)
+ elapsed = time.monotonic() - started
+
+ assert decision.decision == "deny"
+ assert "could not be evaluated in time" in decision.reason
+ assert elapsed < 5.0, f"policy evaluation overran: {elapsed:.2f}s"
diff --git a/python/uv.lock b/python/uv.lock
index e931333ce74..97794a6251c 100644
--- a/python/uv.lock
+++ b/python/uv.lock
@@ -1006,14 +1006,12 @@ source = { editable = "packages/tools" }
dependencies = [
{ name = "agent-framework-core", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
{ name = "psutil", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
- { name = "regex", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or sys_platform == 'win32'" },
]
[package.metadata]
requires-dist = [
{ name = "agent-framework-core", editable = "packages/core" },
{ name = "psutil", specifier = ">=5.9" },
- { name = "regex", specifier = ">=2024.11.6" },
]
[[package]]
From c89e7ad6cd2d615977772e758afd7bb40114035a Mon Sep 17 00:00:00 2001
From: westey <164392973+westey-m@users.noreply.github.com>
Date: Fri, 18 Sep 2026 14:30:50 +0000
Subject: [PATCH 3/4] Fix build errors
---
.../ShellPolicy.cs | 6 +++---
.../ShellPolicyTests.cs | 20 +++++++++++++++++--
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs
index 3a0be56f2a1..1d0d2577066 100644
--- a/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs
+++ b/dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellPolicy.cs
@@ -147,7 +147,7 @@ public sealed class ShellPolicy
/// would stall the authorization path itself. The timeout makes that failure bounded and
/// recoverable rather than a hang.
///
- private static readonly TimeSpan PatternMatchTimeout = TimeSpan.FromSeconds(1);
+ private static readonly TimeSpan s_patternMatchTimeout = TimeSpan.FromSeconds(1);
private readonly IReadOnlyList _denyList;
private readonly IReadOnlyList? _allowList;
@@ -178,11 +178,11 @@ public ShellPolicy(
Func? custom = null)
{
this._denyList = denyList?
- .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, PatternMatchTimeout))
+ .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, s_patternMatchTimeout))
.ToArray() ?? Array.Empty();
this._allowList = allowList?
- .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, PatternMatchTimeout))
+ .Select(pattern => new Regex(pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase, s_patternMatchTimeout))
.ToArray();
this._custom = custom;
diff --git a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
index 9b8ef579512..2ccc2049688 100644
--- a/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
+++ b/dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/ShellPolicyTests.cs
@@ -13,19 +13,30 @@ namespace Microsoft.Agents.AI.Tools.Shell.UnitTests;
///
public sealed class ShellPolicyTests
{
- /// A pattern that backtracks exponentially, and a command it cannot match.
- private const string RedosPattern = "(a|a)*$";
+ ///
+ /// A pattern that backtracks exponentially, and a command it cannot match.
+ ///
+ ///
+ /// Deliberately not (a|a)*$, which is the equivalent used by the Python tests.
+ /// .NET matches that one instantly: (a|a)* can match zero times, so the engine
+ /// finds an empty match at the end anchor and never backtracks. (a+)+$ requires
+ /// at least one character per iteration, which forces the exponential search.
+ ///
+ 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}");
@@ -34,12 +45,15 @@ public void Evaluate_DenyPatternTimesOut_FailsClosed()
[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}");
@@ -48,8 +62,10 @@ public void Evaluate_AllowPatternTimesOut_DoesNotGrantAccess()
[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);
From 5a56e78f32ec4bd6514e88887a006c5f31364211 Mon Sep 17 00:00:00 2001
From: westey <164392973+westey-m@users.noreply.github.com>
Date: Fri, 18 Sep 2026 15:53:50 +0000
Subject: [PATCH 4/4] Address PR comment
---
.../core/agent_framework/_harness/_file_access.py | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py
index 439e780dfdf..5bfc6f592a8 100644
--- a/python/packages/core/agent_framework/_harness/_file_access.py
+++ b/python/packages/core/agent_framework/_harness/_file_access.py
@@ -229,13 +229,7 @@ async def _run_search_with_timeout(
"""
try:
return await asyncio.wait_for(work, timeout=_SEARCH_TIMEOUT_SECONDS)
- except _SearchTimeout as exc:
- raise ValueError(_search_timeout_message()) from exc
- except asyncio.TimeoutError as exc:
- # On Python 3.10 ``asyncio.wait_for`` raises ``asyncio.TimeoutError``
- # which is distinct from the builtin ``TimeoutError`` (the two were
- # unified in 3.11). Catching the asyncio alias works on every
- # supported version.
+ except (_SearchTimeout, asyncio.TimeoutError) as exc:
raise ValueError(_search_timeout_message()) from exc