Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SignalR: Missing Accept-Header #47398

Open
1 task done
ThomasVandenbon opened this issue Mar 24, 2023 · 9 comments · May be fixed by #55271
Open
1 task done

SignalR: Missing Accept-Header #47398

ThomasVandenbon opened this issue Mar 24, 2023 · 9 comments · May be fixed by #55271
Labels
area-signalr Includes: SignalR clients and servers help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@ThomasVandenbon
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

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:

  • Microsoft_DefaultRuleSet-2.0-PROTOCOL-ENFORCEMENT-920300
  • https://xxx:xxx/xxx/negotiate?negotiateVersion=1
  • "matchVariableName": "HeaderValue:user-agent",
    "matchVariableValue": "Microsoft SignalR/6.0 (6.0.5+e5f183b656a0e8bc087108130a5a9b54ae94494e; Windows NT; .NET; .NET 6.0.3)"
  • Request Missing an Accept Header

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-signalr Includes: SignalR clients and servers label Mar 24, 2023
@captainsafia
Copy link
Member

@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
Copy link
Member

BrennanConroy commented Mar 27, 2023

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 ThomasVandenbon changed the title Swagger: Missing Accept-Header SignalR: Missing Accept-Header Mar 28, 2023
@ThomasVandenbon
Copy link
Author

@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.

@BrennanConroy BrennanConroy added this to the .NET 8 Planning milestone May 3, 2023
@ghost
Copy link

ghost commented May 3, 2023

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 dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Mar 2, 2024
Copy link
Contributor

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

@BrennanConroy BrennanConroy modified the milestones: .NET 8 Planning, Backlog Mar 2, 2024
@MattyLeslie
Copy link
Contributor

@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("*/*"));
  1. Ensuring the Accept header is present in NegotiateAsync()
 // Set the Accept header to "*/*"
request.Headers.Accept.Clear();
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("*/*"));

@BrennanConroy
Copy link
Member

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
Copy link
Contributor

MattyLeslie commented Apr 19, 2024

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@captainsafia @BrennanConroy @wtgodbe @ThomasVandenbon @MattyLeslie and others