Skip to content

fix(billing): 7 accounts/billing review follow-ups#276

Merged
oratis merged 2 commits into
mainfrom
claude/billing-hardening-continue-412014
Jul 22, 2026
Merged

fix(billing): 7 accounts/billing review follow-ups#276
oratis merged 2 commits into
mainfrom
claude/billing-hardening-continue-412014

Conversation

@oratis

@oratis oratis commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Hardening pass over the B0–B9 accounts/billing stack — the seven review follow-ups from #260/#261/#264/#265/#266/#267/#272. Each was a way for the merged stack to lose money, stay up under load, or trust something it shouldn't.

Billing correctness — a metered turn can no longer ship free

before after
meter (#264) recordUsage returned null when the audit append failed ⇒ debit skipped ⇒ free, unrecoverable turn pricing is decoupled from the append; the record always comes back priced, and a full disk loses the audit line only
limits (#267) readCounter returned {microUSD:0} on any read error ⇒ the $200/day cap failed open ENOENT/stale-day ⇒ 0 (normal); I/O or parse failure ⇒ ok:false, cap fails closed, and the write path won't clobber an unreadable-but-valid counter
gateway (#264) 2xx upstream with no usage block ⇒ debitTurn(0) = free inference falls back to estimateUsageFromBytes (bytes/4). Non-2xx still settles at 0 — an upstream error isn't the user's bill

Availability + abuse

Multi-instance + replay

  • turn-lease renewal (feat(cloud): B9 — Firestore state backend + turn lease; multi-instance behind LISA_FIRESTORE=1 #272) — TURN_TTL_MS is 180s but chat SSE runs under --timeout 3600, so with LISA_FIRESTORE=1 and MAX_INSTANCES>1 the lease expired mid-turn and a peer double-ran the account. Added renewLease (CAS) + a TTL/3 heartbeat that stops itself if ownership is lost, wired into both the chat and gateway paths.
  • SIWA nonce (feat(accounts): B1 clients — SIWA on, sign-in as primary iOS flow, web login page #261) — there was no nonce anywhere: iOS never set req.nonce, the server verified none. Now end-to-end (server, web, iOS), timing-safe against sha256(rawNonce). LISA_CLOUD_APPLE_REQUIRE_NONCE is default-OFF so already-shipped TestFlight builds keep signing in; a nonce that is sent is always verified. Flip it on once clients update.
  • IAP cert chain (feat(billing): B5 — StoreKit 2 consumable credits + server JWS verify + ASN refunds #265) — the "pinned" G3 root was whatever apple.com served over TLS and then whatever sat in the cache file, and the walk asserted no CA constraint or leaf OID: a signature chain alone never said what each cert was for. Now the G3 SHA-256 is hardcoded and asserted on both the cached and fetched paths, issuers must be ca === true, and Apple's marker OIDs are asserted on the leaf and the WWDR intermediate. .ca is used rather than keyUsage, which reads back undefined on Apple certs. The opts.root seam is preserved; an existing file at LISA_APPLE_ROOT_PATH still skips the pin (documented air-gapped escape hatch).

Verification

npm run typecheck + npm run build clean. 1104 tests pass, 0 fail — 15 new: renewLease, cloudAuth nonce (accept / wrong / absent / unhashed), the IP window, cap fail-closed (unreadable + corrupt), meter append-failure, estimateUsageFromBytes, oidToDer against the two DER byte strings verified on real Apple certs, and a new http-body.test.ts (cap boundary, split multi-byte chars, stream error).

The iOS companion type-checks against the iPhoneSimulator SDK with LISA_ENABLE_SIWA both on and off. That's compile-verified only — the nonce path has not been exercised against a real Apple sign-in, so the first TestFlight build wants a manual sign-in check before LISA_CLOUD_APPLE_REQUIRE_NONCE goes on.

Independent of #274 (per-tenant SSE isolation); both branch from main.

🤖 Generated with Claude Code

Hardening pass over the B0–B9 accounts/billing stack, from the review
follow-ups on #260/#261/#264/#265/#266/#267/#272.

Billing correctness — a metered turn can no longer ship free:
- meter: recordUsage always returns the priced record; the audit-log
  append is best-effort and decoupled from pricing/debit, so ENOSPC on
  usage.jsonl loses the audit line, not the debit (#264).
- limits: readCounter distinguishes ENOENT (0, fine) from an I/O or
  parse failure, and the $200/day global cap now fails CLOSED instead
  of silently reading as $0 spent (#267).
- gateway: a 2xx upstream that reports no usage is priced from a
  bytes/4 estimate rather than debiting 0 (#264).

Availability + abuse:
- Bounded every unauthenticated body read behind readCappedText —
  readJsonBody, the Stripe webhook raw read (signature needs the raw
  string), and the gateway (LISA_GW_MAX_BODY_MB, default 20 MiB).
  Past the cap: 413 + Connection: close (#260/#264/#266).
- accounts: scrypt moves off the event loop; /register gains a per-IP
  sliding-window cap (20 / 10 min). Coarse backstop, not a security
  boundary — XFF is client-controlled (#260).

Correctness under multi-instance + replay:
- turn-lease: heartbeat a held lease at TTL/3 so a chat SSE turn
  running under --timeout 3600 can't outlive its 180s lease and let a
  peer instance double-run the account (#272).
- SIWA: verify Apple's nonce claim against sha256(raw nonce) end to
  end (server, web, iOS). LISA_CLOUD_APPLE_REQUIRE_NONCE is default-OFF
  so already-shipped TestFlight builds keep working (#261).
- IAP: pin Apple Root CA G3 by SHA-256 on both the cached and fetched
  paths, assert basicConstraints CA on issuers, and assert Apple's
  marker OIDs on the leaf and the WWDR intermediate — a signature chain
  alone never said what each cert was FOR (#265).

Verified: typecheck + build clean; 1104 tests pass (15 new). The iOS
companion type-checks against the iPhoneSimulator SDK with SIWA both on
and off — compile-verified, but the nonce path has not been exercised
against a real Apple sign-in.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@oratis

oratis commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Review — accounts/billing hardening

Verdict: strong work. Every one of the seven items closes a hole that was really there, the why is written down where the next reader needs it, and the tests aim at the failure modes rather than the happy path (the EISDIR-as-I/O-error trick in limits.test.ts and the split-multi-byte-char case in http-body.test.ts are exactly right).

Three things I checked that could have been bugs and aren't, so nobody re-litigates them:

  • recordUsage's Promise<UsageRecord | null>Promise<UsageRecord> has exactly two call sites (gateway.ts:260, server.ts:3099) and both are updated — no stale if (rec) left behind.
  • The new chain[1]! role check can't go out of bounds: x5c.length < 2 already throws bad_chain at line 183.
  • remoteAddr is in scope at the new ipRateOk call (defined server.ts:879, used :1056).

One genuine regression below, plus two things worth a decision.


🔴 1. ipRateOk fails closed on map saturation — a self-inflicted DoS

if (genericBuckets.size >= GENERIC_MAX_KEYS && !genericBuckets.has(key)) return false;

The key is auth:${clientIp(req, remoteAddr)}, and clientIp reads x-forwarded-for — which this PR itself correctly documents as client-controlled. So a single attacker, from a single machine, sends 10 000 requests with distinct X-Forwarded-For values, fills genericBuckets with 10 000 live keys, and from that moment every new IP is refused: /register 429s for every legitimate user until the window drains — and it costs the attacker one cheap request per key to keep it topped up.

That inverts the function's own stated contract ("a coarse backstop against a single naive attacker, NOT a security boundary"). A deliberate non-boundary should fail open: the real guards are scrypt's cost and email uniqueness, and neither depends on this map.

    // Still full ⇒ everything in it is live. Fail OPEN: this is a coarse
    // backstop, not a security boundary, and the key space is attacker-
    // controlled (spoofable XFF) — refusing here would let one client lock
    // every new IP out of /register. scrypt cost + email uniqueness remain.
    if (genericBuckets.size >= GENERIC_MAX_KEYS && !genericBuckets.has(key)) return true;

🟠 2. Daily-cap fail-closed has whole-service blast radius on a storage blip

globalSpendExceeded turning any non-ENOENT read error into return true is the right direction — silently disabling a $200/day cap is worse. But the radius is total: one unreadable counter 402s service_paused for every account, and globalSpendAdd simultaneously stops accumulating.

What makes this concrete rather than theoretical: on Cloud Run that counter lives on the gcsfuse /data mount, and #275 (merged hours ago) exists precisely because that mount isn't POSIX-faithful (link()ENOSYS). A transient EIO there now takes the whole service down until it clears.

I did not change this — softening a money-safety path you deliberately hardened, in the same merge, without you, is not my call. Suggested follow-up: keep the last successful read in memory and fail closed only after the failure persists (or after k consecutive failures); with no prior good read, fail closed immediately, which keeps your limits.test.ts assertions exactly as written.

🟠 3. The nonce is inert in production until the flag flips — and omitting it is the bypass

With LISA_CLOUD_APPLE_REQUIRE_NONCE default-off, an attacker replaying a stolen identity token just leaves nonce out and the check is skipped wholesale. The back-compat rationale is sound and the default is right for shipped TestFlight builds — but it means #261 buys zero replay protection today. This needs a tracked trigger to flip it (e.g. "once 1.1 is the minimum supported build"), or it quietly stays off forever. Compounding it: the PR notes the nonce path has never been exercised against a real Apple sign-in.


Minor

  • oidToDer emits short-form DER length only. Buffer.from([0x06, content.length, ...content]) is invalid DER once content.length > 127. Not reachable today (two hardcoded short OIDs) but it's a trap for whoever adds a third — worth a throw guard.
  • certHasOid is a raw substring search over the whole DER. It asserts the OID TLV appears anywhere in the cert, not that it's an extension's extnID; a cert carrying those bytes in any other field passes. Low practical risk (12-byte Apple-specific OIDs) and the Node API limitation is real — but the docstring ("enough to keep an unrelated cert from standing in") claims more than the check delivers.
  • /login still has no per-IP cap. Only /register got one; login's existing throttle is per-email, so cycling distinct emails still forces unbounded scrypt. Async scrypt keeps the event loop free (good) but still saturates the libuv threadpool (default 4) and queues every other fs/crypto op behind it.
  • readCappedText's overflow comment. It says the rest of the body "stays in the kernel buffer under TCP backpressure" — but removing a data listener doesn't re-pause a flowing stream; the bytes keep arriving and are discarded. Memory stays bounded either way (which is what matters), so this is a comment fix, not a behaviour one.

Fixing 1 and the two cheap minors (oidToDer guard, /login IP cap) on the branch now; 2 and 3 are yours to call.

…R length

Two review follow-ups on the hardening pass itself.

1. `ipRateOk` refused every NEW key once the bucket map hit GENERIC_MAX_KEYS.
   The key is `auth:<clientIp>` and clientIp reads a spoofable
   x-forwarded-for, so one client could mint 10k distinct keys and lock every
   subsequent IP out of /register + /login — turning a DoS backstop into a DoS
   amplifier. It now fails OPEN on saturation, which is the correct direction
   for something its own docstring calls "NOT a security boundary": scrypt
   cost, email uniqueness and the per-email throttle are the real guards and
   none of them consults this map.

2. `oidToDer` emitted `content.length` as a bare short-form DER length. Past
   127 bytes that is invalid DER, so the needle could never match and the
   cert role check would silently always fail. Now throws instead of lying.
   Also documents what `certHasOid` actually proves (TLV present anywhere in
   the cert, not extnID) so the comment no longer claims more than the check.

Tests: saturation-fails-open (10k-key flood, new IP still admitted, existing
key still capped) and long-form-OID refusal. Full suite 1106 pass / 0 fail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@oratis

oratis commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Correction + fixes pushed (7bb85f2)

First, I was wrong about /login. I claimed it "still has no per-IP cap." It does: server.ts:1046 handles /api/auth/register and /api/auth/login in one block, and ipRateOk sits at the top of it (:1056), so both endpoints are covered. Disregard that bullet — no change needed, and the per-email throttle stacks on top for login. My mistake for reading the diff hunk without the surrounding handler.

The other minors stand.

Pushed

1. ipRateOk now fails OPEN on saturation — the one item I'd call a regression rather than a nit. Reasoning in the commit; short version: the key space is attacker-controlled, so refusing new keys once the map fills converts the backstop into the attack. Test added: flood 10 000 keys → a fresh IP is still admitted, while a key already in the map stays capped.

2. oidToDer throws instead of emitting invalid DER past a 127-byte content length, and certHasOid's docstring now states what it actually proves — the OID's TLV is present somewhere in the cert, not that it is a given extension's extnID. It's a role hint layered on an already-verified pinned-root chain, not a gate on its own. Tests cover the long-form refusal and malformed input.

npm run typecheck clean; full suite 1106 pass / 0 fail / 1 skipped.

Left for you (deliberately not touched)

  • Daily-cap blast radius — softening a money-safety path you just hardened, in the same merge, isn't mine to decide. The gcsfuse angle (fix(lock): O_EXCL fallback when link() is unsupported (gcsfuse /data) #275) is why I'd not leave it indefinitely.
  • Flipping LISA_CLOUD_APPLE_REQUIRE_NONCE — until it's on, the nonce buys nothing, because omitting the field skips the check. Worth an explicit trigger condition rather than a someday.
  • The unexercised SIWA nonce path — your own note; the first 1.1 TestFlight build should do a real Apple sign-in before that flag goes on.

Merging.

@oratis
oratis merged commit 342cd8e into main Jul 22, 2026
1 check passed
@oratis

oratis commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Review — 3 adversarial passes (billing money-paths · auth/DoS · IAP crypto)

Reviewed across the three risk surfaces. Headline: the Apple IAP cert-chain + JWS validation is sound — no forgery path (G3 SHA-256 pinned and asserted on both the cache and fetch paths, real per-link signature verification up to the pinned root, ES256 pinned, leaf 6.11.1 + WWDR marker OIDs and ca===true all enforced, oidToDer correct). But two of the money-path guarantees the PR names are defeatable on the exact failure modes it cites, plus two auth issues.

✅ Fixed on this branch (with tests)

Lease double-run → double-charge (2e84ffa)renewLease returned false for both ownership loss and any transient CAS/network error, and startLeaseRenewal treated any false as terminal. So one Firestore blip over a --timeout 3600 turn permanently stopped the heartbeat, the 180s lease expired, and a peer took it over and double-debited. renewLease now returns "held" | "lost" | "error"; the heartbeat stops only on "lost", keeps beating through "error" (expiry is the backstop).

Login lockout bypass (2e84ffa)loginThrottled() (check) and noteLoginFailure() (record) straddle the now-async passwordMatches(), so N concurrent guesses all cleared the check before any failure landed (50 concurrent ⇒ 50 guesses, 0 throttled). Added a per-email in-flight guard: one password check per email proceeds, the rest reject as throttled, forcing a burst into serial attempts that hit the 5-try lock (+ concurrency test).

Gateway body cap disabled by a mistyped env (2e84ffa)Number(LISA_GW_MAX_BODY_MB || 20) is NaN for "20MB"bytes > NaN always false ⇒ cap never fires. Clamped to a positive finite value.

Per-IP rate-limit DoS + oidToDer — addressed in your 7bb85f2 (saturation now fails open instead of refusing new keys, so a spoofed-XFF key flood can't lock out all new email-auth; oidToDer DER length guarded). 👍

⚠️ Remaining — needs your architectural call

M1 — a metered turn can still ship free on a full disk. recordUsage is correctly decoupled from the audit append, but the debit it returns is only real once debitTurn persists it — and debitTurn → writeBalance → atomicWrite writes balance.json to the same filesystem, unguarded. On ENOSPC/EMFILE (the exact modes meter.ts's own docstring names) debitTurn throws: in chat it's swallowed by runTurn's catch, in the gateway it throws from settle() in the streaming finallyboth after the answer already shipped — and since the usage.jsonl append also failed there's no line to reconcile from. Unrecoverable free turn. meter.test.ts stops at the priced record and never exercises the failing debit write. The Firestore branch (casUpdate on the balance doc) has the same shape.

I didn't improvise this into the money path — it wants a design decision. Options: persist an idempotent debit-intent before streaming and settle after; or make the audit append the durable source of truth + a reconciler; or at minimum make a balance-write failure loud + retried (and consider refusing new turns when the balance store is unwritable — fail-closed on serving, not on billing). Your call on the approach.

Non-blocking follow-ups

  • Firestore globalSpendExceeded fails OPENfeat(billing): B7 — rate limits, daily cap, kill switch, reviewer-account seed #267's fail-closed hardening didn't reach the Firestore branch (cold cache / persistent getDoc failure ⇒ the $200/day cap never trips), the exact multi-instance mode this stack targets.
  • SIWA replay protection inert in the shipped default — with LISA_CLOUD_APPLE_REQUIRE_NONCE off, replaying a genuine nonce-bearing token just omits the nonce field ⇒ no check. Sound when a nonce is supplied or REQUIRE_NONCE=1; worth documenting the default gives no replay protection until nonce-capable clients are the floor.
  • On correct ownership loss the heartbeat stops the timer but doesn't abort the in-flight turn or block its debit — a de-leased instance still finishes + debits alongside the peer. Wire lease-loss → turnAbort.abort() + skip-debit to close the residual.
  • estimateUsageFromBytes over-charges base64-image requests (bytes/4 counts the base64 payload; ~5 MB image ≈ 1.3M "input tokens") — by-design "wrong in our favour," just noting the magnitude.
  • IAP: zero test coverage of the chain walk itself — the money-bearing verify path has no positive/negative test, so a regression ships green. A synthetic mini-CA fixture is the highest-value follow-up. certHasOid substring-matches raw DER (not practically exploitable); the LISA_APPLE_ROOT_PATH pin-bypass is silent on the cache-hit path.

Full suite green with all fixes (1107 pass). Given this is money/auth for a paid launch, I held off merging pending M1.

@oratis

oratis commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

M1 fixed in d94c9d5 (the conservative approach you picked): debitTurn retries the balance write + on final failure logs loudly (reconcilable) and flips a store-health flag; precheckTurn catches a write failure and returns billing_unavailable503, fail-closed on serving (no accumulating unbillable turns), recovering when a write succeeds. server.ts + gateway.ts map it to 503. Regression test: unwritable store → precheck refuses, debit retries+throws, then recovers.

All 6 review findings now addressed (M1/M2/login-concurrency/gateway-NaN here; per-IP DoS + oidToDer in your 7bb85f2). Full suite green — 1108 pass, 0 fail. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant