Fix return_to being dropped across the login flow

app/controllers/logins_controller.rb

@@ -1,6 +1,10 @@
 # frozen_string_literal: true
 
 class LoginsController < ApplicationController
+  # Longer than any URL a browser will navigate to, and `return_to` is stored
+  # in `Login#state`, which is capped at 10KB.
+  MAX_RETURN_TO_BYTES = 2.kilobytes
+
   skip_before_action :signed_in_user, except: [:reauthenticate]
   skip_after_action :verify_authorized
   before_action :set_login, except: [:new, :create, :reauthenticate]
@@ -21,7 +25,7 @@ def new
     render "users/logout" if current_user
 
     referral_link_id = Referral::Link.find_by(slug: params[:referral])&.slug if params[:referral].present?
-    @login = Login.new(state: { return_to: url_from(params[:return_to]), purpose: params[:purpose] }, referral_link_id:)
+    @login = Login.new(state: { return_to: requested_return_to, purpose: params[:purpose] }, referral_link_id:)
 
     @prefill_email = params[:email].presence || current_user(allow_unverified: true)&.email.presence
     @signup = params[:signup] == "true"
@@ -48,21 +52,24 @@ def create
 
     continue_login(preference: login_preference || :email)
   rescue ActiveRecord::RecordInvalid => e
-    flash[:error] = e.record.errors.full_messages.to_sentence
-    return redirect_to auth_users_path
+    return restart_login(error: e.record.errors.full_messages.to_sentence)
   rescue => e
-    flash[:error] = e.message
-    return redirect_to auth_users_path
+    # Exception messages here can carry database internals (a unique violation
+    # quotes the conflicting email), so report them rather than showing them.
+    Rails.error.report(e)
+    return restart_login(error: "Something went wrong signing you in. Please try again or contact HCB for support.")
   end
 
   # get page to choose preference
   def choose_login_preference
-    return redirect_to auth_users_path if @email.nil?
+    if @email.nil?
+      Rails.error.unexpected("[Login] Login #{@login.id} has a user without an email address.")
+      return restart_login(error: "Something went wrong. Please try again or contact HCB for support.")
+    end
 
     if @login.available_factors.none?
       Rails.error.unexpected("[Login] Login ran out of available factors. This should never be possible.")
-      flash[:error] = "Something went wrong. Please try again or contact HCB for support."
-      return redirect_to auth_users_path
+      return restart_login(error: "Something went wrong. Please try again or contact HCB for support.")
     end
 
     session.delete :login_preference
@@ -77,10 +84,7 @@ def set_login_preference
   def email
     resp = LoginCodeService::Request.new(email: @email, ip_address: request.remote_ip, user_agent: request.user_agent).run
 
-    if resp[:error].present?
-      flash[:error] = resp[:error]
-      return redirect_to auth_users_path
-    end
+    return restart_login(error: resp[:error]) if resp[:error].present?
 
     render status: :unprocessable_content
   end
@@ -89,10 +93,7 @@ def email
   def sms
     resp = LoginCodeService::Request.new(email: @email, sms: true, ip_address: request.remote_ip, user_agent: request.user_agent).run
 
-    if resp[:error].present?
-      flash[:error] = resp[:error]
-      return redirect_to auth_users_path
-    end
+    return restart_login(error: resp[:error]) if resp[:error].present?
 
     render status: :unprocessable_content
   end
@@ -116,7 +117,7 @@ def complete
       )
 
       unless ok
-        redirect_to(auth_users_path, flash: { error: service.errors.full_messages.to_sentence })
+        restart_login(error: service.errors.full_messages.to_sentence)
         return
       end
     when "sms"
@@ -178,19 +179,11 @@ def complete
       elsif @referral_link.present?
         redirect_to referral_link_path(@referral_link)
       elsif (@user.full_name.blank? || @user.phone_number.blank?) && !@login.for_application?
-        redirect_to edit_user_path(@user.slug, return_to: @login.return_to)
+        redirect_to edit_user_path(@user.slug, return_to: safe_return_to(@login.return_to))
       elsif @login.authenticated_with_backup_code && @user.backup_codes.active.empty?
         redirect_to security_user_path(@user), flash: { warning: "You've just used your last backup code, and we recommend generating more." }
       else
-        return_path = url_from(@login.return_to)
-        if return_path.present?
-          begin
-            route = Rails.application.routes.recognize_path(return_path)
-            return_path = root_path if route[:controller] == "logins"
-          rescue ActionController::RoutingError
-            return_path = root_path
-          end
-        end
+        return_path = safe_return_to(@login.return_to)
 
         if @user.only_draft_application? && return_path.blank?
           redirect_to application_path(@user.applications.first)
@@ -202,7 +195,7 @@ def complete
       continue_login
     end
   rescue SessionsHelper::AccountLockedError => e
-    redirect_to(auth_users_path, flash: { error: e.message })
+    restart_login(error: e.message)
   end
 
   def reauthenticate
@@ -228,7 +221,49 @@ def continue_login(preference: login_preference)
   end
 
   def login_params
-    params.require(:login).permit(:return_to, :purpose, :referral_link_id)
+    params
+      .require(:login)
+      .permit(:return_to, :purpose, :referral_link_id)
+      # `ApplicationController` filters `params[:return_to]`, but not the
+      # nested `login[return_to]` this form posts, so filter it here.
+      .merge(return_to: safe_return_to(params.dig(:login, :return_to)))
+  end
+
+  # Reduces a candidate `return_to` to somewhere we're willing to send a user:
+  # this host, a route that exists, and not back into the login flow they just
+  # came out of.
+  #
+  # @return [String, nil]
+  def safe_return_to(value)
+    return nil if value.to_s.bytesize > MAX_RETURN_TO_BYTES
+
+    url = url_from(value)
+    return nil if url.blank?
+
+    begin
+      return nil if Rails.application.routes.recognize_path(url)[:controller] == "logins"
+    rescue ActionController::RoutingError
+      return nil
+    end
+
+    url
+  end
+
+  # The page the visitor was trying to reach before we asked them to sign in.
+  def requested_return_to
+    safe_return_to(params[:return_to])
+  end
+
+  # Sends the visitor back to the start of the login flow without dropping the
+  # page they were originally trying to reach.
+  #
+  # @param error [String] flash error to show on the login page
+  # @param login [Login, nil] the login to recover `return_to` from
+  def restart_login(error:, login: @login)
+    return_to = safe_return_to(login&.return_to) || requested_return_to
+    flash[:error] = error
+
+    redirect_to auth_users_path(return_to:)
   end
 
   def login_preference
@@ -240,26 +275,47 @@ def login_preference
   end
 
   def set_login
-    begin
-      if params[:id]
-        @login = Login.incomplete.active.initial.find_by_hashid!(params[:id])
-        @referral_link = @login.referral_link
-        @referral_program = @referral_link&.program
-        unless valid_browser_token?
-          # error! browser token doesn't match the cookie.
-          flash[:error] = "This doesn't seem to be the browser who began this login; please ensure cookies are enabled."
-          redirect_to auth_users_path
-        end
-      elsif session[:auth_email]
-        @login = User.find_by_email(session[:auth_email]).logins.create
-        cookies.signed["browser_token_#{@login.hashid}"] = { value: @login.browser_token, expires: Login::EXPIRATION.from_now }
-      else
-        flash[:error] = "Please try again."
-        redirect_to auth_users_path
+    if params[:id]
+      @login = Login.incomplete.active.initial.find_by_hashid(params[:id])
+
+      unless @login
+        # The login expired or never existed. Recover `return_to` from it so
+        # the user doesn't lose the page they were heading to, but only for the
+        # browser that began it: hashids are enumerable (they're salted with an
+        # empty string), so a login that can't prove which browser started it
+        # doesn't get to hand its `return_to` to whoever asks.
+        expired = Login.initial.incomplete.find_by_hashid(params[:id])
+        expired = nil unless expired&.browser_token.present? && valid_browser_token?(expired)
+
+        return restart_login(error: "Please start again.", login: expired)
       end
-    rescue ActiveRecord::RecordNotFound
-      flash[:error] = "Please start again."
-      redirect_to auth_users_path, flash: { error: "Please start again." }
+
+      @referral_link = @login.referral_link
+      @referral_program = @referral_link&.program
+
+      unless valid_browser_token?
+        # error! browser token doesn't match the cookie. Don't hand this
+        # browser the other browser's `return_to`.
+        return restart_login(
+          error: "This doesn't seem to be the browser who began this login; please ensure cookies are enabled.",
+          login: nil
+        )
+      end
+    elsif session[:auth_email] && (user = User.find_by_email(session[:auth_email]))
+      # Reached when a login begins without a persisted `Login` record: the
+      # passkey flow on the login page and the "Sign in another way" link both
+      # reach the collection routes, so `return_to` arrives as a param.
+      @login = user.logins.create!(state: { return_to: requested_return_to, purpose: params[:purpose] })
+      cookies.signed["browser_token_#{@login.hashid}"] = { value: @login.browser_token, expires: Login::EXPIRATION.from_now }
+    else
+      if session[:auth_email].present?
+        # `UsersController#webauthn_options` stores the email before checking
+        # that it belongs to anyone, so a typo leaves this pointing at nobody.
+        # Left in place it makes every retry fail the same way.
+        session.delete(:auth_email)
+      end
+
+      restart_login(error: "Please try again.")
     end
   end
 
@@ -278,12 +334,16 @@ def fingerprint_info
     }
   end
 
-  def valid_browser_token?
-    return true if Rails.env.test?
-    return true unless @login.browser_token
-    return false unless cookies.signed["browser_token_#{@login.hashid}"]
+  def valid_browser_token?(login = @login)
+    # Specs drive logins without a browser, so they can't produce the cookie.
+    # Those covering this check turn the bypass off (see config/environments/test.rb).
+    # Compared against `true` because an unset `config.x` key reads back as an
+    # empty `OrderedOptions`, which is truthy.
+    return true if Rails.configuration.x.skip_login_browser_token_check == true
+    return true unless login.browser_token
+    return false unless cookies.signed["browser_token_#{login.hashid}"]
 
-    ActiveSupport::SecurityUtils.secure_compare(@login.browser_token, cookies.signed["browser_token_#{@login.hashid}"])
+    ActiveSupport::SecurityUtils.secure_compare(login.browser_token, cookies.signed["browser_token_#{login.hashid}"])
   end
 
 end

app/controllers/users_controller.rb

@@ -4,7 +4,9 @@ class UsersController < ApplicationController
   # `unimpersonate` is gated by its own `current_session&.impersonated?` guard,
   # and must remain reachable from impersonated sessions whose `verified` flag
   # mirrors an unverified target — otherwise the admin is locked out.
-  skip_before_action :signed_in_user, only: [:webauthn_options, :unimpersonate]
+  # `logout` doubles as the "cancel" and "switch accounts" action on the login
+  # pages, where there's no session yet, and signing out without one is a no-op.
+  skip_before_action :signed_in_user, only: [:webauthn_options, :unimpersonate, :logout]
   skip_before_action :redirect_to_onboarding, only: [:edit, :update, :logout, :unimpersonate]
   skip_after_action :verify_authorized, only: [:show,
                                                :revoke_oauth_application,
@@ -92,7 +94,11 @@ def webauthn_options
 
   def logout
     sign_out
-    redirect_to root_path
+
+    # Signing out is how you switch accounts part-way through a login, so send
+    # people back to the login page with the page they were heading to intact.
+    return_to = url_from(params[:return_to])
+    redirect_to return_to.present? ? auth_users_path(return_to:) : root_path
   end
 
   def logout_all
@@ -337,7 +343,7 @@ def suppress_card_locking
   end
 
   def update
-    return_to = params[:return_to]
+    return_to = url_from(params[:return_to])
     @states = ISO3166::Country.new("US").subdivisions.values.map { |s| [s.translations["en"], s.code] }
     @user = User.friendly.find(params[:id])
     authorize @user
@@ -438,7 +444,13 @@ def update
 
         ::StripeCardholderService::Update.new(current_user: @user).run
 
-        redirect_back_or_to edit_user_path(@user)
+        # The login flow sends users here when they're missing a phone number,
+        # so honor `return_to` before falling back to where they came from.
+        if return_to.present?
+          redirect_to(return_to)
+        else
+          redirect_back_or_to edit_user_path(@user)
+        end
       end
     else
       set_onboarding

app/javascript/controllers/webauthn_auth_controller.js

@@ -69,13 +69,20 @@ export default class extends Controller {
 
       this.storeLoginEmail(loginEmail)
 
+      // Without a login id we post to the collection route, which has no
+      // persisted login to read `return_to` off of, so send it along.
+      const returnTo = this.authFormTarget.querySelector(
+        'input[name="login[return_to]"]'
+      )?.value
+
       submitForm(
         this.loginIdValue
           ? `/logins/${this.loginIdValue}/complete`
           : `/logins/complete`,
         {
           credential: JSON.stringify(credential),
           method: 'webauthn',
+          ...(returnTo ? { return_to: returnTo } : {}),
           ...(await this.fingerprint()),
         }
       )

app/views/logins/_badge.html.erb

@@ -4,8 +4,8 @@
     <%= user_mention user, class: "badge bg-muted pl-1 m-0 tooltipped tooltipped--e", aria_label: "Switch account" %>
   <% end %>
   <% if signed_in? %>
-    <%= link_to(logout_users_path, method: :delete, class: "mb-2") { mention } %>
+    <%= link_to(logout_users_path(return_to: @login&.return_to || params[:return_to]), method: :delete, class: "mb-2") { mention } %>
   <% else %>
-    <%= link_to(auth_users_path, class: "mb-2") { mention } %>
+    <%= link_to(auth_users_path(return_to: @login&.return_to || params[:return_to]), class: "mb-2") { mention } %>
   <% end %>
 <% end %>

app/views/logins/choose_login_preference.html.erb

@@ -57,7 +57,7 @@
     <%= form.hidden_field :email, value: @email, data: { "webauthn-auth-target" => "loginEmailInput" } %>
 
     <div class="flex flex-row justify-between items-center my-4">
-      <%= link_to "Cancel", logout_users_path, method: :delete, class: "no-underline block" %>
+      <%= link_to "Cancel", logout_users_path(return_to: @login.return_to), method: :delete, class: "no-underline block" %>
       <%= form.submit "Continue", data: { "webauthn-auth-target" => "continueButton", "form-disable-target" => "submitButton" } %>
     </div>
 

app/views/users/edit.html.erb

@@ -239,6 +239,8 @@
           </div>
         </fieldset>
 
+        <%= hidden_field_tag :return_to, params[:return_to] if params[:return_to] %>
+
         <div class="actions flex">
           <%= form.submit "Save settings", disabled: %>
         </div>

app/views/users/logout.html.erb

@@ -17,7 +17,7 @@
     <% end %>
 
   <div class="flex justify-between items-center flex-wrap w-full">
-    <%= link_to "Sign out", logout_users_path, method: :delete, class: "block mt-0 no-underline" %>
+    <%= link_to "Sign out", logout_users_path(return_to: params[:return_to]), method: :delete, class: "block mt-0 no-underline" %>
 
     <%= ugc_link_to "Continue to HCB", @return_to || root_path, class: "btn bg-info" %>
   </div>

config/application.rb

@@ -126,5 +126,10 @@ class Application < Rails::Application
 
     config.action_controller.include_all_helpers = false
 
+    # Logins check a browser token cookie against the login record so a login
+    # can only be continued in the browser that began it. Only the test
+    # environment turns this off.
+    config.x.skip_login_browser_token_check = false
+
   end
 end

config/environments/test.rb

@@ -58,4 +58,8 @@
 
   # Raise error when a before_action's only/except options reference missing actions.
   config.action_controller.raise_on_missing_callback_actions = true
+
+  # Logins are started without a browser here, so there's no cookie to compare
+  # the login's browser token against. Specs that cover that check flip this off.
+  config.x.skip_login_browser_token_check = true
 end