Add HTTP method constraint for categorizing SSE requests - #470
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The updated SSE detection still allows _reqconntype to be overwritten based on header order (e.g., potentially overriding a WebSocket upgrade), which should be guarded to avoid misclassification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR tightens SSE (Server-Sent Events) request classification during header parsing so that Accept: text/event-stream only marks a request as RCT_EVENT when the HTTP method is GET, aligning header parsing behavior with AsyncWebServerRequest::isSSE() and preventing POST requests from being misrouted.
Changes:
- Require
HTTP_GETinAccept: text/event-streamdetection before setting_reqconntype = RCT_EVENT.
File summaries
| File | Description |
|---|---|
| src/WebRequest.cpp | Adds an HTTP method constraint to SSE detection in request header parsing. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…ader-order overwrites Both the WebSocket (Upgrade: websocket) and SSE (Accept: text/event-stream) branches now classify the connection only when the method is HTTP_GET (per RFC 6455 §4.1 and HTML §9.2 respectively) and only when _reqconntype is still a plain HTTP connection (RCT_DEFAULT/RCT_HTTP). This makes classification header-order independent so neither branch can clobber the other, addressing the Copilot review concern on PR ESP32Async#470 and the symmetric pre-existing issue in the Upgrade branch. Also removes a stray 'l' typo before the SSE classification guard.
mathieucarbou
left a comment
There was a problem hiding this comment.
I added a commit and approving the changes which are more compliant with the RFC.
Also to note: RCT_DEFAULT is never used (assigned). It serves a few purposes even though it's not currently stored:
-
Semantic placeholder for "no special classification" — it's the enum's conceptual "default/initial" slot (value 0, the implicit default for zero-initialized RequestedConnectionType). The isHTTP() predicate lists both RCT_DEFAULT and RCT_HTTP so a request that is either unclassified (RCT_DEFAULT) or explicitly plain HTTP (RCT_HTTP) counts as a normal HTTP request. This is defensive: if any code path ever zero-inits the field (e.g., AsyncWebServerRequest req; without the constructor, or a future reset), it would be RCT_DEFAULT (value 0) rather than RCT_HTTP (value 1), and isHTTP() would still work.
-
Mirrors the upstream/related PsychicHttp enum — the comment at ESPAsyncWebServer.h:407 notes "this enum is similar to Arduino WebServer's AsyncAuthType and PsychicHttp". Keeping RCT_DEFAULT preserves compatibility with the broader ecosystem where 0 is the conventional "unspecified" state.
-
Used in requestedConnTypeToString() — ESPAsyncWebServer.h/WebRequest.cpp's requestedConnTypeToString() has a case RCT_DEFAULT: arm, so the value round-trips through logging/debug even if it's never the live value today.
|
@willmmiles : about that |
No objections from me - I agree it's a meaningless value in the current implementation. |
I will open another PR for that. |
Problem
In
ESPAsyncWebServer.h:586, theisSSEmethod is true only if the HTTP method is aGETrequest and the_reqconntypeisRCT_EVENT.However,
WebRequest.cpp:671's header parsing matches SSE toAccept: text/event-stream, regardless of the HTTP method used. If a request uses thePOSTmethod while still includingtext/event-streamin the Accept header, it will be classified as anRCT_EVENT, but fails to matchisSSEsince it is not aGETrequest, causing it to miss any HTTP handlers for that endpointExample
A
POSTrequest to/mcpwith the headerAccept: application/json, text/event-streamwill end up being routed by the not found handler, because_reqconntypegets set toRCT_EVENTfrom theAcceptheader, regardless of HTTP method. But,isHTTPonly acceptsRCT_DEFAULTandRCT_HTTP, andisSSErequiresGET, so the request satisfies neither.Fix
Added an additional condition to the header-parsing logic in
WebRequest.cpp, requiring that the request method must beGETin addition to the presence ofAccept: text/event-stream.