Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
945 stars 605 forks source link

Error on blank RelayState when RelayStateUsedAsReturnUrl = true #1381

Closed jheitz1117 closed 10 months ago

jheitz1117 commented 1 year ago

NuGet Packages: Sustainsys.Saml2 2.9.0 Sustainsys.Saml2.Owin 2.9.0

Compiled against: .NET Framework 4.7.2 Latest installed: .NET Framework 4.8

Expected behavior: Ignore blank RelayState when RelayStateUsedAsReturnUrl = true (SSO shouldn't require a RelayState)

Observed behavior: If RelayStateUsedAsReturnUrl = true, then a blank RelayState raises an exception as follows:

System.InvalidOperationException: Return Url must be a relative Url.
   at Sustainsys.Saml2.WebSso.AcsCommand.GetLocation(StoredRequestState storedRequestState, IdentityProvider identityProvider, String relayState, IOptions options)
   at Sustainsys.Saml2.WebSso.AcsCommand.ProcessResponse(IOptions options, Saml2Response samlResponse, StoredRequestState storedRequestState, IdentityProvider identityProvider, String relayState)
   at Sustainsys.Saml2.WebSso.AcsCommand.Run(HttpRequestData request, IOptions options)
   at Sustainsys.Saml2.Owin.Saml2AuthenticationHandler.<AuthenticateCoreAsync>d__0.MoveNext()

I have a use case where end-users need to authenticate to my web app using IDP-initiated SSO via Okta. I was able to successfully implement this using Sustainsys.Saml2 in an OWIN pipeline. This implementation did not originally have any requirements for a landing page after login, so I did not set the RelayStateUsedAsReturnUrl property of the IdentityProvider object:

            saml2Options.IdentityProviders.Add(
                new IdentityProvider(new EntityId(siteConfig.SamlIdpIssuerID), saml2Options.SPOptions)
                {
                    LoadMetadata = true,
                    MetadataLocation = siteConfig.SamlIdpMetadataUrl,
                    AllowUnsolicitedAuthnResponse = true
                }
            );

Now my client wishes to add support for deep links. I naturally updated the code to set RelayStateUsedAsReturnUrl = True:

            saml2Options.IdentityProviders.Add(
                new IdentityProvider(new EntityId(siteConfig.SamlIdpIssuerID), saml2Options.SPOptions)
                {
                    LoadMetadata = true,
                    MetadataLocation = siteConfig.SamlIdpMetadataUrl,
                    AllowUnsolicitedAuthnResponse = true,
                    RelayStateUsedAsReturnUrl = true
                }
            );

This seems to work exactly as expected as long as the provided RelayState is a valid URL. Deep links now work as intended. However, Okta has a default relay state that is sent for the IDP-initiated flow whenever no other relay state is provided (such as during a normal login without a deep a link). The default relay state can be blank, and likely is for most integrations with my app. Unfortunately, if RelayStateUsedAsReturnUrl = true, then a blank RelayState causes the AcsCommand to throw an exception as described above.

This behavior effectively breaks the existing SSO functionality with Okta because the blank RelayState that was fine previously (or ignored?) is now being actively rejected. I acknowledge that one possible workaround is to ask the client to update their Okta integration with a non-blank default relay state, but it seems unreasonable to require every other client to update their Okta config to accommodate a feature they didn't ask for. While I do expect RelayStateUsedAsReturnUrl = true to validate that a non-blank RelayState is a valid URL, I also expect it to treat a blank RelayState the same as if RelayStateUsedAsReturnUrl = false.

Can the AcsCommand module be updated to ignore a blank RelayState parameter when RelayStateUsedAsReturnUrl = true?

AndersAbel commented 1 year ago

This looks like a bug in the library.