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

@@ -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)

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'))
+      }
+    )
+  })
 })