fix: address multi-agent review findings (security hardening iteration 2)

Based on GPT-5.5 and Claude Opus 4.6 review of the initial implementation:

- fix(streaming): ExecuteFunctionCallAsync on rejected tool now streams a
  recovery response ('tool not available') back to the model instead of
  yield break, preventing silent stream truncation and OpenAI protocol
  violations (both agents flagged as blocker)

- fix(allowlist): AllowedMcpTools changes from fail-open (empty=allow all)
  to fail-secure (empty=deny all). Added AllowAllMcpTools bool for explicit
  dev override. Converted _AllowedMcpTools to FrozenSet<string> at ctor
  time for O(1) lookup and immutability. Added IsMcpToolAllowed() helper
  to centralize the check across all three call sites.

- fix(rag): Sanitize XML angle brackets in Heading and ChunkText using
  typographic alternatives (U+2039/U+203A) before inserting into
  <retrieved_context> block, preventing boundary-escape attacks

- fix(cache): ResponseIdValidationService.RecordResponseId sets sliding
  expiry inside the GetOrCreate factory (not in a separate Set call)
  eliminating the TOCTOU race between GetOrCreate and cache.Set. Added
  500-ID cap per user to prevent unbounded HashSet growth.

- fix(docs): Corrected endUserId param comment to say 'reserved for
  forwarding' rather than claiming it is already forwarded

Author: BenjaminMichaelis

EssentialCSharp.Chat.Shared/Models/AIOptions.cs

@@ -24,9 +24,18 @@ public class AIOptions
 
     /// <summary>
     /// Static allowlist of MCP tool names the AI agent is permitted to invoke.
-    /// When non-empty, acts as an outer gate independent of the MCP server channel:
-    /// tools not on this list are neither advertised to the model nor executed.
-    /// When empty, all tools advertised by the MCP server are allowed (development default).
+    /// Tools not on this list are neither advertised to the model nor executed.
     /// </summary>
+    /// <remarks>
+    /// When empty and <see cref="AllowAllMcpTools"/> is <c>false</c> (the default), all MCP tool
+    /// calls are denied — fail-secure. Set <see cref="AllowAllMcpTools"/> to <c>true</c> to allow
+    /// all tools without an explicit list (useful in development environments only).
+    /// </remarks>
     public List<string> AllowedMcpTools { get; set; } = [];
+
+    /// <summary>
+    /// When <c>true</c>, bypasses the <see cref="AllowedMcpTools"/> allowlist and permits all
+    /// MCP tools. Should only be set in non-production environments.
+    /// </summary>
+    public bool AllowAllMcpTools { get; set; }
 }

EssentialCSharp.Chat.Shared/Services/AIChatService.cs

@@ -4,6 +4,7 @@
 using ModelContextProtocol.Client;
 using ModelContextProtocol.Protocol;
 using OpenAI.Responses;
+using System.Collections.Frozen;
 
 namespace EssentialCSharp.Chat.Common.Services;
 
@@ -19,12 +20,14 @@ public partial class AIChatService
 #pragma warning restore OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
     private readonly AISearchService _SearchService;
     private readonly ILogger<AIChatService> _Logger;
+    private readonly FrozenSet<string> _AllowedMcpTools;
 
     public AIChatService(IOptions<AIOptions> options, AISearchService searchService, AzureOpenAIClient azureClient, ILogger<AIChatService> logger)
     {
         _Options = options.Value;
         _SearchService = searchService;
         _Logger = logger;
+        _AllowedMcpTools = _Options.AllowedMcpTools.ToFrozenSet(StringComparer.Ordinal);
 
         // Initialize Azure OpenAI client and get the Response Client from it
         _AzureClient = azureClient;
@@ -43,7 +46,8 @@ public AIChatService(IOptions<AIOptions> options, AISearchService searchService,
     /// <param name="tools">Optional tools for the AI to use</param>
     /// <param name="reasoningEffortLevel">Optional reasoning effort level for reasoning models</param>
     /// <param name="enableContextualSearch">Enable vector search for contextual information</param>
-    /// <param name="endUserId">Identifier of the authenticated end-user, forwarded to Azure OpenAI for abuse monitoring</param>
+    /// <param name="endUserId">Authenticated end-user identifier. Currently reserved for forwarding
+    /// to Azure OpenAI for abuse monitoring once the SDK exposes <c>ResponseCreationOptions.User</c>.</param>
     /// <param name="cancellationToken">Cancellation token</param>
     /// <returns>The AI response text and response ID for conversation continuity</returns>
     public async Task<(string response, string responseId)> GetChatCompletion(
@@ -73,7 +77,8 @@ public AIChatService(IOptions<AIOptions> options, AISearchService searchService,
     /// <param name="tools">Optional tools for the AI to use</param>
     /// <param name="reasoningEffortLevel">Optional reasoning effort level for reasoning models</param>
     /// <param name="enableContextualSearch">Enable vector search for contextual information</param>
-    /// <param name="endUserId">Identifier of the authenticated end-user, forwarded to Azure OpenAI for abuse monitoring</param>
+    /// <param name="endUserId">Authenticated end-user identifier. Currently reserved for forwarding
+    /// to Azure OpenAI for abuse monitoring once the SDK exposes <c>ResponseCreationOptions.User</c>.</param>
     /// <param name="cancellationToken">Cancellation token</param>
     /// <returns>An async enumerable of response text chunks and final response ID</returns>
     public async IAsyncEnumerable<(string text, string? responseId)> GetChatCompletionStream(
@@ -135,14 +140,19 @@ private async Task<string> EnrichPromptWithContext(string prompt, bool enableCon
 
         foreach (var result in searchResults)
         {
-            contextualInfo.AppendLine(System.Globalization.CultureInfo.InvariantCulture, $"**From: {result.Record.Heading}**");
-            // Truncate individual chunks to limit attack surface from future data poisoning
+            // Replace XML angle brackets to prevent retrieval content from escaping the sandbox.
+            // Use typographic alternatives (‹›) to preserve readability of C# generics in headings.
+            var heading = SanitizeForXmlContext(result.Record.Heading);
+            contextualInfo.AppendLine(System.Globalization.CultureInfo.InvariantCulture, $"**From: {heading}**");
+
+            // Truncate individual chunks to limit attack surface from future data poisoning,
+            // then sanitize any XML bracket characters.
             var chunkText = result.Record.ChunkText;
             if (chunkText?.Length > 2000)
             {
                 chunkText = chunkText[..2000];
             }
-            contextualInfo.AppendLine(chunkText);
+            contextualInfo.AppendLine(SanitizeForXmlContext(chunkText));
             contextualInfo.AppendLine();
         }
 
@@ -154,6 +164,13 @@ private async Task<string> EnrichPromptWithContext(string prompt, bool enableCon
         return contextualInfo.ToString();
     }
 
+    /// <summary>
+    /// Replaces XML angle bracket characters in retrieval content to prevent boundary escapes.
+    /// Uses typographic alternatives (‹›) rather than stripping to preserve code readability.
+    /// </summary>
+    private static string SanitizeForXmlContext(string? input) =>
+        input?.Replace("<", "\u2039").Replace(">", "\u203A") ?? string.Empty;
+
     /// <summary>
     /// Processes streaming updates from the OpenAI Responses API, handling both regular responses and function calls
     /// </summary>
@@ -219,10 +236,27 @@ private async Task<string> EnrichPromptWithContext(string prompt, bool enableCon
         [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken = default)
     {
         // Defense-in-depth: validate tool name against static allowlist before executing.
-        var allowedTools = _Options.AllowedMcpTools;
-        if (allowedTools.Count > 0 && !allowedTools.Contains(functionCallItem.FunctionName))
+        if (!IsMcpToolAllowed(functionCallItem.FunctionName))
         {
             LogMcpToolCallRejected(_Logger, functionCallItem.FunctionName);
+            // Feed a benign error back to the model so it can recover gracefully,
+            // mirroring what GetChatCompletionCore does on the non-streaming path.
+#pragma warning disable OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
+            var errorItems = new List<ResponseItem>
+            {
+                functionCallItem,
+                new FunctionCallOutputResponseItem(
+                    functionCallItem.CallId,
+                    $"Tool '{functionCallItem.FunctionName}' is not available.")
+            };
+#pragma warning restore OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
+            var recoveryStream = _ResponseClient.CreateResponseStreamingAsync(
+                errorItems, responseOptions, cancellationToken);
+            await foreach (var result in ProcessStreamingUpdatesAsync(
+                recoveryStream, responseOptions, mcpClient, toolCallDepth, cancellationToken))
+            {
+                yield return result;
+            }
             yield break;
         }
 
@@ -324,13 +358,11 @@ private async Task<ResponseCreationOptions> CreateResponseOptionsAsync(
         if (mcpClient is not null)
         {
             var mcpTools = await mcpClient.ListToolsAsync(cancellationToken: cancellationToken);
-            var allowedTools = _Options.AllowedMcpTools;
 
             foreach (McpClientTool tool in mcpTools)
             {
                 // Outer gate: skip tools not on the static config-driven allowlist.
-                // When the allowlist is empty (development default), all tools are allowed.
-                if (allowedTools.Count > 0 && !allowedTools.Contains(tool.Name))
+                if (!IsMcpToolAllowed(tool.Name))
                 {
                     LogMcpToolSkippedNotAllowed(_Logger, tool.Name);
                     continue;
@@ -396,8 +428,7 @@ private async Task<ResponseCreationOptions> CreateResponseOptionsAsync(
                 {
                     // Defense-in-depth: validate tool name against static allowlist before executing.
                     // This catches cases where the model hallucinates a tool name not on the list.
-                    var allowedTools = _Options.AllowedMcpTools;
-                    if (allowedTools.Count > 0 && !allowedTools.Contains(functionCallItem.FunctionName))
+                    if (!IsMcpToolAllowed(functionCallItem.FunctionName))
                     {
                         LogMcpToolCallRejected(_Logger, functionCallItem.FunctionName);
                         // Return a benign error to the model so it can respond gracefully
@@ -454,6 +485,17 @@ private async Task<ResponseCreationOptions> CreateResponseOptionsAsync(
         throw new InvalidOperationException("Maximum tool call iterations exceeded.");
     }
 
+    /// <summary>
+    /// Returns <c>true</c> when the named MCP tool is permitted to execute.
+    /// Respects <see cref="AIOptions.AllowAllMcpTools"/> and the <see cref="AIOptions.AllowedMcpTools"/> allowlist.
+    /// Fails secure: an empty allowlist with <see cref="AIOptions.AllowAllMcpTools"/> = false denies all tools.
+    /// </summary>
+    private bool IsMcpToolAllowed(string toolName)
+    {
+        if (_Options.AllowAllMcpTools) return true;
+        return _AllowedMcpTools.Contains(toolName);
+    }
+
     [LoggerMessage(Level = LogLevel.Information, Message = "AI tool call invoked: tool={ToolName} iteration={Iteration}")]
     private static partial void LogMcpToolCallInvoked(ILogger logger, string toolName, int iteration);
 

EssentialCSharp.Web/Services/ResponseIdValidationService.cs

@@ -14,7 +14,9 @@ namespace EssentialCSharp.Web.Services;
 /// </remarks>
 public sealed class ResponseIdValidationService(IMemoryCache cache)
 {
-    private static readonly MemoryCacheEntryOptions _CacheOptions =
+    private const int MaxTrackedIdsPerUser = 500;
+
+    private static MemoryCacheEntryOptions MakeCacheOptions() =>
         new MemoryCacheEntryOptions().SetSlidingExpiration(TimeSpan.FromHours(2));
 
     /// <summary>
@@ -28,13 +30,26 @@ public void RecordResponseId(string? userId, string? responseId)
         }
 
         var key = CacheKey(userId);
-        var ids = cache.GetOrCreate(key, _ => new HashSet<string>(StringComparer.Ordinal))!;
+        // Set the sliding expiry inside the factory so there is no window between
+        // GetOrCreate and a separate Set call where the entry could be evicted and
+        // recreated as a different HashSet instance.
+        var ids = cache.GetOrCreate(key, entry =>
+        {
+            entry.SetSlidingExpiration(TimeSpan.FromHours(2));
+            return new HashSet<string>(StringComparer.Ordinal);
+        })!;
+
         lock (ids)
         {
             ids.Add(responseId);
+            // Cap the set to prevent unbounded memory growth for users with many requests.
+            if (ids.Count > MaxTrackedIdsPerUser)
+            {
+                // Remove an arbitrary element; we only need the most recent IDs
+                // to be present — older ones falling off is acceptable.
+                ids.Remove(ids.First());
+            }
         }
-        // Re-set to refresh the sliding expiry
-        cache.Set(key, ids, _CacheOptions);
     }
 
     /// <summary>