Files
sure/app/controllers/properties_controller.rb
Guillem Arias Fauste 74f811c334 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).
2026-06-03 15:15:49 +02:00

133 lines
3.9 KiB
Ruby

class PropertiesController < ApplicationController
include AccountableResource, StreamExtensions
before_action :set_property, only: [ :balances, :address, :update_balances, :update_address ]
before_action :require_property_write_permission!, only: [ :update_balances, :update_address ]
def new
@account = Current.family.accounts.build(accountable: Property.new)
end
def create
@account = Current.family.accounts.create!(
property_params.merge(
balance: 0,
status: "draft",
owner: Current.user,
currency: property_params[:currency].presence || Current.family.currency
)
)
@account.auto_share_with_family! if Current.family.share_all_by_default?
redirect_to balances_property_path(@account)
end
def update
if @account.update(property_params)
@success_message = "Property details updated successfully."
if @account.active?
render :edit
else
redirect_to balances_property_path(@account)
end
else
@error_message = "Unable to update property details."
render :edit, status: :unprocessable_entity
end
end
def edit
end
def balances
end
def update_balances
result = nil
Account.transaction do
@account.update!(currency: balance_params[:currency]) if balance_params[:currency].present?
result = @account.set_current_balance(balance_params[:balance].to_d)
raise ActiveRecord::Rollback unless result.success?
end
if result&.success?
@success_message = "Balance updated successfully."
if @account.active?
render :balances
else
redirect_to address_property_path(@account)
end
else
@error_message = result&.error_message
render :balances, status: :unprocessable_entity
end
end
def address
@property = @account.property
@property.address ||= Address.new
end
def update_address
if @account.property.update(address_params)
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 return_path }
format.turbo_stream { stream_redirect_to return_path }
end
else
@success_message = "Address updated successfully."
render :address
end
else
@error_message = "Unable to update address. Please check the required fields."
render :address, status: :unprocessable_entity
end
end
private
def balance_params
params.require(:account).permit(:balance, :currency)
end
def address_params
params.require(:property)
.permit(address_attributes: [ :line1, :line2, :locality, :region, :country, :postal_code ])
end
def property_params
params.require(:account)
.permit(
:name,
:currency,
:accountable_type,
:institution_name,
:institution_domain,
:notes,
accountable_attributes: [ :id, :subtype, :year_built, :area_unit, :area_value ]
)
end
def set_property
@account = accessible_accounts.find(params[:id])
@property = @account.property
end
def require_property_write_permission!
require_account_permission!(@account)
end
end