.NET: Bug fixes for AGUI hosting and workflows#6311
Conversation
…t yet set and improve comments in samples
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 90%
✓ Correctness
The PR makes two meaningful changes: (1) wrapping the AGUI session store in IsolationKeyScopedAgentSessionStore to ensure session isolation is always in the chain (pass-through when no provider is registered), and (2) adding an AprovalSnapshot to InvokeMcpToolExecutor to prevent TOCTOU attacks where state mutates during the approval window. Both changes are correct. The isolation wrapper is non-breaking (Strict=false with null provider means transparent pass-through via GetScopedConversationIdAsync returning bare conversationId). The snapshot pattern correctly captures values at approval-request time, uses a new dictionary copy from GetArguments(), and falls back gracefully via null-coalescing. The same InvokeMcpToolExecutor instance persists between ExecuteAsync and CaptureResponseAsync (confirmed via WorkflowActionVisitor line 522 capturing action.CaptureResponseAsync as a delegate on the same object).
✓ Security Reliability
This PR introduces two well-designed security improvements: (1) TOCTOU mitigation in InvokeMcpToolExecutor by snapshotting evaluated parameters at approval-request time, preventing state mutation attacks during the approval window; (2) automatic session isolation wrapping in the AGUI endpoint to prevent cross-user session access. Both implementations are sound. The snapshot pattern correctly captures immutable values, the fallback paths are defensive and safe, and the session isolation enforcement is properly scoped (strict when a provider is registered, permissive for single-user sample scenarios). No blocking security or reliability issues found.
✓ Test Coverage
The TOCTOU fix in InvokeMcpToolExecutor has excellent test coverage with three security-focused tests covering tool name, arguments, and server URL mutation scenarios. However, the new session-isolation wrapping logic in AGUIEndpointRouteBuilderExtensions.cs has no corresponding tests despite being security-relevant behavior (ensuring IsolationKeyScopedAgentSessionStore is always in the chain).
✗ Design Approach
I found two design-level regressions. In AGUI hosting, the new endpoint-side auto-wrapping now changes the documented "no session store" path from ephemeral to strict isolation, so apps that globally register a claims-based isolation provider can start failing unauthenticated or missing-claim requests even when they intentionally did not configure persistence. In the workflow fix, the approval snapshot is kept only in an executor field, so it is not durable across checkpoint/restore; after a resume that reconstructs executor state, the code silently falls back to re-evaluating live state and reopens the TOCTOU hole this PR is trying to close.
Flagged Issues
- AGUIEndpointRouteBuilderExtensions.cs:109-112 wraps even the Nop fallback store in a strict IsolationKeyScopedAgentSessionStore, contradicting the method's own contract that missing stores stay ephemeral. When a globally registered ClaimsIdentitySessionIsolationKeyProvider returns null (unauthenticated/missing-claim requests), IsolationKeyScopedAgentSessionStore throws in strict mode. Only actual persistent stores should be wrapped; keep the Nop fallback unwrapped.
Automated review by westey-m's agents
There was a problem hiding this comment.
Pull request overview
This PR tightens security and correctness around (1) declarative workflow MCP tool approvals and (2) AG-UI hosting session isolation, plus updates samples to document the new isolation expectations.
Changes:
- Add an “approval-time snapshot” concept for
InvokeMcpToolso resumed execution uses the values the user reviewed (mitigating TOCTOU from state mutation during approval). - Ensure AG-UI endpoint hosting wraps configured
AgentSessionStorewithIsolationKeyScopedAgentSessionStorewhen not already present. - Update AG-UI samples to reference/describe session isolation configuration and add project references needed for those APIs.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/ObjectModel/InvokeMcpToolExecutorTest.cs | Adds tests asserting approved tool name/args/server URL are not affected by post-approval state mutation. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/ObjectModel/InvokeMcpToolExecutor.cs | Adds approval-time snapshot and uses it during CaptureResponseAsync to mitigate TOCTOU. |
| dotnet/src/Microsoft.Agents.AI.Hosting.AGUI.AspNetCore/AGUIEndpointRouteBuilderExtensions.cs | Ensures AG-UI hosting uses an IsolationKeyScopedAgentSessionStore wrapper when missing. |
| dotnet/samples/05-end-to-end/AGUIWebChat/Server/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
| dotnet/samples/05-end-to-end/AGUIWebChat/Server/AGUIWebChatServer.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/05-end-to-end/AGUIClientServer/AGUIServer/Program.cs | Updates warning/docs comment about session isolation in multi-user/persistent deployments. |
| dotnet/samples/05-end-to-end/AGUIClientServer/AGUIDojoServer/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
| dotnet/samples/05-end-to-end/AGUIClientServer/AGUIDojoServer/AGUIDojoServer.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/02-agents/AGUI/Step05_StateManagement/Server/Server.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/02-agents/AGUI/Step05_StateManagement/Server/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
| dotnet/samples/02-agents/AGUI/Step04_HumanInLoop/Server/Server.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/02-agents/AGUI/Step04_HumanInLoop/Server/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
| dotnet/samples/02-agents/AGUI/Step03_FrontendTools/Server/Server.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/02-agents/AGUI/Step03_FrontendTools/Server/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
| dotnet/samples/02-agents/AGUI/Step02_BackendTools/Server/Server.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/02-agents/AGUI/Step02_BackendTools/Server/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
| dotnet/samples/02-agents/AGUI/Step01_GettingStarted/Server/Server.csproj | Adds hosting project reference needed for session-isolation APIs. |
| dotnet/samples/02-agents/AGUI/Step01_GettingStarted/Server/Program.cs | Documents session isolation requirement for multi-user/persistent deployments. |
javiercn
left a comment
There was a problem hiding this comment.
AG-UI changes look good.
Motivation and Context
Description
Bug fixes for AGUI hosting and workflows
Contribution Checklist