c5fa25640623eef3aa9c8f1caaf4be18bab98a77 max Tue Sep 15 05:45:03 2026 -0700 hgLogin: fixes from the code review of the social sign-in work Read the provider label before clearPendingIdentity, not after. cartUsualString hands back the cart's own string and cartRemove frees it, so the confirmation page was naming the provider from memory that had just been released. hgLogin installs pushCarefulMemHandler, which keeps the bytes readable, which is why it looked fine. Let someone who mistyped their address at the choose-a-username page correct it. The confirmation never arrives, and until now there was no way out at all: no password to sign in with, no activated account for the email link, no login cookie for the change-email page, and the user name and provider identity both already taken. The confirmation page now offers a box to replace the address, authorized by the same signed pending identity the account chooser uses, so only a real provider round trip in this browser can get to it. Signing in again while an account waits to be confirmed reuses the token that is still outstanding instead of minting a new one. A new token silently voids the link in the message before it, so a user who clicked the button twice and opened the first mail was told the link was invalid. Promise the account page only where it exists: the change-email flow is behind login.emailLink, which is off by default, and since the address box went away a user there would have had no way to change an address we told them they could change. Clear the confirmation page's explanation variables in the plain signup path too. The hand-off to that page is a JavaScript redirect, so a user who never lands on it leaves them in the cart, and the next plain signup rendered somebody else's explanation over an unrelated address. diff --git src/hg/hgLogin/hgLogin.c src/hg/hgLogin/hgLogin.c index d33f49ce175..a51106c0598 100644 --- src/hg/hgLogin/hgLogin.c +++ src/hg/hgLogin/hgLogin.c @@ -71,30 +71,31 @@ #define delayFraction 1.0 /* standard penalty is 1.0 for most CGIs */ /* Forward declarations for functions used before their definitions. */ static void printSocialButtons(boolean dividerAbove, boolean dividerBelow, char *action); static void printEmailLinkButton(); static boolean emailLinkEnabled(); static boolean recovEmailChangeEnabled(); void changeRecovEmailPage(struct sqlConnection *conn); static void printUsernameNote(); void emailLinkPage(struct sqlConnection *conn); void displayLoginPage(struct sqlConnection *conn); void displayAccHelpPage(struct sqlConnection *conn); void completeAccountPage(struct sqlConnection *conn); void sendEmailLink(struct sqlConnection *conn); static void loginAndReturn(struct sqlConnection *conn, char *userName, uint idx); +static boolean pendingIdentityValid(); /* ---- Global helper functions ---- */ char *browserName() /* Return the browser name like 'UCSC Genome Browser' */ { if isEmpty(cfgOption(CFG_LOGIN_BROWSER_NAME)) return cloneString("NULL_browserName"); else return cloneString(cfgOption(CFG_LOGIN_BROWSER_NAME)); } char *browserAddr() /* Return the browser address like 'http://genome.ucsc.edu' */ { if isEmpty(cfgOption(CFG_LOGIN_BROWSER_ADDR)) @@ -417,30 +418,46 @@ { char *encProvider = htmlEncode(provider); char *encAddress = htmlEncode(address); if (isEmpty(existingUser)) hPrintf("<p>You signed in with %s, and %s did not tell us an email address, so we " "asked you for one. No %s account uses <b>%s</b> yet, so we are making a new " "account for it. Confirming the address is the last step.</p>", encProvider, encProvider, brwName, encAddress); else { char *encUser = htmlEncode(existingUser); hPrintf("<p>Your %s sign-in belongs to the %s account <b>%s</b>, but the address on " "that account, <b>%s</b>, has never been confirmed. Confirm it once and %s will " "sign you straight in from then on.</p>", encProvider, brwName, encUser, encAddress, encProvider); + /* If that address is wrong the confirmation can never arrive, and this account has no + * other way in, so offer to replace it here. pendingIdentityValid() is the check that + * this really is the person who just came back from the provider. */ + if (pendingIdentityValid()) + { + hPrintf("<p>If <b>%s</b> is not an address you can read, enter the right one and we " + "will send the confirmation there instead.</p>", encAddress); + hPrintf("<form method=\"post\" action=\"%s\" name=\"fixEmailForm\">", hgLoginUrl); + hPrintf("<div class=\"inputGroup\">" + "<label for=\"fixEmailAddr\">Email address</label>" + "<input type=\"text\" name=\"hgLogin_email\" value=\"\" size=\"30\" " + "id=\"fixEmailAddr\"></div>"); + hPrintf("<div class=\"formControls\">" + "<input type=\"submit\" name=\"hgLogin.do.changePendingEmail\" " + "value=\"Use this address instead\" class=\"largeButton\"></div></form>"); + } freeMem(encUser); } freeMem(encProvider); freeMem(encAddress); } hPrintf( "<p id=\"confirmationMsg\" class=\"confirmationTxt\">A confirmation email has been sent to you. \n" "Please click the confirmation link in the email to activate your account.</p>" "<p>You may have to look in your spam folder for an email from genome-www@soe.ucsc.edu, " "especially if you use Microsoft Outlook or Hotmail.</p>" "\n" "<p><a href=\"%s\">Return</a></p>", returnURL); cartRemove(cart, "hgLogin_email"); cartRemove(cart, "hgLogin_userName"); cartRemove(cart, "hgLogin_actMailProvider"); @@ -805,30 +822,50 @@ * (the email-link login token and the OAuth state nonce, both makeRandomKey) rather than from * generateRandomPassword, whose output is far shorter and far less varied. The token is opaque * -- it is hashed on the next line and only the hash is ever stored or mailed -- so nothing * downstream depends on its shape. */ char *token = makeRandomKey(128+33); char *tokenMD5 = generateTokenMD5(token); sqlSafef(query,sizeof(query), "UPDATE gbMembers SET lastUse=NOW(),emailToken='%s', emailTokenExpires=DATE_ADD(NOW(), INTERVAL 7 DAY), accountActivated='N' WHERE userName='%s'", tokenMD5, username ); sqlUpdate(conn, query); sendActivateMail(email, username, tokenMD5); return; } +void resendActivateMail(struct sqlConnection *conn, char *email, char *username) +/* Mail the activation link for an account that already has one outstanding, reusing the token + * rather than minting a new one. Every fresh token silently kills the link in the mail before + * it, so a user who clicks the provider button twice and then opens the first message is told + * their link is invalid. Falls back to a new token once the old one has expired. */ +{ +char query[256]; +sqlSafef(query, sizeof(query), + "SELECT emailToken FROM gbMembers WHERE userName='%s' AND emailToken<>'' " + "AND emailTokenExpires > NOW()", username); +char *token = sqlQuickString(conn, query); +if (isEmpty(token)) + { + setupNewAccount(conn, email, username); + return; + } +sendActivateMail(email, username, token); +freeMem(token); +} + void printPwdEyeIcon(char *iconId, char *slashId) /* print a clickable eye icon as a normal sibling right after a password * input (not overlapping it); slashId is the <line> toggled to show * "hidden". No-op if disabled via hg.conf login.pwdEyeIcon. */ { if (!pwdEyeIconEnabled) return; hPrintf( "<span id=\"%s\" title=\"Show/hide password\" " "style=\"display:inline-block; margin-left:6px; vertical-align:middle; " "cursor:pointer; user-select:none;\">" "<svg width=\"18\" height=\"18\" viewBox=\"0 0 24 24\" fill=\"none\" " "stroke=\"#666\" stroke-width=\"2\">" "<path d=\"M1 12s4-7 11-7 11 7 11 7-4 7-11 7-11-7-11-7z\"/>" "<circle cx=\"12\" cy=\"12\" r=\"3\"/>" @@ -1903,30 +1940,37 @@ if (sameWord(returnAddr, "NOEMAIL")) { redirectToLoginPage("hgLogin.do.displayLoginPage=1"); return; } setupNewAccount(conn, email, user); if (confirmRecov) sendRecovEmailConfirmMail(recovEmail, user, recovEmail, "N"); /* send out activate code mail, and display the mail confirmation box */ cartRemove(cart, "hgLogin_email"); cartRemove(cart, "hgLogin_email2"); cartRemove(cart, "hgLogin_userName"); cartRemove(cart, "user"); cartRemove(cart, "token"); +/* This page is shared with the social-login flows, which leave it a note saying which provider + * and address to explain. Those are cleared when that page renders, but the hand-off is a + * JavaScript redirect and a user who never lands on it keeps them in the cart. Drop them here + * so a plain signup can never inherit somebody else's explanation. */ +cartRemove(cart, "hgLogin_actMailProvider"); +cartRemove(cart, "hgLogin_actMailTo"); +cartRemove(cart, "hgLogin_actMailUser"); redirectToLoginPage("hgLogin.do.displayActMailSuccess=1"); } void accountHelp(struct sqlConnection *conn) /* email user username(s) or new password */ { char query[1024]; // room for an address-matching clause holding a long address twice char *email = cartUsualString(cart, "hgLogin_email", ""); char *username = cartUsualString(cart, "hgLogin_userName", ""); char *helpWith = cartUsualString(cart, "hgLogin_helpWith", ""); /* Passwordless email login link */ if (sameString(helpWith,"loginLink")) { sendEmailLink(conn); @@ -2451,32 +2495,35 @@ 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>", 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); + /* Only promise the change-email page where it exists: it, and the confirmation link that + * finishes the change, are all behind login.emailLink, which is off by default. */ + hPrintf("<p>Your email address, as %s gave it to us, is <b>%s</b>.%s</p>", + label, encProviderEmail, + emailLinkEnabled() ? " You can change it later on the account page." : ""); 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); } 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", ""); @@ -2584,45 +2631,51 @@ 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); +/* clearPendingIdentity frees the cart's copy of oauth_pending_provider, and provider points + * straight at it (cartUsualString hands back the cart's own string, not a duplicate), so take + * the label while it is still there. */ +char *providerLabel = cloneString(oauthProviderLabel(provider)); clearPendingIdentity(); if (activateNow) { + freeMem(providerLabel); loginAndReturn(conn, user, idx); return; } /* Unconfirmed address: send the confirmation mail and say so, rather than signing the user in * and leaving a mail nobody has any reason to open. Activating is what makes the address usable * for signing in by email link and for linking a later social login, so it is worth a click. */ setupNewAccount(conn, email, user); /* Tell the confirmation page what to explain. No user name here: this is a brand new account, * which is the one thing that page cannot work out for itself. */ -cartSetString(cart, "hgLogin_actMailProvider", oauthProviderLabel(provider)); +cartSetString(cart, "hgLogin_actMailProvider", providerLabel); cartSetString(cart, "hgLogin_actMailTo", email); cartRemove(cart, "hgLogin_actMailUser"); +freeMem(providerLabel); cartRemove(cart, "hgLogin_email"); cartRemove(cart, "hgLogin_userName"); redirectToLoginPage("hgLogin.do.displayActMailSuccess=1"); } void chooseAccountPage(struct sqlConnection *conn) /* Ask the user which of several accounts sharing an email address to sign in to. Used by * two flows: OAuth (oauth_pending_* in the cart -> the chosen account is linked to the social * identity) and the passwordless email link (emailLogin_* in the cart -> just sign in). */ { char *provider = cartUsualString(cart, "oauth_pending_provider", ""); boolean emailMode = isEmpty(provider); if (emailMode && !emailLinkEnabled()) { // The email-link chooser must not run where passwordless login is switched off. @@ -2852,59 +2905,138 @@ * trust. */ char query[512]; sqlSafef(query, sizeof(query), "UPDATE gbMembers SET accountActivated='Y', dateActivated=NOW(), " "emailToken='', emailTokenExpires='' WHERE idx=%u", linked->idx); sqlUpdate(conn, query); } else if (!sameWord(returnAddr, "NOEMAIL") && isNotEmpty(linked->email)) { /* The address on the account is still nobody's word but the user's, so signing in * would make the confirmation mail pointless: they would click the provider button * again and never confirm. Send them to their inbox, with a fresh link each time, * because the first expires after seven days and for an account with no password * this is the only way to activate it. An install that cannot send mail no longer * creates such an account, but guard rather than leave the user nothing to click. */ - setupNewAccount(conn, linked->email, linked->userName); + resendActivateMail(conn, linked->email, linked->userName); cartSetString(cart, "hgLogin_actMailProvider", oauthProviderLabel(id->provider)); cartSetString(cart, "hgLogin_actMailTo", linked->email); cartSetString(cart, "hgLogin_actMailUser", linked->userName); + /* Keep the identity signed in the cart so the page can offer to correct the + * address. Without that there is no way back at all for someone who mistyped it + * when the account was made: they cannot sign in (this branch), cannot use the + * email link or the change-email page (both want an activated account or a login + * cookie), and cannot start again, because the user name and this provider identity + * are both taken. The signature is what lets changePendingEmail trust the request + * that comes back: only a real provider round trip in this browser can mint it. */ + setPendingIdentity(id); gbMembersFree(&linked); gbMembersFreeList(&matches); displayActMailSuccess(); return; } } loginAndReturn(conn, linked->userName, linked->idx); gbMembersFree(&linked); gbMembersFreeList(&matches); return; } if (n == 1) { linkIdentity(conn, matches->idx, id); loginAndReturn(conn, matches->userName, matches->idx); gbMembersFreeList(&matches); return; } gbMembersFreeList(&matches); setPendingIdentity(id); completeAccountPage(conn); } +void changePendingEmail(struct sqlConnection *conn) +/* Put a different address on the unactivated account behind a still-valid pending identity, and + * send the confirmation there. Reached only from the confirmation page that resolveIdentity + * shows such an account (see displayActMailSuccess), and the only way out for someone who + * mistyped their address when the account was made: with no password and an address they cannot + * read, every other route back in wants an activated account or a login cookie. + * The pending signature is the authorization. Only resolveIdentity mints one, only after a real + * provider round trip, and only for this browser, so a request arriving here without one is + * refused rather than trusted. */ +{ +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 sign-in expired. Please sign in again."); + displayLoginPage(conn); + return; + } +struct oauthIdentity id; +ZeroVar(&id); +id.provider = provider; +id.subject = subject; +struct gbMembers *m = memberForIdentity(conn, &id); +/* Only an account that is still waiting to be confirmed. Once it is activated this page is + * not reachable any more, and changing the address of a working account belongs in the + * change-email flow, which confirms the new address before it takes effect. */ +if ((m == NULL) || sameString(m->accountActivated, "Y")) + { + clearPendingIdentity(); + gbMembersFree(&m); + displayLoginPage(conn); + return; + } +char *email = cartUsualString(cart, "hgLogin_email", ""); +boolean bad = isEmpty(email) || (spc_email_isvalid(email) == 0); +if (!bad) + { + /* Same rule as a new signup: an address the user typed must not be pointed at an account + * somebody else already confirmed it for. */ + char query[1024]; + char *addrMatch = sqlAddressMatch(email); + sqlSafef(query, sizeof(query), + "SELECT count(*) FROM gbMembers WHERE %-s AND accountActivated='Y'", addrMatch); + freeMem(addrMatch); + bad = (sqlQuickNum(conn, query) > 0); + } +if (!bad) + { + char query[512]; + sqlSafef(query, sizeof(query), + "UPDATE gbMembers SET email='%s', emailToken='', emailTokenExpires='' WHERE idx=%u", + email, m->idx); + sqlUpdate(conn, query); + setupNewAccount(conn, email, m->userName); // new address, so a new token is right + cartSetString(cart, "hgLogin_actMailTo", email); + } +else + { + freez(&errMsg); + errMsg = cloneString("Please enter an email address that is not already in use."); + cartSetString(cart, "hgLogin_actMailTo", m->email); + } +cartSetString(cart, "hgLogin_actMailProvider", oauthProviderLabel(provider)); +cartSetString(cart, "hgLogin_actMailUser", m->userName); +cartRemove(cart, "hgLogin_email"); +gbMembersFree(&m); +displayActMailSuccess(); +} + void oauthStart(struct sqlConnection *conn) /* Begin a social login: save an anti-CSRF state nonce (in the cart) and redirect the * browser to the provider's authorization page. */ { char *provider = cgiUsualString("provider", ""); if (!oauthProviderEnabled(provider)) { freez(&errMsg); errMsg = cloneString("That login method is not available on this server."); displayLoginPage(conn); return; } char *state = makeRandomKey(128+33); cartSetString(cart, "oauth_state", state); cartSetString(cart, "oauth_provider", provider); @@ -3196,30 +3328,32 @@ pwdEyeIconEnabled = cfgOptionBooleanDefault(CFG_LOGIN_PWD_EYE_ICON, TRUE); // A provider's OAuth redirect back to us carries 'code' (success) or 'error' (failure) but // none of our own hgLogin.do.* variables, so detect it up front. We gate on an OAuth flow // being in progress (oauth_provider set by oauthStart) so a stray code/error param can't // trigger this. Error returns may omit 'code' and even 'state', so we must not require them. if ((cgiOptionalString("code") != NULL || cgiOptionalString("error") != NULL) && isNotEmpty(cartUsualString(cart, "oauth_provider", ""))) oauthReturn(conn); else if (cartVarExists(cart, "hgLogin.do.oauthStart")) oauthStart(conn); else if (cartVarExists(cart, "hgLogin.do.completeAccount")) completeAccount(conn); else if (cartVarExists(cart, "hgLogin.do.chooseAccount")) chooseAccount(conn); +else if (cartVarExists(cart, "hgLogin.do.changePendingEmail")) + changePendingEmail(conn); else if (cartVarExists(cart, "hgLogin.do.emailLinkPage")) emailLinkPage(conn); else if (cartVarExists(cart, "hgLogin.do.sendEmailLink")) sendEmailLink(conn); else if (cartVarExists(cart, "hgLogin.do.emailLogin")) emailLogin(conn); else if (cartVarExists(cart, "hgLogin.do.changePasswordPage")) changePasswordPage(conn); else if (cartVarExists(cart, "hgLogin.do.changePassword")) changePassword(conn); else if (cartVarExists(cart, "hgLogin.do.changeEmailPage")) changeEmailPage(conn); else if (cartVarExists(cart, "hgLogin.do.changeEmail")) changeEmail(conn); else if (cartVarExists(cart, "hgLogin.do.confirmChangeEmail"))