Skip to content

Audit balance administration - #90

Merged
CaYatur merged 2 commits into
mainfrom
fix/audit-balance
Jul 28, 2026
Merged

Audit balance administration#90
CaYatur merged 2 commits into
mainfrom
fix/audit-balance

Conversation

@CaYatur

@CaYatur CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Closes #68.

A player spending currency was audited as purchase. An admin creating
currency out of nothing was recorded only in the economy ledger — so the
higher-privilege action of the two was the one missing from the global trail,
and an operator reviewing the audit log for suspicious admin activity would not
see balance administration at all.

action: 'balance.set' appeared in the codebase only inside smoke fixtures,
which is exactly what made it look covered when I documented it in #22.

Where the write goes, and why

Inside addBalance/setBalance, not at the call sites the issue suggested.

It is the only place that knows what happened. addBalance clamps at zero,
so an admin asking to remove 500 from a balance of 300 removes 300. An entry
recording the requested amount would be a false record, and a false audit
entry is worse than a missing one. detail carries the applied delta and the
resulting balance: -300 -> 0 (correction).

There are three callers now, not two. Desktop IPC, the web panel, and — since
#85 — an API key. Three call sites writing their own entry is three chances to
drift.

Source is passed by the caller so the three stay distinguishable: panel for
the desktop, webpanel for a session, api for a key. "Which integration
created a million coins" is the question that gets asked later.

Refusals

Recorded with ok: false and detail: 'invalid-mcname'. An admin action aimed
at something that is not a valid Minecraft username is either a typo or somebody
probing, and both are worth being able to find.

What did not change

The ledger keeps its own copy. The two answer different questions — the ledger is
per-server balance history that renders without a join, the audit trail is the
global record of privileged actions — and #68 was a discoverability gap, not
an attribution one. No data moved.

Docs

docs/audit-trail.md had a "Known gap" section for this; it is now the
documented behaviour, including why the delta is the applied one. The action
table also picks up the apikey.* and api.* entries added in #85, which it
was silently missing.

Verification

MSMS_SMOKE_WEB, MSMS_SMOKE, MSMS_SMOKE_AUDIT — all exit 0.

Asserted:

  • all three kinds (balance.grant / .remove / .set) present, with actor and
    serverId
  • source is webpanel from a session, panel from the desktop, api from a key
    (and the key entry's actor starts with key:)
  • a refused change recorded as a failure
  • the recorded delta is the applied one: granting a balance of 300 then
    removing 500 audits -300 -> 0, not -500

Closes #68.

A player spending currency was audited as `purchase`. An admin creating
currency out of nothing was recorded only in the economy ledger — so the
higher-privilege action of the two was the one missing from the global trail,
and an operator reviewing the audit log for suspicious admin activity would not
see balance administration at all.

The entry is written inside addBalance/setBalance rather than at each call site.
Two reasons. It is the only place that knows what actually happened: addBalance
clamps at zero, so an admin asking to remove 500 from a balance of 300 removes
300, and an entry claiming 500 would be a false record. And there are three
callers now — desktop IPC, the web panel, and an API key since #85 — which is
three chances to drift if each writes its own.

Source is passed by the caller so the three stay distinguishable: `panel` for
the desktop, `webpanel` for a session, `api` for a key. "Which integration
created a million coins" is the question that gets asked later.

Refusals are recorded with ok:false. An admin action aimed at something that is
not a valid Minecraft username is either a typo or somebody probing, and both
are worth being able to find.

The ledger keeps its own copy and was not replaced — the two answer different
questions, and #68 was a discoverability gap, not an attribution one.

docs/audit-trail.md described this as a known gap; that section is now the
documented behaviour, and the action table also picks up the apikey.* and api.*
entries added in #85, which it was missing.

Verified with MSMS_SMOKE_WEB: all three kinds present and attributed, server id
and source correct per surface, a refusal recorded as a failure, an API key
distinguishable from a human session, and the recorded delta being the applied
one (-300 -> 0) rather than the requested -500. MSMS_SMOKE and MSMS_SMOKE_AUDIT
also green.
Copilot AI review requested due to automatic review settings July 28, 2026 06:41
@CaYatur CaYatur added bug Something isn't working area:audit Audit trail labels Jul 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…forgettable

Two things the review turned up, both the same shape as #68 itself.

**A purchase made from the admin panel was not audited.** The public site's buy
route records `purchase`; `/api/servers/:id/store/buy` did not. So whether the
same action appeared in the trail depended on which page it was made from -
which is exactly the inconsistency #68 exists to close, one route over. Actor is
the panel account that clicked Buy, with the Minecraft name it delivered to in
the detail, because those are not always the same person's identity.

**The audit source had a default, which is a mis-attribution waiting to
happen.** `source: AuditSource = 'panel'` meant any future caller that forgot
the argument would be recorded as the local desktop operator. A missing audit
entry is a gap; a confidently wrong one is a lie, and the second is worse.

Making it required exposed that it could not simply be un-defaulted - a required
parameter cannot follow the optional `by`/`reason`/`category`. So the tail
becomes one `BalanceChange` object with `by` and `source` both required and
named. Those are the two fields an audit entry cannot be reconstructed without,
and now the type will not let a caller omit either. Twelve call sites updated;
the compiler found all of them, which is the point.

All twelve smoke gates pass.
@CaYatur

CaYatur commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Self review

Two findings, both the same shape as #68 itself — an economy action audited on
one surface and not another. Fixed in d83bcb9.

1. A purchase from the admin panel was not audited

The public site records purchase:

// handlePublic, sub === 'store/buy'
audit.record({ source: 'public', action: 'purchase', actor: player.mcName,})

/api/servers/:id/store/buy had no such call. So the same action — a player
spending currency — appeared in the trail or didn't, depending on which page it
was made from. That is precisely the inconsistency this issue exists to close,
one route over, and I would have shipped it.

Actor is the panel account that clicked Buy, with the Minecraft name it was
delivered to in the detail (to Auditee). Those are not always the same
person's identity: a web account can be linked to any mcName an owner sets.

2. The audit source had a default

source: AuditSource = 'panel'

Any future caller that forgets the argument gets recorded as the local desktop
operator
. A missing audit entry is a gap you can notice; a confidently wrong
one is a lie you cannot, and it points at the one actor who by definition never
needed to authenticate.

Un-defaulting it did not compile — a required parameter cannot follow the
optional by/reason/category. Rather than reorder positionals, the tail is
now one object:

export interface BalanceChange {
  by: string          // required
  source: AuditSource // required
  reason?: string
  category?: string
}

by and source are the two fields an audit entry cannot be reconstructed
without, so the type no longer permits omitting either. Twelve call sites
updated; the compiler found all twelve, which is the argument for doing it this
way rather than adding a runtime check.

Verified the new assertion can fail

Both previous PRs shipped a test that could not fail, so I checked this one
rather than assuming. The balance assertions query for
balance.grant/.remove/.set — actions that did not exist anywhere in the
codebase before this branch, so on main they return an empty set and every one
of those checks fails. The panel-purchase assertion looks for
source === 'webpanel' on a purchase entry, which likewise did not exist.

Reviewed and deliberately left alone

  • audit.record inside economy.ts rather than at the call sites. It is the
    only place that knows the applied delta, and there are three callers. The
    alternative — returning the delta so callers can record it — puts the same
    decision in three places and changes the IPC return shape for no gain.
  • record() runs before initAudit() in principle, since initEconomy() is
    called first in index.ts. Checked: record appends directly and wraps the
    write in try/catch, so ordering is not load-bearing. Balance changes only
    happen at runtime anyway.
  • Audit volume. A store paying out on every event could write a lot of
    entries. The trail is already pruned by age and count, and a payout is exactly
    the kind of privileged action the trail is for.
  • kind is computed before MC_NAME validation so a refusal can be labelled
    grant vs remove. Math.floor(NaN) < 0 is false, so a garbage amount is
    recorded as a refused grant — accurate enough for a rejected call.

@CaYatur
CaYatur merged commit 87f688c into main Jul 28, 2026
1 check passed
@CaYatur
CaYatur deleted the fix/audit-balance branch July 28, 2026 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:audit Audit trail bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Audit: balance administration is not recorded in the audit trail

2 participants