fix: gate app monetization on review - #11828
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
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 |
NubsCarson
left a comment
There was a problem hiding this comment.
[cloud-audit] CHANGES_REQUESTED — the PR's own gate is correct and the demo flow is genuinely unblocked, but the identical monetize-without-review bypass survives via the backup-restore endpoint, so "no way to collect money on an unapproved app" is not yet true.
What I verified (all at head 3e6d777):
- Create-time gate is fail-closed.
packages/cloud/api/v1/apps/route.ts:70-81rejectsmonetization_enabled: truewith 403app_review_requiredBEFOREcreateApp, so no app row and no monetization write occur (integration test assertscreateAppnot called). Onlyfalse/undefinedreachesupdateMonetizationSettings(route.ts:117-124), so create-time pricing defaults persist while monetization stays off. - No money moves while disabled.
packages/cloud/shared/src/lib/services/app-credits.ts:531-534("Only apply markup if monetization is enabled") and creator-earnings writes at :387/:594 are all gated onapp.monetization_enabled, so a draft app carryinginference_markup_percentage: 25charges base cost only. - The authoritative gate is server-side and unchanged:
packages/cloud/api/v1/apps/[id]/monetization/route.ts:134-146requiresisAppMonetizationApproved(app)(approved + content-hash re-gate,app-review.ts:436-445) to enable. The new UIdisabled={!reviewApproved}switch is defense-in-depth only — no new trust placed in the client. - Fresh-app flow is a real path, not a different error: the Monetize tab now renders review status, a "Submit for review" button for draft/rejected, the rationale, refreshes app state via
invalidateQueries(appQueryKey), and enables the toggle on approval. The jsdom test drives draft -> submit -> approved -> enable end-to-end including the PUT payload. This closes the #11801 dead-end.
Blocking finding (pre-existing, but same gate class and trivially fixable with this PR's own pattern):
POST /api/v1/apps/backup/restoreaccepts a FULLY CLIENT-SUPPLIED blob (packages/cloud/api/v1/apps/backup/restore/route.ts:31-36—monetization: { enabled: z.boolean(), ... }) andrestoreAppreapplies it with no review check:packages/cloud/shared/src/lib/services/app-backup.ts:120-124callsappCreditsService.updateMonetizationSettings(created.app.id, { monetizationEnabled: backup.monetization.enabled, ... }). Any user can craft{backup: {..., monetization: {enabled: true, inference_markup_percentage: 1000, ...}}}and get a brand-newreview_status: draftapp with monetization ON — collecting markup/creator earnings on an unapproved app. That is exactly the bypass this PR closes at create-time, one endpoint over. Fix: inrestoreApp, persist pricing but forcemonetizationEnabled: falseand surface a "re-submit for review to re-enable monetization" warning (mirrors route.ts:109-111). Happy to LGTM immediately once restore is gated here or a blocking follow-up issue is filed and linked.
Non-blocking:
plugins/plugin-cloud-apps/src/actions/create-app.ts:196sendsmonetization_enabled: truefor "create a monetized app" agent asks — that flow now hard-403s with NO app created (previously created+monetized). Fail-closed, so money-safe, but consider downgrade-and-warn via the existingwarningsarray (route.ts:107) instead of rejecting, matching the UI's new UX.- The create 403's hardcoded
review_status: "draft"is accurate (no app exists yet) and gives clients the same shape as the PUT 403 — good.
Tests are appropriate for the money surface (route-level integration asserting no side effects on 403, e2e updated, UI flow test). Verdict stands on the restore bypass alone.
[cloud-audit]
|
[cloud-security] LGTM — Fable-verified, and this is the linchpin that closes the monetize-without-review HIGH class. POST /api/v1/apps now 403s Class closure verified: every backend flag-ENABLE path is now gated — create (this PR), restore (#11843 forces off + warns), update (PUT /apps/[id]/monetization already gates on Two residuals — both NOT enable paths, deliberate/documented (flagging for awareness, not blocking): (1) an approved+enabled app that's materially edited keeps accruing inference markup until re-review — but new paid charges are cut immediately by the hash check (bait-and-switch window, per the DECISION comment); (2) grandfathered legacy approvals (no Nit worth addressing: SDK type |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Correction to the previous comment: the SDK nit is addressed in 7d73481 by documenting CreateAppInput.monetization_enabled: true as returning app_review_required until review approval, and by adding the review_status, review_content_hash, and reviewed_at fields to SDK AppDto. Verification: bun run --cwd packages/cloud/sdk typecheck passed. |
|
Heads-up (coordination, no overlap with this diff): while verifying this PR's lane I confirmed the re-review-REJECTION sibling gap and shipped it as #11872 (issue #11870) — #11872 composes with this PR rather than duplicating it: create-time gate (this PR) + restore gate (#11843) + rejection flip + an [cloud-security] |
7d73481 to
df0aa37
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Rebased onto current The old changes-requested blocker is addressed in the combined state now on this branch:
Fresh local verification on the rebased branch:
CI is rerunning on the new head; I’ll address any actionable failures. |
|
Status update after rebasing/pushing the latest branch:
GitHub is still showing the previous CHANGES_REQUESTED state, so this needs a fresh review/approval or dismissal of the stale review state before it can merge. |
…pps earning inference markup (#11870) A re-review BAN set review_status='rejected' but left monetization_enabled true, and the creator-earnings path (deductCredits/reconcileCredits/ processPurchase) gates on that flag alone — only NEW paid charges checked isAppMonetizationApproved. A rejected (prohibited-category) app therefore kept collecting inference markup on every chat/generate-image/messages call and stayed publicly usable, contradicting the invariant documented at api/v1/apps/[id]/route.ts ("a rejected re-review DOES cut everything off"). - runAppReview: a rejection now flips monetization_enabled=false in the same transaction (pricing preserved; re-enable requires fresh approval via PUT /apps/:id/monetization). Composes with the create-time gate (#11828) and the restore gate (#11834/#11843). - Earnings math derives its effective flag from isAppMonetizationActive (enabled AND not rejected) so rows persisted rejected+enabled before this fix earn nothing either; the draft re-gate deliberately keeps accruing per the documented grandfather DECISION. - Real-PGlite ledger proof: approved+enabled earns 25% markup; a re-review ban (real runAppReview, deterministic pre-filter) revokes the flag and later calls earn ZERO; legacy rejected+enabled rows earn nothing (markup + purchase share). Red against pre-fix source, green with the fix. Refs #11834 #11843. Closes #11870. [cloud-security]
|
@NubsCarson could you refresh the review state on this PR when you have a chance? The blocking restore bypass from your CHANGES_REQUESTED review is now covered by merged #11843, #11839 is merged, and this branch has been rebased/pushed on current develop with local root verify passing. Your later LGTM comment appears to confirm the class is closed once those PRs merge; GitHub is still carrying the old CHANGES_REQUESTED state from commit 3e6d777. |
df0aa37 to
c7b4c1d
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Final sync update:
Current GitHub state: mergeable is MERGEABLE, CI is rerunning on the new head, and the remaining non-CI blocker is the stale CHANGES_REQUESTED review from the old restore-bypass finding. |
…gle (#11801) The #11801 submit-for-review wire-up itself landed in #11828 (review-status card + Submit button + approved-gated Switch). This adds the missing direct state coverage: an approved app renders an enabled toggle with no submit button; rejected keeps the toggle gated and offers resubmission; pending (under_review) stays gated with no button. Mutation-checked: dropping disabled={!reviewApproved} reds 3 of the 4 tests.
…o 403 dead-end); monetization switch not trapped ON for legacy rows; i18n labels (#11828 review fixes) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Fixes #11801
Summary
monetization_enabled: truewith403 app_review_requiredbefore creating a draft app.Submit for review, refresh app state after review, and disable monetization enablement until approval.AppDtoreview fields plus backend and UI regression coverage.develop: remove duplicate Worker shim export and reduce type-safety ratchet counts so root verification passes.Evidence
.github/issue-evidence/11801-app-monetization-review/README.mdbun test packages/cloud/api/__tests__/apps-crud.integration.test.ts- PASSbun run --cwd packages/ui test -- src/cloud/applications/components/app-monetization-settings.test.tsx- PASSbun run --cwd packages/cloud/api typecheck- PASSbun run --cwd packages/cloud/shared typecheck- PASSbun run --cwd packages/cloud/sdk typecheck- PASSbun run --cwd packages/ui typecheck- PASSbun run --cwd plugins/plugin-slack typecheck- PASSbun run --cwd packages/agent typecheck- PASSbun run --cwd packages/cloud/api lint- PASSbun run --cwd packages/cloud/shared lint- PASSbun run --cwd packages/ui lint- PASSbun run --cwd packages/app audit:app- PASS, 349/349 Playwright audit checks;/appsmanual review verdicts weregood.REQUIRE_E2E_SERVER=0 bun test packages/cloud/api/test/e2e/group-i-apps-lifecycle.test.ts- PASS with 33 counted skips because the local Cloud Worker and test API keys were unavailable.bun run verify- PASS after rebasing onto currentorigin/develop. Ratchet summary:as unknown as74/75,?? ""615/615,?? {}375/377; Turbo reported 483/483 typecheck/lint tasks successful and dist-path consumers checked 28 configs.N/A Evidence