diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index 479ad9efc..18e38b15e 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -48,7 +48,14 @@ module AccountableResource @account.lock_saved_attributes! end - redirect_to account_params[:return_to].presence || @account, notice: t("accounts.create.success", type: accountable_type.name.underscore.humanize) + # Prefer the form-carried return_to, then the session value StoreLocation + # captured from `?return_to=` (survives multi-step flows where the param + # isn't threaded), then the account page. The form param is sanitized here + # (the session value is already filtered at store time); the session is + # consumed with delete so a stale value can't leak into a later flow. + return_path = safe_return_to(account_params[:return_to]) || session.delete(:return_to).presence || @account + redirect_to return_path, + notice: t("accounts.create.success", type: accountable_type.name.underscore.humanize) end def update diff --git a/app/controllers/concerns/store_location.rb b/app/controllers/concerns/store_location.rb index e2e8d3181..14f98068d 100644 --- a/app/controllers/concerns/store_location.rb +++ b/app/controllers/concerns/store_location.rb @@ -24,9 +24,20 @@ private end def store_return_to - if params[:return_to].present? - session[:return_to] = params[:return_to] - end + safe = safe_return_to(params[:return_to]) + session[:return_to] = safe if safe + end + + # Only allow internal absolute paths (a single leading "/"). Blocks absolute + # URLs, protocol-relative ("//evil"), and backslash tricks ("/\\evil") so a + # crafted ?return_to= can't open-redirect — including via a custom + # turbo_stream redirect, which Rails' redirect host-guard does NOT cover + # (the client `Turbo.visit`es the target and full-navigates cross-origin). + def safe_return_to(value) + # is_a?(String) first: a crafted `?return_to[]=foo` makes params[:return_to] + # an Array, and Array#match? doesn't exist — without this guard the helper + # raises NoMethodError before the redirect hardening can reject it. + value if value.is_a?(String) && value.present? && value.match?(%r{\A/(?![/\\])}) end def clear_previous_path diff --git a/app/controllers/properties_controller.rb b/app/controllers/properties_controller.rb index 1de7a5f8c..9b00d4cee 100644 --- a/app/controllers/properties_controller.rb +++ b/app/controllers/properties_controller.rb @@ -75,9 +75,18 @@ class PropertiesController < ApplicationController if @account.draft? @account.activate! + # The property setup wizard (create → balances → address) is multi-step, + # so the original `?return_to=` only survives in the session (captured by + # StoreLocation), not as a threaded form param. Honor it on completion so + # flows like the savings-goals "Add an account" CTA land back where they + # started instead of on the account page. Sanitized + consumed: the + # turbo_stream branch below isn't covered by Rails' redirect host-guard, + # so an unsafe value must not reach stream_redirect_to. + return_path = safe_return_to(session.delete(:return_to)) || account_path(@account) + respond_to do |format| - format.html { redirect_to account_path(@account) } - format.turbo_stream { stream_redirect_to account_path(@account) } + format.html { redirect_to return_path } + format.turbo_stream { stream_redirect_to return_path } end else @success_message = "Address updated successfully." diff --git a/test/controllers/depositories_controller_test.rb b/test/controllers/depositories_controller_test.rb index 3c6e443a5..9ce0eb70b 100644 --- a/test/controllers/depositories_controller_test.rb +++ b/test/controllers/depositories_controller_test.rb @@ -7,4 +7,35 @@ class DepositoriesControllerTest < ActionDispatch::IntegrationTest sign_in @user = users(:family_admin) @account = accounts(:depository) end + + test "create falls back to the stored return_to when no form param is present" do + get new_account_path(return_to: transactions_path) # StoreLocation captures it into the session + + assert_difference -> { Account.count } => 1 do + post depositories_path, params: { + account: { name: "Return To Checking", currency: "USD", balance: 100, accountable_type: "Depository" } + } + end + + assert_redirected_to transactions_path + end + + test "create prefers the form return_to over the session value" do + get new_account_path(return_to: transactions_path) # session return_to + + post depositories_path, params: { + account: { name: "Form RT Checking", currency: "USD", balance: 100, accountable_type: "Depository", return_to: budgets_path } + } + + assert_redirected_to budgets_path + end + + test "create ignores an external return_to (open-redirect guard)" do + post depositories_path, params: { + account: { name: "Evil RT Checking", currency: "USD", balance: 100, accountable_type: "Depository", return_to: "https://evil.example/phish" } + } + + created = Account.order(:created_at).last + assert_redirected_to account_path(created) # not the external URL + end end diff --git a/test/controllers/properties_controller_test.rb b/test/controllers/properties_controller_test.rb index ce5a97847..81ecd88f8 100644 --- a/test/controllers/properties_controller_test.rb +++ b/test/controllers/properties_controller_test.rb @@ -188,4 +188,89 @@ class PropertiesControllerTest < ActionDispatch::IntegrationTest assert draft_account.active? assert_redirected_to account_path(draft_account) end + + test "address update on draft account honors stored return_to over the account page" do + draft_account = Account.create!( + family: @user.family, + name: "Draft Property RT", + accountable: Property.new, + status: "draft", + balance: 500000, + currency: "USD" + ) + + # The property wizard (create → balances → address) doesn't thread return_to + # as a form param, so StoreLocation's session value is the only carrier. + get new_account_path(return_to: transactions_path) + + patch update_address_property_path(draft_account), params: { + property: { + address_attributes: { + line1: "789 Activate St", + locality: "New York", + region: "NY", + country: "US", + postal_code: "10001" + } + } + } + + draft_account.reload + assert draft_account.active? + assert_redirected_to transactions_path + end + + test "address update ignores an external stored return_to (open-redirect guard)" do + draft_account = Account.create!( + family: @user.family, + name: "Draft Property Evil", + accountable: Property.new, + status: "draft", + balance: 500000, + currency: "USD" + ) + + # A hostile ?return_to is rejected at store time, so the wizard falls back + # to the account page rather than stream-redirecting off-site. + get new_account_path(return_to: "https://evil.example/phish") + + patch update_address_property_path(draft_account), params: { + property: { + address_attributes: { + line1: "1 Safe St", locality: "NYC", region: "NY", country: "US", postal_code: "10001" + } + } + } + + draft_account.reload + assert draft_account.active? + assert_redirected_to account_path(draft_account) + end + + test "address update tolerates a non-String stored return_to without raising" do + draft_account = Account.create!( + family: @user.family, + name: "Draft Property Array", + accountable: Property.new, + status: "draft", + balance: 500000, + currency: "USD" + ) + + # `?return_to[]=foo` makes params[:return_to] an Array; safe_return_to must + # reject it via the is_a?(String) guard instead of raising NoMethodError. + get new_account_path("return_to" => [ "/transactions" ]) + + patch update_address_property_path(draft_account), params: { + property: { + address_attributes: { + line1: "1 Safe St", locality: "NYC", region: "NY", country: "US", postal_code: "10001" + } + } + } + + draft_account.reload + assert draft_account.active? + assert_redirected_to account_path(draft_account) + end end