fix: don't use TOFU for logout callback URLs (#588)

backend/internal/service/oidc_service.go

@@ -73,7 +73,7 @@ func (s *OidcService) Authorize(ctx context.Context, input dto.AuthorizeOidcClie
 	}
 
 	// Get the callback URL of the client. Return an error if the provided callback URL is not allowed
-	callbackURL, err := s.getCallbackURL(client.CallbackURLs, input.CallbackURL, input.ClientID, tx, ctx)
+	callbackURL, err := s.getCallbackURL(&client, input.CallbackURL, tx, ctx)
 	if err != nil {
 		return "", "", err
 	}
@@ -947,13 +947,12 @@ func (s *OidcService) ValidateEndSession(ctx context.Context, input dto.OidcLogo
 		return "", &common.OidcNoCallbackURLError{}
 	}
 
-	callbackURL, err := s.getCallbackURL(userAuthorizedOIDCClient.Client.LogoutCallbackURLs, input.PostLogoutRedirectUri, userAuthorizedOIDCClient.Client.ID, s.db, ctx)
+	callbackURL, err := s.getLogoutCallbackURL(&userAuthorizedOIDCClient.Client, input.PostLogoutRedirectUri)
 	if err != nil {
 		return "", err
 	}
 
 	return callbackURL, nil
-
 }
 
 func (s *OidcService) createAuthorizationCode(ctx context.Context, clientID string, userID string, scope string, nonce string, codeChallenge string, codeChallengeMethod string, tx *gorm.DB) (string, error) {
@@ -1006,18 +1005,52 @@ func (s *OidcService) validateCodeVerifier(codeVerifier, codeChallenge string, c
 	return encodedVerifierHash == codeChallenge
 }
 
-func (s *OidcService) getCallbackURL(urls []string, inputCallbackURL string, clientID string, tx *gorm.DB, ctx context.Context) (callbackURL string, err error) {
+func (s *OidcService) getCallbackURL(client *model.OidcClient, inputCallbackURL string, tx *gorm.DB, ctx context.Context) (callbackURL string, err error) {
 	// If no input callback URL provided, use the first configured URL
 	if inputCallbackURL == "" {
-		if len(urls) > 0 {
+		if len(client.CallbackURLs) > 0 {
-			return urls[0], nil
+			return client.CallbackURLs[0], nil
 		}
 		// If no URLs are configured and no input URL, this is an error
 		return "", &common.OidcMissingCallbackURLError{}
 	}
 
 	// If URLs are already configured, validate against them
-	if len(urls) > 0 {
+	if len(client.CallbackURLs) > 0 {
+		matched, err := s.getCallbackURLFromList(client.CallbackURLs, inputCallbackURL)
+		if err != nil {
+			return "", err
+		} else if matched == "" {
+			return "", &common.OidcInvalidCallbackURLError{}
+		}
+
+		return matched, nil
+	}
+
+	// If no URLs are configured, trust and store the first URL (TOFU)
+	err = s.addCallbackURLToClient(ctx, client, inputCallbackURL, tx)
+	if err != nil {
+		return "", err
+	}
+	return inputCallbackURL, nil
+}
+
+func (s *OidcService) getLogoutCallbackURL(client *model.OidcClient, inputLogoutCallbackURL string) (callbackURL string, err error) {
+	if inputLogoutCallbackURL == "" {
+		return client.LogoutCallbackURLs[0], nil
+	}
+
+	matched, err := s.getCallbackURLFromList(client.LogoutCallbackURLs, inputLogoutCallbackURL)
+	if err != nil {
+		return "", err
+	} else if matched == "" {
+		return "", &common.OidcInvalidCallbackURLError{}
+	}
+
+	return matched, nil
+}
+
+func (s *OidcService) getCallbackURLFromList(urls []string, inputCallbackURL string) (callbackURL string, err error) {
 	for _, callbackPattern := range urls {
 		regexPattern := "^" + strings.ReplaceAll(regexp.QuoteMeta(callbackPattern), `\*`, ".*") + "$"
 		matched, err := regexp.MatchString(regexPattern, inputCallbackURL)
@@ -1028,28 +1061,15 @@ func (s *OidcService) getCallbackURL(urls []string, inputCallbackURL string, cli
 			return inputCallbackURL, nil
 		}
 	}
-		return "", &common.OidcInvalidCallbackURLError{}
-	}
 
-	// If no URLs are configured, trust and store the first URL (TOFU)
+	return "", nil
-	err = s.addCallbackURLToClient(ctx, clientID, inputCallbackURL, tx)
-	if err != nil {
-		return "", err
-	}
-	return inputCallbackURL, nil
 }
 
-func (s *OidcService) addCallbackURLToClient(ctx context.Context, clientID string, callbackURL string, tx *gorm.DB) error {
+func (s *OidcService) addCallbackURLToClient(ctx context.Context, client *model.OidcClient, callbackURL string, tx *gorm.DB) error {
-	var client model.OidcClient
-	err := tx.WithContext(ctx).First(&client, "id = ?", clientID).Error
-	if err != nil {
-		return err
-	}
-
 	// Add the new callback URL to the existing list
 	client.CallbackURLs = append(client.CallbackURLs, callbackURL)
 
-	err = tx.WithContext(ctx).Save(&client).Error
+	err := tx.WithContext(ctx).Save(client).Error
 	if err != nil {
 		return err
 	}

frontend/messages/en-US.json

@@ -341,6 +341,7 @@
 	"send_email": "Send Email",
 	"show_code": "Show Code",
 	"callback_url_description": "URL(s) provided by your client. Will be automatically added if left blank. Wildcards (*) are supported, but best avoided for better security.",
+	"logout_callback_url_description": "URL(s) provided by your client for logout. Wildcards (*) are supported, but best avoided for better security.",
 	"api_key_expiration": "API Key Expiration",
 	"send_an_email_to_the_user_when_their_api_key_is_about_to_expire": "Send an email to the user when their API key is about to expire.",
 	"authorize_device": "Authorize Device",

frontend/src/routes/settings/admin/oidc-clients/oidc-callback-url-input.svelte

@@ -9,11 +9,13 @@
 
 	let {
 		label,
+		description,
 		callbackURLs = $bindable(),
 		error = $bindable(null),
 		...restProps
 	}: HTMLAttributes<HTMLDivElement> & {
 		label: string;
+		description: string;
 		callbackURLs: string[];
 		error?: string | null;
 		children?: Snippet;
@@ -21,7 +23,7 @@
 </script>
 
 <div {...restProps}>
-	<FormInput {label} description={m.callback_url_description()}>
+	<FormInput {label} {description}>
 		<div class="flex flex-col gap-y-2">
 			{#each callbackURLs as _, i}
 				<div class="flex gap-x-2">

frontend/src/routes/settings/admin/oidc-clients/oidc-client-form.svelte

@@ -84,12 +84,14 @@
 		<div></div>
 		<OidcCallbackUrlInput
 			label={m.callback_urls()}
+			description={m.callback_url_description()}
 			class="w-full"
 			bind:callbackURLs={$inputs.callbackURLs.value}
 			bind:error={$inputs.callbackURLs.error}
 		/>
 		<OidcCallbackUrlInput
 			label={m.logout_callback_urls()}
+			description={m.logout_callback_url_description()}
 			class="w-full"
 			bind:callbackURLs={$inputs.logoutCallbackURLs.value}
 			bind:error={$inputs.logoutCallbackURLs.error}