tpeczek/Lib.AspNetCore.ServerSentEvents

Lib hijacks any service calls without Accept headers

Closed this issue · 6 comments

CobusKruger commented

When the middleware checks the Accept header here, it assumes that any API call without an Accept header is looking for an event stream, and promptly creates a new client.

This seems like a terrible default, and it affects calls on all controllers to boot. I believe it should be changed as follows:

if (!requestHeaders.ContainsKey(Constants.ACCEPT_HTTP_HEADER))
{
    return false; // Original: true;
}
tpeczek commented

Hi @CobusKruger,

As crazy as it sounds, the Accept header is not required for SSE, if you take a look at the spec , you will find following point:

User agents may set (Accept, text/event-stream) in request's header list.

So the behavior is in line with spec.

That said, it might be a potential place for parametrization (although the default behavior would still remain to not require the Accept header). I'm also quite interested in your case as I haven't heard or experience it to be a problem in the past (which would mean clients other than SSE abusing the endpoint) while there were issues were some less common clients were not sending the header and as a result not being able to connect.

CobusKruger commented

Thank you for the quick reply.

My problem is that it's picking up other endpoints.

I have a PushController on my service, and a POST method named user. This is called at the expected url: https://example.com/api/push/user

If that call is done without an Accept header, it enters the SSE code, calls AcquireClientId, and proceeds to create a new ServerSentEventsClient. That's because the statement at line 73 returns true.

Perhaps the problem isn't with the Accept header check, but but with the way it executes this code for all requests.

tpeczek commented

It sounds to me like you didn't map the middleware against a specific endpoint but as a part of global pipeline, which is rather undesired.

For example the demo app is mapping to specific endpoints here:

app.UseResponseCompression()
       .UseStaticFiles()
       .UseRouting()
       .UseEndpoints(endpoints =>
       {
           // Set up first Server-Sent Events endpoint.
           endpoints.MapServerSentEvents("/see-heartbeat");

           // Set up second (separated) Server-Sent Events endpoint.
           endpoints.MapServerSentEvents<NotificationsServerSentEventsService>("/sse-notifications");

           endpoints.MapControllerRoute("default", "{controller=Notifications}/{action=sse-notifications-receiver}");
       });
CobusKruger commented

No, I have it mapped against a named endpoint, /sse;

app.UseEndpoints(endpoints =>
{
    endpoints.MapControllers();
    endpoints.MapServerSentEvents("/sse", new ServerSentEventsOptions
    {
        OnPrepareAccept = response =>
        {
            response.Headers.Append("Cache-Control", "no-cache");
            response.Headers.Append("X-Accel-Buffering", "no");
        }
    });
});

The headers, by the way, are to get rid of the 499 timeout errors I get on the /sse call in Azure, but it doesn't seem to be the full answer. But my Angular code is connecting against /sse. The service code sending the notifications by calling the /push/user endpoint had the same 499 timeouts, which was what led me to investigate. When I added Accept headers to the /push/user calls, the timeouts went away (but remained for the /sse calls).

tpeczek commented

When it comes to timeout, the most often reason for them is inactivity, you may want to explore keep alive: https://www.tpeczek.com/2018/08/server-sent-events-and-aspnet-core-you_9.html

Regarding the middleware being triggered for endpoint it is not mapped for, we are approaching the realm of improbable and highly unlikely :). Unless that MapControllers in front of it is mapping a really weird route, it should never happen. If you have a simple project which can repro such behaviour, that is something I would like to take a look at because that would be an actual issue (and something which hasn't been seen before).

CobusKruger commented