Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
960 stars 602 forks source link

Crash when posting to secure page when the user is not logged in. #562

Closed albinsunnanbo closed 4 years ago

albinsunnanbo commented 8 years ago

Reproduction Use the code in the branch https://github.com/albinsunnanbo/authservices/tree/TestFormSubmit

  1. Start the StubIdp and SampleOwinApplication projects
  2. In the SamleOwinApplication web: Click "Secure Form" in the menu.
  3. Click "Post form". Result: Successful postback.
  4. Open the SamleOwinApplication in another browser tab
  5. Log out from the second tab and go back to the first tab
  6. Click "Post form". Expected result: Redirect to login page. Actual result: Exception below

Note this happens only when the posted form contains multiple fields with the same name.

Stack trace;

[InvalidOperationException: Sequence contains more than one element]
   System.Linq.Enumerable.Single(IEnumerable`1 source) +396
   Kentor.AuthServices.WebSso.<>c.<InitBasicFields>b__6_1(KeyValuePair`2 kv) in C:\Git\authservices\Kentor.AuthServices\WebSso\HttpRequestData.cs:136
   System.Linq.Enumerable.ToDictionary(IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer) +236
   System.Linq.Enumerable.ToDictionary(IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector) +57
   Kentor.AuthServices.WebSso.HttpRequestData.InitBasicFields(String httpMethod, Uri url, String applicationPath, IEnumerable`1 formData) in C:\Git\authservices\Kentor.AuthServices\WebSso\HttpRequestData.cs:134
   Kentor.AuthServices.WebSso.HttpRequestData.Init(String httpMethod, Uri url, String applicationPath, IEnumerable`1 formData, IEnumerable`1 cookies, Func`2 cookieDecryptor, ClaimsPrincipal user) in C:\Git\authservices\Kentor.AuthServices\WebSso\HttpRequestData.cs:96
   Kentor.AuthServices.WebSso.HttpRequestData..ctor(String httpMethod, Uri url, String applicationPath, IEnumerable`1 formData, IEnumerable`1 cookies, Func`2 cookieDecryptor, ClaimsPrincipal user) in C:\Git\authservices\Kentor.AuthServices\WebSso\HttpRequestData.cs:66
   Kentor.AuthServices.Owin.<ToHttpRequestData>d__0.MoveNext() in C:\Git\authservices\Kentor.AuthServices.Owin\OwinContextExtensions.cs:38
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   System.Runtime.CompilerServices.TaskAwaiter`1.GetResult() +28
   Kentor.AuthServices.Owin.<ApplyResponseChallengeAsync>d__1.MoveNext() in C:\Git\authservices\Kentor.AuthServices.Owin\KentorAuthServicesAuthenticationHandler.cs:74
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Security.Infrastructure.<ApplyResponseCoreAsync>d__b.MoveNext() +292
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Security.Infrastructure.<ApplyResponseAsync>d__8.MoveNext() +278
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Security.Infrastructure.<TeardownAsync>d__5.MoveNext() +165
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Security.Infrastructure.<Invoke>d__0.MoveNext() +716
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.<RunApp>d__5.MoveNext() +187
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Security.Infrastructure.<Invoke>d__0.MoveNext() +561
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Security.Infrastructure.<Invoke>d__0.MoveNext() +561
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.AspNet.Identity.Owin.<Invoke>d__0.MoveNext() +409
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.AspNet.Identity.Owin.<Invoke>d__0.MoveNext() +409
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.AspNet.Identity.Owin.<Invoke>d__0.MoveNext() +409
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.<RunApp>d__5.MoveNext() +187
   System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +92
   System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +58
   Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.<DoFinalWork>d__2.MoveNext() +185
   Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.StageAsyncResult.End(IAsyncResult ar) +69
   Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.IntegratedPipelineContext.EndFinalWork(IAsyncResult ar) +64
   System.Web.AsyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +380
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +155
albinsunnanbo commented 8 years ago

Is this related to the fact that the middleware is an active middleware?

albinsunnanbo commented 8 years ago

It looks like the HttpRequestData.Form should only mind about fields AuthServices actually uses itself and be resilient against unknown data.

A suggestion would be to instead of parsing all formData in HttpRequestData.InitBasicFields encapsulate form data in a lookup class that find fields dynamically on each lookup. This way we would only look att fields that we actually expects to use.

Without doing any profiling it does not look like we use it frequently. Thus keeping the data in a dictionary should not be required for performance reasons.

AndersAbel commented 4 years ago

With the Owin middleware switched over to passive this shouldn't be a problem any more and the age and inactivity on the issue suggests it's not a problem.