-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[SignalR] Copy cookies from negotiate to WebSockets #24572
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
How many more of these do we need? Can we do this once? |
You mean what other options might need a similar change? I don't think any more do, it's only applies to settings that can be modified by the server, and I think the server can only modify cookies. |
I'd like that to be the definitive answer. Can we easily test Xamarin and WASM? |
You're asking two different things. Yes we can test on Xamarin and WASM, it's easy just annoying and time consuming. And as far as I know, Cookies is the only special option that we need special logic for WebSockets, the other options are already passed to WebSockets. |
Ok |
src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs
Outdated
Show resolved
Hide resolved
Tested Blazor WASM, but can't test watchOS, XCode wont update on my Mac... |
src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs
Outdated
Show resolved
Hide resolved
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.
I would still like the log.
@Pilchie for RC2 |
Approved for .NET 5 RC2. |
Any chance to get it available in dotnet 3.1 as a backport or perhaps as a nuget package to enable this in previous versions? |
Before backporting anything we generally try to stick with workarounds, to avoid risking an unintended regression. Can you describe why the workaround in #23679 isn't sufficient for you? |
The workaround provided in the issue is specific to their scenario, the more general workaround is something like described here: #23350 (reply in thread) Imagine connecting to Azure App Service where it uses affinity cookies with some hash/guid that it generates, I don't think you can hardcode a specific cookie in that case, you need to first get the cookie with an http request then set it on the hubconnection options. |
@BrennanConroy let's open a 3.1 PR and consider for inclusion in 3.1.10. |
I feel like I am seeing this same issue with the javascript client. We have an application using signalR behind an AWS app load balancer. We have configured the load balancer correctly using stickiness based on cookies. The problem is that often when signalR tries to initiate the web socket connection, it sends the id in the querystring, but does not pass along the cookies that the AWS load balancer has set on the negotiate request. Because of this, the load balancer sends it to any of the servers in the target group, and we're receiving a 404 because the id sent in the querystring does not match to any valid connection ids on that server. Is there any way in the javascript client to make sure that the websocket request sends the same cookies sent in the negotiate request? |
Hi @jaydrozd. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
@jaydrozd The browser controls that. Are you using the same domain? |
It looks like it is not a signalR issue after more investigation. It is related to an iOS issue. I'm using this in an Ionic (Capacitor) based app, and it looks like it's related to a WKWebView issue, which happens to be a domain/trust issue. This works correctly in a browser based (angular) app, so it is definitely not a signalR issue. |
Hi @jaydrozd. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
Fixes #23679
Blazor WASM tested and works as expected.