From 2e44faf821fdac0a1ee6c02e7b7e71eb449e2817 Mon Sep 17 00:00:00 2001 From: shum Date: Wed, 26 Aug 2026 12:44:12 +0000 Subject: [PATCH] core: fix badge store haddock and tier test notes --- .../2026-08-21-badges-web-checkout.md | 2 ++ src/Simplex/Chat/Store/Badges.hs | 30 +++++++++---------- tests/Bots/BadgeServiceTests.hs | 24 +++++++++++---- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/plans/badges-codes/2026-08-21-badges-web-checkout.md b/plans/badges-codes/2026-08-21-badges-web-checkout.md index 1c1dd054cf..052282f9df 100644 --- a/plans/badges-codes/2026-08-21-badges-web-checkout.md +++ b/plans/badges-codes/2026-08-21-badges-web-checkout.md @@ -1418,6 +1418,8 @@ Append here when a step contradicts this plan: the step id, what was wrong, and - **C4 — `sendBadgeRequest` also takes the `NetworkRequestMode` and the `User`.** The step's signature is `Maybe C.PrivateKeyEd25519 -> BadgeServiceRequest -> CM BadgeServiceResponse`, but the agent's service request is created under an agent user (`aUserId`) and resolves a short link or a SimpleX name under a request mode. Reading the ACTIVE user inside the send would put C3's per-profile pass on whichever profile happens to be active — the same defect class as the `currentUser` write the C3 review round fixed — so the caller states the profile instead: the two commands pass `withUserId`'s user and `processChatCommand`'s `nm`, the worker passes its own pass's user and `NRMBackground`. It is still one send path with three call sites, not a second one. - **C4 — the service signs the tier the CODE funds, and the client states nothing.** The step, core §5 and UX 2.8 all have the RESPONSE state the badge type, and the client needs it stated: `badge_purchases.initial_badge_type`/`current_badge_type` are `NOT NULL` and a code carries no tier (B3: 20 opaque characters, with none of the tier in them). B7 had added, beyond its own step, a check refusing a `purchaseBadge` whose `badgeRequest` names a tier other than the code's — which makes code redemption unimplementable for any client that was not told the tier out of band, and no client is: G2's and G3's redeem views are a code field and a paste button, and a code may come from `codes issue` (B8) or from an order the app never opened. The two could not both hold. **Resolved on the service side, in C4's own range**, after an independent review confirmed it: `handlePurchaseCode.redeem` applies `withFundedBadgeType` to the request immediately before `resolveIssue`, so the credential carries the code's tier whatever the request named. The security property B7's check defended is kept in full — a credential can never exceed its funding, because the tier is read from the code's row and never from the request — and it is now asserted directly instead of through a proxy: B10's `testBadgeServiceSignsFundedTier` (renamed from `testBadgeServiceTierMismatchIsBadRequest`) presents a **legend** request over a **supporter** code and pins the credential, the wire ledger and the purchase row as supporter. Proved able to fail: with the override dropped, it reports `expected ("credential tier", BTSupporter) but got ("credential tier", BTLegend)` — the service handing out a signed legend credential for a supporter code, which is exactly the defect the assertion exists to catch. Three things deliberately did NOT change: the override is at this ONE call site and not in `issueSignedBadge`, which `handleIssueBadge` shares; `requestMasterKey` still reads the original request, so the credential stays bound to the client's own master key; and `planTxn`'s `currentBadgeType /= badgeType` guard is untouched, because it compares the existing purchase row's tier to the CODE's and is what stops a legend code crediting a supporter purchase (B10's per-signer bucket test still trips exactly that guard, verified, not assumed). The replay path's tier check was deleted with the refusal it belonged to: it could only ever have refused an honest client repeating a request after a timeout without knowing the tier, which is the case the idempotency rule exists for. `codeBadgeTypes` and the tier probe C4 first shipped are gone; `APIPurchaseBadge` sends exactly one request, whose `badgeInfo.badgeType` is `BTSupporter` because the field is not optional on the wire and nothing reads it. +- **C4 — `badgeCodeRequest` hardcodes `BTSupporter` on the wire, and that field is now deliberately false for every legend code.** `purchaseBadge.badgeRequest.badgeInfo.badgeType` is not optional, so the client has to send something, and since the service reads the tier from the code (`withFundedBadgeType`, above) nothing reads what it sends — but the client still **signs** the request that asserts it. It is documented at the three places it matters (`badgeCodeRequest`, `withFundedBadgeType`, and the RPC's "Commands" carve-out), and it means the client now depends on no future service or protocol revision re-enabling a mismatch refusal on `purchaseBadge{code}` — which would break every legend redemption again, silently to the client's author. **Whenever `BadgeServiceCommand` is next revised, `badgeInfo.badgeType` should become absent or `Maybe` for the funding kinds whose tier the client cannot know**, so the wire stops carrying an assertion nobody means. +- **C4 — the tier carve-out is scoped to `code` funding only; D0, E2 and F1 must not copy it.** `invoice`, `apple` and `google` funding all let the client know the tier before it asks — an invoice is created from a `priceId` the client pinned, and a store purchase is made against a SKU the app chose — so on those paths the client CAN state the tier and the RPC's "the service signs exactly this content or rejects the command" must keep holding, exactly as it does for `issueBadge`. Applying `withFundedBadgeType` to a store or invoice purchase would discard a check the client is able to satisfy, for no gain. Today only a haddock on `withFundedBadgeType` says so; this is the plan-level record. - **C4 — `sendBadgeRequest` returns its failures as `BSPError internal` and never throws.** An unconfigured `badgeServiceAddress`, a timeout, an agent failure and a response that does not decode are all answers to the request. Both callers already handle a service error — the worker reports it and keeps its state, the commands raise it as `CEBadgeServiceError` — and raising here would hand the worker's pass a chat error of a different shape for the transport half of the same operation. This is also what C3's report asked for ("a timeout must surface as a `BSPError`, not an exception"). - **C4 — nothing is written when the response cannot be recorded in full, and that needed a pre-flight check to be true.** A credential that fails verification, one carrying no expiry, a statement with no issued period, and a `badgeCredential` with no credential at all each raise `CEBadgeServiceError` and write nothing. The first three are checked before the transaction opens; the claim that the transaction itself is all-or-nothing is **not** true of a `Left` from an `ExceptT` store function, and this was a real hole: `withStore` runs `runExceptT` INSIDE `withImmediateTransaction`, so a `Left` returns normally and the transaction **commits** what preceded it. `insertLedgerEntries` can `Left` on an entry type this build cannot store, and in the redeem path it runs after the payment and purchase rows — an unstorable statement would have left an orphan `payments`/`badge_purchases` pair rather than nothing. `Store/Badges.hs` gains `checkStatementEntries`, the pure half of `insertLedgerEntries`' own row conversion, and both C4's `storeRedeemedBadge` and C3's `storeBadgeIssueResponse` call it before opening their transaction; C3's ordering made it unreachable there today, but the check also gives it the badge-error shape every other failure of a pass has, instead of a store error. The two `ExceptT` calls that remain inside C4's transaction, `supersedePurchases` and `setShownPurchase`, can only fail for a purchase that is not this user's — the row created two statements earlier in the same transaction — so the claim now holds for every reachable shape. The code is consumed in all these cases and is recovered with `codes unredeem` (H2), as a lost response is. diff --git a/src/Simplex/Chat/Store/Badges.hs b/src/Simplex/Chat/Store/Badges.hs index 96d5ad24d9..eebce0ce57 100644 --- a/src/Simplex/Chat/Store/Badges.hs +++ b/src/Simplex/Chat/Store/Badges.hs @@ -519,6 +519,21 @@ getLastBadgeLedgerEntry db badgePurchaseId = do (Only badgePurchaseId) liftEither $ mapM rowToLedgerEntry (listToMaybe rows) +-- | Whether every entry of a statement can be stored, without opening a transaction or touching +-- the database. +-- +-- 'insertLedgerEntries' is the only fallible call its callers make with rows already written in +-- the same transaction, and a @Left@ from it does NOT roll them back: @withStore@ runs its +-- 'ExceptT' inside the transaction, so a @Left@ returns normally and the transaction COMMITS +-- what preceded it. A caller therefore asks this first, outside the transaction, and the only +-- failure that reaches 'insertLedgerEntries' proper is one that would also have failed here. +-- +-- It is exactly 'insertLedgerEntries'' own row conversion with the row discarded, so the two +-- cannot disagree about what is storable. +checkStatementEntries :: BadgeStatement -> Either StoreError () +checkStatementEntries BadgeStatement {entries} = + mapM_ (\StatementEntry {entryType} -> encodeLedgerEntryType =<< storedEntryType entryType) entries + -- | Copies a statement's entries into the client's replica of the service's ledger. -- -- __Order is carried only by array position.__ Every entry the service writes for one command @@ -544,21 +559,6 @@ getLastBadgeLedgerEntry db badgePurchaseId = do -- complete history every time; only @issueBadge@ honours a cursor. Insertion is therefore -- @ON CONFLICT (entry_uuid) DO NOTHING@ against @idx_badge_ledger_uuid@ — a second delivery of -- the same statement writes nothing and changes nothing, rather than merely not crashing. --- | Whether every entry of a statement can be stored, without opening a transaction or touching --- the database. --- --- 'insertLedgerEntries' is the only fallible call its callers make with rows already written in --- the same transaction, and a @Left@ from it does NOT roll them back: @withStore@ runs its --- 'ExceptT' inside the transaction, so a @Left@ returns normally and the transaction COMMITS --- what preceded it. A caller therefore asks this first, outside the transaction, and the only --- failure that reaches 'insertLedgerEntries' proper is one that would also have failed here. --- --- It is exactly 'insertLedgerEntries'' own row conversion with the row discarded, so the two --- cannot disagree about what is storable. -checkStatementEntries :: BadgeStatement -> Either StoreError () -checkStatementEntries BadgeStatement {entries} = - mapM_ (\StatementEntry {entryType} -> encodeLedgerEntryType =<< storedEntryType entryType) entries - insertLedgerEntries :: DB.Connection -> Int64 -> BadgeStatement -> UTCTime -> ExceptT StoreError IO () insertLedgerEntries db badgePurchaseId BadgeStatement {entries, previousEntryId} now = do rows <- liftEither $ mapM entryRow entries diff --git a/tests/Bots/BadgeServiceTests.hs b/tests/Bots/BadgeServiceTests.hs index ad8a7aa46a..9677c33f59 100644 --- a/tests/Bots/BadgeServiceTests.hs +++ b/tests/Bots/BadgeServiceTests.hs @@ -1798,10 +1798,21 @@ testBadgeServiceSecondCodeSamePurchaseKey ps = do -- carries is the CODE's, never the request's. A client cannot know a code's tier before it -- redeems it -- a code carries none (B3) and the response is what states it (core §5) -- so -- 'purchaseBadge{code}' signs the tier the funding bought and ignores the one the request names --- ('withFundedBadgeType', plan §9). The property that replaced the old refusal is asserted --- directly: a LEGEND request over a SUPPORTER code yields a SUPPORTER credential, a supporter --- ledger and a supporter purchase row, so nothing a request says can buy a tier its funding did --- not. Drop the override and this reads back 'legend' on all three. +-- ('withFundedBadgeType', plan §9). A LEGEND request over a SUPPORTER code must therefore be +-- answered with a SUPPORTER credential, on a supporter ledger and a supporter purchase row. +-- +-- __Each of the four assertions catches a different mutation__, and only the first is the proof +-- of the override: +-- +-- * the CREDENTIAL tier is the one 'withFundedBadgeType' decides. Drop the override and this is +-- the only assertion that fails, with 'legend' -- the service handing out a signed credential +-- of a tier the code did not buy, which is the whole point of the test. +-- * the LEDGER tiers and the PURCHASE tier come from @writeTxn@'s own @badgeType@, which is the +-- code's on either side of that mutation. They pin 'planTxn'\/@writeTxn@ instead: that the +-- rows a redemption writes are the funding's tier and not the request's, which no other test +-- states. +-- * the REPLAY returns the credential already issued, rather than refusing a request whose tier +-- differs -- the guard C4 deleted with the refusal it belonged to. -- -- 'issueBadge' is deliberately unchanged: there the tier is the purchase's own -- 'current_badge_type', which the client HOLDS and must state, so a mismatch is still @@ -1820,8 +1831,8 @@ testBadgeServiceSignsFundedTier ps = do let BadgeCredential {badgeInfo = BadgeInfo {badgeType = credentialType}} = cred1 ("credential tier" :: String, credentialType) `shouldBe` ("credential tier", BTSupporter) statementShape statement `shouldBe` [(3, 3, "credit payment (no invoiceId)"), (-1, 2, "debit badge")] - -- the ledger the same request wrote is supporter entry by entry (field 5 of StatementEntry, - -- constructed positionally as everywhere else in this file) + -- the rows the redemption wrote carry the code's tier as well -- this pins writeTxn, not the + -- override (field 5 of StatementEntry, constructed positionally as everywhere else here) let BadgeStatement {entries} = statement entryTypes = map (\(StatementEntry _ _ _ _ bt _ _ _) -> bt) entries ("ledger tiers" :: String, entryTypes) `shouldBe` ("ledger tiers", [BTSupporter, BTSupporter]) @@ -1834,6 +1845,7 @@ testBadgeServiceSignsFundedTier ps = do withServiceDB ps $ \db -> do -- exactly what the one redemption writes: the replay and the issueBadge refusal wrote nothing serviceRowCounts db `shouldReturn` (1, 1, 2, 1, 1) + -- likewise writeTxn's, not the override's [Only purchaseType] <- DB.query_ db "SELECT current_badge_type FROM sx_badge_service_badge_purchases" ("purchase tier" :: String, purchaseType :: BadgeType) `shouldBe` ("purchase tier", BTSupporter)