aspnet-contrib / AspNet.Security.OAuth.Providers

OAuth 2.0 social authentication providers for ASP.NET Core
Apache License 2.0
2.35k stars 533 forks source link

Twitter v2: The oauth state was missing or invalid. #809

Closed Mike-E-angelo closed 9 months ago

Mike-E-angelo commented 9 months ago

Describe the bug

I am seeing a potential bug with the Twitter v2 provider. Essentally, when the returnUrl parameter exceeds a certain length, the oauth2 state becomes invalid and the following exception is thrown:

AspNet.Security.OAuth.Twitter.TwitterAuthenticationHandler: Information: Error from RemoteAuthentication: The oauth state was missing or invalid..
System.Exception: The oauth state was missing or invalid.

Steps To reproduce

Using the default ExternalLoginModel from AspNetCore Scaffolding as a reference, this works:

        public IActionResult OnPost(string provider, string returnUrl = null)
        {
            // Request a redirect to the external login provider.
            var redirectUrl = Url.Page("./ExternalLogin", pageHandler: "Callback", values: new { returnUrl = "/1212121/12121212121/121212121/1" });
            var properties  = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl);
            return new ChallengeResult(provider, properties);
        }

... but this does not (note extra character):

        public IActionResult OnPost(string provider, string returnUrl = null)
        {
            // Request a redirect to the external login provider.
            var redirectUrl = Url.Page("./ExternalLogin", pageHandler: "Callback", values: new { returnUrl = "/1212121/12121212121/121212121/12" }); // Note extra character.
            var properties  = _signInManager.ConfigureExternalAuthenticationProperties(provider, redirectUrl);
            return new ChallengeResult(provider, properties);
        }

When running the above with the extra character, the user is presented with this screen:

image

Expected behaviour

The character length of the returnUrl does not impact oauth2 state validity.

Actual behaviour

User is presented with message:

image

Error is thrown in logs:

AspNet.Security.OAuth.Twitter.TwitterAuthenticationHandler: Information: Error from RemoteAuthentication: The oauth state was missing or invalid..
System.Exception: The oauth state was missing or invalid.

System information

Runtime Environment: OS Name: Windows OS Version: 10.0.19045 OS Platform: Windows RID: win-x64 Base Path: C:\Program Files\dotnet\sdk\8.0.100-rc.1.23455.8\

.NET workloads installed: There are no installed workloads to display.

Host: Version: 8.0.0-rc.1.23419.4 Architecture: x64 Commit: 92959931a3 RID: win-x64

.NET SDKs installed: 3.1.426 [C:\Program Files\dotnet\sdk] 5.0.408 [C:\Program Files\dotnet\sdk] 6.0.100-rc.2.21505.1 [C:\Program Files\dotnet\sdk] 6.0.202 [C:\Program Files\dotnet\sdk] 6.0.203 [C:\Program Files\dotnet\sdk] 6.0.317 [C:\Program Files\dotnet\sdk] 7.0.401 [C:\Program Files\dotnet\sdk] 8.0.100-rc.1.23455.8 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.All 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.All 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.All 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.29 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.0-rc.2.21470.37 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.22 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 8.0.0-rc.1.23421.29 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.29 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.0-rc.2.21470.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.22 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 8.0.0-rc.1.23419.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.29 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.0-rc.2.21470.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.22 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 8.0.0-rc.1.23420.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found: arm64 [C:\Program Files\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\arm64\InstallLocation] x86 [C:\Program Files (x86)\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables: Not set

global.json file: Not found

Learn more: https://aka.ms/dotnet/info

Download .NET: https://aka.ms/dotnet/download



### Additional context

Thank you for all your great effort over there.  This is the only thing I have found lately. :)
martincostello commented 9 months ago

This came up during the initial development of the provider but then went away: https://github.com/dotnet/aspnetcore/issues/39664#issuecomment-1018001027

50:50 on whether it's a latent bug with some weird edge cases, or some latest CEO-driven development broke something on their end.

kevinchalet commented 9 months ago

This issue was also discussed here: https://github.com/dotnet/aspnetcore/issues/42192

50:50 on whether it's a latent bug with some weird edge cases, or some latest CEO-driven development broke something on their end.

It's neither a bug nor one of those stupid Musky decisions: the Twitter devs just opted for a way too short limit for the state parameter, that cannot exceed 500 chars exactly. If your AuthenticationProperties is populated with too much data (e.g because a large ReturnUrl is stored), the resulting DP-protected state will exceed Twitter's limit and you'll get a generic error.

@Mike-E-angelo you could probably work around this issue by creating an ISecureDataFormat<AuthenticationProperties> that stores the actual data in a distributed cache and returns a reference ID with a fixed short length.

Alternatively, you can replace the aspnet-contrib Twitter by its OpenIddict equivalent, that doesn't suffer from this issue as the JWT state tokens it produces are stored in the DB. The OpenIddict client and its ~60 providers generally require a bit more configuration (as there's less magic involved) but is also much more flexible and comes with tons of additional security checks compared to the aspnet-contrib. More info here: https://kevinchalet.com/2022/12/16/getting-started-with-the-openiddict-web-providers/

Mike-E-angelo commented 9 months ago

Oh my goodness, thank you both @martincostello and @kevinchalet for your great support and guidance! It is greatly appreciated. It's also a far cry from Twitter which is really quite abysmal. You may or may not be aware of this, but developers are paying $5k/mo and not getting any dedicated support for the use of Twitter API. 🤯 In light of this and in comparison to Twitter support, I feel like I just got $5k worth of support from your responses alone. :D

It definitely makes me appreciative when I see replies here in GitHub from Microsoft repositories and on developercommunity the replies, and really after having to try deal with Twitter support, any acknowledgment of my existence. 😅 🙏🙏🙏

I will look into the IDistributedCache solution as this is something I have been meaning to do w/ my application, the whole scaling story and all. :D

I will close this ticket now that the limitation is understood. Thank you again for your efforts and time. 🙏

Mike-E-angelo commented 9 months ago

OK I got the ISecureDataFormat<AuthenticationProperties> implemented and everything works as expected, @kevinchalet. Thank you again for the guidance.

There is one hitch though and one I have been noodling on. The ISecureDataFormat<AuthenticationProperties is synchronous and IDistrubutedCache is asynchronous in nature. IDistributedCache has synchronous methods, of course, but if I understand correctly, that could possibly lead to thread starvation if enough calls are made concurrently.

I wanted to throw that your way and gauge my understanding of the situation. Is this something to worry about? Is there another consideration to make it asynchronous all the way? Thank you for any continued insight you can provide.

martincostello commented 9 months ago

Two things I'd throw in the mix for your consideration:

  1. How slow is your cache? If it's very fast, then any sync call isn't going to consume much resources.
  2. How often do you actually expect people to be signing in and out of your application with Twitter? I would have thought relative to the rest of your application's purpose it would be small.
Mike-E-angelo commented 9 months ago

Much along the lines I have been thinking @martincostello. This is actually a limited scenario, so from the outset I would say it's OK to brave the sync path as there should not be much thread utilization during the process. However, if you know anything about the asynchronous underbelly of .NET there's a lot that can bite you. 😬

Additionally, even if the cache is typically fast, if it does hang for whatever reason (HTTP request to Redis, I am guessing) that is a thread that is being held up which is a Bad Thing™ as I understand it with the asynchronous model.

So, I guess I am wanting to be a little confident here that I can get away with this. Sounds like I can, but wanted to be sure of my reasoning. :)