mirror of
https://github.com/we-promise/sure.git
synced 2026-06-06 03:09:02 +00:00
* 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).
277 lines
8.0 KiB
Ruby
277 lines
8.0 KiB
Ruby
require "test_helper"
|
|
|
|
class PropertiesControllerTest < ActionDispatch::IntegrationTest
|
|
include AccountableResourceInterfaceTest
|
|
|
|
setup do
|
|
sign_in @user = users(:family_admin)
|
|
@account = accounts(:property)
|
|
end
|
|
|
|
test "creates property in draft status and redirects to balances step" do
|
|
assert_difference -> { Account.count } => 1 do
|
|
post properties_path, params: {
|
|
account: {
|
|
name: "New Property",
|
|
subtype: "house",
|
|
currency: "EUR",
|
|
institution_name: "Property Lender",
|
|
institution_domain: "propertylender.example",
|
|
notes: "Property notes",
|
|
accountable_type: "Property",
|
|
accountable_attributes: {
|
|
year_built: 1990,
|
|
area_value: 1200,
|
|
area_unit: "sqft"
|
|
}
|
|
}
|
|
}
|
|
end
|
|
|
|
created_account = Account.order(:created_at).last
|
|
assert created_account.accountable.is_a?(Property)
|
|
assert_equal "draft", created_account.status
|
|
assert_equal 0, created_account.balance
|
|
assert_equal "EUR", created_account.currency
|
|
assert_equal "Property Lender", created_account[:institution_name]
|
|
assert_equal "propertylender.example", created_account[:institution_domain]
|
|
assert_equal "Property notes", created_account[:notes]
|
|
assert_equal 1990, created_account.accountable.year_built
|
|
assert_equal 1200, created_account.accountable.area_value
|
|
assert_equal "sqft", created_account.accountable.area_unit
|
|
assert_redirected_to balances_property_path(created_account)
|
|
end
|
|
|
|
test "updates property overview" do
|
|
assert_no_difference [ "Account.count", "Property.count" ] do
|
|
patch property_path(@account), params: {
|
|
account: {
|
|
name: "Updated Property",
|
|
institution_name: "Updated Lender",
|
|
institution_domain: "updatedlender.example",
|
|
notes: "Updated property notes",
|
|
accountable_attributes: {
|
|
id: @account.accountable.id,
|
|
subtype: "condominium"
|
|
}
|
|
}
|
|
}
|
|
end
|
|
|
|
@account.reload
|
|
assert_equal "Updated Property", @account.name
|
|
assert_equal "condominium", @account.subtype
|
|
assert_equal "Updated Lender", @account[:institution_name]
|
|
assert_equal "updatedlender.example", @account[:institution_domain]
|
|
assert_equal "Updated property notes", @account[:notes]
|
|
|
|
# If account is active, it renders edit view; otherwise redirects to balances
|
|
if @account.active?
|
|
assert_response :success
|
|
else
|
|
assert_redirected_to balances_property_path(@account)
|
|
end
|
|
end
|
|
|
|
# Tab view tests
|
|
test "shows balances tab" do
|
|
get balances_property_path(@account)
|
|
assert_response :success
|
|
end
|
|
|
|
test "shows address tab" do
|
|
get address_property_path(@account)
|
|
assert_response :success
|
|
end
|
|
|
|
# Tab update tests
|
|
test "updates balances tab" do
|
|
original_balance = @account.balance
|
|
|
|
patch update_balances_property_path(@account), params: {
|
|
account: {
|
|
balance: 600000,
|
|
currency: "EUR"
|
|
}
|
|
}
|
|
|
|
@account.reload
|
|
assert_equal 600000, @account.balance
|
|
assert_equal "EUR", @account.currency
|
|
|
|
# If account is active, it renders balances view; otherwise redirects to address
|
|
if @account.active?
|
|
assert_response :success
|
|
else
|
|
assert_redirected_to address_property_path(@account)
|
|
end
|
|
end
|
|
|
|
test "updates address tab" do
|
|
patch update_address_property_path(@account), params: {
|
|
property: {
|
|
address_attributes: {
|
|
line1: "456 New Street",
|
|
locality: "San Francisco",
|
|
region: "CA",
|
|
country: "US",
|
|
postal_code: "94102"
|
|
}
|
|
}
|
|
}
|
|
|
|
@account.reload
|
|
assert_equal "456 New Street", @account.accountable.address.line1
|
|
assert_equal "San Francisco", @account.accountable.address.locality
|
|
|
|
# If account is draft, it activates and redirects; otherwise renders address
|
|
if @account.draft?
|
|
assert_redirected_to account_path(@account)
|
|
else
|
|
assert_response :success
|
|
end
|
|
end
|
|
|
|
test "balances update handles validation errors" do
|
|
Account.any_instance.stubs(:set_current_balance).returns(OpenStruct.new(success?: false, error_message: "Invalid balance"))
|
|
|
|
patch update_balances_property_path(@account), params: {
|
|
account: {
|
|
balance: 600000,
|
|
currency: "EUR"
|
|
}
|
|
}
|
|
|
|
assert_response :unprocessable_entity
|
|
end
|
|
|
|
test "address update handles validation errors" do
|
|
Property.any_instance.stubs(:update).returns(false)
|
|
|
|
patch update_address_property_path(@account), params: {
|
|
property: {
|
|
address_attributes: {
|
|
line1: "123 Test St"
|
|
}
|
|
}
|
|
}
|
|
|
|
assert_response :unprocessable_entity
|
|
end
|
|
|
|
test "address update activates draft account" do
|
|
# Create a draft property account
|
|
draft_account = Account.create!(
|
|
family: @user.family,
|
|
name: "Draft Property",
|
|
accountable: Property.new,
|
|
status: "draft",
|
|
balance: 500000,
|
|
currency: "USD"
|
|
)
|
|
|
|
assert draft_account.draft?
|
|
|
|
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 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
|