Connection Id not read properly when additional query string parameters are added #1556

Closed
andyedinborough opened this Issue Feb 21, 2013 · 13 comments

Comments

Projects
None yet
6 participants

We get "The connection id is in the incorrect format" when we specify additional query string parameters for our connections.

For example, our client makes a connect request:

/-/echo/connect?transport=longPolling&connectionToken=P1HYk5t2n1RIlnt4HeTh0tXXwiS2SCWuw79soaT1lLYJsH4Qvi_tm2vI6osHEIIjPD6QTjhCrvrJxM_NgjxRhDfLAzSIvddQXcm8YiC5IrBiwCeXr2e1yEvF4Jgudvdf0&access_token=03b5dece-ffde-427c-9c32-eb0a01f62972

And the server errors out with:

System.InvalidOperationException: The connection id is in the incorrect format.

Server stack trace: 
   at Microsoft.AspNet.SignalR.PersistentConnection.GetConnectionId(HostContext context, String connectionToken)
   at Microsoft.AspNet.SignalR.PersistentConnection.ProcessRequest(HostContext context)
   at Microsoft.AspNet.SignalR.Owin.CallHandler.Invoke(IDictionary`2 environment)
   at Microsoft.AspNet.SignalR.Owin.Handlers.PersistentConnectionHandler.Invoke(IDictionary`2 environment)
   at Microsoft.Owin.Host.SystemWeb.OwinCallContext.Execute()
   at Microsoft.Owin.Host.SystemWeb.OwinHttpHandler.BeginProcessRequest(HttpContextBase httpContext, AsyncCallback callback, Object extraData)

Exception rethrown at [0]: 

Removing our access_token=... from the queryString parameter of the Connection constructor on the client resolves the issue.

Owner

davidfowl commented Feb 21, 2013

Can you provide a repro project?

Well it will probably work perfectly if I do that! ;]

I'll get to you tonight.

Owner

davidfowl commented Feb 21, 2013

That's my secret plan.

So, here's the deal:

For our project, we're specifying an access_token in the querystring to authenticate the connection, but negotiation doesn't send the querystring, so the connectionToken is generated under the default principal. When the client then goes to connect, the querystring is sent, the user is authenticated, the principal is changed... boom (https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Core/PersistentConnection.cs#L255)

For your enjoyment: https://github.com/andyedinborough/signalr-bug-demo

Owner

davidfowl commented Feb 21, 2013

So the issue here is that we need to send the query string for all requests. Do you have a custom http module that sets up the IPrinicpal based on the access token?

@ghost ghost assigned halter73 Feb 21, 2013

Yeah, I think sending the querystring for all requests is the simplest fix.

We handle the AuthenticateRequest event inside global.asax to create the IPrincipal (just like in the mockup I posted). access_token is just one thing it looks for.

Owner

davidfowl commented Feb 21, 2013

Yep, sounds like a simple fix.

@ghost ghost assigned davidfowl and NTaylorMullen Feb 21, 2013

NTaylorMullen added a commit that referenced this issue Feb 21, 2013

Added Negotiate Facts to test #1556.
- This test will not pass until the issue is fixed
#1556

NTaylorMullen added a commit that referenced this issue Feb 21, 2013

Added the query string (connection.qs) to the negotiate request
- This will allow devs to determine authentication on negotiation
#1556

NTaylorMullen added a commit that referenced this issue Feb 21, 2013

Found an issue with our addQs method where we wouldn't always append …
…query strings correctly to a URL

- Without this the #1556 fix fails

jlew-cs commented Feb 21, 2013

I am seeing a somewhat similar problem where when I log my user out of the web application, the principal is being thrown away and the /signalr/abort request arrives after that has happened. At that point, the authentication cookie is gone, and the check inside of GetConnectionId fails. Is there any way to force abort to complete synchronously when the page is unloading in this case?

@jlew-cs It looks like you could just call connection.stop(true) before logging out (https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Client.JS/jquery.signalR.core.js#L581). Although telling it to stop prior to logging out should mean that the stop request is handled first, and it would be ok to run asynchronously.

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Added a BuildCustomQueryString function to the TransportHelper
- Verified its functionality against the test project that was provided in #1556
- Need to add some tests to verify the functionality
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Addressed code review comments
- Added new test cases
- Updated appendCustomQueryString to be more flexible
- Removed enable cross domain from functional test
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Added a BuildCustomQueryString function to the TransportHelper
- Verified its functionality against the test project that was provided in #1556
- Need to add some tests to verify the functionality
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Addressed code review comments
- Added new test cases
- Updated appendCustomQueryString to be more flexible
- Removed enable cross domain from functional test
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Added Negotiate Facts to test #1556.
- This test will not pass until the issue is fixed
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Added the query string (connection.qs) to the negotiate request
- This will allow devs to determine authentication on negotiation
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Found an issue with our addQs method where we wouldn't always append …
…query strings correctly to a URL

- Without this the #1556 fix fails

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Added a BuildCustomQueryString function to the TransportHelper
- Verified its functionality against the test project that was provided in #1556
- Need to add some tests to verify the functionality
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Addressed code review comments
- Added new test cases
- Updated appendCustomQueryString to be more flexible
- Removed enable cross domain from functional test
#1556

NTaylorMullen added a commit that referenced this issue Feb 22, 2013

Address code analysis issue
- Had to verify baseUrl correctness before using it in public method
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Added Negotiate Facts to test #1556.
- This test will not pass until the issue is fixed
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Added the query string (connection.qs) to the negotiate request
- This will allow devs to determine authentication on negotiation
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Found an issue with our addQs method where we wouldn't always append …
…query strings correctly to a URL

- Without this the #1556 fix fails

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Added a BuildCustomQueryString function to the TransportHelper
- Verified its functionality against the test project that was provided in #1556
- Need to add some tests to verify the functionality
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Addressed code review comments
- Added new test cases
- Updated appendCustomQueryString to be more flexible
- Removed enable cross domain from functional test
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Address code analysis issue
- Had to verify baseUrl correctness before using it in public method
#1556
Contributor

Xiaohongt commented Feb 23, 2013

this fix for milestone 1.0.1 is not in release branch yet, only in dev branch

Owner

davidfowl commented Feb 23, 2013

@NTaylorMullen these changes need to go into the release branch as well.

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Added Negotiate Facts to test #1556.
- This test will not pass until the issue is fixed
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Added the query string (connection.qs) to the negotiate request
- This will allow devs to determine authentication on negotiation
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Found an issue with our addQs method where we wouldn't always append …
…query strings correctly to a URL

- Without this the #1556 fix fails

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Added a BuildCustomQueryString function to the TransportHelper
- Verified its functionality against the test project that was provided in #1556
- Need to add some tests to verify the functionality
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Addressed code review comments
- Added new test cases
- Updated appendCustomQueryString to be more flexible
- Removed enable cross domain from functional test
#1556

NTaylorMullen added a commit that referenced this issue Feb 23, 2013

Address code analysis issue
- Had to verify baseUrl correctness before using it in public method
#1556
Contributor

NTaylorMullen commented Feb 23, 2013

@davidfowl done

Contributor

Xiaohongt commented Feb 24, 2013

verified

@Xiaohongt Xiaohongt closed this Feb 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment