From fb9aa87ad93863fdee1eace4477f6f1d0871ffee Mon Sep 17 00:00:00 2001 From: Gary Tou Date: Wed, 5 Aug 2026 18:17:34 -0700 Subject: [PATCH] Fix return_to being dropped across the login flow 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) --- app/controllers/logins_controller.rb | 166 +++++-- app/controllers/users_controller.rb | 20 +- .../controllers/webauthn_auth_controller.js | 7 + app/views/logins/_badge.html.erb | 4 +- .../logins/choose_login_preference.html.erb | 2 +- app/views/users/edit.html.erb | 2 + app/views/users/logout.html.erb | 2 +- config/application.rb | 5 + config/environments/test.rb | 4 + spec/requests/login_return_to_spec.rb | 452 ++++++++++++++++++ 10 files changed, 603 insertions(+), 61 deletions(-) create mode 100644 spec/requests/login_return_to_spec.rb diff --git a/app/controllers/logins_controller.rb b/app/controllers/logins_controller.rb index 453fe3eda4..2c73a3b437 100644 --- a/app/controllers/logins_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 8740a6d01b..5f99899a00 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/javascript/controllers/webauthn_auth_controller.js b/app/javascript/controllers/webauthn_auth_controller.js index 275ac6cb82..23cc15f2eb 100644 --- a/app/javascript/controllers/webauthn_auth_controller.js +++ b/app/javascript/controllers/webauthn_auth_controller.js @@ -69,6 +69,12 @@ 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` @@ -76,6 +82,7 @@ export default class extends Controller { { credential: JSON.stringify(credential), method: 'webauthn', + ...(returnTo ? { return_to: returnTo } : {}), ...(await this.fingerprint()), } ) diff --git a/app/views/logins/_badge.html.erb b/app/views/logins/_badge.html.erb index 67e12cd44e..47d5cc242f 100644 --- a/app/views/logins/_badge.html.erb +++ b/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 %> diff --git a/app/views/logins/choose_login_preference.html.erb b/app/views/logins/choose_login_preference.html.erb index aa3a6419ef..fa57142d57 100644 --- a/app/views/logins/choose_login_preference.html.erb +++ b/app/views/logins/choose_login_preference.html.erb @@ -57,7 +57,7 @@ <%= form.hidden_field :email, value: @email, data: { "webauthn-auth-target" => "loginEmailInput" } %>
- <%= 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" } %>
diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 296249cf83..cd804a77a1 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -239,6 +239,8 @@ + <%= hidden_field_tag :return_to, params[:return_to] if params[:return_to] %> +
<%= form.submit "Save settings", disabled: %>
diff --git a/app/views/users/logout.html.erb b/app/views/users/logout.html.erb index 35084e8e28..1b11580637 100644 --- a/app/views/users/logout.html.erb +++ b/app/views/users/logout.html.erb @@ -17,7 +17,7 @@ <% end %>
- <%= 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" %>
diff --git a/config/application.rb b/config/application.rb index acbf985c29..aecd312890 100644 --- a/config/application.rb +++ b/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 diff --git a/config/environments/test.rb b/config/environments/test.rb index 6c5165e906..7b650cb176 100644 --- a/config/environments/test.rb +++ b/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 diff --git a/spec/requests/login_return_to_spec.rb b/spec/requests/login_return_to_spec.rb new file mode 100644 index 0000000000..386b89df2b --- /dev/null +++ b/spec/requests/login_return_to_spec.rb @@ -0,0 +1,452 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Exercises the promise that clicking a link while signed out lands you back on +# that link once you've authenticated, across every branch of the login flow. +RSpec.describe "Login return_to", type: :request do + include WebAuthnSupport + + let(:destination) { "/my/cards" } + # `#complete` sends users with an incomplete profile to their settings page + # instead of `return_to`, so give the user a phone number unless the example + # is specifically about that branch. + let(:user) { create(:user, phone_number: "+18556254225") } + + # Walks the email-code flow the way a browser would. + def submit_email(email:, return_to: destination) + post logins_path, params: { email:, login: { return_to: } } + Login.order(:id).last + end + + def submit_login_code(login, user) + login_code = create(:login_code, user:) + post complete_login_path(login), params: { method: "email", login_code: login_code.code } + end + + # Finds a link by the text it renders, so the badge and the "Sign out" link + # on the same page can be told apart. + def link_named(text) + response.parsed_body.css("a").find { |a| a.text.strip.include?(text) } + end + + # The "signed in as" badge, which doubles as the switch-accounts link. + def badge_link + response.parsed_body.at_css("a:has([aria-label='Switch account'])") + end + + # Turns off the bypass that lets specs drive logins without a real browser, + # so the browser token actually has to match. + def enforcing_browser_token + Rails.configuration.x.skip_login_browser_token_check = false + yield + ensure + Rails.configuration.x.skip_login_browser_token_check = true + end + + describe "arriving at the login page" do + it "carries the requested page over as return_to" do + get destination + + expect(response).to redirect_to( + auth_users_path(return_to: "http://www.example.com#{destination}", require_reload: true) + ) + end + + # `webauthn_auth_controller.js` reads this field to forward `return_to` when + # it posts a passkey assertion to the collection route. + it "puts return_to in the login form" do + get auth_users_path(return_to: destination) + + expect(response.parsed_body.at_css("input[name='login[return_to]']")[:value]).to eq(destination) + end + + it "keeps return_to on the link to the other sign in methods" do + get auth_users_path(return_to: destination) + + expect(link_named("Sign in another way")[:href]).to eq( + choose_login_preference_logins_path(return_to: destination) + ) + end + + it "omits return_to for the dashboard" do + get "/" + + expect(response).to redirect_to(auth_users_path(require_reload: true)) + end + end + + describe "signing in with an email code" do + it "returns to the requested page" do + login = submit_email(email: user.email) + submit_login_code(login, user) + + expect(response).to redirect_to(destination) + end + + it "returns to an absolute URL on the same host" do + login = submit_email(email: user.email, return_to: "http://www.example.com#{destination}") + submit_login_code(login, user) + + expect(response).to redirect_to("http://www.example.com#{destination}") + end + + it "refuses to store or follow a return_to pointing at another host" do + login = submit_email(email: user.email, return_to: "https://evil.example.com/steal") + + expect(login.return_to).to be_nil + + submit_login_code(login, user) + + expect(response).to redirect_to(root_path) + end + + it "falls back to the dashboard rather than looping back to the login page" do + login = submit_email(email: user.email, return_to: auth_users_path) + submit_login_code(login, user) + + expect(response).to redirect_to(root_path) + end + + it "drops a return_to too long to have come from a browser" do + login = submit_email(email: user.email, return_to: "/#{"a" * 3.kilobytes}") + + expect(login.return_to).to be_nil + + submit_login_code(login, user) + + expect(response).to redirect_to(root_path) + end + end + + describe "signing in with a passkey from the login page" do + it "returns to the requested page" do + create_webauthn_credential(user:) + + # `logins/new` has no persisted login yet, so the Stimulus controller + # posts to the collection route with the form's return_to. + get "/users/webauthn/auth_options", params: { email: user.email } + challenge = response.parsed_body["challenge"] + + post complete_logins_path, params: { + method: "webauthn", + credential: get_webauthn_credential(challenge:).to_json, + return_to: destination + } + + expect(response).to redirect_to(destination) + end + end + + describe "choosing a different sign in method from the login page" do + it "keeps return_to on the login it starts" do + # The "Sign in another way" link is only revealed after the passkey + # lookup, which is what seeds `session[:auth_email]`. + get "/users/webauthn/auth_options", params: { email: user.email } + + get choose_login_preference_logins_path(return_to: destination) + + expect(Login.order(:id).last.return_to).to eq(destination) + end + + it "returns to the requested page after picking a method" do + get "/users/webauthn/auth_options", params: { email: user.email } + get choose_login_preference_logins_path(return_to: destination) + + login = Login.order(:id).last + post set_login_preference_login_path(login), params: { login_preference: "email" } + submit_login_code(login, user) + + expect(response).to redirect_to(destination) + end + + it "starts over rather than erroring when the remembered email has no user" do + get "/users/webauthn/auth_options", params: { email: "nobody@example.invalid" } + + get choose_login_preference_logins_path(return_to: destination) + + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + end + + describe "cancelling part way through a login" do + it "offers a Cancel link that keeps return_to" do + get "/users/webauthn/auth_options", params: { email: user.email } + get choose_login_preference_logins_path(return_to: destination) + + expect(link_named("Cancel")[:href]).to eq(logout_users_path(return_to: destination)) + end + + it "goes back to the login page with return_to intact" do + submit_email(email: user.email) + + # "Cancel" signs out, but there's no session to sign out of yet. + delete logout_users_path, params: { return_to: destination } + + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + + it "keeps return_to on the badge that switches accounts mid-login" do + # The badge only renders once a previous sign in has left the avatar + # cookie behind, and mid-login the visitor isn't signed in yet. + first_login = submit_email(email: user.email, return_to: nil) + submit_login_code(first_login, user) + delete logout_users_path + + login = submit_email(email: user.email) + post email_login_path(login) + + expect(badge_link[:href]).to eq(auth_users_path(return_to: destination)) + end + end + + describe "signing in with two factors" do + it "returns to the requested page only once both factors are met" do + freeze_time + + totp = user.create_totp! + user.update!(use_two_factor_authentication: true) + + login = submit_email(email: user.email) + submit_login_code(login, user) + + expect(response).to redirect_to(choose_login_preference_login_path(login)) + + post set_login_preference_login_path(login), params: { login_preference: "totp" } + expect(response).to redirect_to(totp_login_path(login)) + + post complete_login_path(login), params: { + method: "totp", + code: ROTP::TOTP.new(totp.secret, issuer: User::Totp::ISSUER).now + } + + expect(response).to redirect_to(destination) + end + end + + describe "opening an invite while signed in as the wrong account" do + it "offers a sign out that comes back to the invite" do + login = submit_email(email: user.email) + submit_login_code(login, user) + + get auth_users_path(return_to: destination, error: "unauthorised_card_grant") + + expect(link_named("Sign out")[:href]).to eq(logout_users_path(return_to: destination)) + end + end + + describe "when the user is missing a phone number" do + let(:user) { create(:user, full_name: "Fiona Hackworth", phone_number: nil) } + + before do + login = submit_email(email: user.email) + submit_login_code(login, user) + end + + it "sends them to their settings with return_to" do + expect(response).to redirect_to(edit_user_path(user.slug, return_to: destination)) + end + + it "renders a return_to field so the browser sends it back" do + follow_redirect! + + expect(response.parsed_body.at_css("form input[name='return_to']")[:value]).to eq(destination) + end + + it "returns to the requested page once they've added one" do + patch user_path(user), params: { + return_to: destination, + user: { phone_number: "+18556254225" } + } + + expect(response).to redirect_to(destination) + end + + it "does not send them off to another host" do + patch user_path(user), params: { + return_to: "https://evil.example.com/steal", + user: { phone_number: "+18556254225" } + } + + expect(response).to redirect_to(edit_user_path(user)) + end + end + + describe "when return_to points back at the login page" do + let(:user) { create(:user, full_name: "Fiona Hackworth", phone_number: nil) } + + it "does not bounce a user finishing their profile back into signing in" do + login = submit_email(email: user.email, return_to: auth_users_path) + submit_login_code(login, user) + + expect(response).to redirect_to(edit_user_path(user.slug)) + + patch user_path(user), params: { user: { phone_number: "+18556254225" } } + + expect(response).not_to redirect_to(auth_users_path) + end + end + + describe "when the user has no profile yet" do + let(:user) { create(:user, full_name: nil, phone_number: nil) } + + it "returns to the requested page once they've created one" do + login = submit_email(email: user.email) + submit_login_code(login, user) + + expect(response).to redirect_to(edit_user_path(user.slug, return_to: destination)) + + patch user_path(user), params: { + return_to: destination, + user: { full_name: "Fiona Hackworth", phone_number: "+18556254225" } + } + + expect(response).to redirect_to(destination) + end + end + + describe "when already signed in" do + before do + login = submit_email(email: user.email) + submit_login_code(login, user) + end + + it "offers a sign out link that keeps return_to" do + get auth_users_path(return_to: destination) + + expect(link_named("Sign out")[:href]).to eq(logout_users_path(return_to: destination)) + end + + it "keeps return_to on the badge that switches accounts" do + get auth_users_path(return_to: destination) + + expect(badge_link[:href]).to eq(logout_users_path(return_to: destination)) + end + + it "sends them back to the login page with return_to after signing out" do + delete logout_users_path, params: { return_to: destination } + + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + + it "sends them to the dashboard when there is nowhere to return to" do + delete logout_users_path + + expect(response).to redirect_to(root_path) + end + end + + describe "when the login is restarted" do + it "keeps return_to after the login expires" do + login = submit_email(email: user.email) + + travel(Login::EXPIRATION + 1.minute) do + submit_login_code(login, user) + end + + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + + it "keeps return_to when the account is locked" do + login = submit_email(email: user.email) + user.lock! + + submit_login_code(login, user) + + expect(flash[:error]).to eq("Your HCB account has been locked.") + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + + it "keeps the requested return_to when the login id is unknown" do + post "/logins/nonsense/complete", params: { + method: "email", + login_code: "123456", + return_to: destination + } + + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + + it "keeps return_to when a passkey assertion fails" do + # A passkey belonging to somebody else, so verification rejects it. + stranger = create(:user) + create_webauthn_credential(user: stranger) + credential = get_webauthn_credential(challenge: generate_webauthn_challenge(user: stranger)) + + login = submit_email(email: user.email) + + post complete_login_path(login), params: { + method: "webauthn", + credential: credential.to_json + } + + expect(flash[:error]).to eq("Invalid security key") + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + + it "keeps return_to when the email address is rejected" do + post logins_path, params: { email: "not-an-email", login: { return_to: destination } } + + expect(response).to redirect_to(auth_users_path(return_to: destination)) + end + end + + # Login hashids are salted with an empty string, so they can be computed + # rather than guessed. Nothing about a login may leak to a browser that + # can't produce the token it was started with. + describe "when a different browser presents someone else's login id" do + it "still enforces the browser token when the bypass is unconfigured" do + login = submit_email(email: user.email) + + # An unset `config.x` key reads back as an empty `OrderedOptions`, which + # is truthy. Reading it as "skip the check" would disable this in every + # environment that doesn't set it. + unset = ActiveSupport::OrderedOptions.new + Rails.configuration.x.skip_login_browser_token_check = unset + begin + reset! + post complete_login_path(login), params: { method: "email", login_code: "123456" } + ensure + Rails.configuration.x.skip_login_browser_token_check = true + end + + expect(response).to redirect_to(auth_users_path) + end + + it "refuses to hand over the expired login's return_to" do + login = submit_email(email: user.email) + + enforcing_browser_token do + # A fresh session: no browser token cookie for this login. + reset! + travel(Login::EXPIRATION + 1.minute) do + post complete_login_path(login), params: { method: "email", login_code: "123456" } + end + end + + expect(response).to redirect_to(auth_users_path) + end + + it "refuses to hand over a live login's return_to" do + login = submit_email(email: user.email) + + enforcing_browser_token do + reset! + post complete_login_path(login), params: { method: "email", login_code: "123456" } + end + + expect(response).to redirect_to(auth_users_path) + end + + it "refuses to hand over the return_to of a login with no browser token" do + login = submit_email(email: user.email) + login.update_column(:browser_token_ciphertext, nil) + + travel(Login::EXPIRATION + 1.minute) do + post complete_login_path(login), params: { method: "email", login_code: "123456" } + end + + expect(response).to redirect_to(auth_users_path) + end + end +end