fix(auth): don't link OIDC login to an arbitrary local user when the email claim is empty

The email-only fallback built &user.User{Issuer: IssuerLocal, Email: cl.Email}.
GetUserWithEmail ANDs only non-zero fields, so an empty cl.Email degenerated the
lookup to issuer-only, matching an arbitrary local user and wrongly linking the
OIDC login to that account (account-takeover / wrong-link).

Guard the email-only branch to only run when cl.Email != "" so an empty email
falls through to user creation (nothing to safely match on). As defense in depth,
skip empty username candidates too, so no fallback search can ever degenerate to
issuer-only ({Issuer, Username:"", Email:""}).

This is a latent pre-existing issue (the pre-#2945 code had the same
searchUser.Email = cl.Email with no empty guard); the #2945 refactor only
surfaced it. Adds a regression test.
This commit is contained in:
kolaente 2026-06-19 20:35:22 +02:00
parent 783c0dd52a
commit f04b5a43f9
2 changed files with 40 additions and 2 deletions

View File

@ -385,7 +385,12 @@ func fallbackSearchUsers(cl *claims, provider *Provider, idToken *oidc.IDToken)
// UUID, like PocketID) still link to an existing local account.
var searches []*user.User
if provider.UsernameFallback {
searches = append(searches, &user.User{Issuer: user.IssuerLocal, Username: idToken.Subject, Email: fallbackEmail})
// Skip empty username candidates: GetUserWithEmail ANDs only non-zero fields, so a
// {Issuer, Username:"", Email:""} would degenerate to an issuer-only lookup and link
// an arbitrary local user. idToken.Subject is non-empty per OIDC, but guard anyway.
if idToken.Subject != "" {
searches = append(searches, &user.User{Issuer: user.IssuerLocal, Username: idToken.Subject, Email: fallbackEmail})
}
preferred := strings.ReplaceAll(cl.PreferredUsername, " ", "-")
if preferred != "" && preferred != idToken.Subject {
searches = append(searches, &user.User{Issuer: user.IssuerLocal, Username: preferred, Email: fallbackEmail})
@ -393,7 +398,9 @@ func fallbackSearchUsers(cl *claims, provider *Provider, idToken *oidc.IDToken)
}
// EmailFallback without UsernameFallback: a single email-only lookup (the caller only
// runs this when at least one fallback is enabled, so EmailFallback is guaranteed here).
if len(searches) == 0 {
// Only add it when there is a real email — an empty email would degenerate to an
// issuer-only lookup and link an arbitrary local user.
if len(searches) == 0 && cl.Email != "" {
searches = append(searches, &user.User{Issuer: user.IssuerLocal, Email: cl.Email})
}

View File

@ -328,6 +328,37 @@ func TestGetOrCreateUser(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, usersBefore, usersAfter, "no new user should have been created")
})
t.Run("ProviderFallback: empty email claim does not link to an arbitrary local user", func(t *testing.T) {
db.LoadAndAssertFixtures(t)
s := db.NewSession()
defer s.Close()
usersBefore, err := s.Count(&user.User{})
require.NoError(t, err)
// EmailFallback on, no username fallback, and the IdP sent no email claim. The
// email-only search must not degenerate to an issuer-only lookup matching an
// arbitrary local user. With no email there is nothing safe to match on, so the
// flow falls through to user creation (which then errors because an email is
// required) rather than silently linking an existing local account.
cl := &claims{
Email: "",
PreferredUsername: "brandNewOidcUser",
}
provider := &Provider{
EmailFallback: true,
}
idToken := &oidc.IDToken{Issuer: "https://some.issuer", Subject: "opaque-subject-no-email"}
u, err := getOrCreateUser(s, cl, provider, idToken)
// Must not have linked an existing local user.
require.Error(t, err, "an empty email must not silently link an existing local user")
assert.Nil(t, u, "no existing local user should be returned for an empty email claim")
usersAfter, err := s.Count(&user.User{})
require.NoError(t, err)
assert.Equal(t, usersBefore, usersAfter, "no user should have been linked or created from an empty email claim")
})
t.Run("ProviderFallback: Match to existing local user on username and email", func(t *testing.T) {
db.LoadAndAssertFixtures(t)