fix: review comments

frontend/src/lib/hooks/oidc.ts

@@ -1,64 +1,40 @@
-export type OIDCValues = {
+import { z } from "zod";
-  scope: string;
-  response_type: string;
-  client_id: string;
-  redirect_uri: string;
-  state: string;
-  nonce: string;
-  code_challenge: string;
-  code_challenge_method: string;
-};
 
-interface IuseOIDCParams {
+export const oidcParamsSchema = z.object({
-  values: OIDCValues;
+  scope: z.string().nonempty(),
-  compiled: string;
+  response_type: z.string().nonempty(),
+  client_id: z.string().nonempty(),
+  redirect_uri: z.string().nonempty(),
+  state: z.string().optional(),
+  nonce: z.string().optional(),
+  code_challenge: z.string().optional(),
+  code_challenge_method: z.string().optional(),
+});
+
+export const useOIDCParams = (
+  params: URLSearchParams,
+): {
+  values: z.infer<typeof oidcParamsSchema>;
+  issues: string[];
   isOidc: boolean;
-  missingParams: string[];
+  compiled: string;
-}
+} => {
+  const obj = Object.fromEntries(params.entries());
+  const parsed = oidcParamsSchema.safeParse(obj);
 
-const optionalParams: string[] = [
+  if (parsed.success) {
-  "state",
+    return {
-  "nonce",
+      values: parsed.data,
-  "code_challenge",
+      issues: [],
-  "code_challenge_method",
+      isOidc: true,
-];
+      compiled: new URLSearchParams(parsed.data).toString(),
-
+    };
-export function useOIDCParams(params: URLSearchParams): IuseOIDCParams {
-  let compiled: string = "";
-  let isOidc = false;
-  const missingParams: string[] = [];
-
-  const values: OIDCValues = {
-    scope: params.get("scope") ?? "",
-    response_type: params.get("response_type") ?? "",
-    client_id: params.get("client_id") ?? "",
-    redirect_uri: params.get("redirect_uri") ?? "",
-    state: params.get("state") ?? "",
-    nonce: params.get("nonce") ?? "",
-    code_challenge: params.get("code_challenge") ?? "",
-    code_challenge_method: params.get("code_challenge_method") ?? "",
-  };
-
-  for (const key of Object.keys(values)) {
-    if (!values[key as keyof OIDCValues]) {
-      if (!optionalParams.includes(key)) {
-        missingParams.push(key);
-      }
-    }
-  }
-
-  if (missingParams.length === 0) {
-    isOidc = true;
-  }
-
-  if (isOidc) {
-    compiled = new URLSearchParams(values).toString();
   }
 
   return {
-    values,
+    issues: parsed.error.issues.map((issue) => issue.path.toString()),
-    compiled,
+    values: {} as z.infer<typeof oidcParamsSchema>,
-    isOidc,
+    isOidc: false,
-    missingParams,
+    compiled: "",
   };
-}
+};

frontend/src/pages/authorize-page.tsx

@@ -72,36 +72,27 @@ export const AuthorizePage = () => {
   const scopeMap = createScopeMap(t);
 
   const searchParams = new URLSearchParams(search);
-  const {
+  const oidcParams = useOIDCParams(searchParams);
-    values: props,
-    missingParams,
-    isOidc,
-    compiled: compiledOIDCParams,
-  } = useOIDCParams(searchParams);
-  const scopes = props.scope ? props.scope.split(" ").filter(Boolean) : [];
 
   const getClientInfo = useQuery({
-    queryKey: ["client", props.client_id],
+    queryKey: ["client", oidcParams.values.client_id],
     queryFn: async () => {
-      const res = await fetch(`/api/oidc/clients/${props.client_id}`);
+      const res = await fetch(
+        `/api/oidc/clients/${encodeURIComponent(oidcParams.values.client_id)}`,
+      );
       const data = await getOidcClientInfoSchema.parseAsync(await res.json());
       return data;
     },
-    enabled: isOidc,
+    enabled: oidcParams.isOidc,
   });
 
   const authorizeMutation = useMutation({
     mutationFn: () => {
       return axios.post("/api/oidc/authorize", {
-        scope: props.scope,
+        ...oidcParams.values,
-        response_type: props.response_type,
-        client_id: props.client_id,
-        redirect_uri: props.redirect_uri,
-        state: props.state,
-        nonce: props.nonce,
       });
     },
-    mutationKey: ["authorize", props.client_id],
+    mutationKey: ["authorize", oidcParams.values.client_id],
     onSuccess: (data) => {
       toast.info(t("authorizeSuccessTitle"), {
         description: t("authorizeSuccessSubtitle"),
@@ -115,17 +106,17 @@ export const AuthorizePage = () => {
     },
   });
 
-  if (missingParams.length > 0) {
+  if (oidcParams.issues.length > 0) {
     return (
       <Navigate
-        to={`/error?error=${encodeURIComponent(t("authorizeErrorMissingParams", { missingParams: missingParams.join(", ") }))}`}
+        to={`/error?error=${encodeURIComponent(t("authorizeErrorMissingParams", { missingParams: oidcParams.issues.join(", ") }))}`}
         replace
       />
     );
   }
 
   if (!isLoggedIn) {
-    return <Navigate to={`/login?${compiledOIDCParams}`} replace />;
+    return <Navigate to={`/login?${oidcParams.compiled}`} replace />;
   }
 
   if (getClientInfo.isLoading) {
@@ -152,6 +143,9 @@ export const AuthorizePage = () => {
     );
   }
 
+  const scopes =
+    oidcParams.values.scope.split(" ").filter((s) => s.trim() !== "") || [];
+
   return (
     <Card>
       <CardHeader className="mb-2">

frontend/src/pages/login-page.tsx

@@ -51,15 +51,12 @@ export const LoginPage = () => {
   const formId = useId();
 
   const searchParams = new URLSearchParams(search);
-  const {
+  const redirectUri = searchParams.get("redirect_uri") || undefined;
-    values: props,
+  const oidcParams = useOIDCParams(searchParams);
-    isOidc,
-    compiled: compiledOIDCParams,
-  } = useOIDCParams(searchParams);
 
   const [isOauthAutoRedirect, setIsOauthAutoRedirect] = useState(
     providers.find((provider) => provider.id === oauthAutoRedirect) !==
-      undefined && props.redirect_uri,
+      undefined && redirectUri !== undefined,
   );
 
   const oauthProviders = providers.filter(
@@ -76,14 +73,10 @@ export const LoginPage = () => {
     isPending: oauthIsPending,
     variables: oauthVariables,
   } = useMutation({
-    mutationFn: (provider: string) => {
+    mutationFn: (provider: string) =>
-      const params = isOidc
+      axios.get(
-        ? `?${compiledOIDCParams}`
+        `/api/oauth/url/${provider}${redirectUri ? `?redirect_uri=${encodeURIComponent(redirectUri)}` : ""}`,
-        : props.redirect_uri
+      ),
-          ? `?redirect_uri=${encodeURIComponent(props.redirect_uri)}`
-          : "";
-      return axios.get(`/api/oauth/url/${provider}${params}`);
-    },
     mutationKey: ["oauth"],
     onSuccess: (data) => {
       toast.info(t("loginOauthSuccessTitle"), {
@@ -113,8 +106,12 @@ export const LoginPage = () => {
     mutationKey: ["login"],
     onSuccess: (data) => {
       if (data.data.totpPending) {
+        if (oidcParams.isOidc) {
+          window.location.replace(`/totp?${oidcParams.compiled}`);
+          return;
+        }
         window.location.replace(
-          `/totp${props.redirect_uri ? `?redirect_uri=${encodeURIComponent(props.redirect_uri)}` : ""}`,
+          `/totp${redirectUri ? `?redirect_uri=${encodeURIComponent(redirectUri)}` : ""}`,
         );
         return;
       }
@@ -124,12 +121,12 @@ export const LoginPage = () => {
       });
 
       redirectTimer.current = window.setTimeout(() => {
-        if (isOidc) {
+        if (oidcParams.isOidc) {
-          window.location.replace(`/authorize?${compiledOIDCParams}`);
+          window.location.replace(`/authorize?${oidcParams.compiled}`);
           return;
         }
         window.location.replace(
-          `/continue${props.redirect_uri ? `?redirect_uri=${encodeURIComponent(props.redirect_uri)}` : ""}`,
+          `/continue${redirectUri ? `?redirect_uri=${encodeURIComponent(redirectUri)}` : ""}`,
         );
       }, 500);
     },
@@ -148,7 +145,7 @@ export const LoginPage = () => {
       !isLoggedIn &&
       isOauthAutoRedirect &&
       !hasAutoRedirectedRef.current &&
-      props.redirect_uri
+      redirectUri !== undefined
     ) {
       hasAutoRedirectedRef.current = true;
       oauthMutate(oauthAutoRedirect);
@@ -159,7 +156,7 @@ export const LoginPage = () => {
     hasAutoRedirectedRef,
     oauthAutoRedirect,
     isOauthAutoRedirect,
-    props.redirect_uri,
+    redirectUri,
   ]);
 
   useEffect(() => {
@@ -174,14 +171,14 @@ export const LoginPage = () => {
     };
   }, [redirectTimer, redirectButtonTimer]);
 
-  if (isLoggedIn && isOidc) {
+  if (isLoggedIn && oidcParams.isOidc) {
-    return <Navigate to={`/authorize?${compiledOIDCParams}`} replace />;
+    return <Navigate to={`/authorize?${oidcParams.compiled}`} replace />;
   }
 
-  if (isLoggedIn && props.redirect_uri !== "") {
+  if (isLoggedIn && redirectUri !== undefined) {
     return (
       <Navigate
-        to={`/continue${props.redirect_uri ? `?redirect_uri=${encodeURIComponent(props.redirect_uri)}` : ""}`}
+        to={`/continue${redirectUri ? `?redirect_uri=${encodeURIComponent(redirectUri)}` : ""}`}
         replace
       />
     );

frontend/src/pages/totp-page.tsx

@@ -27,11 +27,8 @@ export const TotpPage = () => {
   const redirectTimer = useRef<number | null>(null);
 
   const searchParams = new URLSearchParams(search);
-  const {
+  const redirectUri = searchParams.get("redirect_uri") || undefined;
-    values: props,
+  const oidcParams = useOIDCParams(searchParams);
-    isOidc,
-    compiled: compiledOIDCParams,
-  } = useOIDCParams(searchParams);
 
   const totpMutation = useMutation({
     mutationFn: (values: TotpSchema) => axios.post("/api/user/totp", values),
@@ -42,13 +39,13 @@ export const TotpPage = () => {
       });
 
       redirectTimer.current = window.setTimeout(() => {
-        if (isOidc) {
+        if (oidcParams.isOidc) {
-          window.location.replace(`/authorize?${compiledOIDCParams}`);
+          window.location.replace(`/authorize?${oidcParams.compiled}`);
           return;
         }
 
         window.location.replace(
-          `/continue${props.redirect_uri ? `?redirect_uri=${encodeURIComponent(props.redirect_uri)}` : ""}`,
+          `/continue${redirectUri ? `?redirect_uri=${encodeURIComponent(redirectUri)}` : ""}`,
         );
       }, 500);
     },

internal/controller/oauth_controller.go

@@ -62,29 +62,7 @@ func (controller *OAuthController) oauthURLHandler(c *gin.Context) {
 		return
 	}
 
-	var reqParams service.OAuthURLParams
+	sessionId, session, err := controller.auth.NewOAuthSession(req.Provider)
-
-	err = c.BindQuery(&reqParams)
-
-	if err != nil {
-		tlog.App.Error().Err(err).Msg("Failed to bind query parameters")
-		c.JSON(400, gin.H{
-			"status":  400,
-			"message": "Bad Request",
-		})
-		return
-	}
-
-	if !controller.isOidcRequest(reqParams) {
-		isRedirectSafe := utils.IsRedirectSafe(reqParams.RedirectURI, controller.config.CookieDomain)
-
-		if !isRedirectSafe {
-			tlog.App.Warn().Str("redirect_uri", reqParams.RedirectURI).Msg("Unsafe redirect URI detected, ignoring")
-			reqParams.RedirectURI = ""
-		}
-	}
-
-	sessionId, _, err := controller.auth.NewOAuthSession(req.Provider, reqParams)
 
 	if err != nil {
 		tlog.App.Error().Err(err).Msg("Failed to create OAuth session")
@@ -107,6 +85,20 @@ func (controller *OAuthController) oauthURLHandler(c *gin.Context) {
 	}
 
 	c.SetCookie(controller.config.OAuthSessionCookieName, sessionId, int(time.Hour.Seconds()), "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
+	c.SetCookie(controller.config.CSRFCookieName, session.State, int(time.Hour.Seconds()), "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
+
+	redirectURI := c.Query("redirect_uri")
+	isRedirectSafe := utils.IsRedirectSafe(redirectURI, controller.config.CookieDomain)
+
+	if !isRedirectSafe {
+		tlog.App.Warn().Str("redirect_uri", redirectURI).Msg("Unsafe redirect URI detected, ignoring")
+		redirectURI = ""
+	}
+
+	if redirectURI != "" && isRedirectSafe {
+		tlog.App.Debug().Msg("Setting redirect URI cookie")
+		c.SetCookie(controller.config.RedirectCookieName, redirectURI, int(time.Hour.Seconds()), "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
+	}
 
 	c.JSON(200, gin.H{
 		"status":  200,
@@ -137,24 +129,20 @@ func (controller *OAuthController) oauthCallbackHandler(c *gin.Context) {
 	}
 
 	c.SetCookie(controller.config.OAuthSessionCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
-
-	oauthPendingSession, err := controller.auth.GetOAuthPendingSession(sessionIdCookie)
-
-	if err != nil {
-		tlog.App.Error().Err(err).Msg("Failed to get OAuth pending session")
-		c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL))
-		return
-	}
-
 	defer controller.auth.EndOAuthSession(sessionIdCookie)
 
 	state := c.Query("state")
-	if state != oauthPendingSession.State {
+	csrfCookie, err := c.Cookie(controller.config.CSRFCookieName)
-		tlog.App.Warn().Err(err).Msg("CSRF token mismatch")
+
+	if err != nil || state != csrfCookie {
+		tlog.App.Warn().Err(err).Msg("CSRF token mismatch or cookie missing")
+		c.SetCookie(controller.config.CSRFCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
 		c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL))
 		return
 	}
 
+	c.SetCookie(controller.config.CSRFCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
+
 	code := c.Query("code")
 	_, err = controller.auth.GetOAuthToken(sessionIdCookie, code)
 
@@ -210,7 +198,7 @@ func (controller *OAuthController) oauthCallbackHandler(c *gin.Context) {
 		username = strings.Replace(user.Email, "@", "_", 1)
 	}
 
-	svc, err := controller.auth.GetOAuthService(sessionIdCookie)
+	service, err := controller.auth.GetOAuthService(sessionIdCookie)
 
 	if err != nil {
 		tlog.App.Error().Err(err).Msg("Failed to get OAuth service for session")
@@ -218,8 +206,8 @@ func (controller *OAuthController) oauthCallbackHandler(c *gin.Context) {
 		return
 	}
 
-	if svc.ID() != req.Provider {
+	if service.ID() != req.Provider {
-		tlog.App.Error().Msgf("OAuth service ID mismatch: expected %s, got %s", svc.ID(), req.Provider)
+		tlog.App.Error().Msgf("OAuth service ID mismatch: expected %s, got %s", service.ID(), req.Provider)
 		c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL))
 		return
 	}
@@ -228,9 +216,9 @@ func (controller *OAuthController) oauthCallbackHandler(c *gin.Context) {
 		Username:    username,
 		Name:        name,
 		Email:       user.Email,
-		Provider:    svc.ID(),
+		Provider:    service.ID(),
 		OAuthGroups: utils.CoalesceToString(user.Groups),
-		OAuthName:   svc.Name(),
+		OAuthName:   service.Name(),
 		OAuthSub:    user.Sub,
 	}
 
@@ -246,39 +234,24 @@ func (controller *OAuthController) oauthCallbackHandler(c *gin.Context) {
 
 	tlog.AuditLoginSuccess(c, sessionCookie.Username, sessionCookie.Provider)
 
-	if controller.isOidcRequest(oauthPendingSession.CallbackParams) {
+	redirectURI, err := c.Cookie(controller.config.RedirectCookieName)
-		tlog.App.Debug().Msg("OIDC request, redirecting to authorize page")
+
-		queries, err := query.Values(oauthPendingSession.CallbackParams)
+	if err != nil || !utils.IsRedirectSafe(redirectURI, controller.config.CookieDomain) {
-		if err != nil {
+		tlog.App.Debug().Msg("No redirect URI cookie found, redirecting to app root")
-			tlog.App.Error().Err(err).Msg("Failed to encode OIDC callback query")
+		c.Redirect(http.StatusTemporaryRedirect, controller.config.AppURL)
-			c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL))
-			return
-		}
-		c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/authorize?%s", controller.config.AppURL, queries.Encode()))
 		return
 	}
 
-	if oauthPendingSession.CallbackParams.RedirectURI != "" {
+	queries, err := query.Values(config.RedirectQuery{
-		queries, err := query.Values(config.RedirectQuery{
+		RedirectURI: redirectURI,
-			RedirectURI: oauthPendingSession.CallbackParams.RedirectURI,
+	})
-		})
 
-		if err != nil {
+	if err != nil {
-			tlog.App.Error().Err(err).Msg("Failed to encode redirect URI query")
+		tlog.App.Error().Err(err).Msg("Failed to encode redirect URI query")
-			c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL))
+		c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL))
-			return
-		}
-
-		c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/continue?%s", controller.config.AppURL, queries.Encode()))
 		return
 	}
 
-	c.Redirect(http.StatusTemporaryRedirect, controller.config.AppURL)
+	c.SetCookie(controller.config.RedirectCookieName, "", -1, "/", fmt.Sprintf(".%s", controller.config.CookieDomain), controller.config.SecureCookie, true)
-}
+	c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/continue?%s", controller.config.AppURL, queries.Encode()))
-
-func (controller *OAuthController) isOidcRequest(params service.OAuthURLParams) bool {
-	return params.Scope != "" &&
-		params.ResponseType != "" &&
-		params.ClientID != "" &&
-		params.RedirectURI != ""
 }

internal/service/auth_service.go

@@ -28,26 +28,12 @@ const MaxOAuthPendingSessions = 256
 const OAuthCleanupCount = 16
 const MaxLoginAttemptRecords = 256
 
-// slightly modified version of the AuthorizeRequest from the OIDC service to basically accept all
-// parameters and pass them to the authorize page if needed
-type OAuthURLParams struct {
-	Scope               string `form:"scope" url:"scope"`
-	ResponseType        string `form:"response_type" url:"response_type"`
-	ClientID            string `form:"client_id" url:"client_id"`
-	RedirectURI         string `form:"redirect_uri" url:"redirect_uri"`
-	State               string `form:"state" url:"state"`
-	Nonce               string `form:"nonce" url:"nonce"`
-	CodeChallenge       string `form:"code_challenge" url:"code_challenge"`
-	CodeChallengeMethod string `form:"code_challenge_method" url:"code_challenge_method"`
-}
-
 type OAuthPendingSession struct {
-	State          string
+	State     string
-	Verifier       string
+	Verifier  string
-	Token          *oauth2.Token
+	Token     *oauth2.Token
-	Service        *OAuthServiceImpl
+	Service   *OAuthServiceImpl
-	ExpiresAt      time.Time
+	ExpiresAt time.Time
-	CallbackParams OAuthURLParams
 }
 
 type LdapGroupsCache struct {
@@ -612,7 +598,7 @@ func (auth *AuthService) IsBypassedIP(acls config.AppIP, ip string) bool {
 	return false
 }
 
-func (auth *AuthService) NewOAuthSession(serviceName string, params OAuthURLParams) (string, OAuthPendingSession, error) {
+func (auth *AuthService) NewOAuthSession(serviceName string) (string, OAuthPendingSession, error) {
 	auth.ensureOAuthSessionLimit()
 
 	service, ok := auth.oauthBroker.GetService(serviceName)
@@ -631,11 +617,10 @@ func (auth *AuthService) NewOAuthSession(serviceName string, params OAuthURLPara
 	verifier := service.NewRandom()
 
 	session := OAuthPendingSession{
-		State:          state,
+		State:     state,
-		Verifier:       verifier,
+		Verifier:  verifier,
-		Service:        &service,
+		Service:   &service,
-		ExpiresAt:      time.Now().Add(1 * time.Hour),
+		ExpiresAt: time.Now().Add(1 * time.Hour),
-		CallbackParams: params,
 	}
 
 	auth.oauthMutex.Lock()
@@ -646,7 +631,7 @@ func (auth *AuthService) NewOAuthSession(serviceName string, params OAuthURLPara
 }
 
 func (auth *AuthService) GetOAuthURL(sessionId string) (string, error) {
-	session, err := auth.GetOAuthPendingSession(sessionId)
+	session, err := auth.getOAuthPendingSession(sessionId)
 
 	if err != nil {
 		return "", err
@@ -656,7 +641,7 @@ func (auth *AuthService) GetOAuthURL(sessionId string) (string, error) {
 }
 
 func (auth *AuthService) GetOAuthToken(sessionId string, code string) (*oauth2.Token, error) {
-	session, err := auth.GetOAuthPendingSession(sessionId)
+	session, err := auth.getOAuthPendingSession(sessionId)
 
 	if err != nil {
 		return nil, err
@@ -676,7 +661,7 @@ func (auth *AuthService) GetOAuthToken(sessionId string, code string) (*oauth2.T
 }
 
 func (auth *AuthService) GetOAuthUserinfo(sessionId string) (config.Claims, error) {
-	session, err := auth.GetOAuthPendingSession(sessionId)
+	session, err := auth.getOAuthPendingSession(sessionId)
 
 	if err != nil {
 		return config.Claims{}, err
@@ -696,7 +681,7 @@ func (auth *AuthService) GetOAuthUserinfo(sessionId string) (config.Claims, erro
 }
 
 func (auth *AuthService) GetOAuthService(sessionId string) (OAuthServiceImpl, error) {
-	session, err := auth.GetOAuthPendingSession(sessionId)
+	session, err := auth.getOAuthPendingSession(sessionId)
 
 	if err != nil {
 		return nil, err
@@ -730,7 +715,7 @@ func (auth *AuthService) CleanupOAuthSessionsRoutine() {
 	}
 }
 
-func (auth *AuthService) GetOAuthPendingSession(sessionId string) (*OAuthPendingSession, error) {
+func (auth *AuthService) getOAuthPendingSession(sessionId string) (*OAuthPendingSession, error) {
 	auth.ensureOAuthSessionLimit()
 
 	auth.oauthMutex.RLock()