fix(retirement): review fixes — IDOR, adjustment cap, bucket access

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.

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

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 ]

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

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

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

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

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