From 65d8129cf297c3c0eff7c52493f65c601d3a7bde Mon Sep 17 00:00:00 2001 From: Guillem Arias Date: Sat, 30 May 2026 09:39:31 +0200 Subject: [PATCH] =?UTF-8?q?fix(retirement):=20review=20fixes=20=E2=80=94?= =?UTF-8?q?=20IDOR,=20adjustment=20cap,=20bucket=20access?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR #2046 review (superagent P1, Codex P2, jjmata): - IDOR (P1): a statement could reference another plan's pension_source via a crafted pension_source_id, leaking the source name + points history. Goal::RetirementStatement now validates the source belongs to the same plan. - Adjustment cap was bypassable: the limit lived only on Goal::Retirement (parent validations don't run on child saves), so the CRUD path allowed an 11th. Goal::RetirementAdjustment now enforces it on create. - Bucket account selection (and the show-page candidate list) now filter through accounts.accessible_by(Current.user), so a private account shared away from the user can't be added via a crafted POST. - Comment clarifying the deliberate update_column in soft_replace!. Tests for the IDOR guard + the child-level cap. --- .../retirement/buckets_controller.rb | 4 +++- app/controllers/retirement_controller.rb | 2 +- app/models/goal/retirement_adjustment.rb | 11 +++++++++++ app/models/goal/retirement_statement.rb | 12 ++++++++++++ config/locales/models/retirement/en.yml | 8 ++++++++ .../models/goal/retirement_adjustment_test.rb | 12 ++++++++++++ test/models/goal/retirement_statement_test.rb | 19 +++++++++++++++++++ 7 files changed, 66 insertions(+), 2 deletions(-) diff --git a/app/controllers/retirement/buckets_controller.rb b/app/controllers/retirement/buckets_controller.rb index 486fb4178..c5bd2b023 100644 --- a/app/controllers/retirement/buckets_controller.rb +++ b/app/controllers/retirement/buckets_controller.rb @@ -4,7 +4,9 @@ class Retirement::BucketsController < ApplicationController # Replace-all: the form submits the full set of selected account ids. def update requested = Array(params.dig(:bucket, :account_ids)).reject(&:blank?) - valid_ids = Current.family.accounts.where(id: requested).pluck(:id) + # accessible_by, not just family-scoped: a private account shared away + # from this user must not be addable to their bucket via a crafted POST. + valid_ids = Current.family.accounts.accessible_by(Current.user).where(id: requested).pluck(:id) @plan.transaction do @plan.retirement_bucket_entries.where.not(account_id: valid_ids).destroy_all diff --git a/app/controllers/retirement_controller.rb b/app/controllers/retirement_controller.rb index c898c52ae..7b682acb6 100644 --- a/app/controllers/retirement_controller.rb +++ b/app/controllers/retirement_controller.rb @@ -6,7 +6,7 @@ class RetirementController < ApplicationController @adjustments = @plan.adjustments.ordered @statements = @plan.statements.chronological.reverse @bucket_account_ids = @plan.retirement_bucket_entries.pluck(:account_id).to_set - @bucket_candidates = Current.family.accounts.visible.alphabetically + @bucket_candidates = Current.family.accounts.visible.accessible_by(Current.user).alphabetically @breadcrumbs = [ [ t("breadcrumbs.home"), root_path ], [ t("breadcrumbs.retirement"), nil ] diff --git a/app/models/goal/retirement_adjustment.rb b/app/models/goal/retirement_adjustment.rb index caf2da561..adbe21078 100644 --- a/app/models/goal/retirement_adjustment.rb +++ b/app/models/goal/retirement_adjustment.rb @@ -15,6 +15,9 @@ class Goal::RetirementAdjustment < ApplicationRecord validates :currency, presence: true validates :label, presence: true, length: { maximum: 255 } validates :ordinal, presence: true, numericality: { only_integer: true } + # The cap also lives on Goal::Retirement, but parent validations don't run + # when a child is saved directly (the CRUD path), so enforce it here too. + validate :within_plan_limit, on: :create scope :ordered, -> { order(:ordinal, :created_at) } @@ -24,4 +27,12 @@ class Goal::RetirementAdjustment < ApplicationRecord return false if age < from_age to_age.nil? || age <= to_age end + + private + def within_plan_limit + return if goal_retirement.nil? + return if goal_retirement.adjustments.where.not(id: id).count < Goal::Retirement::ADJUSTMENTS_LIMIT + + errors.add(:base, :limit_reached, count: Goal::Retirement::ADJUSTMENTS_LIMIT) + end end diff --git a/app/models/goal/retirement_statement.rb b/app/models/goal/retirement_statement.rb index 1babd1a8c..24aaaac5a 100644 --- a/app/models/goal/retirement_statement.rb +++ b/app/models/goal/retirement_statement.rb @@ -9,6 +9,9 @@ class Goal::RetirementStatement < ApplicationRecord validates :received_on, presence: true validates :projected_monthly_amount, presence: true, numericality: { greater_than_or_equal_to: 0 } validates :projected_currency, presence: true + # Prevent IDOR: a statement may only reference a pension source from its + # own plan, even if a crafted request supplies another plan's source id. + validate :pension_source_belongs_to_plan # Append-only audit: soft-deleted rows stay in the table for history but # drop out of every normal read. Edits go through soft_replace!. @@ -37,6 +40,8 @@ class Goal::RetirementStatement < ApplicationRecord def soft_replace!(attrs) new_statement = nil self.class.transaction do + # update_column is deliberate: flip the soft-delete flag without + # re-running validations/callbacks on the archived row. update_column(:deleted, true) new_statement = pension_source.statements.create!( attributes @@ -48,6 +53,13 @@ class Goal::RetirementStatement < ApplicationRecord end private + def pension_source_belongs_to_plan + return if pension_source.nil? || goal_retirement_id.nil? + return if pension_source.goal_retirement_id == goal_retirement_id + + errors.add(:pension_source, :must_belong_to_plan) + end + def monetizable_currency projected_currency end diff --git a/config/locales/models/retirement/en.yml b/config/locales/models/retirement/en.yml index ec9f13d51..688b8f25f 100644 --- a/config/locales/models/retirement/en.yml +++ b/config/locales/models/retirement/en.yml @@ -15,3 +15,11 @@ en: attributes: adjustments: too_many: "can't exceed %{count} per plan." + "goal/retirement_statement": + attributes: + pension_source: + must_belong_to_plan: must belong to this retirement plan. + "goal/retirement_adjustment": + attributes: + base: + limit_reached: "You can't add more than %{count} adjustments." diff --git a/test/models/goal/retirement_adjustment_test.rb b/test/models/goal/retirement_adjustment_test.rb index 4436c2e40..dc7de839e 100644 --- a/test/models/goal/retirement_adjustment_test.rb +++ b/test/models/goal/retirement_adjustment_test.rb @@ -28,4 +28,16 @@ class Goal::RetirementAdjustmentTest < ActiveSupport::TestCase test "amount_today_money uses the adjustment currency" do assert_equal Money.new(-680, "USD"), @adj.amount_today_money end + + test "child create is blocked past the plan adjustments limit" do + plan = goals(:retirement_bob) + (Goal::Retirement::ADJUSTMENTS_LIMIT - plan.adjustments.count).times do |i| + plan.adjustments.create!(label: "fill #{i}", amount_today: -1, currency: "USD", from_age: 60, ordinal: 100 + i) + end + + over_limit = plan.adjustments.new(label: "over", amount_today: -1, currency: "USD", from_age: 61, ordinal: 999) + + assert_not over_limit.valid? + assert over_limit.errors[:base].present? + end end diff --git a/test/models/goal/retirement_statement_test.rb b/test/models/goal/retirement_statement_test.rb index 77717c0fd..96d8bc58e 100644 --- a/test/models/goal/retirement_statement_test.rb +++ b/test/models/goal/retirement_statement_test.rb @@ -34,4 +34,23 @@ class Goal::RetirementStatementTest < ActiveSupport::TestCase test "money uses projected_currency" do assert_equal Money.new(1510, "EUR"), @statement.projected_monthly_amount_money end + + test "rejects a pension source from another plan (IDOR guard)" do + other_plan = Goal::Retirement.create!( + family: families(:dylan_family), owner: users(:family_member), + name: "Other plan", currency: "USD" + ) + other_source = other_plan.pension_sources.create!( + name: "Foreign", kind: "state", country: "DE", pension_system: "de_grv", + tax_treatment: "de_renten", payout_shape: "monthly_for_life", start_age: 67, amount: 1, currency: "EUR" + ) + + statement = goals(:retirement_bob).statements.new( + pension_source: other_source, received_on: Date.current, + projected_monthly_amount: 100, projected_currency: "EUR" + ) + + assert_not statement.valid? + assert_includes statement.errors.attribute_names, :pension_source + end end