Skip to content

Commit

Permalink
Fix AuthController.RedirectToProvider not round-tripping returnUrl (#425
Browse files Browse the repository at this point in the history
)

This PR fixes an issue with `AuthController.RedirectToProvider` not encoding `id` and `returnUrl` values within the OIDC `state` parameter, leading the server to redirect to the wrong URL after login in some cases.

The `state` parameter is expected to decode as if it was a query string, but the values within the parameter weren't being encoded as query string values, so the decoding was lossy and would not round-trip the return URL if that URL contained more than one query parameter (e.g. for an exception detail URL: `/exceptions/detail?id=...&log=...&store=...`).

**Steps to reproduce:**

1. Ensure Opserver is configured with OIDC sign-in and an exception is present in a configured exception store
2. Go to the exception detail URL (e.g. `https://opserver.example.com/exceptions/detail?id=00000000-0000-0000-0000-000000000000&log=Core&store=Example+%3A+Production`) while not logged in
3. Sign in with the identity provider
4. Observe the HTTP response

**Before this PR**, Opserver will redirect you to `https://opserver.example.com/exceptions/detail?id=00000000-0000-0000-0000-000000000000` after logging in (missing the `log` and `store` query parameters in the URL), so you'll get the "Error was not found. If this link worked previously, the error was hard deleted." page.

**After this PR**, Opserver should redirect you to `https://opserver.example.com/exceptions/detail?id=00000000-0000-0000-0000-000000000000&log=Core&store=Example+%3A+Production` as expected.

Let me know if you'd prefer the OIDC `state` parameter encoding/decoding to have unit tests, or any other feedback or desired changes!

---

<details><summary>LINQPad script to compare before and after code:</summary>

```csharp
void Main()
{
    "Before PR Auth URL:".Dump();
    var beforePRAuthUrl = BeforePR_GetAuthUrl();
    beforePRAuthUrl.Dump();
    DecodeStateFromAuthUrl(beforePRAuthUrl).Dump();
    
    "After PR Auth URL:".Dump();
    var afterPRAuthUrl = AfterPR_GetAuthUrl();
    afterPRAuthUrl.Dump();
    DecodeStateFromAuthUrl(afterPRAuthUrl).Dump();
}

Dictionary<string, Microsoft.Extensions.Primitives.StringValues> DecodeStateFromAuthUrl(string authUrl)
{
    var query = QueryHelpers.ParseQuery(new Uri(authUrl).Query);
    var state = query["state"];
    
    // decode the state and ensure the passed identifier matches
    // what we have in the cookies passed from the user agent
    var decodedState = QueryHelpers.ParseQuery(state);
    return decodedState;
}

string BeforePR_GetAuthUrl()
{
    // construct the URL to the authorization endpoint
    var authorizationUrl = new UriBuilder("https://idp.example.net/oauth2/v1/authorize");
    var scopes = new string[] { "openid", "email", "groups", "profile" };
    var queryString = new QueryString(authorizationUrl.Query)
        .Add("response_type", "code")
        .Add("client_id", "00000000000000000000")
        .Add("scope", string.Join(' ', scopes))
        .Add("redirect_uri", "https://opserver.example.com/login/oauth/callback")
        .Add("state", $"id=0000000000000000000000000000000000000000000=&returnUrl=/exceptions/detail?id=00000000-0000-0000-0000-000000000000&log=Core&store=Example+%3A+Production")
        .Add("nonce", Guid.NewGuid().ToString("N"));

    authorizationUrl.Query = queryString.ToUriComponent();

    return authorizationUrl.ToString();
}

string AfterPR_GetAuthUrl()
{
    // construct the URL to the authorization endpoint
    var authorizationUrl = new UriBuilder("https://oidc.example.net/oauth2/v1/authorize");
    var scopes = new string[] { "openid", "email", "groups", "profile" };
    var encodedState = new QueryString()
        .Add("id", "0000000000000000000000000000000000000000000=")
        .Add("returnUrl", "/exceptions/detail?id=00000000-0000-0000-0000-000000000000&log=Core&store=Example+%3A+Production");
    var queryString = new QueryString(authorizationUrl.Query)
        .Add("response_type", "code")
        .Add("client_id", "00000000000000000000")
        .Add("scope", string.Join(' ', scopes))
        .Add("redirect_uri", "https://opserver.example.com/login/oauth/callback")
        .Add("state", encodedState.ToUriComponent())
        .Add("nonce", Guid.NewGuid().ToString("N"));

    authorizationUrl.Query = queryString.ToUriComponent();

    return authorizationUrl.ToString();
}
```

</details>

**Result:**

![Screenshot of the attached LINQPad script showing the code before PR with an incorrectly-encoded state parameter, and the code after PR with correctly-encoded state parameter](https://github.com/opserver/Opserver/assets/844245/526027d6-6301-4b25-a3f5-8cb131048b55)
  • Loading branch information
dionrhys authored Aug 22, 2023
1 parent cae2dd9 commit e5fc22c
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/Opserver.Web/Controllers/AuthController.OIDC.cs
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,15 @@ private IActionResult RedirectToProvider(string returnUrl)
// construct the URL to the authorization endpoint
var authorizationUrl = new UriBuilder(oidcSettings.AuthorizationUrl);
var scopes = oidcSettings.Scopes ?? OIDCSecuritySettings.DefaultScopes;
var encodedState = new QueryString()
.Add(OidcIdentifierKey, oidcIdentifier)
.Add(OidcReturnUrlKey, returnUrl ?? "/");
var queryString = new QueryString(authorizationUrl.Query)
.Add("response_type", "code")
.Add("client_id", oidcSettings.ClientId)
.Add("scope", string.Join(' ', scopes))
.Add("redirect_uri", redirectUri)
.Add("state", $"{OidcIdentifierKey}={oidcIdentifier}&{OidcReturnUrlKey}={returnUrl ?? "/"}")
.Add("state", encodedState.ToUriComponent())
.Add("nonce", Guid.NewGuid().ToString("N"));

authorizationUrl.Query = queryString.ToUriComponent();
Expand Down

0 comments on commit e5fc22c

Please sign in to comment.