Bug: API valuations create/show/update bypass per-account share permissions

[galuis116]

Before you start (required)

General checklist

• I have removed personal / sensitive data from screenshots and logs
• I have searched existing issues and discussions to ensure this is not a duplicate issue

How are you using Sure?

• I was a paying Maybe customer (hosted version)
• I use it on PikaPod, Umbrel or similar (VPS included)
• I am a self-hosted user (local only)

Self hoster checklist

• Self hosted app commit SHA (find in user menu): 89f42497
  • I have confirmed that my app's commit is the latest version of Sure
• Where are you hosting?
  • Render
  • Docker Compose
  • Umbrel
  • PikaPod
  • Other (please specify)

---

Bug description

Api::V1::ValuationsController does not scope create, show, or update by per-account share permissions, so a non-owner family member with a valid API key can write reconciliation valuations to accounts that are unshared with them (or shared read_only), and can fetch / mutate any valuation belonging to the family regardless of whether the underlying account is shared with them. The index action already filters through Account.accessible_by(current_resource_owner), so the rest of the resource has a clear precedent — only these three actions skipped it.

Writing a reconciliation valuation on an account a user cannot see in the UI silently rewrites that account's historical balance series (a reconciliation rewrites the balance on the given date and propagates), so this is not just an information-disclosure issue — it is a cross-share data-mutation issue inside a multi-user family.

The bypass paths are:

• create (app/controllers/api/v1/valuations_controller.rb:86)

  account = current_resource_owner.family.accounts.find(valuation_account_id)

  scopes only by family, not by writable_by(current_resource_owner).

• set_valuation (used by show and update, app/controllers/api/v1/valuations_controller.rb:234-238)

  @entry = current_resource_owner.family
             .entries
             .where(entryable_type: "Valuation")
             .find(params[:id])

  scopes only by family, not by the entry's account-level access.

• update then calls @entry.account.update_reconciliation(...) (valuations_controller.rb:170-174) without re-checking that @entry.account is in Account.writable_by(current_resource_owner), so a member who has been granted only read_only access to an account can still mutate its valuations through the API.

The canonical pattern is already established in the same controller's index (line 15) and in the sister Api::V1::TransactionsController (create at line 93 uses family.accounts.writable_by(current_resource_owner).find(...); set_transaction merges Account.accessible_by(current_resource_owner)).

To Reproduce

Set up: any Sure family with two users — an admin who owns an account and a member who has not been granted access (or only read_only access) to that account. The fixtures already model this exactly: family_admin (Bob) owns every account in dylan_family; family_member (Jakob) is shared only depository (full_control) and credit_card (read_only). The other_asset, investment, loan, property, and connected accounts are unshared with Jakob.

Steps to reproduce (using bin/rails console against the seeded fixtures, or any equivalent real-world setup):

1. As the family member (Jakob), create a read_write API key via the UI or ApiKey.create!.
2. Pick an account that the admin owns but has not shared with the member — e.g. accounts(:other_asset). Note its id.
3. Call:

   POST /api/v1/valuations
   X-Api-Key: <member's read_write key>
   Content-Type: application/json
   
   { "valuation": { "account_id": "<other_asset.id>", "amount": 12345.67, "date": "2026-05-24" } }
   

4. The API returns 201 Created and writes a new reconciliation valuation against the admin's unshared account. The admin's historical balance series for that account is now altered on 2026-05-24.

Additional bypasses with the same setup:

5. Call GET /api/v1/valuations/<id> for any valuation id in the family (including on the unshared other_asset). The API returns the valuation regardless of whether the account is shared with the caller.
6. Call PATCH /api/v1/valuations/<id> for a valuation on credit_card (which is shared with Jakob read_only). The API mutates the valuation, even though the member's permission on that account is read-only.

Equivalent Minitest reproduction (against the existing fixtures):

test "create writes a reconciliation on an unshared account" do
  member = users(:family_member)
  member.api_keys.active.destroy_all
  key = ApiKey.create!(
    user: member, name: "Member RW",
    scopes: [ "read_write" ], source: "web",
    display_key: "test_member_#{SecureRandom.hex(8)}"
  )
  unshared_account = accounts(:other_asset)  # owned by family_admin, NOT shared with Jakob

  before_count = unshared_account.entries.valuations.count
  post api_v1_valuations_url,
       params: { valuation: {
         account_id: unshared_account.id,
         amount: 12_345.67,
         date: Date.current.to_s
       } },
       headers: api_headers(key)

  # Current (buggy) behavior: 201 Created, count goes up by 1.
  # Expected behavior: 404 Not Found, count unchanged.
  assert_equal before_count, unshared_account.entries.valuations.count
  assert_response :not_found
end

Expected behavior

All three actions should match the precedent set by index and by TransactionsController:

• create should resolve the account through current_resource_owner.family.accounts.writable_by(current_resource_owner).find(...). An unshared or read-only account should produce ActiveRecord::RecordNotFound, surfaced as the existing 404 response — no new error code, no information disclosure about whether the account exists in the family.

• set_valuation should resolve the entry through entries whose account_id is in Account.accessible_by(current_resource_owner). A valuation on an unshared account should produce 404.

• update should additionally verify that @entry.account is in Account.writable_by(current_resource_owner) before invoking update_reconciliation / mutating notes. A valuation on a read_only-shared account should be readable via show but not mutable via update (currently it is — that is also the bug).

Screenshots and/or recordings

Not applicable — this is an API-only bug; the bypass paths are reachable through any HTTP client (curl, Postman) with a valid X-Api-Key header. No UI reproduction is possible because the Web UI correctly hides unshared accounts; the bug is exclusively in the JSON API surface added by the recent OpenAPI / rswag expansion (docs/api/openapi.yaml).

[dosubot]

This is a valid bug — I can confirm the permission bypass exists by looking at the source. The create action in Api::V1::ValuationsController uses a family-only scope [1] while the equivalent TransactionsController#create already uses the correct writable_by pattern [2]. The set_valuation method similarly only scopes by family [3], skipping per-account access checks entirely.

The fix is straightforward — align with the existing patterns in the same codebase:

create (line 86) — change:

account = current_resource_owner.family.accounts.find(valuation_account_id)

to:

account = current_resource_owner.family.accounts.writable_by(current_resource_owner).find(valuation_account_id)

set_valuation (lines 234–238) — scope entries to accessible accounts:

accessible_account_ids = current_resource_owner.family.accounts.accessible_by(current_resource_owner).select(:id)

@entry = current_resource_owner.family
           .entries
           .where(entryable_type: "Valuation")
           .where(account_id: accessible_account_ids)
           .find(params[:id])

update — additionally gate mutation behind writable_by:

unless current_resource_owner.family.accounts.writable_by(current_resource_owner).exists?(id: @entry.account_id)
  return render json: { error: "not_found" }, status: :not_found
end

The Account model already provides both accessible_by and writable_by scopes [4], so no new model-level work is needed. The writable_by scope correctly permits accounts the user owns OR has been shared with full_control permission, and accessible_by includes read_only shares as well. These were introduced as part of the family sharing feature [5].

Using find with the tighter scope will naturally raise ActiveRecord::RecordNotFound, which the existing rescue_from handler already maps to a 404 — so no information is disclosed about whether the account exists.

To reply, just mention @dosu.

---

Share context across your team and agents. Try Dosu.