Add implementing-websocket-endpoints skill - #142
Conversation
Eval Results: implementing-websocket-endpoints3-Run Validation: +28.1% PASS ✅
Score Breakdown
Why This Skill WorksThe model consistently scores BL=3 on websocket topics because:
Model: claude-opus-4.6 (baseline + skill runs), claude-opus-4.6 (pairwise judge) |
There was a problem hiding this comment.
Pull request overview
This PR adds a new skill for implementing WebSocket endpoints in ASP.NET Core 8+, covering common implementation pitfalls and anti-patterns that developers encounter when working with raw WebSockets instead of SignalR.
Changes:
- Adds comprehensive WebSocket implementation guidance covering middleware setup, message fragmentation, connection management, and authentication patterns
- Includes a realistic evaluation scenario testing a chat endpoint with origin validation, query string authentication, and broadcast functionality
- Documents critical gotchas including the absence of MapWebSocket(), CloseAsync vs CloseOutputAsync, and AllowedOrigins security considerations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/dotnet/skills/implementing-websocket-endpoints/SKILL.md | Comprehensive skill documentation with code examples and common mistakes section |
| src/dotnet/tests/implementing-websocket-endpoints/eval.yaml | Evaluation scenario testing WebSocket chat endpoint implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,232 @@ | |||
| ```skill | |||
There was a problem hiding this comment.
The entire file is incorrectly wrapped in a ```skill code block. This differs from the established convention where SKILL.md files have frontmatter directly at the top (without a code block wrapper). The opening triple-backtick on line 1 and closing triple-backtick on line 232 should be removed so the frontmatter and content are directly in markdown format, not inside a code block.
| ```skill |
| 5. **Forgetting `KeepAliveInterval`**: Load balancers and proxies close idle connections. The default 2 minutes may be too long — set to 30 seconds. | ||
|
|
||
| 6. **Not handling concurrent broadcasts safely**: Use `ConcurrentDictionary` and snapshot collections before iteration. | ||
| ``` |
There was a problem hiding this comment.
The closing triple-backtick wrapping the entire file should be removed. The file should end after the final common mistake item, with no code block wrapper around the entire content.
| ``` |
| // CRITICAL ORDERING: UseWebSockets MUST come before the endpoint that handles WebSockets | ||
| app.UseWebSockets(); // ← BEFORE | ||
| app.UseRouting(); | ||
| app.UseAuthorization(); | ||
| // WebSocket handling endpoint comes after routing | ||
| ``` |
There was a problem hiding this comment.
The UseWebSockets call is duplicated - once on line 71 with WebSocketOptions, and again on line 83 without parameters. The second call on line 83 should be removed as the middleware is already registered on line 71. Additionally, the comment about ordering is misleading because the code shows UseWebSockets being called with options before UseRouting, which is the correct approach - you don't need to call it twice.
| while (!result.CloseStatus.HasValue) | ||
| { | ||
| if (result.MessageType == WebSocketMessageType.Text) | ||
| { | ||
| var message = Encoding.UTF8.GetString(buffer, 0, result.Count); | ||
|
|
||
| // CRITICAL: For large messages, EndOfMessage may be false | ||
| // You must accumulate fragments until EndOfMessage == true | ||
| if (!result.EndOfMessage) | ||
| { | ||
| // Accumulate into a MemoryStream or larger buffer | ||
| // Don't process partial messages! | ||
| } | ||
|
|
||
| // Echo back (or process the message) | ||
| var responseBytes = Encoding.UTF8.GetBytes($"Echo: {message}"); | ||
| await webSocket.SendAsync( | ||
| new ArraySegment<byte>(responseBytes), | ||
| WebSocketMessageType.Text, | ||
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | ||
| ct); |
There was a problem hiding this comment.
The logic here is inconsistent with the comment. The code checks if EndOfMessage is false and comments "Don't process partial messages!" but then proceeds to process and echo the message anyway on lines 116-121 regardless of the EndOfMessage value. The echo should only happen when EndOfMessage is true, or fragments should be accumulated first. As written, this will send incomplete message echoes back to the client.
| while (!result.CloseStatus.HasValue) | |
| { | |
| if (result.MessageType == WebSocketMessageType.Text) | |
| { | |
| var message = Encoding.UTF8.GetString(buffer, 0, result.Count); | |
| // CRITICAL: For large messages, EndOfMessage may be false | |
| // You must accumulate fragments until EndOfMessage == true | |
| if (!result.EndOfMessage) | |
| { | |
| // Accumulate into a MemoryStream or larger buffer | |
| // Don't process partial messages! | |
| } | |
| // Echo back (or process the message) | |
| var responseBytes = Encoding.UTF8.GetBytes($"Echo: {message}"); | |
| await webSocket.SendAsync( | |
| new ArraySegment<byte>(responseBytes), | |
| WebSocketMessageType.Text, | |
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | |
| ct); | |
| var messageBuilder = new StringBuilder(); | |
| while (!result.CloseStatus.HasValue) | |
| { | |
| if (result.MessageType == WebSocketMessageType.Text) | |
| { | |
| var chunk = Encoding.UTF8.GetString(buffer, 0, result.Count); | |
| messageBuilder.Append(chunk); | |
| // CRITICAL: For large messages, EndOfMessage may be false | |
| // You must accumulate fragments until EndOfMessage == true | |
| if (!result.EndOfMessage) | |
| { | |
| // Accumulate into the StringBuilder | |
| // Don't process partial messages! | |
| } | |
| else | |
| { | |
| var message = messageBuilder.ToString(); | |
| messageBuilder.Clear(); | |
| // Echo back (or process the message) | |
| var responseBytes = Encoding.UTF8.GetBytes($"Echo: {message}"); | |
| await webSocket.SendAsync( | |
| new ArraySegment<byte>(responseBytes), | |
| WebSocketMessageType.Text, | |
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | |
| ct); | |
| } |
| prompt: | | ||
| I need to add a WebSocket endpoint to my ASP.NET Core 8 API for a real-time chat feature. Requirements: | ||
|
|
||
| 1. WebSocket endpoint at /ws/chat | ||
| 2. Track connected clients and broadcast messages to all when one client sends | ||
| 3. Handle proper connect/disconnect lifecycle | ||
| 4. Authenticate users via a token in the query string (browser WebSocket API doesn't support custom headers) | ||
| 5. Only allow connections from our frontend at https://myapp.com | ||
|
|
||
| I've been looking for something like `app.MapWebSocket("/ws/chat", handler)` but can't find it. How does WebSocket work in ASP.NET Core 8? | ||
| assertions: | ||
| - type: "output_matches" | ||
| pattern: "(UseWebSockets|WebSocketOptions)" | ||
| - type: "output_matches" | ||
| pattern: "(AcceptWebSocketAsync)" | ||
| - type: "output_matches" | ||
| pattern: "(ReceiveAsync|SendAsync)" | ||
| - type: "output_matches" | ||
| pattern: "(AllowedOrigins|Origin)" | ||
| rubric: | ||
| - "Explained that MapWebSocket does not exist in ASP.NET Core — WebSockets use UseWebSockets() middleware with manual upgrade via AcceptWebSocketAsync" | ||
| - "Configured WebSocketOptions with KeepAliveInterval and AllowedOrigins restricted to https://myapp.com for cross-origin protection" | ||
| - "Implemented a proper receive loop checking EndOfMessage for fragmented messages and CloseStatus for disconnect" | ||
| - "Used CloseOutputAsync (not CloseAsync) when responding to client-initiated close to avoid deadlock" | ||
| - "Implemented a thread-safe connection manager using ConcurrentDictionary for tracking and broadcasting to connected clients" | ||
| - "Handled authentication via query string token since browser WebSocket API cannot send custom headers after handshake" | ||
| expect_tools: ["bash"] | ||
| timeout: 120 |
There was a problem hiding this comment.
The eval.yaml file should not have leading spaces before "scenarios:". According to the repository convention, YAML files should start at column 0. The two leading spaces on line 1 should be removed for consistency with other eval.yaml files in the repository.
| prompt: | | |
| I need to add a WebSocket endpoint to my ASP.NET Core 8 API for a real-time chat feature. Requirements: | |
| 1. WebSocket endpoint at /ws/chat | |
| 2. Track connected clients and broadcast messages to all when one client sends | |
| 3. Handle proper connect/disconnect lifecycle | |
| 4. Authenticate users via a token in the query string (browser WebSocket API doesn't support custom headers) | |
| 5. Only allow connections from our frontend at https://myapp.com | |
| I've been looking for something like `app.MapWebSocket("/ws/chat", handler)` but can't find it. How does WebSocket work in ASP.NET Core 8? | |
| assertions: | |
| - type: "output_matches" | |
| pattern: "(UseWebSockets|WebSocketOptions)" | |
| - type: "output_matches" | |
| pattern: "(AcceptWebSocketAsync)" | |
| - type: "output_matches" | |
| pattern: "(ReceiveAsync|SendAsync)" | |
| - type: "output_matches" | |
| pattern: "(AllowedOrigins|Origin)" | |
| rubric: | |
| - "Explained that MapWebSocket does not exist in ASP.NET Core — WebSockets use UseWebSockets() middleware with manual upgrade via AcceptWebSocketAsync" | |
| - "Configured WebSocketOptions with KeepAliveInterval and AllowedOrigins restricted to https://myapp.com for cross-origin protection" | |
| - "Implemented a proper receive loop checking EndOfMessage for fragmented messages and CloseStatus for disconnect" | |
| - "Used CloseOutputAsync (not CloseAsync) when responding to client-initiated close to avoid deadlock" | |
| - "Implemented a thread-safe connection manager using ConcurrentDictionary for tracking and broadcasting to connected clients" | |
| - "Handled authentication via query string token since browser WebSocket API cannot send custom headers after handshake" | |
| expect_tools: ["bash"] | |
| timeout: 120 | |
| prompt: | | |
| I need to add a WebSocket endpoint to my ASP.NET Core 8 API for a real-time chat feature. Requirements: | |
| 1. WebSocket endpoint at /ws/chat | |
| 2. Track connected clients and broadcast messages to all when one client sends | |
| 3. Handle proper connect/disconnect lifecycle | |
| 4. Authenticate users via a token in the query string (browser WebSocket API doesn't support custom headers) | |
| 5. Only allow connections from our frontend at https://myapp.com | |
| I've been looking for something like `app.MapWebSocket("/ws/chat", handler)` but can't find it. How does WebSocket work in ASP.NET Core 8? | |
| assertions: | |
| - type: "output_matches" | |
| pattern: "(UseWebSockets|WebSocketOptions)" | |
| - type: "output_matches" | |
| pattern: "(AcceptWebSocketAsync)" | |
| - type: "output_matches" | |
| pattern: "(ReceiveAsync|SendAsync)" | |
| - type: "output_matches" | |
| pattern: "(AllowedOrigins|Origin)" | |
| rubric: | |
| - "Explained that MapWebSocket does not exist in ASP.NET Core — WebSockets use UseWebSockets() middleware with manual upgrade via AcceptWebSocketAsync" | |
| - "Configured WebSocketOptions with KeepAliveInterval and AllowedOrigins restricted to https://myapp.com for cross-origin protection" | |
| - "Implemented a proper receive loop checking EndOfMessage for fragmented messages and CloseStatus for disconnect" | |
| - "Used CloseOutputAsync (not CloseAsync) when responding to client-initiated close to avoid deadlock" | |
| - "Implemented a thread-safe connection manager using ConcurrentDictionary for tracking and broadcasting to connected clients" | |
| - "Handled authentication via query string token since browser WebSocket API cannot send custom headers after handshake" | |
| expect_tools: ["bash"] | |
| timeout: 120 |
| builder.Services.AddWebSockets(options => | ||
| { | ||
| // WRONG — this method doesn't exist! Use raw middleware options: | ||
| }); |
There was a problem hiding this comment.
The comment states that AddWebSockets method doesn't exist, but this is not entirely accurate. While there's no service registration method for WebSocket options (since WebSocket middleware is configured directly), the code snippet creates confusion by showing an incomplete wrong example. Consider either removing this entire code block (lines 63-66) or making it clearer that you configure WebSocket options when calling UseWebSockets on the IApplicationBuilder, not through dependency injection services.
| builder.Services.AddWebSockets(options => | |
| { | |
| // WRONG — this method doesn't exist! Use raw middleware options: | |
| }); | |
| // Note: There is no AddWebSockets(...) service registration method. | |
| // You configure WebSocket options when calling UseWebSockets on IApplicationBuilder. |
|
|
||
| // CRITICAL: ToList() snapshot to avoid modification during iteration | ||
| var tasks = _connections.Values | ||
| .Where(s => s.State == WebSocketState.Open) // Only open sockets |
There was a problem hiding this comment.
The comment on line 166 says "ToList() snapshot to avoid modification during iteration" but the code doesn't actually call ToList(). The LINQ query creates a deferred execution chain that will still iterate over the live ConcurrentDictionary.Values collection. To create a snapshot, add .ToList() after the Where clause and before the Select, like: _connections.Values.Where(s => s.State == WebSocketState.Open).ToList().Select(...).
| .Where(s => s.State == WebSocketState.Open) // Only open sockets | |
| .Where(s => s.State == WebSocketState.Open) // Only open sockets | |
| .ToList() |
| // CRITICAL: WebSocket connections don't support standard HTTP auth headers | ||
| // after the initial handshake. The auth happens on the HTTP upgrade request. |
There was a problem hiding this comment.
The comment is slightly misleading. The browser WebSocket API doesn't support custom headers at all, not just "after the initial handshake". The comment should clarify that custom headers can only be sent during the HTTP upgrade request, and the browser WebSocket API doesn't provide a way to set these (hence the need for query strings or cookies). The phrase "after the initial handshake" suggests headers work initially but not later, when really the browser API prevents setting custom headers entirely.
| // CRITICAL: WebSocket connections don't support standard HTTP auth headers | |
| // after the initial handshake. The auth happens on the HTTP upgrade request. | |
| // CRITICAL: HTTP auth headers can only be sent on the initial HTTP upgrade request. | |
| // Browser WebSocket APIs cannot set custom headers at all, so use query strings or cookies. |
|
|
||
| // Option 1: Query string token (common for browser clients) | ||
| app.Map("/ws", async (HttpContext context) => | ||
| { | ||
| // Browser WebSocket API doesn't support custom headers | ||
| // Use query string: ws://server/ws?access_token=xxx | ||
| var token = context.Request.Query["access_token"]; | ||
| if (string.IsNullOrEmpty(token)) | ||
| { | ||
| context.Response.StatusCode = 401; | ||
| return; | ||
| } | ||
|
|
||
| // Validate token here... | ||
|
|
||
| if (context.WebSockets.IsWebSocketRequest) | ||
| { | ||
| using var ws = await context.WebSockets.AcceptWebSocketAsync(); | ||
| await HandleWebSocketAsync(ws, context.RequestAborted); | ||
| } | ||
| }); | ||
|
|
||
| // Option 2: Cookie auth works naturally (cookies are sent on upgrade request) | ||
| // Option 3: Use [Authorize] attribute if using cookie or negotiate auth |
There was a problem hiding this comment.
Using an access_token in the query string for WebSocket authentication (ws://server/ws?access_token=xxx) risks leaking the token via server/proxy logs, browser history, and Referer headers, allowing unauthorized reuse of the token. An attacker (or any party with log access) who obtains this URL can impersonate the user over the WebSocket until the token is revoked or expires. Prefer cookie-based authentication or established ASP.NET Core auth middleware (e.g., bearer or cookie auth tied to [Authorize]) so credentials are sent in headers/cookies over wss:// and are not embedded in the URL.
| // Option 1: Query string token (common for browser clients) | |
| app.Map("/ws", async (HttpContext context) => | |
| { | |
| // Browser WebSocket API doesn't support custom headers | |
| // Use query string: ws://server/ws?access_token=xxx | |
| var token = context.Request.Query["access_token"]; | |
| if (string.IsNullOrEmpty(token)) | |
| { | |
| context.Response.StatusCode = 401; | |
| return; | |
| } | |
| // Validate token here... | |
| if (context.WebSockets.IsWebSocketRequest) | |
| { | |
| using var ws = await context.WebSockets.AcceptWebSocketAsync(); | |
| await HandleWebSocketAsync(ws, context.RequestAborted); | |
| } | |
| }); | |
| // Option 2: Cookie auth works naturally (cookies are sent on upgrade request) | |
| // Option 3: Use [Authorize] attribute if using cookie or negotiate auth | |
| // | |
| // DO NOT pass access tokens in the query string, e.g.: | |
| // ws://server/ws?access_token=xxx | |
| // URLs are often logged and may leak tokens. Instead, rely on standard | |
| // ASP.NET Core authentication (cookies or bearer tokens in headers). | |
| // Example: secure WebSocket endpoint using ASP.NET Core auth | |
| // Assumes you've configured authentication/authorization in Program.cs: | |
| // builder.Services.AddAuthentication(...); | |
| // builder.Services.AddAuthorization(); | |
| // app.UseAuthentication(); | |
| // app.UseAuthorization(); | |
| app.Map("/ws", async (HttpContext context) => | |
| { | |
| // Authentication happens on the HTTP upgrade request. | |
| // Cookies and Authorization headers are sent automatically. | |
| if (!context.User?.Identity?.IsAuthenticated ?? true) | |
| { | |
| context.Response.StatusCode = StatusCodes.Status401Unauthorized; | |
| return; | |
| } | |
| if (!context.WebSockets.IsWebSocketRequest) | |
| { | |
| context.Response.StatusCode = StatusCodes.Status400BadRequest; | |
| return; | |
| } | |
| using var ws = await context.WebSockets.AcceptWebSocketAsync(); | |
| await HandleWebSocketAsync(ws, context.RequestAborted); | |
| }) | |
| .RequireAuthorization(); // Enforce auth using configured schemes (cookie, bearer, etc.) | |
| // Cookie auth works naturally (cookies are sent on the upgrade request). | |
| // Bearer tokens can be sent in the Authorization header (not in the URL). | |
| // You can also use [Authorize] on minimal APIs/controllers that upgrade to WebSockets. |
| { | ||
| // CRITICAL: KeepAliveInterval sends ping frames to keep connection alive | ||
| // Default is 2 minutes. Set to match your infrastructure timeouts. | ||
| KeepAliveInterval = TimeSpan.FromSeconds(30), |
| WebSocketMessageType.Text, | ||
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | ||
| ct); | ||
| } |
|
Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. |
New Skill: implementing-websocket-endpoints
Adds a skill for implementing raw WebSocket endpoints in ASP.NET Core 8+, covering common pitfalls.
Key Gotchas Covered
Eval Results (3-run validation)