fix(accounts): honor stored return_to after subtype account creation (#2109)

* fix(accounts): honor stored return_to after subtype account creation

Closes #1766.

The savings-goals empty-state "Add an account" CTA passes ?return_to, which
StoreLocation captures into session[:return_to], but account-creation flows
didn't always consume it:

- AccountableResource#create honored a form-carried return_to but not the
  session value, so if the param wasn't threaded through the multi-step
  new-account flow the user still landed on the account page. Added a
  session[:return_to] fallback (the form param still wins).
- PropertiesController is a 3-step wizard (create → balances → address) that
  never threaded return_to as a form param, and its final redirect went
  straight to account_path. It now honors session[:return_to] on completion.

Rails blocks external-host redirects, so return_to can't open-redirect.
valuations#create uses redirect_back_or_to (referer-based) — different flow,
left as-is.

Tests: depository create prefers the form return_to and falls back to the
session value; property wizard completion honors the stored return_to.

* fix(accounts): block open-redirect via return_to; consume session value

Two AI-review findings on #2109:

- Open-redirect (codex): the property wizard's turbo_stream completion uses
  stream_redirect_to, which the client resolves with Turbo.visit — that
  full-navigates cross-origin, bypassing Rails' redirect host-guard. A crafted
  ?return_to=https://evil could walk the user off-site. Filter return_to at the
  StoreLocation choke point (store time) to internal absolute paths only, and
  sanitize the separate form-param channel, so an unsafe value can't reach
  redirect_to / stream_redirect_to.
- Stale session (coderabbit): session[:return_to] was read but never consumed.
  Consume it with delete at redirect time so it can't leak into a later flow.

Adds guard tests (external return_to falls back to the account page).

* fix(security): guard safe_return_to against non-String return_to

A crafted `?return_to[]=foo` makes params[:return_to] an Array, and
Array#match? doesn't exist, so safe_return_to raised NoMethodError
before the open-redirect hardening could reject it. Add an
is_a?(String) check as the first gate. Other CodeRabbit/Codex
return_to findings on this PR were already addressed (consume-side
re-validation + session.delete).

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

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

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."

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

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