Skip to content

Add BTCPay as an alternative cloud billing provider#305

Open
schjonhaug wants to merge 7 commits into
masterfrom
schjonhaug/issue-163
Open

Add BTCPay as an alternative cloud billing provider#305
schjonhaug wants to merge 7 commits into
masterfrom
schjonhaug/issue-163

Conversation

@schjonhaug

Copy link
Copy Markdown
Owner

Summary

  • add a provider-aware cloud billing flow so /api/billing/* can use Stripe or BTCPay
  • add BTCPay cloud subscription checkout and pricing configuration for Personal and Team plans
  • update the billing/subscription UI to stop assuming Stripe-only management and support BTCPay success redirects

Validation

  • cargo check
  • cargo test donations -- --nocapture
  • pnpm lint
  • pnpm exec jest --runInBand src/components/__tests__/plans-modal-basic.test.tsx

Notes

  • pnpm build is blocked in the sandbox because next/font cannot fetch Google fonts without network access.
  • pnpm exec tsc --noEmit reports existing repo-wide test typing issues unrelated to this change (missing Jest globals in test files).

@schjonhaug schjonhaug force-pushed the schjonhaug/issue-163 branch from b131fd3 to d4a9431 Compare April 8, 2026 10:47
@github-actions

github-actions Bot commented Apr 8, 2026

Copy link
Copy Markdown

⚠️ Claude code review failed. Check the workflow logs for details.

@github-actions

github-actions Bot commented Apr 8, 2026

Copy link
Copy Markdown

🤖 Gemini Code Review

Generic billing endpoints in api.ts look good. Ready to review.

Pull Request Review: BTCPay Cloud Billing Integration

This PR introduces BTCPay Server as an alternative cloud billing provider alongside Stripe. While the abstraction of billing providers and the configuration management are well-executed, there is a critical functional gap in the BTCPay subscription fulfillment flow that will prevent users from actually being upgraded after payment.


🛑 Critical Issues & Bugs

  1. Broken Subscription Fulfillment (BTCPay):

    • Missing User Linkage: In backend/src/btcpay_client.rs, the create_cloud_subscription_checkout method creates a checkout session but does not pass the user_id or any metadata to BTCPay. Without this, even if a payment is made, the system has no way of knowing which Canary user to upgrade.
    • Missing Webhooks: The backend lacks a webhook listener for BTCPay (e.g., /api/btcpay/webhook). Stripe subscriptions rely on webhooks to update the database; BTCPay requires a similar mechanism to handle asynchronous payment confirmations.
    • Frontend Race Condition: frontend/src/app/subscription/success.tsx attempts to refreshBillingStatus() after a 2-second delay for BTCPay. Since the backend is never notified of the payment (no user link, no webhook), this refresh will always show the old subscription status, leading to a confusing user experience.
  2. Stripe-Specific Logic in Generic Endpoint:

    • The get_checkout_session_details handler in backend/src/handlers/billing.rs is now mounted at a generic /api/billing/session/{session_id} route but explicitly fails if the active provider is not Stripe. While BTCPay might not support status polling in the same way, the frontend BillingSuccessPage still attempts to call this for Stripe sessions, but for BTCPay it hits a "pending" state that never resolves.

✨ Code Quality & Best Practices

  • Provider Abstraction: The introduction of BillingProvider and the refactoring of billing.rs to use generic handlers is excellent. This significantly cleans up the api.rs router and prepares the codebase for future providers.
  • Robust Configuration: backend/src/config.rs handles the complex logic of provider precedence (Stripe wins if both are configured) and validation very well. The added unit tests for configuration edge cases are a great addition.
  • Client Implementation: BtcPayClient is clean and follows the established patterns in the codebase, utilizing reqwest with appropriate timeouts.

⚙️ Performance & Security

  • No Secret Leaks: Configuration is handled strictly via environment variables.
  • Synchronous Pricing: get_billing_pricing for BTCPay is synchronous and efficient, as it reads from the local configuration rather than making external API calls.
  • I/O Safety: All external calls to BTCPay include timeouts, preventing the backend from hanging on slow provider responses.

🧪 Test Coverage

  • Good: Unit tests for AppConfig cover the new billing provider logic.
  • Good: Frontend tests in plans-modal-basic.test.tsx were updated to account for BTCPay's different management capabilities (no portal).
  • Missing: There are no integration or system tests verifying the end-to-end BTCPay billing flow (checkout -> payment simulation -> status update). Given the fulfillment issues noted above, these tests would likely have caught the missing linkage.

💡 Recommendations

  1. Implement Metadata Linkage: Update create_cloud_subscription_checkout to pass the user_id as metadata (or orderId) to BTCPay.
  2. Add BTCPay Webhook: Implement a webhook handler that validates BTCPay signatures and updates the user's subscription_tier and subscription_status in the metadata_db.
  3. Refine Success Page: Instead of a hardcoded 2-second wait, consider a polling mechanism or a WebSocket notification once the backend receives the BTCPay webhook.
  4. UI Clarification: Since BTCPay currently only supports monthly plans (returning BAD_REQUEST for yearly), the frontend should ideally disable or hide the yearly toggle when BTCPay is the active provider.

@github-actions

github-actions Bot commented Apr 8, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  1. High – BTCPay checkout is not correlated to the authenticated user
  • In backend/src/handlers/billing.rs:63, the user is loaded, but in the BTCPay path the checkout call only sends offering/plan/redirect and does not include any user identifier or metadata.
  • The BTCPay payload in backend/src/btcpay_client.rs:161 contains only storeId, offeringId, planId, and successRedirectLink.
  • Risk: after payment, there may be no deterministic way to map the subscription event back to the Canary user account (especially if webhook/event handling needs an internal user key).
  • Suggestion: include a signed/internal user reference in BTCPay-supported metadata fields (or persist a pending checkout record before redirect) and validate it on callback/webhook.
  1. Medium – Non-functional “manage/reactivate/update payment” buttons for providers without portal support
  • handleManageBilling now no-ops when can_manage_billing is false in frontend/src/app/subscription/page.tsx:39.
  • But action buttons in canceled/expired/past_due states are still rendered unconditionally in frontend/src/app/subscription/page.tsx:189, :211, :233.
  • Result: users can click CTA buttons that do nothing (bad UX and likely support noise).
  • Suggestion: hide/disable these buttons when !can_manage_billing, or route them to upgrade checkout with provider-specific copy.
  1. Low – Test coverage is missing for new multi-provider billing behavior
  • This PR adds config tests (good), but I don’t see coverage for key new billing flows in handlers:
    • provider selection precedence in /billing/checkout
    • BTCPay yearly rejection (unsupported_billing_period)
    • /billing/portal behavior when provider cannot manage billing
    • /billing/session/{id} Stripe-only enforcement
  • Suggestion: add handler-level tests around these branches to prevent regressions in the provider switch logic.

@github-actions

github-actions Bot commented Apr 8, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Pull Request Review: BTCPay Server Billing Integration

This PR adds BTCPay Server as an alternative cloud billing provider alongside Stripe. The implementation is reasonably structured, but there are several critical issues that should be addressed before shipping.


Critical Issues

1. No BTCPay Webhook Handler — Subscriptions Never Activate

backend/src/btcpay_client.rs, backend/src/handlers/billing.rs

There is no BTCPay webhook receiver. After a user pays via BTCPay and is redirected back, the backend has no mechanism to receive payment confirmation from BTCPay. Subscription status will remain in its pre-payment state indefinitely.

The success page (frontend/src/app/subscription/success.tsx, lines 43–55) acknowledges this by setting status: 'pending' and scheduling a single refreshBillingStatus() call after 2 seconds — but that poll will see no change because no payment event was ever recorded. A signed BTCPay IPN/webhook endpoint calling update_user_subscription is required to complete the flow.

2. Cancelled/Expired Status Buttons Silently No-op for BTCPay Users

frontend/src/app/subscription/page.tsx, lines 199, 221, 243, 284

handleManageBilling immediately returns if can_manage_billing is false. For BTCPay subscribers this is always false (the backend gates it on Stripe credentials at billing.rs line 380). The "Reactivate", "Update Payment", etc. buttons all invoke handleManageBilling and silently do nothing with no feedback to the user.

3. BTCPay Pending Status on Success Page Is Permanent

frontend/src/app/subscription/success.tsx, lines 43–55

Without a webhook, isSuccessful remains false and the user sees "Processing" state permanently. There is no guidance about on-chain confirmation wait times or what to expect next. The UX is misleading.


Security Concerns

4. Donation Endpoints Are Unauthenticated and Rate-unlimited

backend/src/api.rs, lines 439–446; backend/src/handlers/donations.rs

GET /api/donations/one-time and GET /api/donations/recurring require no authentication and have no rate limiting. Each request calls the BTCPay API and creates a new invoice. An unauthenticated attacker could flood your BTCPay Server with invoice creation requests.

5. No SSRF Validation on BTCPAY_URL

backend/src/btcpay_client.rs, lines 54–67

The base_url env var is accepted without validating it starts with https://. While it's static at startup, adding a check in AppConfig::load() consistent with the existing CANARY_MEMPOOL_URL validation would be defensive.

6. BTCPay Error Bodies Could Log Sensitive Data

backend/src/btcpay_client.rs, lines 91–96

Full HTTP error bodies from BTCPay are included in error messages. If BTCPay reflects authorization headers back in errors, the API key could appear in logs. Use a generic error message and log the detail at trace level.


Architectural Issues

7. Duplicated active_billing_provider() Logic

backend/src/config.rs lines 398–406 and backend/src/handlers/billing.rs lines 21–33

Two independent implementations of the same decision logic exist and can diverge. The handler version is authoritative at runtime, but config.rs's version gives a false impression of being the source of truth. The handler should delegate to AppConfig::active_billing_provider().

8. is_stripe_enabled() Re-reads Env Vars on Every Call

backend/src/config.rs, lines 391–394

pub fn is_stripe_enabled(&self) -> bool {
    Self::non_empty_env_var("STRIPE_SECRET_KEY").is_some()
        && Self::non_empty_env_var("STRIPE_WEBHOOK_SECRET").is_some()
}

Stripe keys are re-read from env on every call, while BTCPay credentials are stored in struct fields at load time. This is inconsistent and harder to test.

9. btcpay_cloud_plan_config() Re-reads Env Vars on Every Call

backend/src/config.rs, lines 433–464

Six env vars are re-read every time this is called. It should be parsed once in AppConfig::load() and stored as Option<BtcPayPlanConfig>, like other BTCPay fields.

10. Unnecessary Wrapper Functions Creating Duplicate Routes

backend/src/handlers/billing.rs, lines 772–806

create_stripe_checkout_session and create_stripe_customer_portal are pure aliases mounted on /stripe/checkout and /stripe/portal, with identical functionality also on /billing/checkout and /billing/portal. This doubles the API surface for no benefit. Remove the wrapper aliases or the /stripe/* routes.

11. validate_cloud_config() Requires All BTCPay Vars Unconditionally

backend/src/config.rs, lines 553–568

An operator using BTCPay for cloud billing but not donation redirects (or vice versa) is forced to supply all five BTCPay variables. These are independent features and should be validated independently.


Performance

12. BTCPay Pricing Allocates New HashMaps on Every Request

backend/src/btcpay_client.rs, lines 229–245

tier_features() creates a new HashMap with String allocations on every GET /api/billing/pricing call. The data is static. Cache the PricingInfo in the BtcPayClient struct, analogous to StripeBilling::cached_pricing.


Code Quality

13. Stale Log Messages Reference create_stripe_checkout_session

backend/src/handlers/billing.rs, line 157

info!("create_stripe_checkout_session completed in {:?}", elapsed);

The function is now create_checkout_session (provider-agnostic). Same stale name at lines 250 and 256 in create_customer_portal.

14. alert() Used for Error Feedback

frontend/src/app/wallets/add/[[...slug]]/page.tsx, line 89

A new alert(t('add.checkoutError')) was added in this PR. This is inconsistent with the project's shadcn/ui toast notification patterns.

15. session_id Is Misleading for BTCPay

backend/src/handlers/billing.rs, line 150

session_id: format!("btcpay-{}", tier.as_str()),

This synthetic static string is returned to the frontend but immediately rejected when used. It should be Option<String>None for BTCPay where no session ID concept applies.


Test Coverage

16. No Integration Tests for BTCPay Billing Flow

tests/stripe_integration_tests.rs covers Stripe comprehensively. There are no equivalent tests for:

  • create_checkout_session with BTCPay provider
  • get_billing_pricing with BTCPay provider
  • get_billing_status with BTCPay active
  • donate_one_time / donate_recurring paths

The BtcPayPlanConfig parsing unit tests in config.rs are good, but handler-level integration is untested.


Summary

Priority Issue
Critical No BTCPay webhook → subscriptions never activate
Critical Cancelled/expired action buttons silently no-op for BTCPay users
Security Unauthenticated, rate-unlimited donation invoice creation endpoints
Architecture Duplicated active_billing_provider() logic that can diverge
Architecture btcpay_cloud_plan_config() / is_stripe_enabled() re-read env vars on every call
Architecture Remove duplicate /stripe/* wrapper routes
Architecture Over-strict validate_cloud_config() couples unrelated features
Quality Stale log messages, alert() usage, misleading session_id
Testing No integration tests for the BTCPay billing flow

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