bank: check a caller-supplied owner against the session principal

[Yaraslaut]

Fixes #471.

bank::resolveOwner() returned action.owner whenever it was non-empty and consulted the session principal only when it was not. Nothing compared the two, so ten actions across eight models served a signed-in customer another customer's data for the price of typing their username — and two of them wrote to it.

The resolution I chose, and why

The issue offered two, and asked for one to be picked outright.

Option 2 — keep the field, verify it. resolveOwner now returns the session principal always, and throws Unauthorized{"owner does not match the session principal"} when a non-empty action.owner names anybody else.

[[nodiscard]] inline std::string resolveOwner(const std::string& explicitOwner) {
    std::string principal = sessionPrincipal();
    if (!explicitOwner.empty() && explicitOwner != principal) {
        throw Unauthorized{"owner does not match the session principal"};
    }
    return principal;
}

Three reasons for verifying rather than ignoring:

1. owner is load-bearing on the wire. It is CustomerModel's bridge routing key — BRIDGE_MODEL_KEY(CustomerModel, ListAccounts, &ListAccounts::owner) and BRIDGE_KEY_FROM(OpenAccount, &OpenAccount::owner). Under option 1 a request naming another customer would still be routed to that customer's model instance and then quietly handed the caller's own rows: a wrong answer where a refusal belongs. The issue warned specifically against touching this surface without checking it.
2. OpenAccount is a create. Silently redirecting the ownership of a newly created row is worse than refusing it.
3. It matches the in-tree pattern the issue named. db::loadOwned throws Unauthorized on a mismatch rather than substituting; the ten owner-named actions now behave the same way as the id-addressed half of the very same models.

I checked the callers the issue told me to check before assuming option 1 was safe: bank's Qt GUI (gui/controllers/*.cpp) constructs ListAccounts{} and OpenAccount{.kind = …} with owner left empty at every call site, and src/cli/main.cpp never mentions owner at all. So both options were behaviourally safe for the shipped clients — it is the wire surface and the create that decide it.

One central change fixes both trees: principal.hpp is shared between the native models and the WASM shadow models under gui_wasm/ (which resolve bank/core/* to the same headers), so the six resolveOwner call sites in the WASM build are covered too.

The two decisions the issue asked to be recorded

Empty session → refuse. It already did, and that is preserved exactly: resolveOwner returns the empty principal and each call site's existing owner.empty() guard throws with a message naming what it was about to do ("no session principal to own the account", "no session principal to list accounts for", "no session principal"). Keeping the refusal at the call sites rather than centralising it preserves those distinct messages, which the corpus pins. What changes is the combination that used to slip through: an anonymous caller naming a real customer is now a mismatch, and is refused rather than served. That case has new coverage in banking-without-a-session-is-refused.scenario.

SpendingByKind → scoped. Verified: it carried no owner field at all, so resolveOwner never saw it, and it reported on any account in the database including for a caller with no session. It now goes through db::loadOwned<db::AccountRecord>(…, sessionPrincipal(), "account") — the same guard every other id-addressed action uses. I treated this as in scope rather than as a note: both scenarios pinned it citing morph#471 as the reason, so leaving it would have left two files pointing at a closed issue as though it were open.

Also verified: id-addressed actions were indeed unaffected, and db::loadOwned is the working in-tree model this fix copies rather than inventing a shape.

A side effect worth naming: the comparison happens before the users lookup, so an unregistered owner name and a registered one are now answered identically. The old ordering resolved the name first and reported unknown user: <name>, which told an unauthorized caller whether a username existed.

The tests were already there — they are flipped, not written

PR #470's corpus pinned the permissive behaviour as expect ok deliberately. This flips those assertions and rewrites the header comments that explained why a passing assertion recorded a defect; no comment is left pointing at #471 as though it were still open.

• an-owner-named-outright-is-not-checked-against-the-session.scenario → renamed ...-is-checked-against-the-session.scenario, and grown by the two actions its inventory was missing: ListCards and OpenAccount. All ten owner-resolving actions are now exercised.
• For the two writes, an error return is not the assertion:
  • MarkAllRead is followed by a read from the quarry's own session: still exactly one unread notification, still the same message.
  • OpenAccount is attempted with overdraftMinor=987654, a marker no other scenario in the corpus uses, and both customers' account lists are then searched for it — absent from the quarry's, and the snooper's list is empty outright.
• A positive control was added: the same request body that is refused from the wrong session is served from the right one, so the file proves the field is verified rather than merely disabled.
• banking-without-a-session-is-refused.scenario gains the anonymous-caller-names-an-owner case across six actions, and asserts no account was created for the named member (marker overdraft 876543).
• a-statement-covers-every-account-an-owner-has.scenario read a third party's empty statement to pin the empty-statement shape; it now reads it from that owner's own session.

The control: red without the fix, green with it

With the production change reverted (git checkout 45d7d4a6 -- principal.hpp budget_model.cpp), ladder_bank_server rebuilt, and the final flipped scenarios in place, the corpus goes red — three files, exit 1:

$ python3 scripts/scenario/run_scenarios.py --rung bank --build-dir build/gcc-debug
CONTROL EXIT=1
  FAIL a-statement-covers-every-account-an-owner-has.scenario
  FAIL an-owner-named-outright-is-checked-against-the-session.scenario
  FAIL banking-without-a-session-is-refused.scenario
FAILED: bank

The decisive one, in full — the snooper's session served the quarry's account, balance and all:

FAIL step 25 (line 104): do ListAccounts owner=$prey
  expected: @kind == err
  actual:   @kind == ok
  client:   s-vault (modelId=13224947916653032786)
  reply:    ok body={"accounts":[{"id":13,"owner":"quarry","number":"DE70829970312884849099",
                     "kind":0,"currency":0,"balanceMinor":760000,"overdraftMinor":0,
                     "status":0,"interestBps":0}]}
  captures: snoop=snooper, prey=quarry, qAcct=13, ...

FAIL step 44 (line 154): do SpendingByKind accountId=$acct sinceMs=0
  expected: @kind == err
  actual:   @kind == ok
  reply:    ok body={"accountId":16,"totalDebitsMinor":0,"byKind":[]}

FAIL step 21 (line 115): do GenerateStatement owner=perlis-no-accounts fromMs=0 toMs=0
  expected: @kind == err
  actual:   @kind == ok
  reply:    ok body={"owner":"perlis-no-accounts","fromMs":0,"toMs":0,"lines":[],...}

Restore the fix, rebuild, and it is green — twice, against the same database:

$ python3 scripts/scenario/run_scenarios.py --rung bank --build-dir build/gcc-debug --twice
GREEN TWICE EXIT=0
bank: second pass, same database
every scenario passed in: bank

Everything else that was run

Check	Result
Whole corpus, six rungs	every scenario passed in: pastebin, bookmarks, polls, kanban, bank, ledger (exit 0)
bank_tests	All tests passed (145 assertions in 21 test cases)
ctest -L ladder -LE stress	100% tests passed out of 1026
ctest -LE stress (full)	1569/1570 — see note below
scenario_coverage.py	exit 0; bank actions 41/41 dispatched (0 exempt), workflows 22/22
test_morph_scenario.py	exit 0
check_spec_citations.sh	Prose lint OK — 746 references, every cited path resolves
check_rung_filters.sh	All 39 rung-filter checks passed
check_catch_test_names.sh, check_test_type_names.sh, check_deprecated_markers.sh, check_journal_stamps.sh, check_ci_clang_pin.sh, check_install_export.sh, check_automoc_includes.sh	all exit 0
clang-format --dry-run --Werror (clang 22.1.8)	clean on both changed C++ files
clang-tidy -p build/gcc-debug (clang 22.1.8)	zero findings in budget_model.cpp and principal.hpp

The one failure, and why it is not this branch's. The full ctest -LE stress run reported one SEGFAULT: "App's fetch timer really does fire under a pumping loop" (ladder-bookmarks). It is a pre-existing flake, not a regression:

• The same test passed in this branch's -L ladder -LE stress run (219/1026), and passed 3/3 on re-run afterwards.
• ladder_bookmarks_tests was never relinked by this change. The incremental build after the fix did 16 steps, all of them bank (bank_lib, bank_cli, ladder_bank_server, bank_tests); the bookmarks binary's mtime predates the edit.

Explicitly out of scope

Bank's server trusting the client's asserted principal at all — its AuthModel mints no token — is documented in examples/bank/README.md and untouched here. This change is narrower: even taking the asserted principal at face value, these ten actions ignored it. The validate()-shadowing and closingBalanceMinor findings in bank's README are likewise separate and left alone; examples/bank/README.md's "defects the corpus pins on purpose" section drops from three entries to those two.

Not verified

The gui_wasm build was not compiled — no Emscripten toolchain here. The change it inherits is the shared resolveOwner plus an #include "bank/core/errors.hpp" that the WASM model translation units already include directly, and <stdexcept> is all errors.hpp pulls in, so the risk is a compile-order one rather than a behavioural one. wasm-demo.yml will cover it. The coverage gates (check_coverage_*.sh) no-op without a clang-coverage build tree and were not run against one.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!