From 8a7577497131229badb35cb4b3a4227b1300afff Mon Sep 17 00:00:00 2001 From: Elias Schneider Date: Tue, 16 Jun 2026 11:52:52 +0200 Subject: [PATCH] fix: callback URL validation not validated if prompt=none --- .../internal/controller/oidc_controller.go | 32 +++++++++++++ backend/internal/dto/oidc_dto.go | 10 ++++ backend/internal/service/oidc_service.go | 38 ++++++++++++++- frontend/src/lib/services/oidc-service.ts | 15 ++++++ frontend/src/lib/types/oidc.type.ts | 4 ++ frontend/src/routes/authorize/+page.svelte | 47 ++++++++++++------- tests/specs/oidc.spec.ts | 42 +++++++++++++++++ 7 files changed, 170 insertions(+), 18 deletions(-) diff --git a/backend/internal/controller/oidc_controller.go b/backend/internal/controller/oidc_controller.go index db8b7502..919361a6 100644 --- a/backend/internal/controller/oidc_controller.go +++ b/backend/internal/controller/oidc_controller.go @@ -32,6 +32,7 @@ func NewOidcController(group *gin.RouterGroup, authMiddleware *middleware.AuthMi } group.POST("/oidc/authorize", authMiddleware.WithAdminNotRequired().Add(), oc.authorizeHandler) + group.POST("/oidc/authorize/callback-url", oc.authorizeCallbackURLHandler) group.POST("/oidc/authorization-required", authMiddleware.WithAdminNotRequired().Add(), oc.authorizationConfirmationRequiredHandler) group.GET("/oidc/par-request-info", authMiddleware.WithAdminNotRequired().Add(), oc.parRequestInfoHandler) @@ -111,6 +112,7 @@ func (oc *OidcController) authorizeHandler(c *gin.Context) { c.JSON(http.StatusOK, gin.H{ "error": err.Error(), "requiresRedirect": true, + "callbackURL": callbackURL, }) return } @@ -127,6 +129,36 @@ func (oc *OidcController) authorizeHandler(c *gin.Context) { c.JSON(http.StatusOK, response) } +// authorizeCallbackURLHandler godoc +// @Summary Resolve a validated callback URL for an OIDC authorization request +// @Description Resolves the redirect URI against the client's configured callback URLs without consuming PAR records. +// @Tags OIDC +// @Accept json +// @Produce json +// @Param request body dto.AuthorizeOidcClientCallbackRequestDto true "Authorization callback parameters" +// @Success 200 {object} dto.AuthorizeOidcClientCallbackResponseDto "Resolved callback URL" +// @Router /api/oidc/authorize/callback-url [post] +func (oc *OidcController) authorizeCallbackURLHandler(c *gin.Context) { + var input dto.AuthorizeOidcClientCallbackRequestDto + if err := c.ShouldBindJSON(&input); err != nil { + _ = c.Error(err) + return + } + + callbackURL, err := oc.oidcService.ResolveAuthorizeCallbackURL( + c.Request.Context(), + input.ClientID, + input.CallbackURL, + input.RequestURI, + ) + if err != nil { + _ = c.Error(err) + return + } + + c.JSON(http.StatusOK, dto.AuthorizeOidcClientCallbackResponseDto{CallbackURL: callbackURL}) +} + // isOidcPromptError checks if an error is a prompt-related OIDC error that should trigger a redirect func isOidcPromptError(err error) bool { var loginReq *common.OidcLoginRequiredError diff --git a/backend/internal/dto/oidc_dto.go b/backend/internal/dto/oidc_dto.go index 03ebf201..dfebd7a2 100644 --- a/backend/internal/dto/oidc_dto.go +++ b/backend/internal/dto/oidc_dto.go @@ -105,6 +105,16 @@ type AuthorizeOidcClientResponseDto struct { Issuer string `json:"issuer"` } +type AuthorizeOidcClientCallbackRequestDto struct { + ClientID string `json:"clientID" binding:"required"` + CallbackURL string `json:"callbackURL"` + RequestURI string `json:"requestURI"` +} + +type AuthorizeOidcClientCallbackResponseDto struct { + CallbackURL string `json:"callbackURL"` +} + type AuthorizationRequiredDto struct { ClientID string `json:"clientID" binding:"required"` Scope string `json:"scope"` diff --git a/backend/internal/service/oidc_service.go b/backend/internal/service/oidc_service.go index 87b87b12..ef6f0054 100644 --- a/backend/internal/service/oidc_service.go +++ b/backend/internal/service/oidc_service.go @@ -216,7 +216,7 @@ func (s *OidcService) Authorize(ctx context.Context, input dto.AuthorizeOidcClie return "", "", err } if !hasAlreadyAuthorized { - return "", "", &common.OidcConsentRequiredError{} + return "", callbackURL, &common.OidcConsentRequiredError{} } } @@ -820,7 +820,41 @@ func (s *OidcService) ResolveAllowedCallbackURL(ctx context.Context, clientID, i return "", err } - if inputCallbackURL == "" || len(client.CallbackURLs) == 0 { + return resolveConfiguredCallbackURL(&client, inputCallbackURL) +} + +// ResolveAuthorizeCallbackURL resolves the callback URL for a browser authorization +// request without authorizing the user or consuming PAR state. +func (s *OidcService) ResolveAuthorizeCallbackURL(ctx context.Context, clientID, inputCallbackURL, requestURI string) (string, error) { + client, err := s.GetClient(ctx, clientID) + if err != nil { + return "", err + } + + if client.RequiresPushedAuthorizationRequests && requestURI == "" { + return "", &common.OidcPARRequiredError{} + } + + if requestURI != "" { + par, err := s.GetPushedAuthorizationRequest(ctx, clientID, requestURI) + if err != nil { + return "", err + } + inputCallbackURL = par.Parameters.RedirectURI + } + + return resolveConfiguredCallbackURL(&client, inputCallbackURL) +} + +func resolveConfiguredCallbackURL(client *model.OidcClient, inputCallbackURL string) (string, error) { + if inputCallbackURL == "" { + if len(client.CallbackURLs) > 0 { + return client.CallbackURLs[0], nil + } + return "", &common.OidcMissingCallbackURLError{} + } + + if len(client.CallbackURLs) == 0 { return "", &common.OidcMissingCallbackURLError{} } diff --git a/frontend/src/lib/services/oidc-service.ts b/frontend/src/lib/services/oidc-service.ts index fb4594ab..78257ed0 100644 --- a/frontend/src/lib/services/oidc-service.ts +++ b/frontend/src/lib/services/oidc-service.ts @@ -1,6 +1,7 @@ import type { ListRequestOptions, Paginated } from '$lib/types/list-request.type'; import type { AccessibleOidcClient, + AuthorizeCallbackResponse, AuthorizeResponse, OidcClient, OidcClientCreate, @@ -44,6 +45,20 @@ class OidcService extends APIService { return res.data as AuthorizeResponse; }; + resolveAuthorizeCallbackURL = async ( + clientId: string, + callbackURL: string, + requestURI?: string + ) => { + const res = await this.api.post('/oidc/authorize/callback-url', { + clientId, + callbackURL, + requestURI + }); + + return res.data as AuthorizeCallbackResponse; + }; + isAuthorizationRequired = async (clientId: string, scope: string, requestURI?: string) => { const res = await this.api.post('/oidc/authorization-required', { scope, diff --git a/frontend/src/lib/types/oidc.type.ts b/frontend/src/lib/types/oidc.type.ts index 9fd78d4f..8c110225 100644 --- a/frontend/src/lib/types/oidc.type.ts +++ b/frontend/src/lib/types/oidc.type.ts @@ -70,6 +70,10 @@ export type AuthorizeResponse = { requiresRedirect?: boolean; }; +export type AuthorizeCallbackResponse = { + callbackURL: string; +}; + export type AccessibleOidcClient = OidcClientMetaData & { lastUsedAt: Date | null; }; diff --git a/frontend/src/routes/authorize/+page.svelte b/frontend/src/routes/authorize/+page.svelte index 15dc6f54..9b065fcd 100644 --- a/frontend/src/routes/authorize/+page.svelte +++ b/frontend/src/routes/authorize/+page.svelte @@ -12,7 +12,7 @@ import appConfigStore from '$lib/stores/application-configuration-store'; import userStore from '$lib/stores/user-store'; import { cachedProfilePicture } from '$lib/utils/cached-image-util'; - import { getWebauthnErrorMessage } from '$lib/utils/error-util'; + import { getAxiosErrorMessage, getWebauthnErrorMessage } from '$lib/utils/error-util'; import { startAuthentication, type AuthenticationResponseJSON } from '@simplewebauthn/browser'; import { onMount } from 'svelte'; import { slide } from 'svelte/transition'; @@ -64,15 +64,19 @@ const hasPromptSelectAccount = promptValues.includes('select_account'); onMount(() => { + void handleInitialAuthorization(); + }); + + async function handleInitialAuthorization() { // Conflicting prompt values - none can't be combined with any interactive prompt if (hasPromptNone && (hasPromptConsent || hasPromptLogin || hasPromptSelectAccount)) { - redirectWithError('interaction_required'); + await redirectWithError('interaction_required'); return; } // If prompt=none and user is not signed in, redirect immediately with login_required if (hasPromptNone && !$userStore) { - redirectWithError('login_required'); + await redirectWithError('login_required'); return; } @@ -85,9 +89,9 @@ } if ($userStore) { - authorize(); + await authorize(); } - }); + } async function useDifferentAccount() { try { @@ -130,7 +134,7 @@ // If prompt=none and consent required, redirect with error if (hasPromptNone && authorizationRequired) { - redirectWithError('consent_required'); + await redirectWithError('consent_required'); return; } @@ -169,7 +173,7 @@ // Check if backend returned a redirect error if (result.requiresRedirect && result.error) { if (hasPromptNone) { - redirectWithError(result.error); + await redirectWithError(result.error, result.callbackURL); } else { errorMessage = result.error; isLoading = false; @@ -184,16 +188,27 @@ } } - function redirectWithError(error: string) { - const redirectURL = new URL(callbackURL); - if (redirectURL.protocol == 'javascript:' || redirectURL.protocol == 'data:') { - throw new Error('Invalid redirect URL protocol'); - } + async function redirectWithError(error: string, validatedCallbackURL?: string) { + isLoading = true; - window.location.href = createRedirectURL(callbackURL, { - error, - state: authorizeState - }); + try { + const safeCallbackURL = + validatedCallbackURL || + (await oidService.resolveAuthorizeCallbackURL(client!.id, callbackURL, requestURI)) + .callbackURL; + const redirectURL = new URL(safeCallbackURL); + if (redirectURL.protocol == 'javascript:' || redirectURL.protocol == 'data:') { + throw new Error('Invalid redirect URL protocol'); + } + + window.location.href = createRedirectURL(safeCallbackURL, { + error, + state: authorizeState + }); + } catch (e) { + errorMessage = getAxiosErrorMessage(e); + isLoading = false; + } } function onSuccess(code: string, callbackURL: string, issuer: string) { diff --git a/tests/specs/oidc.spec.ts b/tests/specs/oidc.spec.ts index 894845e4..aaf06f92 100644 --- a/tests/specs/oidc.spec.ts +++ b/tests/specs/oidc.spec.ts @@ -673,6 +673,28 @@ test.describe('OIDC prompt parameter', () => { expect(redirectUrl.searchParams.get('state')).toBe('nXx-6Qr-owc1SHBa'); }); + test('prompt=none does not redirect login_required to an unregistered redirect_uri', async ({ + page + }) => { + await page.context().clearCookies(); + const oidcClient = oidcClients.nextcloud; + const urlParams = createUrlParams(oidcClient); + urlParams.set('prompt', 'none'); + urlParams.set('redirect_uri', 'https://attacker.example/collect'); + + let attackerRedirected = false; + await page.route('https://attacker.example/**', async (route) => { + attackerRedirected = true; + await route.fulfill({ status: 200, body: 'attacker' }); + }); + + await page.goto(`/authorize?${urlParams.toString()}`); + + await expect(page.getByText(/Invalid callback URL/i)).toBeVisible(); + expect(attackerRedirected).toBe(false); + expect(page.url()).not.toContain('attacker.example'); + }); + test('prompt=none redirects errors with response_mode=fragment', async ({ page }) => { await page.context().clearCookies(); const oidcClient = oidcClients.nextcloud; @@ -706,6 +728,26 @@ test.describe('OIDC prompt parameter', () => { expect(redirectUrl.searchParams.get('state')).toBe('nXx-6Qr-owc1SHBa'); }); + test('prompt=none does not redirect consent_required to an unregistered redirect_uri', async ({ + page + }) => { + const oidcClient = oidcClients.immich; + const urlParams = createUrlParams(oidcClient); + urlParams.set('prompt', 'none'); + urlParams.set('redirect_uri', 'https://attacker.example/collect'); + + let attackerRedirected = false; + await page.route('https://attacker.example/**', async (route) => { + attackerRedirected = true; + await route.fulfill({ status: 200, body: 'attacker' }); + }); + + await page.goto(`/authorize?${urlParams.toString()}`); + + await expect(page.getByText(/Invalid callback URL/i)).toBeVisible(); + expect(attackerRedirected).toBe(false); + }); + test('prompt=none succeeds when user is authenticated and authorized', async ({ page }) => { const oidcClient = oidcClients.nextcloud; const urlParams = createUrlParams(oidcClient);