-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix User-Agent usage in SignalR #30764
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
Conversation
@@ -608,6 +607,7 @@ private HttpClient CreateHttpClient() | |||
// Check if the key is User-Agent and remove if empty string then replace if it exists. | |||
if (string.Equals(header.Key, Constants.UserAgent, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In WASM, the user would need to set "X-SignalR-User-Agent", whereas outside of WASM they need to set "User-Agent" to override the default we set. I could see us keeping this behavior, or saying "User-Agent" will apply to both.
No test? |
I guess I can spend a couple hours trying to write a WASM test. Wont be able to test the other issue though, as that requires some version of mono. |
Ok, some resemblance of a test added. |
@@ -47,6 +50,12 @@ | |||
await hubConnection.SendAsync("SendMessage", "SignalR Client", $"Echo {transportType}"); | |||
} | |||
|
|||
protected async Task GetUserAgentHeader() | |||
{ | |||
userAgent = await hubConnection.InvokeAsync<string>("GetHeaderValue", "X-SignalR-User-Agent"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you want both user agents right? To make sure the normal is set as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal is set by the browser, we shouldn't be testing browser behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So before our code was just ineffective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In WASM it was ineffective in some browsers, and set in others
Details at #29183 (comment)
Fixes #30137
Fixes #29183