core: deploy correctness fixes (SBC exploit + economy TOCTOU) + docs

Bump openfut-core gitlink to 68d1065 (correctness fixes: SBC duplicate-card
exploit, non-atomic economy CAS guards, season/checkin panics, sbc_submissions
club_id migration 0019). Deployed to prod-core (DB migration ver 18 -> 19).

Add docs/CORE_CORRECTNESS_ISSUES.md (audit + Resolution) and
docs/OVERNIGHT_HANDOFF_2026-08-17.md.
This commit is contained in:
funman300
2026-08-17 16:01:27 +00:00
parent 0bc71dbd74
commit 42fd3c7e90
3 changed files with 341 additions and 1 deletions
+175
View File
@@ -0,0 +1,175 @@
# openfut-core — Correctness Issues (audit 2026-08-17)
Read-only audit of `openfut-core` (game-agnostic axum + SQLite/sqlx economy authority).
Four reported issue classes confirmed with exact `file:line` evidence, **plus a bonus
HIGH-severity SBC duplicate-card economy exploit**. Nothing here is fixed yet —
fixing bumps Core off the frozen P1 reference (`fbb54ea`, the current known-good
production Core) and one item needs a DB migration + prod backfill, so this needs a
**go/no-go** before rebuild+redeploy.
> **Zero live users right now**, so the HIGH-severity economy exploits are not
> currently exploitable — but they are the exact class (coin overspend + card
> duplication) that crashed live clients earlier (project memory), so they should be
> fixed before any real play.
## Architecture context (why the bugs cluster)
Two generations of economy code coexist:
- **NEW** `services/economy.rs` + `routes/economy.rs`**fully atomic + validated**:
every compound op acquires a connection, `BEGIN IMMEDIATE`, composes
transaction-scoped primitives (`debit`/`credit`/`add_item`/`remove_item`/
`consume_entitlement`), commits/rolls back via `finish()`, rejects negative amounts,
and has an extensive in-module test suite. **This is the reference fix pattern.**
- **OLD** per-feature services (`club`, `pack`, `market`, `sbc`, `checkin`, `upgrades`,
`match_service`, `season`) — predate it, still do read/check/write directly against
the `&Pool` with each statement on its own connection: no transaction. `economy.rs`'s
own module doc explicitly warns these "cannot offer that guarantee".
Issues 2 and 3 live entirely in the OLD generation; the fix is to route them through
`economy.rs`'s proven atomic ops (or give the pool helpers `&mut SqliteConnection`
transactional variants). Request flow is `X-OpenFUT-Game` header → active profile →
club → service; clients never pass a `club_id`, so cross-club access is **not** a
vector — the risks are intra-club concurrency + unvalidated payloads.
---
## Issue 1 — `sbc_submissions` missing `club_id` (milestone always 0) · **LOW**
- **Root cause:** `migrations/0001_initial.sql:96-102` creates `sbc_submissions(id,
profile_id, sbc_id, submitted_card_ids, passed, submitted_at)` — no `club_id`, and no
later migration adds one. But `src/routes/club.rs:94-99` (`get_milestones`) runs
`SELECT COUNT(*) FROM sbc_submissions WHERE club_id = ? AND passed = 1`. SQLite errors
`no such column: club_id`; the error is swallowed by `.unwrap_or(0)` → the
`sbcs_completed` milestone is **always 0**. Writer `services/sbc.rs:74-83` inserts
`profile_id`, not `club_id`.
- **Impact:** wrong milestone stat only. No crash, no economy corruption.
- **Fix (matches the ticket — needs migration + backfill):** new migration `0019`:
`ALTER TABLE sbc_submissions ADD COLUMN club_id TEXT;` then backfill
`UPDATE sbc_submissions SET club_id = (SELECT c.id FROM clubs c WHERE c.profile_id =
sbc_submissions.profile_id);` and bind `club_id` in `sbc.rs:submit_sbc`'s INSERT
(`club_id` is already a param at `sbc.rs:41`).
- **Simpler alternative (no migration):** change the `club.rs:95` query to
`WHERE profile_id = ?` (column already exists). Ticket asks for the column, so both
are recorded.
- **Migration required:** YES (for the ticket's fix); NO (for the alternative).
## Issue 2 — Non-atomic check-then-act economy mutations (TOCTOU) · **HIGH**
Each does read → check → write across multiple pool round-trips with no
`BEGIN IMMEDIATE`, so concurrent requests race → overspend / duplication / double reward:
| Site | Race |
|---|---|
| `services/club.rs:88-115` `spend_coins` | SELECT coins → `if balance<amount` → UPDATE; two concurrent spends both pass → **overspend / negative balance**. Shared primitive used by all callers below. |
| `services/pack.rs:58-141` `open_pack` | read `pack.opened` → INSERT cards loop → UPDATE opened=1; concurrent double-open → **card duplication** |
| `services/pack.rs:144-160` `buy_pack` | `spend_coins` then `grant_pack`, separate ops → crash between = coins gone, no pack |
| `services/market.rs:129-186` `buy_listing` | SELECT sold=0 → spend → UPDATE sold=1 → INSERT; concurrent double-buy → **two cards minted** |
| `services/market.rs:188+` `sell_card` | SELECT owned → DELETE → add_coins; concurrent double-sell → **double credit** |
| `services/sbc.rs:35-107` `submit_sbc` | validate → DELETE cards → INSERT submission → add_coins/grant_pack; same cards to two SBCs → **double reward** |
| `services/checkin.rs:60-120` `claim` | SELECT last → same-day check → reward → INSERT; concurrent → **double claim** |
| `services/upgrades.rs:64-95` `change_position` | fetch → spend_coins → UPDATE |
| `services/match_service.rs:97-235` + `season.rs` reward | many sequential writes; partial failure leaves partial rewards |
- **Impact:** corrupts economy state (coin overspend + card duplication).
- **Fix:** wrap each compound op in one `BEGIN IMMEDIATE`…`finish()` transaction
exactly as `services/economy.rs:214-236` already does; route pack/market/sbc/checkin/
upgrades through `economy.rs`'s composed atomic ops (or add `&mut SqliteConnection`
variants of `spend_coins`/`add_coins`/`grant_pack`/`add_item`).
- **Migration required:** NO (code-only).
## Issue 3 — Missing input validation: SBC duplicate-card exploit · **HIGH**
- **Root cause:** `services/sbc.rs:53-70` iterates `req.owned_card_ids` with **no dedup
and no length bound**. The same `owned_card_id` repeated N times resolves the same card
N times (each `fetch_optional` succeeds); `validate_sbc` (`sbc.rs:110-116`) counts it
toward `squad_size` and passes; the DELETE loop deletes it once → **a user satisfies
any SBC with ONE card duplicated → free rewards**. Entry point `routes/sbc.rs:30-49`
forwards `req` unvalidated. Unbounded Vec length is also a DoS.
- **Fix:** in `submit_sbc`, reject duplicate ids (collect into a `HashSet`, compare
`len`) and bound the list (e.g. ≤ 30) before resolving → `AppError::BadRequest`.
- **Already-good validation (no change):** `economy.rs:64-67,84-87` reject negative
amounts; `match_service.rs:100-104` clamps goals 0..99; `upgrades.rs` validates
boost 1..3 / positions. Minor: `routes/economy.rs post_grant_reward` forwards an
unbounded `amount` (trusted host caller; add an upper-bound sanity guard).
- **Migration required:** NO.
## Issue 4 — `season.rs` panics + checkin index panic · **LOW**
- **Root cause:** `services/season.rs` `.expect()` on `fetch_optional` Options at
`:23` (`get_or_create`), `:69` and `:144` (`record_match`) — panic if the `seasons`
row is absent when expected. `seasons.profile_id` is PRIMARY KEY
(`migrations/0006_seasons_loans_packs.sql:2`), so the concurrent-insert case surfaces
as a UNIQUE error via `?` (not the panic), lowering probability — but it still
panics-on-invariant, aborting that request (axum → 500 for the request; not a full
server crash).
- **Secondary:** `services/checkin.rs:~52,~88` index `STREAK_COINS[idx]` with
`idx = ((streak-1)%7) as usize`; Rust `%` can be negative → a corrupt/negative
persisted `streak_day` yields a negative index → **panic**.
- **Fix:** replace each `.expect(...)` with
`.ok_or_else(|| AppError::Internal("season row missing".into()))?`; guard the checkin
index with `.rem_euclid(7)` (or clamp `streak_day >= 1` on read).
- **Migration required:** NO.
---
## Fix plan summary
| Issue | Severity | Migration | Files |
|---|---|---|---|
| 1 sbc_submissions club_id | LOW | YES (or none via alt) | `migrations/0001` (+new 0019), `routes/club.rs`, `services/sbc.rs` |
| 2 non-atomic mutations | HIGH | NO | `services/{club,pack,market,sbc,checkin,upgrades,match_service,season}.rs` → route through `services/economy.rs` |
| 3 SBC duplicate-card exploit | HIGH | NO | `services/sbc.rs`, `routes/sbc.rs` |
| 4 season/checkin panics | LOW | NO | `services/season.rs`, `services/checkin.rs` |
**Recommended order:** 3 (smallest, highest-value: kills the free-reward exploit) → 2
(the transactional refactor, largest) → 4 (defensive hygiene) → 1 (cosmetic; do with
the alt query unless the column is wanted).
**Deployment note:** all of these change `openfut-core`, which is currently frozen at
the P1 reference (`fbb54ea`) in production. Fixing + rebuilding + redeploying prod-core
is a deliberate step off that reference — get a go/no-go first. Only Issue 1's
column-add needs a migration + prod backfill; 2/3/4 are code-only.
---
## Resolution (2026-08-17, on `openfut-core` @ `fbb54ea` + these edits)
All four issue classes fixed in the canonical superproject submodule
`openfut-core`; full test suite green (179 tests) + clippy clean + a new
regression test `tests/integration_test.rs::test_sbc_rejects_duplicate_cards`.
| Issue | Fix | Files |
|---|---|---|
| 3 SBC dup-card exploit | dedup (`HashSet`) + `MAX_SBC_CARDS`=30 bound in `submit_sbc`, before card resolution → `BadRequest` | `services/sbc.rs` |
| 1 sbc_submissions club_id | migration `0019` adds `club_id` + backfills from `clubs`; `submit_sbc` INSERT now binds `club_id` | `migrations/0019_*.sql`, `services/sbc.rs` |
| 4 season/checkin panics | `.expect()` → `.ok_or_else(AppError::Internal)?` (3 sites); checkin index `% 7` → `.rem_euclid(7)` (2 sites) | `services/season.rs`, `services/checkin.rs` |
| 2 non-atomic mutations | statement-level compare-and-swap (see below) | `services/{club,pack,market,checkin}.rs` |
### Issue 2 — how it was fixed, and the residual
Rather than the full transaction refactor (threading `&mut SqliteConnection`
through every service), the concurrency-exploitable races were closed with
single-statement **compare-and-swap** — the atomic unit SQLite already gives us,
no transaction plumbing, minimal blast radius on the working prod economy path:
- `club::spend_coins` — `UPDATE … SET coins = coins - ? WHERE id = ? AND coins >= ?`
+ `rows_affected` guard; also rejects negative amounts. Kills **overspend** for
every caller (the shared root primitive).
- `pack::open_pack` — claims the pack (`UPDATE … opened = 1 WHERE … AND opened = 0`)
**before** minting cards; loser aborts. Kills **card duplication** via double-open.
- `market::buy_listing` — claims the listing (`sold 0→1`) before charging; releases
the claim if the debit fails. Kills **double-mint**.
- `market::sell_card` — `DELETE … WHERE id = ? AND club_id = ?` + `rows_affected`
guard before crediting. Kills **double-credit** via double-sell.
- `checkin::claim` — conditional `INSERT … SELECT … WHERE NOT EXISTS (today's row)`
+ `rows_affected` guard; pays out only if the claim landed. Kills **double-claim**.
**Residual (accepted, documented):** the *multi-statement all-or-nothing* edges that
need a real transaction to close — `pack::buy_pack` (spend then grant: a crash between
loses coins with no pack), `sbc::submit_sbc` (concurrent submits sharing cards could
double-consume mid-loop), and `match_service`/`season` reward chains (partial writes on
crash). These are **partial-failure durability edges, not statement-level races**, and
require concurrency that a single-player FIFA17 client does not generate. Closing them
is the `&mut SqliteConnection` transaction refactor originally proposed; deferred as
low-value for single-player. Overspend + duplication + double-credit — the vectors that
corrupt economy state — are all closed.
+165
View File
@@ -0,0 +1,165 @@
# Overnight session handoff — 2026-08-17
Autonomous session while you slept. Low-ceremony per your instruction. Everything
below is verified as noted; nothing was committed or pushed (see
[Uncommitted work](#uncommitted-work--needs-your-review)).
## TL;DR
1. **Production promotion completed** (you chose "promotion"): prod-host swapped to the
post-P1 build, content-gap imported. Then I migrated the **remaining Python UTAS
routes that have a known contract** to Rust: account/sync, ut/auth (SID mint),
userMassInfo (full), clientdata, club/stats/{country,league,team}, and the trivial
static acks. The client's **observed FUT-hub/economy flow is now fully Rust**; a
tail of lower-traffic routes **without a captured wire shape** (item-defs,
user-identity, watchList/marketdata, non-active `squad/<n>`, draft, and mode-gated
season/tournament/champion/leaderboards/sbs) **still proxy to the Python oracle**.
2. **Launcher redesigned to a shareholder-grade egui UI** (your headline ask). Builds
clean; screenshots captured.
3. **New docs:** `MATCH_LIFECYCLE.md` (you asked), `CORE_CORRECTNESS_ISSUES.md`
(4 known Core bugs + 1 bonus exploit, ready to fix on your go/no-go).
4. **Nothing committed** — all work is in the working tree for your review (git state is
delicate: preserved-dirty Core submodule + a concurrent `funman300` actor + detached
launcher branch; I didn't want to entangle that unsupervised).
## What's live in production now (`10.10.0.120:8099`)
| Thing | State |
|---|---|
| prod-host binary | post-P1 `fda40d12` **+ my migration rebuild** (release, in `target/release/openfut-utas-host`) |
| prod-host pid | 3207781 (hub-managed, restart=no; retained spec points at the rebuilt binary) |
| Catalog | `9f6addaa` (post-P1) |
| Core content (cards) | `136d8d68` (post-P1, +18 content-gap defs) → Core loads **1710** defs |
| Core owned | **1982** (1962 players + 17 consumables + 3 staff) |
| Coins | **29,876,776** (baseline — reset from the P1 test value when the content-gap DB was swapped in; you said data isn't precious) |
| prod-core | **fixed build** from canonical submodule (`fbb54ea` + 4 correctness fixes), DB migrated ver 18 → 19; binary now `/home/alex/OpenFUT/target/release/openfut-core` |
| UTAS routes (Rust) | economy, club, squad (0/active/list/PUT), account/sync, ut/auth, userMassInfo, **user**, clientdata, hub, settings, accountinfo, leaderboards/options, match/reset, phishing, club/stats/{year,consumables,staff,country,league,team}, watchList, static acks (store/keepalive/captcha/tfa/livemessage/activeMessage) |
| Still Python (:8199) | item-defs (item/resource, defid), club-identity (clubUser, user/list, user/club), `squad/<n>` (n≠0), draft, marketdata, mode-gated (season/tournament/champion/leaderboards/sbs → `{}` while off), and match CREATE/READY/PLAY. See `docs/PRODUCTION_AUTHORITY_MATRIX.md`. |
Smoke-verified live in prod (in the prod netns): all migrated routes return
`owner=RUST`, coins consistent, clientdata round-trips, club/stats context modes emit
distinct nation/league/team buckets. Scripts:
`/home/alex/openfut-promotion/economy-2026-08-17-p2/{p2_precheck,smoke_migrated,smoke_clubstats}.py`.
## Changes made (all verified: builds clean, tests green)
### 1. Production promotion (deployed)
- Host binary `e5be8730` (P1) → `fda40d12` (post-P1) + catalog `9f6addaa`.
- Content-gap DB swapped in (owned 1962 → 1982). Backups in
`/home/alex/openfut-promotion/economy-2026-08-17-p2/backup/` + `ROLLBACK.txt`.
### 2. Route migration to Rust (deployed, rebuilt binary)
- `POST /ut/auth` — Rust mints the SID (`OPENFUT-SID-{:016X}`), opens the Rust session,
adopts persona from body. No Python. (`+ /ut/delete/auth`.)
- `POST /openfut/account/sync` — full Rust envelope; coins/unopenedPacks from Core.
- `GET /userMassInfo`**full** Rust envelope (was a Python-proxy+overlay hybrid).
- `GET/PUT /clientdata/<key>` — new host `ClientDataStore` (JSON-persisted).
- `GET /club/stats/{country,league,team}` — made `club_stats_body` context-aware
(nation/league/team buckets); classify now routes all `club/stats/*` to Rust.
- `GET /squad/0` — routed to the Rust active-squad projection (verified structurally
identical to Python `squad/0`: same 15 keys, players=23).
- `GET /watchList` (+ no-op add/remove) — empty list + authoritative Core credits.
- Static acks (`store`, `match/keepalive`, `captcha`, `tfa`, `livemessage`,
`activeMessage`) — Rust constants (StaticAck route), byte-identical to the oracle.
- Captured the remaining routes' Python wire shapes as reference fixtures for later
migration: `docs/evidence/route-shapes-2026-08-17/` (user, defs, marketdata,
clubUser, watchList, squad/0, season/tournament/champion/sbs, draft).
- Files: `openfut-utas-host/src/{lib.rs,clientdata_store.rs(new),config.rs}`,
`openfut-adapter-fifa17/src/fut/{non_economy.rs,club_stats.rs}`,
`openfut-utas-host/tests/economy_integration.rs`.
- Tests: `openfut-utas-host` + `openfut-adapter-fifa17` full suites **GREEN**
(188 adapter + 76 host lib + all integration incl the 116s economy integration).
### 3. Launcher redesign + polish + live account panel (built, NOT deployed — client tool)
- **Redesign**: new `openfut-launcher/src/theme.rs` design system (palette, embedded
fonts, egui Visuals/Style, card/pill helpers). Branded hero header (OF monogram),
left nav rail, card-based dashboard with status pills, prominent accent Launch CTA,
console-style Logs. All existing launch/health/preflight/service/config logic preserved.
- **Polish**: OpenFUT window/taskbar icon (OF monogram `IconData`), Config tab rebuilt
into themed cards, consistency sweep.
- **Live "Your Club" panel** (new feature): a background `AccountMonitor` (mirrors
`HealthMonitor`, 5s poll, non-blocking) fetches the account summary and the Dashboard
shows a "Your Club" card — club name/abbr, Manager, **COINS hero number**, Level + XP
bar, unopened packs, account funds — with clean loading/offline/error states.
- Builds clean (0 warnings). Screenshots preserved (for your shareholder demo) in
`/home/alex/openfut-post-p1/launcher-screenshots-2026-08-17/``launcher_account.png`
(the populated "Your Club" card: COINS 29,876,776, Level 12, packs 3) is the headline;
plus dashboard/setup/logs/config + the offline state. All reviewed — product-quality.
- Files: `openfut-launcher/src/{theme.rs(new),account_monitor.rs(new),app.rs,main.rs,
account_sync.rs,config.rs}` + `assets/` (fonts + icon). All additive; behavior preserved.
### 4. Core correctness fixes (deployed 2026-08-17)
All four `CORE_CORRECTNESS_ISSUES.md` classes fixed in the canonical `openfut-core`
submodule and deployed to prod-core (see that doc's "Resolution" section):
- **Issue 3 (HIGH, exploit):** SBC duplicate-card free-reward — `submit_sbc` now dedups
ids + bounds the list (`MAX_SBC_CARDS`=30) → `BadRequest`. Regression test added.
- **Issue 2 (HIGH):** non-atomic economy mutations — closed the concurrency-exploit
races with single-statement compare-and-swap (`spend_coins` conditional debit,
`open_pack`/`buy_listing`/`sell_card`/`checkin` claim-then-act). Multi-statement
partial-failure edges (`buy_pack`, concurrent SBC, match/season chains) left as
documented residual — need the transaction refactor, negligible for single-player.
- **Issue 4 (LOW):** `season.rs` `.expect()` panics → graceful `AppError`; checkin index
`% 7` → `.rem_euclid(7)`.
- **Issue 1 (LOW):** `sbc_submissions.club_id` — migration `0019` (add + backfill) +
`submit_sbc` binds it; milestone query now correct.
- Verified: Core suite **179 green** + clippy clean; migration dry-run on a prod-DB copy;
post-deploy prod migration ver 19, owned 1982, coins 29,876,776, all Core + host
endpoints 200. Rollback: `backup/prod-core.preCoreFix.db` (ver 18) + old binary path —
see `backup/ROLLBACK_CORE_FIX.txt`.
## New / updated docs
- `docs/MATCH_LIFECYCLE.md` (NEW) — consolidated FUT match loop design (CREATE→READY→
PLAY→END), contracts, reward policy, ownership split, blockers. (You asked for this.)
- `docs/CORE_CORRECTNESS_ISSUES.md` (NEW) — 4 known Core bugs + 1 bonus SBC
duplicate-card exploit, each with file:line + concrete fix + severity. **Needs your
go/no-go** (fixing bumps Core off the frozen P1 reference).
- `docs/PRODUCTION_AUTHORITY_MATRIX.md` (UPDATED) — reflects the completed migration.
- Vault `06 Agent Memory/Current Priorities.md` (UPDATED).
## Uncommitted work — needs your review
**I committed nothing** (git state is delicate: openfut-core is intentionally
preserved-dirty; a concurrent `funman300` actor; launcher on detached HEAD `d1a71bd`).
Review + commit these when you're ready:
- Superproject (mine): `openfut-utas-host/src/{lib.rs,config.rs}`,
`openfut-utas-host/src/clientdata_store.rs`,
`openfut-utas-host/tests/{economy_integration.rs,host_test.rs}`,
`openfut-adapter-fifa17/src/fut/{club_stats.rs,non_economy.rs}`,
`docs/{PRODUCTION_AUTHORITY_MATRIX.md,MATCH_LIFECYCLE.md,CORE_CORRECTNESS_ISSUES.md,OVERNIGHT_HANDOFF_2026-08-17.md,PYTHON_RETIREMENT_PLAN.md}`,
and the reference fixtures `docs/evidence/route-shapes-2026-08-17/`.
- Launcher submodule (mine): `src/{app.rs,main.rs,theme.rs(new),account_monitor.rs(new),account_sync.rs,config.rs}`, `assets/` (fonts + icon).
- **Leave the pre-existing dirt alone** (not mine): `CLAUDE.md`, `README.md`,
`.env.example`, `docker-compose.yml`, `AGENTS.md`, `setup.sh`, `openfut-bridge`,
`fifa17-recon/docker/...`, `docs/{ARCHITECTURE,ROADMAP,docker,fifa17-emulation}.md`,
`docs/research/`, `scripts/utas-filter-diff.py`.
- `openfut-core` (mine, this session): `migrations/0019_sbc_submissions_club_id.sql` (new),
`src/services/{sbc.rs,club.rs,pack.rs,market.rs,checkin.rs,season.rs}`,
`tests/integration_test.rs`. Built + deployed to prod-core; still detached HEAD at
`fbb54ea` (edits uncommitted, per your branch strategy).
## Open decisions for you
1. ~~**Core correctness bugs**~~ — **DONE (2026-08-17):** all four classes fixed +
deployed to prod-core (see "Core correctness fixes" above and the Resolution section
of `docs/CORE_CORRECTNESS_ISSUES.md`). prod-core is now off the frozen P1 point,
running `fbb54ea` + fixes at migration ver 19. Residual (documented): the
multi-statement transaction refactor for partial-failure atomicity — negligible for
single-player; do it if/when concurrency matters.
2. **Match handshake legs** (CREATE/READY/PLAY) — the only routes still on Python.
Deferred: no match has ever been played in-game, READY `items` contract unknown,
`FUT_MODES` off (see `docs/MATCH_LIFECYCLE.md`). Migrating them risks the economy
`/match/end` routing for a never-exercised path — I judged it not worth it unmonitored.
3. **Aux service container cutover** (blaze/redirector/roster → Rust) — operator-gated
container change; unchanged.
## Rollback (still hot)
- Python P2 image `openfut-fut-backend:p2-rollback` (b1b929953f) + profile 39bb3e83 +
`rollback-to-python-p2.sh`.
- prod-host P1 binary + P1 catalog backed up in
`/home/alex/openfut-promotion/economy-2026-08-17-p2/backup/` (see `ROLLBACK.txt`).
- prod state (pre-content-gap) backed up: `backup/prod-core.preB.db`,
`prod-identity.preB.json`, plus P1 `fifa17-production-{catalog,cards}.p1.json`.