mirror of
https://github.com/immich-app/immich
synced 2026-08-15 13:03:57 +00:00
fix(oauth): send and verify a nonce to tolerate IdP-injected nonces
Immich's OAuth flow never sent a `nonce`, so oauth4webapi's default `expectNoNonce` rejected any id_token carrying one. Providers that inject a nonce on federated logins (e.g. AWS Cognito relaying Google) therefore broke with OAUTH_JWT_CLAIM_COMPARISON_FAILED: unexpected ID Token "nonce" claim. Generate a real nonce in authorize(), round-trip it (web via httpOnly cookie, mobile via the callback DTO since the client generates its own state/PKCE), and pass it as expectedNonce to authorizationCodeGrant so the value is verified per the OIDC spec. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
447cc40a50
commit
5ba3446fdf
11 changed files with 61 additions and 11 deletions
|
|
@ -11,7 +11,7 @@ class OAuthService {
|
|||
final log = Logger('OAuthService');
|
||||
OAuthService(this._apiService);
|
||||
|
||||
Future<String?> getOAuthServerUrl(String serverUrl, String state, String codeChallenge) async {
|
||||
Future<String?> 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<LoginResponseDto?> oAuthLogin(String oauthUrl, String state, String codeVerifier) async {
|
||||
Future<LoginResponseDto?> 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),
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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 */
|
||||
|
|
|
|||
|
|
@ -49,7 +49,7 @@ export class OAuthController {
|
|||
@Res({ passthrough: true }) res: Response,
|
||||
@GetLoginDetails() loginDetails: LoginDetails,
|
||||
): Promise<OAuthAuthorizeResponseDto> {
|
||||
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<LoginResponseDto> {
|
||||
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,
|
||||
|
|
|
|||
|
|
@ -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' });
|
||||
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
);
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<AuthDto> {
|
||||
key = Array.isArray(key) ? key[0] : key;
|
||||
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ export const respondWithCookie = <T>(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 },
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue