663d6e61cc7cbd60100bbe9ae89a4c876530348b max Tue Aug 11 05:10:02 2026 -0700 hgLogin: make the change-email confirm link single-use and record lastUse on the email-link chooser sign-in, refs #37929 Two issues Gerardo found in QA: (i) The change-email confirmation link was stateless (signature + expiry only), so opening it again within the hour re-applied the change -- and a stale link could silently override a newer email change. Fold the account's current email into the signed string: confirmChangeEmail recomputes the signature from the address currently on the account, so once the change is applied the address is no longer the one the link was signed against and the link stops validating. No new column or stored token needed. (ii) chooseAccount's email-link branch (the multi-account case) signed the user in without updating gbMembers.lastUse, unlike the single-account emailLogin path. Add the lastUse update for the chosen account. diff --git src/hg/hgLogin/hgLogin.c src/hg/hgLogin/hgLogin.c index 05e12e01b8c..216aeb150d3 100644 --- src/hg/hgLogin/hgLogin.c +++ src/hg/hgLogin/hgLogin.c @@ -1013,51 +1013,57 @@ hPrintf("<h2>%s</h2>", brwName); hPrintf( "<p align=\"left\">" "</p>" "<h3>Password has been changed.</h3>"); cartRemove(cart, "hgLogin_password"); cartRemove(cart, "hgLogin_newPassword1"); cartRemove(cart, "hgLogin_newPassword2"); sqlSafef(query,sizeof(query),"SELECT * FROM gbMembers WHERE userName='%s'", user); struct gbMembers *m = gbMembersLoadByQuery(conn, query); struct dyString *cookieJS = getLoginCookieJS(user, m->idx); jsInline(cookieJS->string); returnToURL(150); } -static char *changeEmailSig(char *user, char *newEmail, char *expStr) +static char *changeEmailSig(char *user, char *curEmail, char *newEmail, char *expStr) /* HMAC-MD5 over a pending email change, keyed by the secret login.cookieSalt. It goes in the * confirmation link so that clicking the link -- and only clicking it -- applies the change, - * proving the new address really reaches the requester. Result is allocd. */ + * proving the new address really reaches the requester. curEmail is the account's address when + * the link was minted; because confirmChangeEmail recomputes the signature from the address + * currently on the account, a link stops validating once it has been used (the address is no + * longer curEmail), so each link works exactly once and a stale link cannot silently undo a + * newer change. Result is allocd. */ { char *salt = cfgOption(CFG_LOGIN_COOKIE_SALT); if (isEmpty(salt)) errAbort("Confirming an email change requires %s in hg.conf, set to a secret random " "string. Without a secret we cannot sign the confirmation link.", CFG_LOGIN_COOKIE_SALT); char buf[1024]; -safef(buf, sizeof(buf), "changeEmail|%s|%s|%s", - emptyForNull(user), emptyForNull(newEmail), emptyForNull(expStr)); +safef(buf, sizeof(buf), "changeEmail|%s|%s|%s|%s", + emptyForNull(user), emptyForNull(curEmail), emptyForNull(newEmail), emptyForNull(expStr)); return hmacMd5(salt, buf); } -static void sendChangeEmailConfirmMail(char *newEmail, char *user) -/* Email a one-time link to newEmail that, when opened, changes user's address to newEmail. */ +static void sendChangeEmailConfirmMail(char *newEmail, char *user, char *curEmail) +/* Email a one-time link to newEmail that, when opened, changes user's address to newEmail. + * curEmail is the account's current address; it is folded into the signature so the link stops + * working once the change has been applied (see changeEmailSig). */ { char expStr[32]; safef(expStr, sizeof(expStr), "%ld", clock1() + 3600); // link good for one hour -char *sig = changeEmailSig(user, newEmail, expStr); +char *sig = changeEmailSig(user, curEmail, newEmail, expStr); char url[1024]; safef(url, sizeof(url), "%s?hgLogin.do.confirmChangeEmail=1&user=%s&newEmail=%s&exp=%s&sig=%s", hgLoginUrl, cgiEncode(user), cgiEncode(newEmail), expStr, sig); char subject[256]; safef(subject, sizeof(subject), "Confirm your new %s email address", brwName); char *remoteAddr = getenv("REMOTE_ADDR"); char message[4096]; safef(message, sizeof(message), "Someone (probably you, from IP address %s) asked to change the email address on the %s " "account \"%s\" to this address.\nTo confirm the change, open this link in your browser:\n\n" "%s\n\nThe link works once and expires in one hour.\n\n%s\n%s", emptyForNull(remoteAddr), brwName, user, url, signature, returnAddr); sendActMailOut(newEmail, subject, message); freeMem(sig); @@ -1178,78 +1184,83 @@ if (isNotEmpty(curPwd)) { char *given = cartUsualString(cart, "hgLogin_curPassword", ""); if (isEmpty(given) || !checkPwd(given, curPwd)) { freez(&errMsg); errMsg = cloneString("Please enter your current password."); changeEmailPage(conn); return; } } /* Do not change the address yet: email a one-time confirmation link to the NEW address and * apply the change only when it is clicked (see confirmChangeEmail). This proves the address * is real and controlled by the requester, so an unconfirmed address cannot silently become * the account's recovery address. */ -sendChangeEmailConfirmMail(email1, user); +sqlSafef(query, sizeof(query), "SELECT email FROM gbMembers WHERE userName='%s'", user); +char *curEmail = sqlQuickString(conn, query); +sendChangeEmailConfirmMail(email1, user, curEmail); cartRemove(cart, "hgLogin_newEmail1"); cartRemove(cart, "hgLogin_newEmail2"); cartRemove(cart, "hgLogin_curPassword"); char *encEmail = htmlEncode(email1); hPrintf("<div class=\"centeredContainer formBox\"><h2>%s</h2>", brwName); hPrintf("<h3>Almost done. Please check your email</h3>"); hPrintf("<p>We sent a confirmation link to <b>%s</b>. Open the link in that message to finish " "changing your email address. The link works once and expires in one hour.</p></div>", encEmail); freeMem(encEmail); returnToURL(3000); } void confirmChangeEmail(struct sqlConnection *conn) /* Apply a confirmed email change. Reached by opening the signed link sent to the new address * (see sendChangeEmailConfirmMail); the signature and its expiry are the authorization, so this * does not require a login cookie -- the link may be opened from the new mailbox in any browser. */ { if (!emailLinkEnabled()) { displayLoginPage(conn); return; } char *user = cgiUsualString("user", ""); char *newEmail = cgiUsualString("newEmail", ""); char *expStr = cgiUsualString("exp", ""); char *sig = cgiUsualString("sig", ""); -char *expected = changeEmailSig(user, newEmail, expStr); +/* Recompute the signature over the address currently on the account. Once the change has been + * applied that address is newEmail, so re-opening the same link no longer matches: the link works + * exactly once, and a stale link cannot silently undo a newer change. */ +char query[512]; +sqlSafef(query, sizeof(query), "SELECT email FROM gbMembers WHERE userName='%s'", user); +char *oldEmail = sqlQuickString(conn, query); +char *expected = changeEmailSig(user, emptyForNull(oldEmail), newEmail, expStr); boolean sigOk = isNotEmpty(sig) && sameString(sig, expected); freeMem(expected); if (!sigOk || isEmpty(user) || spc_email_isvalid(newEmail) == 0) { freez(&errMsg); - errMsg = cloneString("This confirmation link is not valid."); + errMsg = cloneString("This confirmation link is not valid or has already been used."); displayLoginPage(conn); return; } if (clock1() > atol(expStr)) { freez(&errMsg); errMsg = cloneString("This confirmation link has expired. Please request the change again."); displayLoginPage(conn); return; } -char query[512]; -sqlSafef(query, sizeof(query), "SELECT email FROM gbMembers WHERE userName='%s'", user); -char *oldEmail = sqlQuickString(conn, query); sqlSafef(query, sizeof(query), "UPDATE gbMembers SET email='%s', lastUse=NOW() WHERE userName='%s'", newEmail, user); sqlUpdate(conn, query); /* Alert the previous address that the change happened, so a hijack is noticed. */ if (isNotEmpty(oldEmail) && differentWord(oldEmail, newEmail)) sendChangeEmailAlertMail(oldEmail, user, newEmail); char *encEmail = htmlEncode(newEmail); hPrintf("<div class=\"centeredContainer formBox\"><h2>%s</h2>", brwName); hPrintf("<h3>Your email address has been changed.</h3>"); hPrintf("<p>Your email address is now <b>%s</b>.</p></div>", encEmail); freeMem(encEmail); returnToURL(1500); } void signupPage(struct sqlConnection *conn) @@ -2145,30 +2156,35 @@ "AND accountActivated='Y'", chosenIdx, email, email, tokenMd5); struct gbMembers *m = gbMembersLoadByQuery(conn, query); if (m == NULL) { freez(&errMsg); errMsg = cloneString("Please choose one of the listed accounts."); chooseAccountPage(conn); return; } /* Consume the token on every account that shared it (single use), then sign in. */ sqlSafef(query, sizeof(query), "UPDATE gbMembers SET loginToken='' WHERE (email='%s' OR recovEmail='%s') AND loginToken='%s'", email, email, tokenMd5); sqlUpdate(conn, query); + /* Record the sign-in on the account actually chosen, the same as the single-account path in + * emailLogin, so lastUse reflects the login. */ + sqlSafef(query, sizeof(query), + "UPDATE gbMembers SET lastUse=NOW() WHERE idx=%u", m->idx); + sqlUpdate(conn, query); cartRemove(cart, "emailLogin_email"); cartRemove(cart, "emailLogin_tokenMd5"); cartRemove(cart, "hgLogin_chosenIdx"); loginAndReturn(m->userName, m->idx); gbMembersFree(&m); return; } /* OAuth mode. */ char *subject = cartUsualString(cart, "oauth_pending_subject", ""); char *email = cartUsualString(cart, "oauth_pending_email", ""); if (isEmpty(subject) || isEmpty(email) || !pendingIdentityValid()) { clearPendingIdentity(); freez(&errMsg);