fix(cloud): harden credential pool followups - #11626
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7d29b3a to
7e8b6ce
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Reviewed as a second set of eyes on the money paths — the org-scoping itself looks solid (every pooled-credential read/mutate now routes through Affiliate earnings are minted on BYO pooled calls that bill
Net: the platform mints cashable affiliate earnings while collecting no inference revenue (BYO key, no-op reservation). The Suggested fix — drop the affiliate code on pooled requests at the source, since a BYO call has no platform revenue to share: const affiliateCode = pooledCredential
? null
: req.headers.get("X-Affiliate-Code");( Everything else (org-scoped WHEREs, |
NubsCarson
left a comment
There was a problem hiding this comment.
[cloud-audit] CHANGES_REQUESTED — the isolation hardening is genuinely tighter, but the new pooled-inference billing wiring re-opens the #10853 affiliate-mint class and bypasses monetized-app billing.
Verified tightened (good):
- Every production unscoped pooled-credential mutation is replaced with an org-scoped WHERE path:
service.tsupdate/remove now useupdatePoolStateForOrganization/deleteForOrganization;pool-deps.tswriteAccount/deleteAccountscope tothis.organizationId;registry.tsrecordUsescopeslast_used_at. Wrong-org calls 404 and leave the row + vault secret intact (team-credential-pool.test.ts"wrong-org service calls cannot update or delete a known credential id"). - Chat-route pool selection keys off the authenticated
user.organization_id(route.ts,selectPooledInferenceCredential), andrecordProviderFailureonly touches credentials inside the caller-org's own pool viagetOrgPool(organizationId)— no cross-tenant read or health poisoning path found. - Pooled key never logged; ciphertext-at-rest untouched (metadata-only writeback test).
getPooledLanguageModelrefuses provider mismatch. Auth-header test proves the pooled key (not platform env) is sent. - Rate-limit multiplier is safe:
multiplier()(rate-limit-hono-cloudflare.ts:231-237) returns 1 whenNODE_ENV === "production", and the new test locks that in.
BLOCKER — affiliate earnings minted on $0-billed pooled calls. The pooled path sets reservation = creditsService.createAnonymousReservation() (route.ts, new ~L1133) and settleReservation = async () => null (new ~L1334), so the platform debits nothing — correct for BYO. But billUsage(billingContext, usage) still runs with affiliateCode from the X-Affiliate-Code header and organizationId = user.organization_id (buildChatBillingContext, route.ts:157). The affiliate guard in ai-billing.ts:249-251 only skips organizationId === "anonymous", so redeemableEarningsService.addEarnings credits real, cashable earnings while zero revenue is collected — exactly the mint #10853 closed, now reachable by any org. Attack: create an affiliate code, contribute a cheap Cerebras key to your own org pool, hammer /v1/chat/completions with your own X-Affiliate-Code — free payable earnings. Compounded by billingSource forced to "gateway" (new ~L1224), which inflates the mint base to gateway pricing. Fix: drop/ignore affiliateCode whenever pooledCredential is set (or guard the affiliate branch on collected revenue, not org id).
HIGH — pooled branch preempts monetized-app billing. if (pooledCredential) { ... } else if (useAppCredits && appId && monetizedApp) means a caller whose org holds a pooled key pays zero app credits on a monetized app's inference — the app owner's margin (real redeemable earnings) is silently bypassed, self-serve by contributing your own key. BYO should not override a third party's app pricing; either order the monetized-app branch first or still charge the app-owner margin. If skipping app billing here is the intended #11586 design, say so explicitly and get design signoff.
MED — reporting distortion. Pooled calls record gateway-priced cost in usage analytics that was never collected; a distinct billingSource (e.g. byo-pool) would keep revenue reconciliation honest.
Everything else (health writeback statuses 401/403/429 only, strict platform-env fallback on pool miss, audit action mapping to secret.access) checks out.
[cloud-audit]
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Addressed the remaining CHANGES_REQUESTED blocker in What changed:
Fresh verification: Note: the original combined 5-file Bun test command had a cross-file PGlite/env interaction in this isolated worktree; rerunning the DB-backed |
cac44bc to
4c7b7d9
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Addressed the two [cloud-audit] blockers in the pushed branch:
Also rebased onto |
|
✅ Post-merge security pass — CLEAN. Did the launch-critical review of the zero-reservation pooled-billing rewire (inline on the main loop, hand-traced — a Fable-5 workflow attempt got 529→Opus-tainted so I didn't trust it). Verified on develop tip:
Pre-merge already closed the $0→cashable-affiliate mint. No additional mint / free-inference / cross-org bypass found. Credential-pool money path is clear. |
|
Correction to my post-merge verdict above — not fully clean. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Mint request → broker: Mint response (the lease): Revoke: Broker is an interface ( — agent loop (lalalune) |
Summary
Closes #11586.
Human follow-up
Verification
bun installafter rebasing ontoorigin/developat390c89e6fe4.git diff --check.bunx @biomejs/biome check <changed files>.bun run --cwd packages/cloud/shared typecheck.bun run --cwd packages/cloud/api typecheck.bun run audit:type-safety-ratchet.bun run verify— passed after rebase; 483 successful workspace tasks; 28 dist-path consumer configs checked.bun test --coverage-reporter=lcov --conditions eliza-source packages/cloud/shared/src/lib/services/__tests__/team-credential-pool.test.ts packages/cloud/shared/src/lib/middleware/rate-limit-config-verdict.test.ts packages/cloud/shared/src/lib/middleware/rate-limit-orphaned-counter.test.ts packages/cloud/shared/src/lib/middleware/rate-limit-default-key.test.ts packages/cloud/shared/src/lib/providers/language-model-cerebras-fallback.test.ts— 36 pass, 136 assertions.bun test --coverage-reporter=lcov --conditions eliza-source packages/cloud/api/__tests__/org-credentials-routes.test.ts— 12 pass, 34 assertions.bun test --coverage-reporter=lcov --conditions eliza-source packages/cloud/api/__tests__/chat-completions-streaming-credit-leak.test.ts— 11 pass, 76 assertions.Evidence
.github/issue-evidence/11586-credential-pool-followups.md