Skip to content

Loading…

longPolling does not trigger start().fail when authorization fails #2288

Closed
kbirger opened this Issue · 17 comments

6 participants

@kbirger

It seems like if you add an AuthorizeAttribute to your hub, this does not extend to the longpolling 'negotiate' call, so if your connection falls back to longPolling, SignalR does not properly reject the promise on the call to $.connection.hub.start.

It seems like the dynamic resources SignalR creates (handlers) should be in a context that receives the authorization treatment (unfortunately they are in /signalR/*, not specific to a hub).

Alternatively, since it does eventually fail (some point after the ping loop), this should reject the promise.

Steps to reproduce:

  • Create hub with AuthorizeAttribute
  • Create page that connects to it, and call start({transport: ["longPolling"]});
  • attach a fail() to the call to start. It does not get called!
@signalrcoreteam

@NTaylorMullen can you please verify that this is fixed in 2.0 with your latest changes.

@NTaylorMullen

This was fixed in #2106, marking as done until verification.

@muratg muratg was assigned
@kbirger

Though I have yet to do more experimenting, what I saw yesterday was still not desirable behavior.

In the project we are working on we have an authentication http module that injects a User and identity object, and an AuthorizeAttribute in SignalR which reads them (standard).

Yesterday I saw the following: One of the calls signalR makes now gets 302'd to the forms authentication redirect page, so Chrome gets a parse error, as well as a content error on that call, and aborts execution of SignalR code, still not firing the .done() handlers. Internet Explorer is (unsurprisingly) even worse - it actually propagates the 302 to the parent page and bounces the whole tab to the forms authentication sign-in URL.

All that should be required is that the negotiation resource be tested against the AuthorizeAttribute on the Hub, and to return a 403 error code when authorization fails.

The application should be able to decide what to do with this. Perhaps a redirect is not required.

@NTaylorMullen

Have you tried your application on the latest dev branch (its not released yet). The #2106 fix was meant to address the errors that you're seeing.

@kbirger

Oh, sorry, I didn't realize that it was in not merged in yet. The fix looks legitimate. I'll have to let you know a little bit later how it works. Do you think it's going to hit the 2.0 RC?

@NTaylorMullen

It's in the branch for 2.0 RC, so unless it gets pulled out then yes!

@kbirger

Great. Well thanks for the hard work. 2.0 is looking really good to me so far. When I finalize our update to 2.0 beta I'm going to try the fix from dev.

@kbirger

This still fails to behave properly. When attempting to connect to longpolling, if the XHR request to /signalr/connect?transport=longPollingXXXXXXX fails with a 302, the connection.error handler is called with the response (which is the full html of the redirect page, which is not helpful), but .start().fail() is not called, because you're now simply calling connection.stop().

Also seeing an error on SSE now because the 302 response is the wrong content type.

Error: EventSource's response has a MIME type ("text/html") that is not "text/event-stream". Aborting the connection.

Have you considered not using the forms authentication module? You don't have forms in SignalR, per se, so the forms-related stuff (like 302s) don't strictly apply. You guys created this wonderful AuthorizeAttribute framework...

It is worth noting that I can circumvent the issue by doing the following in the AuthenticateRequest event:

if (Request.Headers["X-Requested-With"] == "XMLHttpRequest")
{
    Response.SuppressFormsAuthenticationRedirect = true;
}

Simply tells formsauthenticationmodule to not convert 401s into 302s.

@davidfowl
SignalR member

@kbirger This is fixed in the dev branch not beta2. What version are you using?

Also we don't use forms auth in SignalR. I'm not sure what you mean by that.

@kbirger

I'm still new to Github, but I took this code: https://github.com/SignalR/SignalR/tree/0aa09254a7171e2f7c8fa1375bef9a8448a4ebaa which seems to have the commits @NTaylorMullen made for #2106.

You don't use it directly, but it seems like between 1.1.2 and 2.0 beta 2 there were changes (perhaps regarding integrated pipeline?) that cause resources under the /signalr/ path (/negotiate, /connect, etc) to run through pipeline modules, like forms authentication, if you have one registered for your web application.

In my original report I pointed out that those resources weren't passing through Authorization, which caused undesirable behavior. In 2.0, they pass through all the modules, but this still seems to be undesirable and buggy. Putting those calls through the pipeline does trigger authentication if you have it, but if that authentication isn't friendly towards AJAX requests, you run into problems. Also, I might add that as of beta 2, IIS Express will no longer run SignalR due to lack of integrated pipeline support.

The current dev code seems to displace the original issue some, but it does not correct it completely.

@davidfowl
SignalR member

@NTaylorMullen has confirmed that another causes it to hang.

In 2.0 here's what happened:

https://github.com/SignalR/SignalR/blob/dev/src/Microsoft.AspNet.SignalR.Core/PersistentConnection.cs#L156

We don't know that forms auth is in the pipeline and there's nothing we'd going to do about that. The change we made was to return a 401 in some cases and a 403 in other cases. In some cases the client can legitimately handle the 401 challenge from the server (for other kinds of authentication). It's just happens to be the fact that forms auth causes a redirect to a login page, which will properly turn into a fail when @NTaylorMullen fixes the issue.

@davidfowl
SignalR member

"Also, I might add that as of beta 2, IIS Express will no longer run SignalR due to lack of integrated pipeline support."

What? This makes 0 sense. If that were the case, nothing would be working, not websockets, not anything. The fact is the latest SignalR 2.x requires integrated pipeline support.

@kbirger

My point is not that you should do something about the fact that forms authentication is in the pipeline, but rather that it is not clear to me why these requests need to go through pipeline to begin with when there is already an authorization mechanism in SignalR.

The IIS Express comment was a side note. To clarify, I was trying to explain my inference. A coworker pointed out that IIS Express will no longer run the project, from this I inferred that integrated pipeline is now supported, and that those requests are going through it.

Can you point me to the code in dev that is supposed to convert 302s into 40x errors please?

@davidfowl
SignalR member

Ok let me be clear:

SignalR is a framework built on top of http. When hosting in ASP.NET IIS passes the request to ASP.NET which passes it to the Owin layer which passes it to SignalR. There's no skipping this process. There's a FormsAuthenticationModule in ASP.NET that runs when you have it enabled, that turns the 401 into a 302. There's no code in SignalR that does this.

IIS Express definitely works with 2.x so I have no idea what you mean. If you can provide clear steps to reproduce this issue then file another bug.

@kbirger

So what was causing all of this to be skipped in 1.1.2? I could get to /signalr/connect and negotiate without any authentication.

@davidfowl
SignalR member

If you read the comment before, I explain the changes between 1.1.2 and 2.x. I'll paste it again:

In 2.0 here's what happened:

https://github.com/SignalR/SignalR/blob/dev/src/Microsoft.AspNet.SignalR.Core/PersistentConnection.cs#L156

We don't know that forms auth is in the pipeline and there's nothing we'd going to do about that. The change we made was to return a 401 in some cases and a 403 in other cases. In some cases the client can legitimately handle the 401 challenge from the server (for other kinds of authentication). It's just happens to be the fact that forms auth causes a redirect to a login page, which will properly turn into a fail when @NTaylorMullen fixes the issue.

@gustavo-armenta

I see start.fail() is invoked when user is not authorized.
I will re-test again after fix #2145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.