Consider throwing a 400 instead of a 500

[janpieterz]

Not sure if it should be here or in MVC, but this seems like the right place.

I'm playing around with getting this to work for API calls. I notice that when I don't deliver an anti forgery token, the request results in a 500. Maybe this is just me, but shouldn't this be a 400? The server shouldn't fail (and log a 500) simply because I (or any other third party) forgets to add this token (read: bad request).

Exception I get is:

 An unhandled exception has occurred: The required antiforgery form field "__RequestVerificationToken" is not present.
      System.InvalidOperationException: The required antiforgery form field "__RequestVerificationToken" is not present.
         at Microsoft.AspNet.Antiforgery.DefaultAntiforgeryTokenStore.<GetRequestTokensAsync>d__4.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Antiforgery.DefaultAntiforgery.<ValidateRequestAsync>d__9.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Mvc.Controllers.FilterActionInvoker.<InvokeAuthorizationFilterAsync>d__47.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Mvc.Controllers.FilterActionInvoker.<InvokeAuthorizationFilterAsync>d__47.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Mvc.Controllers.FilterActionInvoker.<InvokeAsync>d__44.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Mvc.Infrastructure.MvcRouteHandler.<RouteAsync>d__6.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Mvc.Routing.InnerAttributeRoute.<RouteAsync>d__10.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Routing.RouteCollection.<RouteAsync>d__9.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Builder.RouterMiddleware.<Invoke>d__4.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at Microsoft.AspNet.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.IISPlatformHandler.IISPlatformHandlerMiddleware.<Invoke>d__8.MoveNext()
      --- End of stack trace from previous location where exception was thrown ---
         at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
         at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
         at Microsoft.AspNet.Diagnostics.ExceptionHandlerMiddleware.<Invoke>d__6.MoveNext()

[rynowak]

Yeah, the logging aspect of this is probably pretty relevant. I think a big part of the reason why we historically throw (500) here is that it's expected to be a programming error on the part the app developer.

This isn't an exception you'd ever want to handle in application code. At the same time, we don't want to redirect you into an auth flow since it's a misconfigured client.

@lodejard @blowdart @Eilon

[Eilon]

I'm fine changing this to a 400 - the client made an error and "forgot" to send the token. (Of course, philosophically, any client error could be conceived of as a server error because the programmer "forgot" to program something - support for a certain URL, verb, header, etc.)

[Eilon]

@rynowak BTW why the "auth flow" remark? A 400 won't take you to an auth flow (only 401/403 would do that).

[rynowak]

I know that I don't know anything about auth.

[ajaybhargavb]

Spoke to @pranavkm and @lodejard. Since there is a possibility for 500 to be thrown due to multiple reasons, the idea was to throw a custom AntiforgeryException for cases we want to throw a bad request which we can then catch in ValidateAntiforgeryTokenAuthorizationFilter and throw a 400.
Thoughts @Eilon @rynowak ?

[rynowak]

So, I totes love custom exception types, but there's a difference between a failure of the antiforgery system, and an invalid token. Are you talking about the former or the latter?

[ajaybhargavb]

Spoke in person. We decided to throw a custom exception for no/invalid token cases and let the other cases just throw a 500.

[ajaybhargavb]

80fa290

[ajaybhargavb]

aspnet/Mvc@bf1fcf6