Skip to content

ledger: RunReportJob never checks the job belongs to the ledger it names #371

Description

@Yaraslaut

What happens

LedgerModel::execute(const RunReportJob&) carries both a jobId and a
ledgerId, and never checks that the first belongs to the second.

// examples/ledger/src/models/ledger_model.cpp (2a035fe8 + the #367 fix)
auto jobRows = mapper.Query<db::ReportJobRecord>()
                   .Where(::Lightweight::FieldNameOf<&db::ReportJobRecord::id>, "=",
                          static_cast<std::uint64_t>(*action.jobId))
                   .All();
if (jobRows.empty()) {
    throw NotFound{"RunReportJob: no such job"};
}

Further down it does verify that action.ledgerId names an existing ledger
("the ledger guard every sibling action already has", morph#250) — but not that
the job row's own ReportJobRecord::ledger is that ledger. The aggregation then
runs against the action's ledger:

resultJson = computeReportJson(mapper, action.ledgerId, reportPeriod);
...
finishReportJob(mapper, *action.jobId, ReportStatus::Done, std::move(resultJson));

so RunReportJob{jobId: <book two's job>, ledgerId: 1} settles book two's
job row Done carrying book one's totals, and a subsequent
GetReportStatus jobId=<book two's job> hands those back as book two's report.
The job is also terminal afterwards, so the correct body can never be computed
for it.

This is the same shape as morph#367 (an id resolved without the scope the
action names) at a third site. It was found while fixing that issue and is
filed separately because it is a different action, a different consequence
(a report body cross-filed, not a balance moved), and — unlike #367 — not
reachable from a client today.

Verification status

Inferred from reading the code. Not reproduced. Read at
examples/ledger/src/models/ledger_model.cpp:1240-1246 and the surrounding
execute(RunReportJob) body, on branch worktree-agent-a99217a5bca2f10a4
(base commit 2a035fe8, plus the #367 fix, which does not touch this action).

No test was written and no dispatch was performed, because the action cannot be
driven from outside the process: execute(RunReportJob) refuses any principal
but kReportRunnerPrincipal, and LedgerAuthorizer refuses a Login for the
reserved system: namespace. scripts/scenario/scenario_coverage.py records
exactly this as the reason the action is undrivable (see its "Actions recorded
as undrivable" output, and morph#362).

The only dispatcher today pairs the two correctly.
ledger::app::App::runPendingReportsOnce() reads them off the job row in one
statement — SELECT id, ledger_id FROM ledger_report_jobs WHERE status = ?
so no mismatched pair reaches the model in the shipped server. The defect is
therefore latent: it is a missing check, not a live wrong answer, and its
severity today is "the model trusts a caller-supplied pairing that only one
caller constructs".

Suggested fix

The same shape the rest of the file now uses — after the jobRows.empty()
check, compare jobRows.front().ledger.Value() against *action.ledgerId and
throw NotFound{"RunReportJob: job does not belong to this ledger"}. Matches
execute(UndoTransaction)'s journal check and the accountInLedger helper
added for #367.

Note the throw must stay inside the existing try, for the reason that block
already documents: an exception escaping the method leaves the row Pending and
the app re-sweeps it forever.

What would change the verdict

  • Close it if the check is added, or if a comment in
    execute(RunReportJob) records a deliberate decision that the runner is the
    only caller and the pairing is its responsibility.
  • Escalate it if any client-reachable path ever dispatches RunReportJob
    with a caller-chosen ledgerId, or if a second in-process dispatcher appears
    that does not read both values off the same row.

Related, deliberately not folded in

execute(GetReportStatus) takes a jobId and no ledgerId at all, so any
authenticated principal can read any book's report body. That is visible in the
DTO rather than hidden in a query, and this rung has no per-principal book
ownership anywhere (GetLedger accepts any ledgerId from any principal), so
it is not filed here as a defect — noted only so the next reader does not have
to rediscover it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: ladderSubsystem: ladderbugSomething isn't workingtriage: validWell-framed; implement as written

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions