From 01f29465a7876b011e84b9df5978e3b5c7f319da Mon Sep 17 00:00:00 2001 From: Olivier Lambert Date: Mon, 9 Nov 2026 11:15:24 +0100 Subject: [PATCH] fix(xo-server-auth-oidc): check the userinfo sub, only read the mapped profile fields - the userinfo response is rejected when its sub does not match the one of the ID token (OpenID Connect Core 5.3.2) - the username is only read from the fields of the profile, as before this branch --- packages/xo-server-auth-oidc/index.js | 24 ++++++------- packages/xo-server-auth-oidc/index.test.js | 39 ++++++++++++++++------ 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/packages/xo-server-auth-oidc/index.js b/packages/xo-server-auth-oidc/index.js index 9ff4799670..da92a46c59 100644 --- a/packages/xo-server-auth-oidc/index.js +++ b/packages/xo-server-auth-oidc/index.js @@ -93,9 +93,8 @@ function profileField(profile, usernameField) { * @returns {boolean} */ function hasUsername(usernameField, claims) { - return ( - claims[CLAIM_OF_PROFILE_FIELD[usernameField] ?? usernameField] !== undefined || claims[usernameField] !== undefined - ) + const claim = CLAIM_OF_PROFILE_FIELD[usernameField] + return claim !== undefined && claims[claim] !== undefined } exports.hasUsername = hasUsername @@ -103,17 +102,11 @@ exports.hasUsername = hasUsername * The username, from the userinfo response when it was fetched, then from the ID token. * * @param {string} usernameField - * @param {{ claims: object, idProfile?: object, uiProfile?: object }} sources + * @param {{ idProfile?: object, uiProfile?: object }} sources * @returns {string | undefined} */ -function resolveUsername(usernameField, { claims, idProfile, uiProfile }) { - return ( - profileField(uiProfile, usernameField) ?? - profileField(idProfile, usernameField) ?? - // a field the profile does not map - uiProfile?._json?.[usernameField] ?? - claims[usernameField] - ) +function resolveUsername(usernameField, { idProfile, uiProfile }) { + return profileField(uiProfile, usernameField) ?? profileField(idProfile, usernameField) } exports.resolveUsername = resolveUsername @@ -194,6 +187,13 @@ class AuthOidc { async (issuer, uiProfile, idProfile, context, idToken, accessToken, refreshToken, params, done) => { try { const claims = JSON.parse(Buffer.from(idToken.split('.')[1], 'base64').toString()) + + // the userinfo response must describe the user of the ID token, else it must not be used + // (https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse) + if (uiProfile !== undefined && uiProfile.id !== idProfile.id) { + throw new Error('the sub of the userinfo response does not match the one of the ID token') + } + const sources = { claims, idProfile, uiProfile } const name = resolveUsername(usernameField, sources) diff --git a/packages/xo-server-auth-oidc/index.test.js b/packages/xo-server-auth-oidc/index.test.js index dcb46ccef4..fe4eb9cb9d 100644 --- a/packages/xo-server-auth-oidc/index.test.js +++ b/packages/xo-server-auth-oidc/index.test.js @@ -123,8 +123,8 @@ describe('the username', () => { assert.equal(hasUsername('username', { sub: 's', preferred_username: 'alice' }), true) assert.equal(hasUsername('email', { sub: 's', email: 'alice@example.org' }), true) assert.equal(hasUsername('displayName', { sub: 's', name: 'Alice' }), true) - // a claim the profile does not map - assert.equal(hasUsername('upn', { sub: 's', upn: 'alice@corp' }), true) + // only the claim the profile field comes from + assert.equal(hasUsername('username', { sub: 's', username: 'alice' }), false) }) it('is missing from a thin ID token, which only has the minimal claims', () => { @@ -141,14 +141,15 @@ describe('the username', () => { assert.equal(resolveUsername('username', { claims, idProfile }), 'from-token') }) - it('reads emails and unmapped claims', () => { - const uiProfile = { id: 's', emails: [{ value: 'alice@example.org' }], _json: { upn: 'alice@corp' } } - assert.equal( - resolveUsername('email', { claims: { sub: 's' }, idProfile: { id: 's' }, uiProfile }), - 'alice@example.org' - ) - assert.equal(resolveUsername('upn', { claims: { sub: 's' }, idProfile: { id: 's' }, uiProfile }), 'alice@corp') - assert.equal(resolveUsername('upn', { claims: { sub: 's', upn: 'bob@corp' }, idProfile: { id: 's' } }), 'bob@corp') + it('reads emails', () => { + const uiProfile = { id: 's', emails: [{ value: 'alice@example.org' }], _json: {} } + assert.equal(resolveUsername('email', { idProfile: { id: 's' }, uiProfile }), 'alice@example.org') + }) + + it('is only read from the fields of the profile, not from raw claims', () => { + const uiProfile = { id: 's', _json: { username: 'alice', upn: 'alice@corp' } } + assert.equal(resolveUsername('username', { idProfile: { id: 's' }, uiProfile }), undefined) + assert.equal(resolveUsername('upn', { idProfile: { id: 's' }, uiProfile }), undefined) }) it('is undefined when neither source has it', () => { @@ -334,4 +335,22 @@ describe('a sign in', () => { } ) }) + + it('rejects a userinfo response about another user', async () => { + await withProvider( + { idTokenClaims: { sub: 'subject' }, userInfo: { sub: 'other', preferred_username: 'alice' } }, + async ({ issuer }) => { + await assert.rejects(signIn(issuer), error => String(error).includes('does not match')) + } + ) + }) + + it('rejects a userinfo response without sub', async () => { + await withProvider( + { idTokenClaims: { sub: 'subject' }, userInfo: { preferred_username: 'alice' } }, + async ({ issuer }) => { + await assert.rejects(signIn(issuer), error => String(error).includes('does not match')) + } + ) + }) })