0ebe7cd89f87af0c5e8310eb7bed6f65d2596604 max Mon Sep 14 14:37:19 2026 -0700 hgLogin: do not ask for an email address when the provider already gave us one The "choose a username" page showed an editable email box for every social signup, including the providers that do send us an address. For CILogon that meant being asked for something we then took at face value: the box arrived prefilled with CILogon's address, whatever came back was accepted, and the account was created and signed in. A box whose contents we never check is worse than no box, because it reads as a question we check the answer to. The page now asks only when the provider released no usable address. Where the provider gave one, the page says which address the account will use and offers no input at all. completeAccount reads that address from the pending identity rather than from hgLogin_email, so a leftover value in the cart cannot stand in for it. Where the provider released nothing, ORCID being the case that matters, nothing changes: the user is asked for an address, and the account stays inactive until the mailed link is opened. refs #38339, refs #38341 diff --git src/hg/hgLogin/hgLogin.c src/hg/hgLogin/hgLogin.c index 3cd93d33a15..541e90f019f 100644 --- src/hg/hgLogin/hgLogin.c +++ src/hg/hgLogin/hgLogin.c @@ -2330,103 +2330,121 @@ "created=NOW(), lastUse=NOW() " "ON DUPLICATE KEY UPDATE idx=%u, email='%s', lastUse=NOW()", idx, id->provider, id->subject, email, idx, email); sqlUpdate(conn, query); } /* What we trust about an email address in a social login is that the *provider* released it, * not that the provider set email_verified. CILogon leaves that flag at 0 even for a real * institutional sign-in (#37984 note-50), and its addresses come from the university's own * identity provider rather than from anything the person can type, so insisting on the flag * would make every CILogon user confirm an address by mail and would still never let them reach * an account they already have. What we do not trust is an address the *user* typed: either * because the provider released none (ORCID releases only an ORCID iD, by design) or because * they edited the one that was released. Those have to be confirmed by mail. * To tighten this later, add the email_verified test back in the two places that call - * oauthAddressFromProvider() and in resolveIdentity's matching query; the flag is still carried + * oauthProviderEmail() and in resolveIdentity's matching query; the flag is still carried * through the cart in oauth_pending_email_verified, it is just not consulted. */ -static boolean oauthAddressFromProvider(char *email) -/* TRUE when email is exactly the address the identity provider released, i.e. it came from the - * provider and not from the user typing into the form. */ +static char *oauthProviderEmail() +/* The address the provider released for the pending identity, or NULL if it released none or + * released something that is not a usable address. + * When this is non-NULL the "choose a username" page must not ask for an address at all. We + * already have one, from a source the person cannot type into, so a text box would only invite + * an edit -- and a box we then accept unchanged, without ever writing to it, is the worst of + * both worlds: it looks like a question we check the answer to, and it is not. Either we have + * an address and use it, or we do not have one and must confirm what the user types. */ { -char *released = cartUsualString(cart, "oauth_pending_email", ""); -return isNotEmpty(released) && isNotEmpty(email) && sameString(email, released); +char *email = cartUsualString(cart, "oauth_pending_email", ""); +if (isEmpty(email) || spc_email_isvalid(email) == 0) + return NULL; +return email; } void completeAccountPage(struct sqlConnection *conn) /* Ask a first-time social-login user to confirm a username (and email) for a new account. */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char *email = cartUsualString(cart, "oauth_pending_email", ""); char *name = cartUsualString(cart, "oauth_pending_name", ""); if (isEmpty(provider) || !pendingIdentityValid()) { clearPendingIdentity(); displayLoginPage(conn); return; } +char *providerEmail = oauthProviderEmail(); char *suggested = cartUsualString(cart, "hgLogin_userName", ""); if (isEmpty(suggested)) suggested = suggestUsername(conn, email, name); -/* Show back what the user typed, so an error does not wipe the address they have to correct; - * the provider's address is only the starting suggestion. */ -char *typedEmail = cartUsualString(cart, "hgLogin_email", ""); char *encSuggested = htmlEncode(suggested); // both go into value="" attributes; escape (XSS) -char *encEmail = htmlEncode(isNotEmpty(typedEmail) ? typedEmail : email); char *label = oauthProviderLabel(provider); hPrintf("<div id=\"completeAccountBox\" class=\"centeredContainer formBox\">" "<h2>%s</h2>", brwName); hPrintf("<h3>Choose a username</h3>"); hPrintf("<p>You signed in with %s. Pick a username for your new %s account. " "You can change the suggested name below.</p>", label, brwName); -/* Explain why this is always a new account when the provider released no address at all (ORCID - * does this by design: its OpenID Connect offers only the "openid" scope, so the ORCID iD is all - * we ever get). Without an address we cannot tell a returning user from a new one, so every - * first sign-in lands here, which surprised real users (#38341). Keyed on whether an address - * arrived, not on the provider's name: a mirror can call a provider anything it likes in - * hg.conf, so a name test would silently miss it (#38213). */ -if (isEmpty(email)) +/* Explain why this is always a new account when the provider released no address (ORCID does + * this by design: its OpenID Connect offers only the "openid" scope, so the ORCID iD is all we + * ever get). Without an address we cannot tell a returning user from a new one, so every first + * sign-in lands here, which surprised real users (#38341). Keyed on whether an address arrived, + * not on the provider's name: a mirror can call a provider anything it likes in hg.conf, so a + * name test would silently miss it (#38213). */ +if (providerEmail == NULL) hPrintf("<p>A new %s account is created for any %s sign-in we have not seen before, because " "%s does not share your email address with us. So you cannot sign in to an existing " "account this way. Use another sign-in option if you do not want to create a new " "account.</p>", brwName, label, label); printUsernameNote(); hPrintf("<span style='color:red;'>%s</span>", errMsg ? errMsg : ""); hPrintf("<form method=\"post\" action=\"%s\" name=\"completeAccountForm\">", hgLoginUrl); hPrintf("<div class=\"inputGroup\">" "<label for=\"userName\">Username</label>" "<input type=\"text\" name=\"hgLogin_userName\" value=\"%s\" size=\"30\" id=\"userName\">" "</div>", encSuggested); +if (providerEmail == NULL) + { + /* No address from the provider, so we have to ask -- and because anyone can type anything + * here, the account is not usable until the mailed link is opened. Say that next to the box + * rather than springing the confirmation page on the user after they submit. Show back what + * they typed so an error does not wipe the address they are being asked to correct. */ + char *encTyped = htmlEncode(cartUsualString(cart, "hgLogin_email", "")); hPrintf("<div class=\"inputGroup\">" "<label for=\"emailAddr\">Email address</label>" "<input type=\"text\" name=\"hgLogin_email\" value=\"%s\" size=\"30\" id=\"emailAddr\">" - "</div>", encEmail); -/* Say up front that the address has to be confirmed, so the confirmation page is not a surprise - * and people are less likely to type an address they cannot read. Same condition completeAccount - * uses to decide whether to send the mail. */ -if (!oauthAddressFromProvider(email) && !sameWord(returnAddr, "NOEMAIL")) + "</div>", encTyped); + freeMem(encTyped); + if (!sameWord(returnAddr, "NOEMAIL")) hPrintf("<p style=\"font-size:0.9em\">We will email a confirmation link to this address. " "Open the link to finish creating your account.</p>"); + } +else + { + /* We already have an address from the provider, so do not ask for one. A box here would be + * a question we do not check the answer to. */ + char *encProviderEmail = htmlEncode(providerEmail); + hPrintf("<p>Your email address, as %s gave it to us, is <b>%s</b>. You can change it later " + "on the account page.</p>", label, encProviderEmail); + freeMem(encProviderEmail); + } hPrintf("<div class=\"formControls\">" "<input type=\"submit\" name=\"hgLogin.do.completeAccount\" value=\"Create account\" class=\"largeButton\">" " <a href=\"%s\" class=\"cancelButton\">Cancel</a>" "</div></form></div><!-- END - completeAccountBox -->", getReturnToUrlForAttr()); cartSaveSession(cart); freeMem(encSuggested); -freeMem(encEmail); } void completeAccount(struct sqlConnection *conn) /* Create the account for a first-time social-login user, link the identity, and log in. */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); char *subject = cartUsualString(cart, "oauth_pending_subject", ""); if (isEmpty(provider) || isEmpty(subject) || !pendingIdentityValid()) { clearPendingIdentity(); freez(&errMsg); errMsg = cloneString("Your login session expired. Please sign in again."); displayLoginPage(conn); return; } @@ -2448,90 +2466,92 @@ } if (strlen(encUserName) > 32) { freez(&errMsg); errMsg = cloneString("Please use a shorter user name: less than 32 characters after URL encoding."); completeAccountPage(conn); return; } if (userNameTaken(conn, user)) { freez(&errMsg); errMsg = cloneString("A user with this name already exists. Please choose another."); completeAccountPage(conn); return; } -char *email = cartUsualString(cart, "hgLogin_email", ""); +/* Where the provider gave us an address, that is the account's address, full stop. The form did + * not offer a box for it, so anything sitting in hgLogin_email is left over from an earlier page + * in this cart and must not be allowed to stand in for it. */ +char *providerEmail = oauthProviderEmail(); +char *email = (providerEmail != NULL) ? providerEmail : cartUsualString(cart, "hgLogin_email", ""); if (isEmpty(email)) { freez(&errMsg); errMsg = cloneString("Please enter an email address."); completeAccountPage(conn); return; } if (spc_email_isvalid(email) == 0) { freez(&errMsg); errMsg = cloneString("Invalid email address format."); completeAccountPage(conn); return; } -boolean fromProvider = oauthAddressFromProvider(email); /* An address the user typed here must not be used to reach an account that already holds it. * Typing an address someone else registered used to create a silent second account sharing it * (#38341), which then makes both owners pick from a chooser on every later sign-in. Send the * user to a sign-in method that can actually show the address is theirs instead. An address the * provider released is not affected: resolveIdentity has already matched it against existing * accounts and would not have sent us here. * Only activated accounts count, the same rule resolveIdentity and chooseAccount apply: an * unactivated row holds an address nobody ever proved they own, so letting one block a signup * would let anyone reserve a stranger's address. */ -if (!fromProvider) +if (providerEmail == NULL) { char query[1024]; char *addrMatch = sqlAddressMatch(email); sqlSafef(query, sizeof(query), "SELECT count(*) FROM gbMembers WHERE %-s AND accountActivated='Y'", addrMatch); freeMem(addrMatch); if (sqlQuickNum(conn, query) > 0) { char buf[1024]; safef(buf, sizeof(buf), "An account with this email address already exists. %s did not give us that address, " "so we cannot tell that it is yours and cannot sign you in to that account. To reach " "it, sign in with a provider that does give us your email address, or with your " "username and password. To create a new account instead, enter a different email " "address.", oauthProviderLabel(provider)); freez(&errMsg); errMsg = cloneString(buf); completeAccountPage(conn); return; } } char *name = cartUsualString(cart, "oauth_pending_name", ""); char *realName = isNotEmpty(name) ? name : user; -/* The new account is created "activated" -- its email trusted for future auto-linking (see - * resolveIdentity) -- only when the provider actually verified this address and the user kept - * it unchanged. Otherwise it is created inactive and the usual confirmation mail goes out, so - * an unverified address can never be planted as a trusted one. An install that sends no mail - * has no way to confirm anything, so there it is activated on the spot, the same compromise - * signup() makes. */ +/* The new account is created "activated" -- its address trusted for future auto-linking (see + * resolveIdentity) -- when the address came from the provider. An address the user typed gets + * an inactive account and the usual confirmation mail, so a typed address can never be planted + * as a trusted one. An install that sends no mail has no way to confirm anything, so there it + * is activated on the spot, the same compromise signup() makes. */ boolean canMail = !sameWord(returnAddr, "NOEMAIL"); -boolean activateNow = fromProvider || !canMail; +boolean activateNow = (providerEmail != NULL) || !canMail; struct dyString *q = sqlDyStringCreate( "INSERT INTO gbMembers SET userName='%s', realName='%s', password='', email='%s', " "lastUse=NOW(), dateActivated=NOW(), accountActivated='%s'", user, realName, emptyForNull(email), activateNow ? "Y" : "N"); sqlUpdate(conn, dyStringContents(q)); dyStringFree(&q); uint idx = sqlLastAutoId(conn); struct oauthIdentity pending; ZeroVar(&pending); pending.provider = provider; pending.subject = subject; pending.email = email; linkIdentity(conn, idx, &pending); @@ -2725,31 +2745,31 @@ * 1. If the provider released an email matching MORE THAN ONE account, always let the * user pick which one -- even if this identity was linked before. Because login cookies * never expire, a user goes through OAuth very rarely, so an occasional pick is cheap * and it lets a person with several same-email accounts choose freely each time. * 2. Else if the (provider,subject) is already linked, log into that account -- unless that * account is still waiting for its address to be confirmed, in which case send the user * back to their inbox rather than let them skip the confirmation forever. * 3. Else if the email matches exactly one account, auto-link and log in. * 4. Else send the user to the "choose a username" page to finish a new account. * (Providers that don't release an email, e.g. ORCID, never reach step 1 or 3 and rely on * the stored link from step 2.) */ { struct gbMembers *matches = NULL; int n = 0; /* Any address the provider released counts here, whether or not it set email_verified -- see - * the note above oauthAddressFromProvider(). Requiring the flag would send every CILogon user + * the note above oauthProviderEmail(). Requiring the flag would send every CILogon user * to the "choose a username" page even when they already have an account with that address, * which is how the duplicate accounts in #38341 got made. */ if (isNotEmpty(id->email)) { char query[1024]; /* Match the provider email against the primary address and any confirmed recovery address, * the same as password and email-link login do (see sqlAddressMatch). The isNotEmpty() * guard above keeps an empty id->email out of the query, so a blank recovEmail='' row can * never match. * Match only activated accounts. gbMembers has no unique key on email, and the plain * signup form will create an unactivated row for any address a person types -- the * activation mail goes to the address's real owner, who ignores it. Without this filter * someone could pre-register a victim's address, and the victim's first social login would * then auto-link to (and sign in as) the attacker's account. */ char *addrMatch = sqlAddressMatch(id->email);