Page MenuHomePhabricator

OAuth identfy endpoint should not expose unconfirmed email address
Closed, ResolvedPublic

Description

The identify endpoint for OAuth (typically used for logins with Wikipedia as an external identity provider) has separate email and confirmed_email fields, matching User::getEmail() and User::isEmailConfirmed(), so it mirrors the unfortunate design choice of the User class to return the unconfirmed email address, with the developer needing to check a separate field to tell that it is not actually the user's address. I can't think of a scenario where the unconfirmed email would be useful for the OAuth client, OTOH relying on the email for login and not checking whether it's confirmed is an easy mistake to make. We should not set the email unless it's confirmed.

(Wikis with email confirmation disabled should of course be excepted.)

Event Timeline

Change 693616 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/OAuth@master] Hide unconfirmed email address in identify/profile endpoint

https://gerrit.wikimedia.org/r/693616

If oauth apps are identifying people by email adress, seems like there is an impersonation risk here if they forget to verify the confirmed flag.

I'm trying to find some good specifications on what we can expect from profile information and it looks like openid in StandardClaims specifies both email and email_verified (https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims)
I tried to find such specifications for OAuth1.0/OAuth2.0 too - but I didn't have much luck - I found references to OpenId Connect in "OAuth 2.0 Rich Authorization Requests" which specifies email and email_verified too https://www.rfc-editor.org/rfc/rfc9396#fig26 (because it follows the OpenId Standard Claim rules).

I don't have too good knowledge about the OAuth specification, therefore let me ask. Why are we trying to hide the email if it's not verified, instead of following the Openid-Connect principles?

OIDC is the right specification, the identify endpoint is our somewhat homegrown OIDC variant; the OAuth spec itself doesn't really concern itself with providing user data. (Historically, we built the identify endpoint when OAuth 2 and OIDC weren't a thing yet but the usage pattern more or less matching what later became OIDC already existed at some large site. Then when we added OAuth 2 support it was a relatively low-effort thing and it mostly just reused the existing logic.)

Just because OIDC allows exposing unverified emails doesn't mean we should, though. It's hard to see what they could be useful for, and if some client forgets to check the verified flag and naively accepts an unverified email address, the consequences range from spam to account hijacking.

Is it okay to return null for email? OIDC (a remarkably poorly written spec in general) doesn't really make it clear - it uses the concept of "standard claims" but doesn't actually state whether those claims are required to be present, nor how to indicate the lack of a value. User::getEmail() returns an empty string when there is no email address at all, so this will introduce a new type. But we aren't anywhere near small-print-compliant with OIDC (e.g. we have no email scope; see also the list of differences in T254063#8081745) so I don't think it's worth caring about much.

Change 693616 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Hide unconfirmed email address in identify/profile endpoint

https://gerrit.wikimedia.org/r/693616

@Tgr Most likely we can resolve this ticket as the gerrit patch is merged. What do you think about the proposed follow-up to add the email_verified field?.

The null value in the email field caused errors for the wikipedia library, which we were able to resolve pretty quickly: T349279

Change 967313 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/OAuth@master] OIDC: Return '' instead of null for email in profile

https://gerrit.wikimedia.org/r/967313

Change 967314 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/OAuth@master] OIDC: Add email_verified field to user profile

https://gerrit.wikimedia.org/r/967314

The null value in the email field caused errors for the wikipedia library, which we were able to resolve pretty quickly: T349279

Hm, sorry, in hindsight introducing a new type for that value was a bad idea.

Change 967314 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] OIDC: Add email_verified field to user profile

https://gerrit.wikimedia.org/r/967314

Change 967313 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] OIDC: Return '' instead of null for email in profile

https://gerrit.wikimedia.org/r/967313

Change 969151 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/OAuth@wmf/1.42.0-wmf.2] OIDC: Return '' instead of null for email in profile

https://gerrit.wikimedia.org/r/969151

Tgr claimed this task.

Change 969151 merged by jenkins-bot:

[mediawiki/extensions/OAuth@wmf/1.42.0-wmf.2] OIDC: Return '' instead of null for email in profile

https://gerrit.wikimedia.org/r/969151

Mentioned in SAL (#wikimedia-operations) [2023-10-26T20:34:58Z] <brennen@deploy2002> Started scap: Backport for [[gerrit:969151|OIDC: Return '' instead of null for email in profile (T283456)]]

Mentioned in SAL (#wikimedia-operations) [2023-10-26T20:36:12Z] <brennen@deploy2002> brennen and tgr: Backport for [[gerrit:969151|OIDC: Return '' instead of null for email in profile (T283456)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2023-10-26T20:42:24Z] <brennen@deploy2002> Finished scap: Backport for [[gerrit:969151|OIDC: Return '' instead of null for email in profile (T283456)]] (duration: 07m 25s)