Skip to content

Add HTTP method constraint for categorizing SSE requests - #470

Merged
mathieucarbou merged 2 commits into
ESP32Async:mainfrom
dylan-mccormick:sse-method-fix
Jul 31, 2026
Merged

mathieucarbou merged 2 commits into
ESP32Async:mainfrom
dylan-mccormick:sse-method-fix

Conversation

@dylan-mccormick

Copy link
Copy Markdown

Problem

In ESPAsyncWebServer.h:586, the isSSE method is true only if the HTTP method is a GET request and the _reqconntype is RCT_EVENT.

bool isSSE() const {
  return _method == AsyncWebRequestMethod::HTTP_GET && isExpectedRequestedConnType(RCT_EVENT);
}

However, WebRequest.cpp:671's header parsing matches SSE to Accept: text/event-stream, regardless of the HTTP method used. If a request uses the POST method while still including text/event-stream in the Accept header, it will be classified as an RCT_EVENT, but fails to match isSSE since it is not a GET request, causing it to miss any HTTP handlers for that endpoint

Example

A POST request to /mcp with the header Accept: application/json, text/event-stream will end up being routed by the not found handler, because _reqconntype gets set to RCT_EVENT from the Accept header, regardless of HTTP method. But, isHTTP only accepts RCT_DEFAULT and RCT_HTTP, and isSSE requires GET, so the request satisfies neither.

server.on("/mcp", HTTP_POST, [](AsyncWebServerRequest* r) {
    r->send(200, "text/plain", "OK");
});

Fix

Added an additional condition to the header-parsing logic in WebRequest.cpp, requiring that the request method must be GET in addition to the presence of Accept: text/event-stream.

Copilot AI review requested due to automatic review settings July 30, 2026 00:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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_GET in Accept: text/event-stream detection 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.

Comment thread src/WebRequest.cpp Outdated
…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.
Copilot AI review requested due to automatic review settings July 30, 2026 20:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mathieucarbou mathieucarbou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mathieucarbou

Copy link
Copy Markdown
Member

@willmmiles : about that RCT_DEFAULT enum... This is dead code... We could drop it I think. If you are ok with that, I could remove it, and add a RCT_DEFAULT for now which is marked as deprecated for future removal ?

@willmmiles

Copy link
Copy Markdown

@willmmiles : about that RCT_DEFAULT enum... This is dead code... We could drop it I think. If you are ok with that, I could remove it, and add a RCT_DEFAULT for now which is marked as deprecated for future removal ?

No objections from me - I agree it's a meaningless value in the current implementation.

@mathieucarbou

Copy link
Copy Markdown
Member

@willmmiles : about that RCT_DEFAULT enum... This is dead code... We could drop it I think. If you are ok with that, I could remove it, and add a RCT_DEFAULT for now which is marked as deprecated for future removal ?

No objections from me - I agree it's a meaningless value in the current implementation.

I will open another PR for that.

@mathieucarbou
mathieucarbou merged commit f443b9a into ESP32Async:main Jul 31, 2026
37 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants