dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.27k stars 9.96k forks source link

SignalR: Missing Accept-Header #47398

Closed ThomasVandenbon closed 4 months ago

ThomasVandenbon commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

The Azure WAF is reporting an anomaly when negotiating a SignalR connection from a Console app to the Azure SignalR Service.

The anomaly that is being reported:

Describe the solution you'd like

In Microsoft.AspNetCore.SignalR.Client an Accept Header should be added to the negotiating requests.

Additional context

I'm running version 6.0.5 of the SignalR Client, as I'm unable to upgrade to 7.0.4 at the moment. I cannot find the release notes for Microsoft.AspNetCore.SignalR.Client to check if the Accept Header hasn't been added, but since I cannot find an issue about this (open or closed), I would assume it hasn't been fixed yet.

captainsafia commented 1 year ago

@BrennanConroy Although the issue title cites Swagger this doesn't seem to be OpenAPI related. Any hunch as to what is going on here?

BrennanConroy commented 1 year ago

Any hunch as to what is going on here?

Yeah, looks like no one is adding an Accept header on negotiate, and I'd guess other requests as well. We'd need to figure out what header value(s) are acceptable.

Looking at the browser it sends "*/*" for the negotiate request.

ThomasVandenbon commented 1 year ago

@BrennanConroy Although the issue title cites Swagger this doesn't seem to be OpenAPI related. Any hunch as to what is going on here?

My apologies, I was working on both SignalR and Swagger at the time and it seems I had a brain fart while naming the issue.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

dotnet-policy-service[bot] commented 7 months ago

Looks like this issue has been identified as a candidate for community contribution. If you're considering sending a PR for this issue, look for the Summary Comment link in the issue description. That comment has been left by an engineer on our team to help you get started with handling this issue. You can learn more about our Help Wanted process here

MattyLeslie commented 5 months ago

@BrennanConroy Might the following changes within HttpConnection.cs be a viable solution to this ?

1) Adding httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("*/*")); to CreateHttpClient()

var httpClient = new HttpClient(httpMessageHandler);
httpClient.Timeout = HttpClientTimeout;
httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("*/*"));

2) Ensuring the Accept header is present in NegotiateAsync()

 // Set the Accept header to "*/*"
request.Headers.Accept.Clear();
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("*/*"));
BrennanConroy commented 5 months ago

In negotiate yes (although Clear seems unnecessary). Does having default headers on HttpClient also add them to requests if the HttpRequestMessage added a specific value for the header already? Might want to just set Accept on all call sites where we send requests if DefaultRequestHeaders appends.

MattyLeslie commented 5 months ago

@BrennanConroy I believe the HttpRequestMessage header value would override the DefaultRequestHeaders header value. So, I agree that we might want to just ensure all call sites set Accept to the appropriate value when sending requests. Other than NegotiateAsync() are there any other call sites or areas you'd liked checked?

BrennanConroy commented 5 months ago

There is the send from LongPolling and ServerSentEvents, and the get from LongPolling. The ServerSentEvents "get" already sets Accept.