opserver / Opserver

Stack Exchange's Monitoring System
https://opserver.github.io/Opserver/
MIT License
4.52k stars 826 forks source link

Fix AuthController.RedirectToProvider not round-tripping returnUrl #425

Closed dionrhys closed 1 year ago

dionrhys commented 1 year ago

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!


LINQPad script to compare before and after code: ```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 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(); } ```

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

NickCraver commented 1 year ago

@dionrhys Thanks for this! If you want to add tests I'm totally game to review, but appreciate the manual bits here and wanted to unblock ASAP as well.

dionrhys commented 1 year ago

Np and thank you! I had a look at adding tests for OIDC state reading and writing specifically but it's quite a shallow part of the logic here that I wasn't sure adding the complexity of extracting these out was worth the effort. Adding tests for the controller actions as a whole seemed like quite a bit of work as well.