ledger: StoreTransaction posts legs onto accounts belonging to another book

[Yaraslaut]

What happens

StoreTransaction looks each leg's account up by id alone, with no filter on the ledger the action names:

// examples/ledger/src/models/ledger_model.cpp:694-701
for (const auto& leg : action.legs) {
    auto rows = mapper.Query<db::AccountRecord>()
                    .Where(::Lightweight::FieldNameOf<&db::AccountRecord::id>, "=", *leg.accountId)
                    .All();
    if (rows.empty()) {
        throw NotFound{"StoreTransaction: no such account"};
    }
    legAccounts.push_back(rows.front());
}

Account ids are a table-wide autoincrement, so another book's account id is a perfectly well-formed number naming a real row. The entry is accepted, the journal is filed under the ledger the action named, and the foreign account's balance moves.

Reproduction

Against a real ladder_ledger_server (two seeded ledgers rows):

OpenAccount ledgerId=1 name="B1 wallet" kind=0 currency=1   -> id 1
OpenAccount ledgerId=2 name="B2 spend"  kind=1 currency=1   -> id 2

StoreTransaction ledgerId=1 legs=[{accountId:1, -1.00}, {accountId:2, +1.00}]
  -> ok  {"accounts":[{"id":1,...,"balance":{"num":-100,...}}]}

GetLedger ledgerId=1 -> [{"id":1, balance -100}]           # no mention of account 2
GetLedger ledgerId=2 -> [{"id":2, balance  100}]           # moved, with no journal here

Book one's reply does not mention the account it just moved, because every read is scoped to its ledger. Book two shows a balance change with no journal of its own to explain it. The two books disagree and neither report says so.

A leg naming an id that exists in no book at all is refused (StoreTransaction: no such account), which is what shows the lookup runs — it simply does not ask which book the row is in.

Why this reads as an inconsistency rather than a policy

UndoTransaction does make the check, at ledger_model.cpp:909:

throw NotFound{"UndoTransaction: journal does not belong to this ledger"};

so the scoping is thought about elsewhere in the same file. Nothing in execute(StoreTransaction) or in transaction_dto.hpp records a decision to omit it.

Same shape, second site

ImportLedgerChunk looks its row account up the same way (ledger_model.cpp:1074-1079), so a chunk whose counterAccountId or whose CSV account_id column names another book's account posts there too.

Where it is pinned

scripts/scenario/scenarios/ledger/two-books-are-isolated.scenario asserts the current behaviour, with a comment naming it, so a fix has to come through that file and change it deliberately.

Suggested fix

Add .Where(FieldNameOf<&db::AccountRecord::ledger>, "=", *action.ledgerId) to both lookups, with a distinct message ("StoreTransaction: account does not belong to this ledger") so it is not confused with the not-found path.

Found by the out-of-process scenario corpus; the in-process tests use one book.

[Yaraslaut]

Triage: valid — real, and the "inconsistency not policy" reading is right

Verified

• execute(StoreTransaction) looks each leg's account up by id alone (ledger_model.cpp:694-701) — no ledger predicate.
• ImportLedgerChunk's row-account lookup has the same shape (:1074-1079).
• UndoTransaction does scope, against the resolved journal row: throw NotFound{"UndoTransaction: journal does not belong to this ledger"} (:908-910).
• All 15 ledger scenarios pass against a live ladder_ledger_server, including two-books-are-isolated.scenario, so the pin is accurate and a fix has to come through it.

The question I checked before agreeing: is this a privilege boundary?

It matters for how it gets fixed, and the answer is no. ledger has no per-ledger ownership or role table — LedgerAuthorizer (auth/ledger_authorizer.hpp:95) extends SigningAuthorizer and gates on a valid token, nothing more, and a-second-editor-on-the-same-book.scenario pins two principals working the same book by design. So the correct refusal here is a scoping message, not unauthorized, and this is a data-integrity defect rather than a cross-tenant security hole. The issue's own "it discloses nothing a caller could not already read" is right, and the reason it is still worth fixing — two books that disagree with no journal to explain it — stands on its own.

Nothing in transaction_dto.hpp or at either call site records a decision to omit the check, and the same file makes it elsewhere, so this is not an undocumented deliberate cut.

One addition to the suggested fix: ordering

.Where(FieldNameOf<&db::AccountRecord::ledger>, "=", *action.ledgerId) is the right predicate, but note that StoreTransaction resolves the ledger row itself at :731-735 ("StoreTransaction: no such ledger") — which is after the leg loop at :694-701. Filtering on the raw *action.ledgerId at :694 would make a call naming a nonexistent ledger report "account does not belong to this ledger" instead of "no such ledger", which is a worse message for the more common mistake. Either hoist the ledger resolution above the leg loop and filter on the resolved row (the shape DeleteRule and UndoTransaction already use), or keep the not-found check first. ImportLedgerChunk resolves its ledger at :1034, so check the same ordering there.

Labels: triage: valid, bug, area: ladder.