Fix return_to being dropped across the login flow - #14514
Open
garyhtou wants to merge 1 commit into
Open
Conversation
Signing in was supposed to land you back on the page you originally clicked, but five branches of the flow threw the destination away: - Passkey sign in from the login page posts to the collection route, where set_login built a fresh Login with empty state. - "Sign in another way" reached that same route. - Users missing a phone number were sent to settings, whose form never rendered a return_to field. - Signing out to switch accounts (invite links) dropped it, both on the "Sign out" link and on the badge that switches accounts mid-login. - Restarting a login (expiry, locked account, failed passkey, rejected email) went to a bare /users/auth. All return_to handling now goes through one safe_return_to helper: same host, a route that exists, not back into the login flow, length bounded. This also covers login[return_to], which the filter in ApplicationController never reached since it only sees the top level param. The browser token check was keyed off Rails.env.test?, so no spec could exercise it. It is now a config flag, compared against true because an unset config.x key reads back as a truthy OrderedOptions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
garyhtou
force-pushed
the
login-return-to-edge-cases
branch
from
August 6, 2026 08:11
e1dc48e to
fb9aa87
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the problem
When you click a link while signed out, we send you to
/users/auth?return_to=...and are supposed to drop you back on that link once you've authenticated. Users reported landing on the dashboard instead. Auditing every branch of the login flow turned up five ways the destination gets thrown away.Describe your changes
Roughly in order of how often they'd bite:
logins/newhas no persistedLoginyet, so the Stimulus controller posts the assertion to the collection route.set_loginbuilt a brand newLoginwith empty state there, discarding thereturn_tositting in the form. It now seeds the login from the param, and the controller forwards the field.return_tothe same way. The link was already passing it; nothing read it.return_tofield, andusers#updateonly honored one when the user's name had also been blank.users#logoutnow returns you to the login page withreturn_tointact. Same for the account badge, which is the switch-accounts affordance on every code entry page and dropped it in both its signed in and signed out forms./users/auth. Arestart_loginhelper preservesreturn_to, recovering it from an expired login only when the browser token matches.Two things hardened along the way:
login[return_to]bypassed the host filter inApplicationController, which only sees the top level param. Allreturn_tohandling now goes through onesafe_return_tohelper: same host, a route that exists, not back into the login flow, and length bounded so it can't overflow the 10KB cap onLogin#state.Rails.env.test?, so no spec could reach it. It's now a config flag. It's compared againsttruebecause an unsetconfig.xkey reads back as a truthyOrderedOptions, which would have disabled the check everywhere.Testing
spec/requests/login_return_to_spec.rbwalks the real HTTP flows, including a real WebAuthn assertion rather than a stub. Every fix was mutation checked: reverting the production change turns a specific example red. That check is what caught two sign out assertions that were originally masking each other, so either fix could have silently reverted.Full suite green. Not verified in a browser: the local dev database has an unrelated migration rolled back, so the one browser-only piece (a line of JS forwarding the form's
return_to) is covered by asserting the field it reads is rendered.