diff --git a/mobile/lib/services/oauth.service.dart b/mobile/lib/services/oauth.service.dart index d8b1604e00..401cb5a14e 100644 --- a/mobile/lib/services/oauth.service.dart +++ b/mobile/lib/services/oauth.service.dart @@ -11,7 +11,7 @@ class OAuthService { final log = Logger('OAuthService'); OAuthService(this._apiService); - Future getOAuthServerUrl(String serverUrl, String state, String codeChallenge) async { + Future getOAuthServerUrl(String serverUrl, String state, String codeChallenge, String nonce) async { // Resolve API server endpoint from user provided serverUrl await _apiService.resolveAndSetEndpoint(serverUrl); final redirectUri = '$callbackUrlScheme:///oauth-callback'; @@ -22,6 +22,7 @@ class OAuthService { redirectUri: redirectUri, state: Optional.present(state), codeChallenge: Optional.present(codeChallenge), + nonce: Optional.present(nonce), ), ); @@ -31,7 +32,7 @@ class OAuthService { return authUrl; } - Future oAuthLogin(String oauthUrl, String state, String codeVerifier) async { + Future oAuthLogin(String oauthUrl, String state, String codeVerifier, String nonce) async { String result = await FlutterWebAuth2.authenticate(url: oauthUrl, callbackUrlScheme: callbackUrlScheme); log.info('Received OAuth callback: $result'); @@ -41,7 +42,12 @@ class OAuthService { } return await _apiService.oAuthApi.finishOAuth( - OAuthCallbackDto(url: result, state: Optional.present(state), codeVerifier: Optional.present(codeVerifier)), + OAuthCallbackDto( + url: result, + state: Optional.present(state), + codeVerifier: Optional.present(codeVerifier), + nonce: Optional.present(nonce), + ), ); } } diff --git a/mobile/lib/widgets/forms/login/login_form.dart b/mobile/lib/widgets/forms/login/login_form.dart index 5185c5c318..cda23bb49c 100644 --- a/mobile/lib/widgets/forms/login/login_form.dart +++ b/mobile/lib/widgets/forms/login/login_form.dart @@ -326,6 +326,7 @@ class LoginForm extends HookConsumerWidget { String? oAuthServerUrl; final state = generateRandomString(32); + final nonce = generateRandomString(32); final codeVerifier = randomCodeVerifier(); final codeChallenge = await generatePKCECodeChallenge(codeVerifier); @@ -335,6 +336,7 @@ class LoginForm extends HookConsumerWidget { normalizeServerUrl(serverEndpointController.text), state, codeChallenge, + nonce, ); // Invalidate all api repository provider instance to take into account new access token @@ -357,7 +359,7 @@ class LoginForm extends HookConsumerWidget { if (oAuthServerUrl != null) { try { - final loginResponseDto = await oAuthService.oAuthLogin(oAuthServerUrl, state, codeVerifier); + final loginResponseDto = await oAuthService.oAuthLogin(oAuthServerUrl, state, codeVerifier, nonce); if (loginResponseDto == null || !context.mounted) { return; diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index d00faab9a0..dbbcf52681 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -20657,6 +20657,10 @@ "description": "OAuth code verifier (PKCE)", "type": "string" }, + "nonce": { + "description": "OAuth nonce parameter", + "type": "string" + }, "state": { "description": "OAuth state parameter", "type": "string" @@ -20678,6 +20682,10 @@ "description": "OAuth code challenge (PKCE)", "type": "string" }, + "nonce": { + "description": "OAuth nonce parameter", + "type": "string" + }, "redirectUri": { "description": "OAuth redirect URI", "type": "string" diff --git a/packages/sdk/src/fetch-client.ts b/packages/sdk/src/fetch-client.ts index b2f98b58ef..4df846c320 100644 --- a/packages/sdk/src/fetch-client.ts +++ b/packages/sdk/src/fetch-client.ts @@ -1405,6 +1405,8 @@ export type NotificationUpdateDto = { export type OAuthConfigDto = { /** OAuth code challenge (PKCE) */ codeChallenge?: string; + /** OAuth nonce parameter */ + nonce?: string; /** OAuth redirect URI */ redirectUri: string; /** OAuth state parameter */ @@ -1421,6 +1423,8 @@ export type OAuthBackchannelLogoutDto = { export type OAuthCallbackDto = { /** OAuth code verifier (PKCE) */ codeVerifier?: string; + /** OAuth nonce parameter */ + nonce?: string; /** OAuth state parameter */ state?: string; /** OAuth callback URL */ diff --git a/server/src/controllers/oauth.controller.ts b/server/src/controllers/oauth.controller.ts index 54f5c1f10b..57414015e1 100644 --- a/server/src/controllers/oauth.controller.ts +++ b/server/src/controllers/oauth.controller.ts @@ -49,7 +49,7 @@ export class OAuthController { @Res({ passthrough: true }) res: Response, @GetLoginDetails() loginDetails: LoginDetails, ): Promise { - const { url, state, codeVerifier } = await this.service.authorize(dto); + const { url, state, nonce, codeVerifier } = await this.service.authorize(dto); return respondWithCookie( res, { url }, @@ -57,6 +57,7 @@ export class OAuthController { isSecure: loginDetails.isSecure, values: [ { key: ImmichCookie.OAuthState, value: state }, + { key: ImmichCookie.OAuthNonce, value: nonce }, { key: ImmichCookie.OAuthCodeVerifier, value: codeVerifier }, ], }, @@ -78,6 +79,7 @@ export class OAuthController { ): Promise { const body = await this.service.callback(dto, request.headers, loginDetails); res.clearCookie(ImmichCookie.OAuthState); + res.clearCookie(ImmichCookie.OAuthNonce); res.clearCookie(ImmichCookie.OAuthCodeVerifier); return respondWithCookie(res, body, { isSecure: loginDetails.isSecure, diff --git a/server/src/dtos/auth.dto.ts b/server/src/dtos/auth.dto.ts index 40b4e25b6d..cc1d7d405f 100644 --- a/server/src/dtos/auth.dto.ts +++ b/server/src/dtos/auth.dto.ts @@ -107,6 +107,7 @@ const OAuthCallbackSchema = z url: z.string().min(1).describe('OAuth callback URL'), state: z.string().optional().describe('OAuth state parameter'), codeVerifier: z.string().optional().describe('OAuth code verifier (PKCE)'), + nonce: z.string().optional().describe('OAuth nonce parameter'), }) .meta({ id: 'OAuthCallbackDto' }); @@ -115,6 +116,7 @@ const OAuthConfigSchema = z redirectUri: z.string().describe('OAuth redirect URI'), state: z.string().optional().describe('OAuth state parameter'), codeChallenge: z.string().optional().describe('OAuth code challenge (PKCE)'), + nonce: z.string().optional().describe('OAuth nonce parameter'), }) .meta({ id: 'OAuthConfigDto' }); diff --git a/server/src/enum.ts b/server/src/enum.ts index 8882b554f3..a6123a0120 100644 --- a/server/src/enum.ts +++ b/server/src/enum.ts @@ -14,6 +14,7 @@ export enum ImmichCookie { SharedLinkToken = 'immich_shared_link_token', OAuthState = 'immich_oauth_state', OAuthCodeVerifier = 'immich_oauth_code_verifier', + OAuthNonce = 'immich_oauth_nonce', } export enum ImmichHeader { diff --git a/server/src/repositories/oauth.repository.ts b/server/src/repositories/oauth.repository.ts index 1a09fff70d..e3cd24c877 100644 --- a/server/src/repositories/oauth.repository.ts +++ b/server/src/repositories/oauth.repository.ts @@ -10,6 +10,7 @@ import { discovery, fetchUserInfo, None, + randomNonce, randomPKCECodeVerifier, randomState, skipSubjectCheck, @@ -41,9 +42,10 @@ export class OAuthRepository { this.logger.setContext(OAuthRepository.name); } - async authorize(config: OAuthConfig, redirectUrl: string, state?: string, codeChallenge?: string) { + async authorize(config: OAuthConfig, redirectUrl: string, state?: string, codeChallenge?: string, nonce?: string) { const client = await this.getClient(config); state ??= randomState(); + nonce ??= randomNonce(); let codeVerifier: string | null; if (codeChallenge) { @@ -57,6 +59,7 @@ export class OAuthRepository { redirect_uri: redirectUrl, scope: config.scope, state, + nonce, }; if (config.prompt) { @@ -70,7 +73,7 @@ export class OAuthRepository { const url = buildAuthorizationUrl(client, params).href; - return { url, state, codeVerifier }; + return { url, state, nonce, codeVerifier }; } async getLogoutEndpoint(config: OAuthConfig) { @@ -83,12 +86,17 @@ export class OAuthRepository { url: string, expectedState: string, codeVerifier: string, + expectedNonce?: string, ): Promise<{ profile: OAuthProfile; sid?: string; idToken?: string }> { const client = await this.getClient(config); const pkceCodeVerifier = client.serverMetadata().supportsPKCE() ? codeVerifier : undefined; try { - const tokens = await authorizationCodeGrant(client, new URL(url), { expectedState, pkceCodeVerifier }); + const tokens = await authorizationCodeGrant(client, new URL(url), { + expectedState, + pkceCodeVerifier, + expectedNonce, + }); let profile: OAuthProfile; const tokenClaims = tokens.claims(); diff --git a/server/src/services/auth.service.spec.ts b/server/src/services/auth.service.spec.ts index 48dcf5a509..69c1ce8476 100644 --- a/server/src/services/auth.service.spec.ts +++ b/server/src/services/auth.service.spec.ts @@ -38,7 +38,12 @@ describe(AuthService.name, () => { beforeEach(() => { ({ sut, mocks } = newTestService(AuthService)); - mocks.oauth.authorize.mockResolvedValue({ url: 'http://test', state: 'state', codeVerifier: 'codeVerifier' }); + mocks.oauth.authorize.mockResolvedValue({ + url: 'http://test', + state: 'state', + nonce: 'nonce', + codeVerifier: 'codeVerifier', + }); mocks.oauth.getLogoutEndpoint.mockResolvedValue('http://end-session-endpoint'); }); @@ -853,6 +858,7 @@ describe(AuthService.name, () => { 'http://mobile-redirect?code=abc123', 'xyz789', 'foo', + undefined, ); }); } diff --git a/server/src/services/auth.service.ts b/server/src/services/auth.service.ts index 5603819212..51e0246cf8 100644 --- a/server/src/services/auth.service.ts +++ b/server/src/services/auth.service.ts @@ -279,6 +279,7 @@ export class AuthService extends BaseService { this.resolveRedirectUri(oauth, dto.redirectUri), dto.state, dto.codeChallenge, + dto.nonce, ); } @@ -298,12 +299,14 @@ export class AuthService extends BaseService { throw new BadRequestException('OAuth code verifier is missing'); } + const expectedNonce = dto.nonce ?? this.getCookieOauthNonce(headers) ?? undefined; + const url = this.resolveRedirectUri(oauth, dto.url); const { profile, sid: oauthSid, idToken: oauthBearerToken, - } = await this.oauthRepository.getProfileAndOAuthSid(oauth, url, expectedState, codeVerifier); + } = await this.oauthRepository.getProfileAndOAuthSid(oauth, url, expectedState, codeVerifier, expectedNonce); const normalizedEmail = profile.email ? profile.email.trim().toLowerCase() : undefined; const { autoRegister, defaultStorageQuota, storageLabelClaim, storageQuotaClaim, roleClaim } = oauth; this.logger.debug(`Logging in with OAuth: ${JSON.stringify(profile)}`); @@ -409,12 +412,14 @@ export class AuthService extends BaseService { throw new BadRequestException('OAuth code verifier is missing'); } + const expectedNonce = dto.nonce ?? this.getCookieOauthNonce(headers) ?? undefined; + const { oauth } = await this.getConfig({ withCache: false }); const { profile: { sub: oauthId }, sid, idToken, - } = await this.oauthRepository.getProfileAndOAuthSid(oauth, dto.url, expectedState, codeVerifier); + } = await this.oauthRepository.getProfileAndOAuthSid(oauth, dto.url, expectedState, codeVerifier, expectedNonce); const duplicate = await this.userRepository.getByOAuthId(oauthId); if (duplicate && duplicate.id !== auth.user.id) { this.logger.warn(`OAuth link account failed: sub is already linked to another user (${duplicate.email}).`); @@ -491,6 +496,11 @@ export class AuthService extends BaseService { return cookies[ImmichCookie.OAuthCodeVerifier] || null; } + private getCookieOauthNonce(headers: IncomingHttpHeaders): string | null { + const cookies = parse(headers.cookie || ''); + return cookies[ImmichCookie.OAuthNonce] || null; + } + async validateSharedLinkKey(key: string | string[]): Promise { key = Array.isArray(key) ? key[0] : key; diff --git a/server/src/utils/response.ts b/server/src/utils/response.ts index d5356285f0..ec744e3cbe 100644 --- a/server/src/utils/response.ts +++ b/server/src/utils/response.ts @@ -17,6 +17,7 @@ export const respondWithCookie = (res: Response, body: T, { isSecure, values [ImmichCookie.AccessToken]: defaults, [ImmichCookie.MaintenanceToken]: { ...defaults, maxAge: Duration.fromObject({ days: 1 }).toMillis() }, [ImmichCookie.OAuthState]: defaults, + [ImmichCookie.OAuthNonce]: defaults, [ImmichCookie.OAuthCodeVerifier]: defaults, // no httpOnly so that the client can know the auth state [ImmichCookie.IsAuthenticated]: { ...defaults, httpOnly: false },