fix: OWASP AI Agent Security hardening for chat and MCP endpoints#1073
Open
BenjaminMichaelis wants to merge 4 commits intomainfrom
Open
fix: OWASP AI Agent Security hardening for chat and MCP endpoints#1073BenjaminMichaelis wants to merge 4 commits intomainfrom
BenjaminMichaelis wants to merge 4 commits intomainfrom
Conversation
- fix-1064: Wrap RAG vector search results in <retrieved_context> XML tags with injection-prevention header; truncate chunks to 2000 chars; update system prompt to treat retrieved content as read-only reference - fix-1066: Add AllowedMcpTools static allowlist to AIOptions; filter tools at option-build time in CreateResponseOptionsAsync; add defense- in-depth check before CallToolAsync in both GetChatCompletionCore and ExecuteFunctionCallAsync - fix-1067: Add endUserId parameter to GetChatCompletion/Stream; pass userId from ChatController claims; console app passes 'console-local'; wired through CreateResponseOptionsAsync (SDK note: User property not yet available in OpenAI SDK v2.7.0) - fix-1068: Add MaxTokensPerUser=10 cap to McpApiTokenService; add GetActiveTokenCountAsync; pre-check in McpTokenController before CreateTokenAsync - fix-1069: Add ILogger<AIChatService> to AIChatService constructor; mark class partial; add [LoggerMessage] source-generated log methods for tool invocations, rejections, and contextual search; never log tool arguments or prompt/response content - fix-1070: Add ResponseIdValidationService (IMemoryCache-backed, userId->HashSet<responseId>, 2h sliding expiry); validate previousResponseId in ChatController before AI calls; record new responseIds after completion; cache-miss allows (graceful degradation) - fix-1071: Move lastResponseId from localStorage to sessionStorage (clears on tab close); add 2h TTL enforcement on history load; update clearChatHistory to clean both storage locations
…n 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
- Remove dead MakeCacheOptions() method from ResponseIdValidationService - Add comment explaining why user_question prompt is intentionally not XML-sanitized (C# generics like List<T> must not be corrupted)
BenjaminMichaelis
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies OWASP AI Agent Security hardening across the chat and MCP tool-call paths by adding prompt trust-boundary framing/sanitization, MCP tool allowlisting, per-user response-id validation, and MCP token issuance limits.
Changes:
- Hardened prompt enrichment by sandboxing retrieved vector-search content inside explicit
<retrieved_context>boundaries and sanitizing/truncating retrieved chunks. - Added a config-driven MCP tool allowlist and enforced it both when advertising tools to the model and before executing tool calls.
- Reduced client-side persistence risk and added server-side validation to bind
previousResponseIdto the authenticated user; added a per-user active MCP token cap.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| EssentialCSharp.Web/wwwroot/js/chat-module.js | Moves lastResponseId persistence to sessionStorage and adds a short TTL for local chat history. |
| EssentialCSharp.Web/Services/ResponseIdValidationService.cs | Adds server-side tracking/validation of response IDs per user using IMemoryCache. |
| EssentialCSharp.Web/Services/McpApiTokenService.cs | Adds active-token counting and introduces MaxTokensPerUser. |
| EssentialCSharp.Web/Program.cs | Registers memory cache and the response-id validation service. |
| EssentialCSharp.Web/Controllers/McpTokenController.cs | Enforces per-user active MCP token cap before issuing new tokens. |
| EssentialCSharp.Web/Controllers/ChatController.cs | Validates client-supplied previousResponseId, records issued response IDs, and plumbs endUserId. |
| EssentialCSharp.Web/appsettings.json | Updates system prompt to treat <retrieved_context> as untrusted data; adds MCP tool allowlist. |
| EssentialCSharp.Chat/Program.cs | Passes a concrete endUserId into chat calls (console host). |
| EssentialCSharp.Chat.Shared/Services/AIChatService.cs | Implements retrieved-context sandboxing, MCP allowlist enforcement, and AI security logging; plumbs endUserId. |
| EssentialCSharp.Chat.Shared/Models/AIOptions.cs | Adds AllowedMcpTools and AllowAllMcpTools configuration options. |
Comment on lines
+72
to
+75
| if (!cache.TryGetValue(CacheKey(userId), out HashSet<string>? ids)) | ||
| { | ||
| return true; // Cache miss — allow (graceful degradation) | ||
| } |
Comment on lines
+25
to
+28
| int activeCount = await tokenService.GetActiveTokenCountAsync(userId, cancellationToken); | ||
| if (activeCount >= McpApiTokenService.MaxTokensPerUser) | ||
| return BadRequest(new { Error = $"You have reached the maximum of {McpApiTokenService.MaxTokensPerUser} active MCP tokens. Revoke an existing token before creating a new one." }); | ||
|
|
Comment on lines
+25
to
+29
| // Reject history older than 2 hours to align with server-side response ID cache window | ||
| const twoHoursMs = 2 * 60 * 60 * 1000; | ||
| if (data.timestamp && (Date.now() - data.timestamp) < twoHoursMs) { | ||
| chatMessages.value = data.messages || []; | ||
| } else { |
Comment on lines
+88
to
+92
| public Task<int> GetActiveTokenCountAsync( | ||
| string userId, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| DateTime now = DateTime.UtcNow; |
Comment on lines
+60
to
+64
| public bool ValidateResponseId(string? userId, string? responseId) | ||
| { | ||
| if (string.IsNullOrEmpty(responseId)) | ||
| { | ||
| return true; // New conversation — nothing to validate |
Comment on lines
+43
to
+47
| 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()); |
| yield break; | ||
| } | ||
|
|
||
| LogMcpToolCallInvoked(_Logger, functionCallItem.FunctionName, toolCallDepth); |
Comment on lines
+346
to
+351
| // Forward the authenticated end-user's identifier to Azure OpenAI. | ||
| // This enables Microsoft Defender for Cloud's prompt-shield and abuse detection. | ||
| // See: https://learn.microsoft.com/en-us/azure/defender-for-cloud/gain-end-user-context-ai | ||
| // NOTE: The OpenAI .NET SDK (v2.7.0) does not currently expose a User property on ResponseCreationOptions. | ||
| // When the SDK adds support, set: options.User = endUserId; | ||
| _ = endUserId; // Suppress unused-variable warning until SDK support is available |
- ResponseIdValidationService: redesign from per-user HashSet to per-responseId cache entries, eliminating arbitrary eviction (ids.Remove(ids.First())) and reducing lock contention; each ID now has its own 2-hour TTL - McpApiTokenService.CreateTokenAsync: enforce MaxTokensPerUser limit inside a serializable transaction, making the cap atomic under concurrency; move limit check out of controller into service - McpTokenController: remove pre-check (now redundant); catch InvalidOperationException from service and return 400 - chat-module.js: clear sessionStorage lastResponseId when localStorage history is rejected due to TTL expiry, preventing stale responseId - AIChatService: add LogMcpToolCallInvokedStream [LoggerMessage] for the streaming code path (previously shared LogMcpToolCallInvoked with iteration semantics, now split: iteration vs depth are distinct) - AIChatService: reword endUserId comment to accurately reflect that the SDK does not support it yet (was misleadingly claiming it was forwarded) - Tests: add GetActiveTokenCountAsync tests (active, revoked, expired, zero, at-max-limit) to McpApiTokenServiceTests - Tests: add ResponseIdValidationServiceTests (new conversation, cache miss, owner validates, cross-user rejected, null inputs, multi-user isolation)
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| public class ResponseIdValidationServiceTests | ||
| { | ||
| private static ResponseIdValidationService CreateService() | ||
| => new(new MemoryCache(new MemoryCacheOptions())); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements all actionable findings from an OWASP AI Agent Security Cheat Sheet review of the chat and MCP subsystems. The implementation was reviewed in two rounds by GPT-5.5 and Claude Opus 4.6, which found and resolved additional issues before this PR was created.
Fixes: #1064
Fixes: #1066
Fixes: #1067
Fixes: #1068
Fixes: #1069
Fixes: #1070
Fixes: #1071