fix(family-sharing): scope form + import account lists to accessible_accounts (#1803)

[Rene0422]

Summary

Closes part of #1803 (the UI leak half).

Non-admin family members could see ids, currencies, and names of accounts the family admin owns but has not shared with them. The account detail routes already enforced access (unshared accounts 404), but several form-render paths still embedded the full Current.family.accounts list in the page.

• app/views/transfers/_form.html.erb:1 — JSON map of every family account id + currency for the JS currency-difference check.
• app/views/transactions/_form.html.erb:3 — same pattern.
• app/views/import/uploads/show.html.erb:59, 115, 147 — QIF and CSV upload account dropdowns. The matching Import::UploadsController#update already used accessible_accounts.find(...), so this was a pure UI leak.
• app/controllers/transactions_controller.rb:374 (exchange_rate) — found by family scope instead of user scope. The action isn't currently routed (the live endpoint is ExchangeRatesController#show), but tightened for defense in depth.

All four switch to the existing accessible_accounts helper, which delegates to Current.user.accessible_accounts (owned + shared via AccountShare).

Out of scope

The second half of #1803 — integration tokens (Plaid / EnableBanking / SimpleFIN / Lunchflow *_items) being scoped to family_id instead of user_id, so members overwrite each other's sessions — needs a schema change, backfill, and controller/view rework. Tracking it as a follow-up rather than mixing it into this PR.

Provider-item controllers

The audit also found Current.family.accounts.find(params[:account_id]) in many *_items_controller.rb files (plaid, simplefin, enable_banking, snaptrade, ibkr, sophtron, indexa_capital, mercury, lunchflow, brex, coinbase, binance, kraken). Every one of those controllers has before_action :require_admin!, so they're not reachable by the non-admin member described in the bug. Tightening them is a separate hardening pass.

Test plan

• bin/rails test test/controllers/transfers_controller_test.rb — new test "new only exposes accounts accessible to the signed-in user" passes.
• bin/rails test test/controllers/transactions_controller_test.rb — new test "new only exposes account ids accessible to the signed-in user" passes.
• bin/rails test test/controllers/import/uploads_controller_test.rb — new test "csv upload dropdown only lists accounts accessible to the signed-in user" passes.
• Full suite: bin/rails test
• bin/rubocop -f github -a
• bundle exec erb_lint ./app/**/*.erb -a
• bin/brakeman --no-pager
• Manual: sign in as a non-admin family member; open new transfer, new transaction, and a CSV import upload page; confirm the dropdowns and inline JSON only contain shared accounts.

Summary by CodeRabbit

• Bug Fixes

  • Forms, import upload flows, and exchange-rate lookups now limit account lists to accounts the signed-in user can access, preventing exposure of unpermitted accounts.

• Tests

  • Added integration tests verifying transaction, transfer, and import upload pages only show accounts accessible to the signed-in user.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR scopes account lists to the current user's accessible_accounts in the exchange-rate action and multiple views (import uploads, transaction form, transfer form), and adds integration tests to assert inaccessible accounts are not rendered.

Changes

Per-user Account Access Control

Layer / File(s)	Summary
Exchange rate controller access control app/controllers/transactions_controller.rb	exchange_rate now finds the target account via Current.user.accessible_accounts.find(params[:account_id]) instead of Current.family.accounts.find().
Import upload form account filtering app/views/import/uploads/show.html.erb, test/controllers/import/uploads_controller_test.rb	QIF, CSV upload, and CSV paste account selects now populate from accessible_accounts.visible...; integration test asserts CSV dropdown only lists accessible accounts.
Transaction form account/currency mapping app/views/transactions/_form.html.erb, test/controllers/transactions_controller_test.rb	account_currencies is built from accessible_accounts.pluck(:id, :currency) instead of Current.family.accounts; test verifies unshared account IDs are not present in the rendered new-transaction form.
Transfer form account/currency mapping app/views/transfers/_form.html.erb, test/controllers/transfers_controller_test.rb	Transfer form's transfer-form controller data is populated from accessible_accounts; test asserts only accessible account IDs are rendered on the new-transfer page.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• [Bug/Architecture] Family Sharing: Unshared accounts visible in UI (404) & API Tokens scoped to Family prevent individual syncing #1803: Addresses the same access-control objective of replacing family-wide account lists with per-user accessible accounts to prevent UI leaks.

Possibly related PRs

• we-promise/sure#1460: Related work that applies authorization scoping to transaction search and accessible_account_ids; both PRs address account-scoping consistency.

Suggested labels

codex, aardvark

Suggested reviewers

• jjmata
• sokie

Poem

🐰 I hopped through dropdowns, neat and spry,
I hid the accounts a user can't spy.
From rates to imports and forms I trod,
I baked small tests to guard the odds.
Hooray — no leaks, the UI stays shy.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(family-sharing): scope form + import account lists to accessible_accounts' directly describes the main change: scoping account lists to accessible_accounts across multiple forms and imports, which is the primary objective.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/controllers/transactions_controller_test.rb`:
- Around line 392-393: The assertions call refute_includes on response.body (a
String) with integer needles (accounts(:investment).id and accounts(:loan).id),
which can raise TypeError; change the needles to strings (e.g.
accounts(:investment).id.to_s and accounts(:loan).id.to_s or
"#{accounts(:investment).id}") so refute_includes response.body, <id>.to_s is
used for both checks.

In `@test/controllers/transfers_controller_test.rb`:
- Around line 21-23: The assertions are comparing integers to response.body (a
String), causing a TypeError; update the three assertions that reference
accounts(:depository).id, accounts(:investment).id, and accounts(:loan).id so
they compare string values (e.g., call to_s or use string interpolation) when
calling assert_includes and refute_includes against response.body to ensure the
types match.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 68134a64-b75f-4685-8a53-48edf60e1e4d

📥 Commits

Reviewing files that changed from the base of the PR and between d8a12ad and b57feb6.

📒 Files selected for processing (7)

• app/controllers/transactions_controller.rb
• app/views/import/uploads/show.html.erb
• app/views/transactions/_form.html.erb
• app/views/transfers/_form.html.erb
• test/controllers/import/uploads_controller_test.rb
• test/controllers/transactions_controller_test.rb
• test/controllers/transfers_controller_test.rb

[coderabbitai]

🧹 Nitpick comments (1)

test/controllers/transactions_controller_test.rb (1)

383-394: 💤 Low value

Consider adding a positive assertion for accessible accounts.

The test verifies that inaccessible accounts are not exposed, but does not confirm that at least one accessible account IS included. The transfers test includes assert_includes response.body, accounts(:depository).id.to_s for this purpose.

✅ Optional enhancement

   assert_response :success
+  assert_includes response.body, accounts(:depository).id.to_s
   refute_includes response.body, accounts(:investment).id.to_s
   refute_includes response.body, accounts(:loan).id.to_s

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/controllers/transactions_controller_test.rb` around lines 383 - 394, In
the test "new only exposes account ids accessible to the signed-in user" add a
positive assertion that at least one permitted account is present in the
response (e.g. assert_includes response.body, accounts(:depository).id.to_s) so
the test both rejects inaccessible accounts and confirms accessible ones are
included; you can also add assert_includes for accounts(:credit_card).id.to_s if
you want to assert multiple visible accounts.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/controllers/transactions_controller_test.rb`:
- Around line 383-394: In the test "new only exposes account ids accessible to
the signed-in user" add a positive assertion that at least one permitted account
is present in the response (e.g. assert_includes response.body,
accounts(:depository).id.to_s) so the test both rejects inaccessible accounts
and confirms accessible ones are included; you can also add assert_includes for
accounts(:credit_card).id.to_s if you want to assert multiple visible accounts.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a6cec951-e711-47e2-ad77-739a30a80238

📥 Commits

Reviewing files that changed from the base of the PR and between 6e328ff and 2f768ce.

📒 Files selected for processing (2)

• test/controllers/transactions_controller_test.rb
• test/controllers/transfers_controller_test.rb

[jjmata]

Good security fix — targeted and well-tested.

A couple of observations:

Efficiency bonus: The switch from Current.family.accounts.map { |a| [a.id, a.currency] }.to_h to accessible_accounts.pluck(:id, :currency).to_h in the transfer/transaction forms avoids instantiating Account objects altogether — a nice side effect of the access-scoping change.

exchange_rate action: The PR description notes this action is not currently routed (the live endpoint is ExchangeRatesController#show). Tightening it anyway is the right call for defense in depth — if it ever gets re-routed, the fix is already there.

Test pattern: The refute_includes response.body, accounts(:investment).id.to_s approach is a practical way to verify the inline JSON map doesn't leak account IDs. One thing to be aware of: UUIDs are long enough that false-positive collisions are effectively impossible, but if the fixture ever has a very short integer-style id this could theoretically match noise in the page. With UUIDs (as this app uses) it's a non-issue.

accessible_accounts in views: The helper appears to be defined on ApplicationController and exposed to views. Worth a quick sanity check that it returns a chainable ActiveRecord::Relation so the .visible.alphabetically.pluck(...) calls in the import upload view work as expected — they should, given the method signature, but it's the one behavioural thing the new tests don't directly exercise.

---

Generated by Claude Code