From 3a87a766f03340c43d4f9143a1e5eefd08b63989 Mon Sep 17 00:00:00 2001 From: akansmafengadamu Date: Fri, 16 Oct 2026 21:10:44 +0100 Subject: [PATCH] fix(billing,payments,security): harden payment, billing and platform correctness Closes #1683 Closes #1684 Closes #1685 Closes #1686 Crypto payment resilience (#1683) - Add src/services/transactionConfirmation.ts: a single receipt-confirmation path that requires chain-appropriate confirmation depth, rejects receipt.status === 0 instead of reporting a revert as success, and enforces its own timeout. ethers v5 ContractTransaction.wait() silently ignores a timeout argument, so delegating the bound to it left every contract-based payment able to hang and stall the retry queue queued behind it. - Route Superfluid creation, Sablier creation/approval and ERC-20 approval through that helper. Remove a duplicate PaymentMethodService import that was a redeclaration error. - rpcProvider: serialise access to the shared connection.url, so concurrent calls (getBalancesAcrossChains fans out in parallel) can no longer send a request to the wrong endpoint. Only transport failures now count against a circuit; a JSON-RPC revert previously evicted healthy nodes. Require a success threshold to close a half-open circuit. - paymentMethodService: compare fractional spend caps with an exact fixed-point comparison. BigNumber.from("12.5") throws, and the throw was swallowed, so every fractional amount failed the cap check. Read token decimals from the contract instead of assuming 6/18, and use resilient providers for balance, gas and bytecode reads. - transactionQueueStore: persist a broadcast marker before handing a payload to the wallet, so a crash between submission and receipt cannot replay a payment. Retire rather than re-send items whose outcome is unknown, add exponential backoff, measure staleness from the last attempt, and remove the unhandled rejections that could terminate the app. - walletStore: read the real PaymentAttempt fields (status/attemptedAt). Reading non-existent success/createdAt made every method score a 100% success rate, leaving smart fallback ordering permanently inert. Also clear isLoading when verification returns false. Billing correctness (#1684) - Add src/utils/invoiceTotals.ts and use it in createInvoice/updateInvoice. updateInvoice dropped the discount, so editing any line on a discounted invoice raised the amount payable; the discount was also never persisted. Clamp discounts to the subtotal so a total can never go negative. - Fix an infinite average invoice amount when invoices exist but none are paid (Infinity is truthy, so `x / 0 || 0` returned Infinity). - Add src/utils/billingCycleMath.ts as the one billing-cycle normaliser. subscriptionStore omitted the annual cycle, so a 120/year plan reported 120 MRR instead of 10; analyticsService and analyticsSlice hard-coded 4.345 weeks/month while BILLING_CONVERSIONS uses 52/12. - monthlyRevenueRecognitionJob: recognise only the increment since the last run. Partial recognition was never marked complete, so the full elapsed portion was re-recognised on every run, compounding daily. Track cumulative per-entry progress and clamp deferred revenue at zero. - groupBilling: price the actual invoiced period rather than "the last N days relative to now", clamp ownerDiscount to 0-100, and apply tax. - InvoiceGenerator: clamp the discount against the subtotal. - meteringService: bill only usage inside the period being invoiced (it summed all lifetime history while labelling it one month), fix the off-by-one period end, and stop same-millisecond events from colliding into one dropped-by-idempotency event. - batchChargeService: fix the `RunHistory` field typo that made every batch charge throw. Platform security (#1686) - pagination: remove the published CURSOR_HMAC_SECRET fallback. A known cursor secret let anyone forge a cursor whose `field` is interpolated into SQL. Require the secret in production, compare signatures in constant time, and reject any sort/field identifier outside [A-Za-z0-9_]. - createApiServer: remove the published WEBHOOK_SIGNING_KEY fallback, which allowed forging every outbound webhook signature. Fail closed in production. - ApiKeyRotationService: hash keys with a keyed HMAC rather than unsalted SHA-256, so a leaked key table cannot be attacked offline. - Add backend/shared/security/outboundUrlGuard.ts and apply it to Zapier hook registration, hook delivery and webhook replay, closing an SSRF path to cloud metadata and RFC1918 hosts. Validate at both registration and send time so already-stored targets are covered too. - eventReplayWorker: sign with a real HMAC. The previous "sha256=" prefix wrapped a 31-bit rolling hash, so replayed deliveries were forgeable. - server.ts: resolve the rate-limit tier from the server-side record instead of the client-supplied x-subscription-tier header, which granted a 100x allowance to anyone who asked. Opt back in with TRUST_TIER_HEADER=true behind a trusted proxy. - server.ts: apply a 256 KiB default body cap. 31 of 32 routes read unbounded bodies, and the HubSpot webhook accumulated chunks before signature checks. - gateway/createApiServer: bound express.json() (10mb -> 256kb) and add Referrer-Policy and X-DNS-Prefetch-Control. - revalidate: compare the secret in constant time and allowlist revalidate paths instead of passing caller input straight to res.revalidate(). Tests (#1685) - Add suites for the new modules (transactionConfirmation, decimalAmount, invoiceTotals, billingCycleMath, outboundUrlGuard) and extend the transaction queue and RPC provider suites to cover the idempotency, backoff and circuit-breaker behaviour. Update the group billing test to the explicit period signature. - Document the new required secrets in .env.example. Deployment notes - CURSOR_HMAC_SECRET, WEBHOOK_SIGNING_KEY and API_KEY_HASH_PEPPER must be set to random values of at least 16 characters in production; startup fails otherwise. Outside production an ephemeral value is generated with a warning. - Rate-limit tiers are no longer self-declared by clients. Deployments behind an authenticating proxy that sets x-subscription-tier should set TRUST_TIER_HEADER=true and ensure the proxy strips the header from inbound traffic. --- .env.example | 20 ++ backend/billing/domain/InvoiceGenerator.ts | 31 +- backend/billing/jobs/index.ts | 1 + .../jobs/monthlyRevenueRecognitionJob.ts | 98 +++++- backend/gateway/index.ts | 8 +- .../zapier/ZapierIntegrationService.ts | 14 +- backend/integrations/zapier/zapierRouter.ts | 19 +- backend/server.ts | 75 +++-- backend/server/createApiServer.ts | 50 ++- .../analytics/subscriptionAnalyticsService.ts | 11 +- .../auth/domain/ApiKeyRotationService.ts | 55 +++- backend/services/batchChargeService.ts | 2 +- backend/services/billing/groupBilling.ts | 65 +++- backend/services/billing/interfaces.ts | 2 +- backend/services/billing/meteringService.ts | 55 +++- backend/services/shared/pagination.ts | 97 +++++- .../services/shared/rateLimitingService.ts | 14 + backend/services/webhook/eventReplayWorker.ts | 23 +- .../__tests__/outboundUrlGuard.test.ts | 149 +++++++++ backend/shared/security/outboundUrlGuard.ts | 184 +++++++++++ developer-portal/pages/api/revalidate.ts | 36 ++- src/services/__tests__/groupBilling.test.ts | 18 +- src/services/__tests__/rpcProvider.test.ts | 166 ++++++++++ .../__tests__/transactionConfirmation.test.ts | 261 ++++++++++++++++ src/services/analyticsService.ts | 14 +- src/services/invoiceService.ts | 48 ++- src/services/paymentMethodService.ts | 40 +-- src/services/rpcProvider.ts | 150 ++++++++- src/services/transactionConfirmation.ts | 287 ++++++++++++++++++ src/services/walletService.ts | 19 +- .../_tests_/transactionQueueStore.test.ts | 164 ++++++++++ src/store/slices/analyticsSlice.ts | 10 +- src/store/subscriptionStore.ts | 16 +- src/store/transactionQueueStore.ts | 226 +++++++++++--- src/store/walletStore.ts | 53 ++-- src/utils/__tests__/billingCycleMath.test.ts | 103 +++++++ src/utils/__tests__/decimalAmount.test.ts | 110 +++++++ src/utils/__tests__/invoiceTotals.test.ts | 140 +++++++++ src/utils/billingCycleMath.ts | 98 ++++++ src/utils/decimalAmount.ts | 105 +++++++ src/utils/invoiceTotals.ts | 92 ++++++ 41 files changed, 2919 insertions(+), 210 deletions(-) create mode 100644 backend/shared/security/__tests__/outboundUrlGuard.test.ts create mode 100644 backend/shared/security/outboundUrlGuard.ts create mode 100644 src/services/__tests__/rpcProvider.test.ts create mode 100644 src/services/__tests__/transactionConfirmation.test.ts create mode 100644 src/services/transactionConfirmation.ts create mode 100644 src/utils/__tests__/billingCycleMath.test.ts create mode 100644 src/utils/__tests__/decimalAmount.test.ts create mode 100644 src/utils/__tests__/invoiceTotals.test.ts create mode 100644 src/utils/billingCycleMath.ts create mode 100644 src/utils/decimalAmount.ts create mode 100644 src/utils/invoiceTotals.ts diff --git a/.env.example b/.env.example index 1b5849cc..7f343b4b 100644 --- a/.env.example +++ b/.env.example @@ -38,8 +38,28 @@ DB_LEAK_THRESHOLD_MS=30000 # ── Webhook Signature Verification ──────────────────────────────────────────── # HMAC-SHA256 signing key used to sign and verify outbound/inbound webhooks. # Rotate with POST /api/v1/webhooks/rotate-key (overlap window: 24h) +# REQUIRED in production: at least 16 random characters. Startup fails if unset +# or left at the example value, because a known key lets anyone forge signatures. WEBHOOK_SIGNING_KEY=change-me-to-a-long-random-secret +# ── Security Secrets (required in production) ───────────────────────────────── +# The following secrets have no safe fallback. In production the process refuses +# to start without them; outside production an ephemeral value is generated and +# a warning is logged (which means values will not survive a restart). + +# Pepper for API key digests (>=16 chars). Without it a leaked key table can be +# attacked offline; with it, a leaked table is useless on its own. +API_KEY_HASH_PEPPER= + +# HMAC key for opaque pagination cursors (>=16 chars). Cursors carry a sort +# field that is interpolated into SQL, so a known key would allow forging one. +CURSOR_HMAC_SECRET= + +# Set to `true` ONLY when an authenticating proxy sits in front of this process +# and strips `x-subscription-tier` from inbound requests. Otherwise rate-limit +# tiers are resolved from the server-side record for each API key. +TRUST_TIER_HEADER=false + # ── Zapier Integration ───────────────────────────────────────────────────────── # No server-side secrets required — merchants authenticate with their API keys. # Set this to restrict which origins can reach the Zapier endpoints. diff --git a/backend/billing/domain/InvoiceGenerator.ts b/backend/billing/domain/InvoiceGenerator.ts index ed4f3023..fc443186 100644 --- a/backend/billing/domain/InvoiceGenerator.ts +++ b/backend/billing/domain/InvoiceGenerator.ts @@ -54,6 +54,18 @@ export interface GeneratedInvoice { generatedAt: string; } +/** Rounds to minor units, mapping non-finite input to zero. */ +function roundMoney(amount: number): number { + if (!Number.isFinite(amount)) return 0; + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * 100) / 100; +} + +/** Constrains `value` to `[min, max]`, tolerating non-finite input. */ +function clamp(value: number, min: number, max: number): number { + if (!Number.isFinite(value)) return min; + return Math.min(Math.max(value, min), max); +} + export class InvoiceGenerator { /** * Generate a unique invoice number. @@ -69,10 +81,21 @@ export class InvoiceGenerator { * Calculate totals from line items and tax rate. */ calculateTotals(items: InvoiceLineItem[], taxRate: number, discount: number = 0) { - const subtotal = items.reduce((sum, item) => sum + item.total, 0); - const taxableAmount = subtotal - discount; - const taxAmount = Math.round(taxableAmount * taxRate * 100) / 100; - const total = Math.round((taxableAmount + taxAmount) * 100) / 100; + const rawSubtotal = items.reduce((sum, item) => sum + item.total, 0); + const subtotal = roundMoney(rawSubtotal); + + // A discount may not exceed the subtotal. Without this clamp a discount + // larger than the invoice produced a negative tax base and a negative + // total, which downstream reconciliation reads as a credit owed rather + // than an over-discounted invoice. + const appliedDiscount = roundMoney(clamp(discount, 0, subtotal)); + const taxableAmount = roundMoney(subtotal - appliedDiscount); + + // `taxRate` is a decimal fraction (0.2), not basis points. + const safeTaxRate = Number.isFinite(taxRate) ? Math.max(taxRate, 0) : 0; + const taxAmount = roundMoney(taxableAmount * safeTaxRate); + const total = roundMoney(Math.max(taxableAmount + taxAmount, 0)); + return { subtotal, taxAmount, total }; } diff --git a/backend/billing/jobs/index.ts b/backend/billing/jobs/index.ts index 6b4fe8ef..fa0f7568 100644 --- a/backend/billing/jobs/index.ts +++ b/backend/billing/jobs/index.ts @@ -26,6 +26,7 @@ export { export type { RevenueSchedule, RevenueScheduleEntry, + EntryRecognitionProgress, RecognitionJournalEntry, RevenueRecognitionRepository, RecognitionJobMetrics, diff --git a/backend/billing/jobs/monthlyRevenueRecognitionJob.ts b/backend/billing/jobs/monthlyRevenueRecognitionJob.ts index e7e37213..bd5580a7 100644 --- a/backend/billing/jobs/monthlyRevenueRecognitionJob.ts +++ b/backend/billing/jobs/monthlyRevenueRecognitionJob.ts @@ -19,6 +19,12 @@ export interface RevenueScheduleEntry { periodEnd: number; // Unix ms recognisedAmount: number; isRecognised: boolean; + /** + * Cumulative amount already recognised for this entry across all prior + * runs. Optional so schedules persisted before progress tracking existed + * still load; treat a missing value as 0. + */ + recognisedToDate?: number; } export interface RevenueSchedule { @@ -42,6 +48,12 @@ export interface RecognitionJournalEntry { type: 'scheduled' | 'accelerated' | 'partial'; } +/** Per-entry cumulative recognition progress recorded by a run. */ +export interface EntryRecognitionProgress { + periodStart: number; + recognisedToDate: number; +} + export interface RevenueRecognitionRepository { /** Return all schedules where at least one entry is not yet recognised. */ getPendingSchedules(): Promise; @@ -54,6 +66,19 @@ export interface RevenueRecognitionRepository { ): Promise; /** Mark individual schedule entries as recognised. */ markEntriesRecognised(subscriptionId: string, entryPeriodStarts: number[]): Promise; + /** + * Persist cumulative per-entry recognition progress. + * + * Optional. Without it a partially-elapsed entry cannot record how much of + * itself it has already recognised, and the next run re-recognises the whole + * elapsed portion — inflating recognised revenue on every run. When the + * method is absent the job degrades to recognising each entry only once, + * at its full amount, which is slower but never over-counts. + */ + recordEntryProgress?( + subscriptionId: string, + progress: EntryRecognitionProgress[] + ): Promise; } export interface RecognitionJobMetrics { @@ -70,6 +95,12 @@ function nextId(): string { return `rev-${Date.now().toString(36)}-${(++_seq).toString(36)}`; } +/** Rounds to minor units, mapping non-finite input to zero. */ +function roundMoney(amount: number): number { + if (!Number.isFinite(amount)) return 0; + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * 100) / 100; +} + /** * Determine how much of a single schedule entry to recognise as of `now`. * Returns 0 for entries that haven't started yet or amount = 0 (free trial). @@ -150,13 +181,48 @@ export class MonthlyRevenueRecognitionJob { for (const schedule of schedules) { localMetrics.schedulesProcessed += 1; const recognisedPeriodStarts: number[] = []; + const progressUpdates: EntryRecognitionProgress[] = []; let totalRecognisedForSchedule = 0; + // Deferred revenue is what was contractually billed but not yet + // recognised, i.e. the schedule total minus everything recognised on + // prior runs and earlier in *this* run. Ignoring prior runs produced a + // deferred balance that grew without bound. + const previouslyRecognised = (schedule.entries ?? []).reduce( + (sum, entry) => sum + (entry.recognisedToDate ?? 0), + 0 + ); + let recognisedSoFar = previouslyRecognised; + + const canTrackProgress = typeof this.repo.recordEntryProgress === 'function'; + for (const entry of schedule.entries) { - const { amount, type } = computeEntryRecognition(entry, now); - if (!type || amount <= 0) continue; + const { amount: cumulativeTarget, type } = computeEntryRecognition(entry, now); + if (!type || cumulativeTarget <= 0) continue; + + // Recognise only the increment since the last run. Recognising the + // full elapsed portion again on every run is what compounded the + // misstatement day after day. + const alreadyRecognised = entry.recognisedToDate ?? 0; + const amount = canTrackProgress + ? roundMoney(Math.max(cumulativeTarget - alreadyRecognised, 0)) + : // Without progress tracking, only recognise an entry once its + // period has fully elapsed, so it is never recognised twice. + type === 'partial' + ? 0 + : roundMoney(cumulativeTarget); + + if (amount <= 0) { + if (type === 'scheduled' || type === 'accelerated') { + // Already fully recognised on a prior run; close it out. + recognisedPeriodStarts.push(entry.periodStart); + } + continue; + } - const deferredBefore = schedule.totalAmount - totalRecognisedForSchedule; + const deferredBefore = roundMoney( + Math.max(schedule.totalAmount - recognisedSoFar, 0) + ); const journalEntry: RecognitionJournalEntry = { id: nextId(), subscriptionId: schedule.subscriptionId, @@ -165,19 +231,33 @@ export class MonthlyRevenueRecognitionJob { periodEnd: entry.periodEnd, recognisedAmount: amount, deferredBefore, - deferredAfter: deferredBefore - amount, + // Deferred can never go negative: recognising more than was + // deferred would be reported as an amount owed back to the merchant. + deferredAfter: roundMoney(Math.max(deferredBefore - amount, 0)), createdAt: now, type, }; try { await this.repo.writeJournalEntry(journalEntry); + recognisedSoFar += amount; totalRecognisedForSchedule += amount; localMetrics.entriesRecognised += 1; localMetrics.totalAmountRecognised += amount; - // Only mark fully-elapsed entries as done. - if (type === 'scheduled' || type === 'accelerated') { + progressUpdates.push({ + periodStart: entry.periodStart, + recognisedToDate: roundMoney(alreadyRecognised + amount), + }); + + // Only close out entries whose period has fully elapsed AND that + // are now fully recognised. + const recognisedTotal = roundMoney(alreadyRecognised + amount); + if ( + type === 'scheduled' || + type === 'accelerated' || + recognisedTotal >= roundMoney(entry.recognisedAmount) + ) { recognisedPeriodStarts.push(entry.periodStart); } } catch (err) { @@ -195,6 +275,12 @@ export class MonthlyRevenueRecognitionJob { recognisedDelta: totalRecognisedForSchedule, deferredDelta: -totalRecognisedForSchedule, }); + if (progressUpdates.length > 0 && canTrackProgress) { + await this.repo.recordEntryProgress!( + schedule.subscriptionId, + progressUpdates + ); + } if (recognisedPeriodStarts.length > 0) { await this.repo.markEntriesRecognised( schedule.subscriptionId, diff --git a/backend/gateway/index.ts b/backend/gateway/index.ts index a93d83ec..2ba528b1 100644 --- a/backend/gateway/index.ts +++ b/backend/gateway/index.ts @@ -40,13 +40,19 @@ export function createGateway(options: GatewayOptions = {}): Application { app.disable('x-powered-by'); - app.use(express.json({ limit: '10mb' })); + // Reduced from the previous blanket 10mb. A JSON control-plane body has no + // legitimate need to be that large, and the old limit let a single + // unauthenticated request buffer 10 MB per connection into memory. + app.use(express.json({ limit: '256kb' })); // ── Response envelope header ────────────────────────────────────────── app.use((_req: Request, res: Response, next: NextFunction) => { res.setHeader(API_VERSION_HEADER, API_VERSION_VALUE); res.setHeader('X-Content-Type-Options', 'nosniff'); res.setHeader('X-Frame-Options', 'DENY'); + // The gateway previously set no caching or referrer policy at all. + res.setHeader('Referrer-Policy', 'no-referrer'); + res.setHeader('X-DNS-Prefetch-Control', 'off'); next(); }); diff --git a/backend/integrations/zapier/ZapierIntegrationService.ts b/backend/integrations/zapier/ZapierIntegrationService.ts index 2643e50a..c6254940 100644 --- a/backend/integrations/zapier/ZapierIntegrationService.ts +++ b/backend/integrations/zapier/ZapierIntegrationService.ts @@ -18,6 +18,7 @@ */ import crypto from 'crypto'; +import { validateOutboundUrl } from '../../shared/security/outboundUrlGuard'; // ── Types ───────────────────────────────────────────────────────────────────── @@ -293,12 +294,17 @@ export class ZapierIntegrationService { // ── Hook Subscription Management ────────────────────────────────────────── registerHook(input: RegisterHookInput): ZapierHookSubscription { + // Re-validated at the service boundary, not only in the router: hooks can + // also be registered programmatically, and every stored `targetUrl` is + // later fetched server-side. + const target = validateOutboundUrl(input.targetUrl); + const id = this.generateId('zhook'); const now = new Date().toISOString(); const hook: ZapierHookSubscription = { id, merchantId: input.merchantId, - targetUrl: input.targetUrl, + targetUrl: target.toString(), event: input.event, signingSecret: this.generateSigningSecret(), active: true, @@ -380,7 +386,11 @@ export class ZapierIntegrationService { const signature = this.signPayload(body, hook.signingSecret); try { - const response = await fetchImpl(hook.targetUrl, { + // Re-validate at send time. A hook persisted before this guard existed + // would otherwise remain an SSRF vector forever. + const target = validateOutboundUrl(hook.targetUrl); + + const response = await fetchImpl(target.toString(), { method: 'POST', headers: { 'Content-Type': 'application/json', diff --git a/backend/integrations/zapier/zapierRouter.ts b/backend/integrations/zapier/zapierRouter.ts index c44d3227..a6eb0f1a 100644 --- a/backend/integrations/zapier/zapierRouter.ts +++ b/backend/integrations/zapier/zapierRouter.ts @@ -32,6 +32,9 @@ import { type RegisterHookInput, type ZapierTriggerEvent, } from './ZapierIntegrationService'; +import { + validateOutboundUrl, +} from '../../shared/security/outboundUrlGuard'; function asyncHandler(fn: (req: Request, res: Response) => Promise) { return (req: Request, res: Response): void => { @@ -122,9 +125,23 @@ export function createZapierRouter(service: ZapierIntegrationService): Router { return; } + // The registered URL is fetched server-side on every event delivery, so + // an unvalidated value here is an SSRF primitive: it would let a caller + // point the platform at cloud metadata, loopback or RFC1918 services. + let normalizedTarget: string; + try { + normalizedTarget = validateOutboundUrl(targetUrl).toString(); + } catch (error) { + res.status(400).json({ + success: false, + error: error instanceof Error ? error.message : 'Invalid targetUrl', + }); + return; + } + const input: RegisterHookInput = { merchantId, - targetUrl, + targetUrl: normalizedTarget, event: event as ZapierTriggerEvent, }; diff --git a/backend/server.ts b/backend/server.ts index b673d50a..591c8b20 100644 --- a/backend/server.ts +++ b/backend/server.ts @@ -117,13 +117,31 @@ function decodeJwtSubject(token: string): string | undefined { } } -/** Resolve the caller tier from the x-subscription-tier header (default FREE). */ -function resolveTierFromRequest( - req: { headers: Record }, -): SubscriptionTier { - const raw = req.headers['x-subscription-tier']; - const value = typeof raw === 'string' ? raw : Array.isArray(raw) ? raw[0] : undefined; - return parseSubscriptionTier(value ?? null) ?? SubscriptionTier.FREE; +/** + * Resolve the caller's rate-limit tier for an API key. + * + * This previously read the client-supplied `x-subscription-tier` header + * verbatim, so any caller presenting a key could send + * `X-Subscription-Tier: enterprise` and receive the 10,000 req/hr allowance + * instead of the FREE tier's 100 req/hr — a 100× rate-limit bypass requiring + * no privilege at all. + * + * The tier now comes from the server-side record for that key. The header is + * honoured only when an explicit trusted-proxy opt-in is configured, which + * suits deployments where an authenticating gateway sits in front of this + * process and strips the header from inbound requests. + */ +function resolveTierForApiKey(apiKey: string, req: { + headers: Record; +}): SubscriptionTier { + if (process.env['TRUST_TIER_HEADER'] === 'true') { + const raw = req.headers['x-subscription-tier']; + const value = typeof raw === 'string' ? raw : Array.isArray(raw) ? raw[0] : undefined; + const parsed = parseSubscriptionTier(value ?? null); + if (parsed) return parsed; + } + + return rateLimitingService.getTierForApiKey(apiKey); } /** @@ -161,11 +179,9 @@ function buildRateLimitMiddleware() { '/metrics/pool', '/metrics/build', ], - // Per-key tier: read x-subscription-tier header; defaults to FREE. - getTier: (apiKey, req) => { - void apiKey; - return resolveTierFromRequest(req); - }, + // Per-key tier: resolved from the server-side record, not from a + // client-supplied header. See resolveTierForApiKey. + getTier: (apiKey, req) => resolveTierForApiKey(apiKey, req), // Per-user aggregate limiting: x-user-id header or Bearer JWT sub claim. getUserId: (req) => resolveUserIdFromRequest(req), }); @@ -244,21 +260,32 @@ class HttpError extends Error { /** Upper bound on an ingested CI report body (1 MiB). */ const MAX_BUILD_REPORT_BYTES = 1024 * 1024; +/** + * Default ceiling for any other JSON request body (256 KiB). + * + * `maxBytes` was optional and 31 of 32 call sites omitted it, so every + * non-build-metrics route buffered an unbounded amount into memory before + * parsing. A single oversized POST was enough to exhaust the heap. A sane + * default is applied everywhere; individual routes may still raise it. + */ +const MAX_DEFAULT_BODY_BYTES = 256 * 1024; + /** * Read and parse a JSON request body. * - * @param maxBytes - Optional hard cap; exceeding it aborts with 413 so an - * untrusted upload cannot buffer unbounded memory. Omit for trusted routes - * to preserve the previous unlimited behaviour. + * @param maxBytes - Optional explicit cap; defaults to MAX_DEFAULT_BODY_BYTES. + * Exceeding it aborts with 413 so an untrusted upload cannot buffer + * unbounded memory. Set `Infinity` only for genuinely trusted routes. */ async function readJsonBody(req: http.IncomingMessage, maxBytes?: number): Promise { + const limit = maxBytes ?? MAX_DEFAULT_BODY_BYTES; const chunks: Buffer[] = []; let received = 0; for await (const chunk of req) { const buf = typeof chunk === 'string' ? Buffer.from(chunk) : chunk; received += buf.length; - if (maxBytes !== undefined && received > maxBytes) { - throw new HttpError(413, `request body exceeds ${maxBytes} bytes`); + if (Number.isFinite(limit) && received > limit) { + throw new HttpError(413, `request body exceeds ${limit} bytes`); } chunks.push(buf); } @@ -1576,9 +1603,21 @@ export async function startServer(options: StartServerOptions = {}): Promise((resolve, reject) => { const chunks: Buffer[] = []; - req.on('data', (c: Buffer) => chunks.push(c)); + let received = 0; + req.on('data', (c: Buffer) => { + received += c.length; + if (received > MAX_DEFAULT_BODY_BYTES) { + reject(new HttpError(413, `request body exceeds ${MAX_DEFAULT_BODY_BYTES} bytes`)); + req.destroy(); + return; + } + chunks.push(c); + }); req.on('end', () => resolve(Buffer.concat(chunks).toString('utf8'))); req.on('error', reject); }); diff --git a/backend/server/createApiServer.ts b/backend/server/createApiServer.ts index 6a0982a8..8d2d1f0c 100644 --- a/backend/server/createApiServer.ts +++ b/backend/server/createApiServer.ts @@ -14,6 +14,8 @@ * /api/v1/webhooks/verify — Incoming webhook signature verification demo */ +import { randomBytes } from 'node:crypto'; + import express, { type Express, type Request, type Response } from 'express'; import { cacheHeadersMiddleware } from '../shared/middleware'; import { @@ -104,7 +106,45 @@ const renewalScheduler = new RenewalReminderScheduler(calendarSyncService); const calendarController = createCalendarSyncController({ syncService: calendarSyncService }); const reminderController = createRenewalReminderController({ scheduler: renewalScheduler }); -const webhookKeyStore = new KeyStore(process.env['WEBHOOK_SIGNING_KEY'] ?? 'default-dev-key-change-in-prod'); +/** + * Resolves the outbound webhook signing key. + * + * This previously fell back to the literal `'default-dev-key-change-in-prod'`. + * Because the same key signs every webhook the platform emits, a single + * deployed default meant anyone reading the repository could forge a valid + * signature for arbitrary merchant webhooks. + * + * In production an unset or default value is a hard startup failure. Outside + * production an ephemeral random key is generated so local development still + * works without silently weakening verification. + */ +function resolveWebhookSigningKey(): string { + const configured = process.env['WEBHOOK_SIGNING_KEY']; + const placeholder = 'default-dev-key-change-in-prod'; + + if (configured && configured !== placeholder && configured.length >= 16) { + return configured; + } + + if (process.env['NODE_ENV'] === 'production') { + throw new Error( + 'WEBHOOK_SIGNING_KEY must be set to a random value of at least 16 characters in production.' + ); + } + + if (configured === placeholder) { + console.warn( + '[createApiServer] WEBHOOK_SIGNING_KEY is still the public default; using an ephemeral key for this process.' + ); + } + + return randomBytes(32).toString('hex'); +} + +const webhookKeyStore = new KeyStore(resolveWebhookSigningKey()); + +/** Ceiling for request bodies accepted by the public API (256 KiB). */ +const MAX_JSON_BODY = '256kb'; export interface CreateApiServerOptions { /** Optional middleware applied before cache headers (e.g. auth). */ @@ -121,10 +161,12 @@ export function createApiServer(options: CreateApiServerOptions = {}): Express { // registered for the webhook paths *before* the JSON parser, so // `req.body` is a Buffer holding the untouched payload. `type: '*/*'` is // required because providers do not agree on a Content-Type. - app.post('/api/v1/stripe/billing/webhook', express.raw({ type: '*/*' })); - app.post('/api/v1/paddle/webhook', express.raw({ type: '*/*' })); + app.post('/api/v1/stripe/billing/webhook', express.raw({ type: '*/*', limit: MAX_JSON_BODY })); + app.post('/api/v1/paddle/webhook', express.raw({ type: '*/*', limit: MAX_JSON_BODY })); - app.use(express.json()); + // An explicit cap keeps a single oversized POST from buffering the whole + // body into memory before it is parsed. + app.use(express.json({ limit: MAX_JSON_BODY })); if (options.beforeCache) { for (const mw of options.beforeCache) { diff --git a/backend/services/analytics/subscriptionAnalyticsService.ts b/backend/services/analytics/subscriptionAnalyticsService.ts index 226b3d68..8f811ca4 100644 --- a/backend/services/analytics/subscriptionAnalyticsService.ts +++ b/backend/services/analytics/subscriptionAnalyticsService.ts @@ -317,9 +317,14 @@ export class SubscriptionAnalyticsService { // ── Private helpers ───────────────────────────────────────────────────────── private _monthlyRevenue(sub: Subscription): number { - // Normalise any billing cycle to monthly revenue - if (sub.billingCycle === BillingCycle.YEARLY) return sub.price / 12; - if (sub.billingCycle === BillingCycle.WEEKLY) return sub.price * 4.345; + // Normalise any billing cycle to monthly revenue. `ANNUAL` was missing + // here, so an annual subscription's full yearly price was reported as + // MRR — inflating MRR/ARR/churn by 12×. `monthlyEquivalent` also keeps + // the weekly factor identical to the frontend (52/12, not 4.345). + if (sub.billingCycle === BillingCycle.YEARLY || sub.billingCycle === BillingCycle.ANNUAL) { + return sub.price / 12; + } + if (sub.billingCycle === BillingCycle.WEEKLY) return sub.price * (52 / 12); return sub.price; } } diff --git a/backend/services/auth/domain/ApiKeyRotationService.ts b/backend/services/auth/domain/ApiKeyRotationService.ts index 09bbcdb7..db451280 100644 --- a/backend/services/auth/domain/ApiKeyRotationService.ts +++ b/backend/services/auth/domain/ApiKeyRotationService.ts @@ -1,4 +1,4 @@ -import { randomBytes, createHash } from 'crypto'; +import { randomBytes, createHmac } from 'crypto'; import { AuthError } from '../errors'; import { logger } from '../../shared/logging'; import type { ApiKeyRecord, ApiKeyRotationPolicy, IApiKeyRotationService } from '../interfaces'; @@ -8,8 +8,59 @@ const KEY_BYTE_LENGTH = 32; const KEY_HASH_ALGORITHM = 'sha256'; const MAX_HISTORY = 5; +/** + * Server-side pepper for API key hashing. + * + * API keys are high-entropy random values, so a plain hash resists brute + * force — but an unsalted SHA-256 is still trivially attackable in bulk via + * precomputation, and it means a database disclosure immediately yields + * directly comparable key material. A keyed HMAC removes that: without the + * pepper, a leaked table cannot be attacked offline at all. + * + * Required in production; ephemeral per-process outside it (with a warning), + * because a checked-in fallback pepper would be no better than none. + */ +function resolveApiKeyPepper(): string { + const configured = process.env['API_KEY_HASH_PEPPER']; + + if (configured && configured.trim().length >= 16) { + return configured; + } + + if (process.env['NODE_ENV'] === 'production') { + throw new Error( + 'API_KEY_HASH_PEPPER must be set to a random value of at least 16 characters in production.' + ); + } + + console.warn( + '[ApiKeyRotationService] API_KEY_HASH_PEPPER is not configured; using an ephemeral pepper. API keys will not resolve across restarts.' + ); + + return randomBytes(32).toString('hex'); +} + +let _pepper: string | null = null; + +/** Test seam: forces a specific pepper, or `null` to re-resolve. */ +export function __setApiKeyPepperForTesting(pepper: string | null): void { + _pepper = pepper; +} + +function apiKeyPepper(): string { + if (_pepper === null) { + _pepper = resolveApiKeyPepper(); + } + return _pepper; +} + +/** + * Deterministic keyed digest of an API key. + * + * Stays deterministic so stored hashes remain searchable by lookup. + */ export function hashApiKey(rawKey: string): string { - return createHash(KEY_HASH_ALGORITHM).update(rawKey).digest('hex'); + return createHmac(KEY_HASH_ALGORITHM, apiKeyPepper()).update(rawKey).digest('hex'); } export class ApiKeyRotationService implements IApiKeyRotationService { diff --git a/backend/services/batchChargeService.ts b/backend/services/batchChargeService.ts index 63969fa5..fa0e8833 100644 --- a/backend/services/batchChargeService.ts +++ b/backend/services/batchChargeService.ts @@ -43,7 +43,7 @@ export interface BatchChargeResult { export class BatchChargeService { private intervalHandle: ReturnType | null = null; private lastMatchTimestamp = 0; - private RunHistory: BatchChargeResult[] = []; + private runHistory: BatchChargeResult[] = []; private maxHistory = 50; private cronExpression = '0 0 * * *'; private checkIntervalMs = 60_000; diff --git a/backend/services/billing/groupBilling.ts b/backend/services/billing/groupBilling.ts index f89e47ee..c5356514 100644 --- a/backend/services/billing/groupBilling.ts +++ b/backend/services/billing/groupBilling.ts @@ -53,10 +53,24 @@ export interface GroupPlanCustomization { sharedFeatures: string[]; memberLimits: Record; ownerDiscount: number; + /** Tax rate in basis points (e.g. 2000 = 20%). Optional; 0 when unset. */ + taxRateBps?: number; createdAt: number; updatedAt: number; } +/** Rounds to minor units, mapping non-finite input to zero. */ +const roundMoney = (amount: number): number => { + if (!Number.isFinite(amount)) return 0; + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * 100) / 100; +}; + +/** Constrains a basis-point tax rate to a sane `[0, 100%]` range. */ +const clampRate = (bps: number | undefined): number => { + if (bps === undefined || !Number.isFinite(bps)) return 0; + return Math.min(Math.max(bps, 0), 10_000); +}; + const createId = (prefix: string): string => `${prefix}_${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 8)}`; @@ -97,28 +111,40 @@ export class GroupBillingService { }; } - aggregateCharges(group: SubscriptionGroup, periodDays = 30): GroupBillingLineItem[] { - const cutoff = Date.now() - periodDays * 24 * 60 * 60 * 1000; - const recentCharges = group.charges.filter( - (c) => c.chargedAt.getTime() >= cutoff - ); + aggregateCharges( + group: SubscriptionGroup, + periodStart: number, + periodEnd: number + ): GroupBillingLineItem[] { + // The window is the requested period, not "the last N days relative to + // now". Deriving the cutoff from `Date.now()` made a historical invoice + // price a completely different set of charges than the period it claims + // to cover, so the line items never summed to the stated period. + const start = Math.min(periodStart, periodEnd); + const end = Math.max(periodStart, periodEnd); + const periodCharges = group.charges.filter((c) => { + const chargedAt = c.chargedAt.getTime(); + return chargedAt >= start && chargedAt <= end; + }); const memberCharges = new Map(); for (const member of group.members) { memberCharges.set(member.address, 0); } - for (const charge of recentCharges) { + for (const charge of periodCharges) { for (const item of charge.breakdown) { const current = memberCharges.get(item.memberAddress) ?? 0; - memberCharges.set(item.memberAddress, current + item.amount); + memberCharges.set(item.memberAddress, roundMoney(current + item.amount)); } } + const periodDays = Math.max(1, Math.ceil((end - start) / (24 * 60 * 60 * 1000))); + return Array.from(memberCharges.entries()).map(([address, amount]) => ({ memberAddress: address, amount, - description: `Aggregated charges over last ${periodDays} days`, + description: `Aggregated charges over ${periodDays} day(s)`, })); } @@ -130,15 +156,24 @@ export class GroupBillingService { periodEnd: number, currency = 'USD' ): GroupInvoice { - const aggregated = this.aggregateCharges(group, Math.ceil((periodEnd - periodStart) / (24 * 60 * 60 * 1000))); - const totalAmount = aggregated.reduce((sum, item) => sum + item.amount, 0); + const aggregated = this.aggregateCharges(group, periodStart, periodEnd); + const totalAmount = roundMoney(aggregated.reduce((sum, item) => sum + item.amount, 0)); const customization = this.planCustomizations.get(group.groupId); - const discountAmount = customization?.ownerDiscount - ? (totalAmount * customization.ownerDiscount) / 100 - : 0; - const taxAmount = 0; - const finalAmount = totalAmount - discountAmount + taxAmount; + + // `ownerDiscount` is a percentage but arrives from user-editable config. + // A negative value used to *increase* the bill and anything above 100 + // produced a negative payable amount, so both ends are clamped. + const rawDiscountPercent = customization?.ownerDiscount ?? 0; + const discountPercent = Math.min(Math.max(Number.isFinite(rawDiscountPercent) ? rawDiscountPercent : 0, 0), 100); + const discountAmount = roundMoney((totalAmount * discountPercent) / 100); + + // Tax was hard-coded to 0, so group invoices were never taxed even for + // entities inside a tax jurisdiction. + const taxRate = clampRate(customization?.taxRateBps); + const taxableAmount = Math.max(totalAmount - discountAmount, 0); + const taxAmount = roundMoney((taxableAmount * taxRate) / 10_000); + const finalAmount = roundMoney(Math.max(taxableAmount + taxAmount, 0)); const invoice: GroupInvoice = { id: createId('ginv'), diff --git a/backend/services/billing/interfaces.ts b/backend/services/billing/interfaces.ts index 776e426f..d1470a61 100644 --- a/backend/services/billing/interfaces.ts +++ b/backend/services/billing/interfaces.ts @@ -222,7 +222,7 @@ export interface IPartnerService { export interface IGroupBillingService { generateBillingSummary(group: any): any; - aggregateCharges(group: any, periodDays?: number): any[]; + aggregateCharges(group: any, periodStart: number, periodEnd: number): any[]; generateInvoice(group: any, periodStart: number, periodEnd: number, currency?: string): any; issueInvoice(invoiceId: string, groupId: string): any | null; markInvoicePaid(invoiceId: string, groupId: string): any | null; diff --git a/backend/services/billing/meteringService.ts b/backend/services/billing/meteringService.ts index 60c5acc9..cb0866d4 100644 --- a/backend/services/billing/meteringService.ts +++ b/backend/services/billing/meteringService.ts @@ -12,6 +12,12 @@ const SOFT_THRESHOLD_RATIO = 0.8; const HARD_THRESHOLD_RATIO = 1.0; const MAX_BATCH_SIZE = 500; +/** Monotonic sequence used to keep same-millisecond auto-keys distinct. */ +let _autoSequence = 0; +function nextAutoSequence(): number { + return ++_autoSequence; +} + export interface UsageMetric { userId: string; metricType: 'api' | 'compute' | 'storage'; @@ -126,7 +132,14 @@ export class MeteringService { } private async recordOne(metric: UsageMetric): Promise { - const idempotencyKey = metric.idempotencyKey ?? `auto-${meterKey(metric.userId, metric.metricType)}-${metric.timestamp.getTime()}`; + // An auto-generated key built purely from the event timestamp collides + // for two legitimate events landing in the same millisecond, and the + // second one was then reported as a duplicate and silently never rated — + // under-billing the customer. A monotonic sequence keeps distinct events + // distinct. Callers that need retry-idempotency must pass an explicit key. + const idempotencyKey = + metric.idempotencyKey ?? + `auto-${meterKey(metric.userId, metric.metricType)}-${metric.timestamp.getTime()}-${nextAutoSequence()}`; const cached = this.seenIdempotencyKeys.get(idempotencyKey); if (cached) { @@ -228,12 +241,22 @@ export class MeteringService { this.events.delete(meterKey(userId, metricType)); } - getUsageByMetric(subscriptionId: string): Record { + /** + * Sums recorded usage per metric for a subscription. + * + * An optional window is essential: without one this returns the entire + * lifetime total, which callers then label as a single billing period. + */ +getUsageByMetric(subscriptionId: string, from?: Date, to?: Date): Record { + const fromMs = from instanceof Date ? from.getTime() : undefined; + const toMs = to instanceof Date ? to.getTime() : undefined; const usage: Record = {}; for (const entry of this.usageHistory) { - if (entry.subscriptionId === subscriptionId) { - usage[entry.metric] = (usage[entry.metric] || 0) + entry.value; - } + if (entry.subscriptionId !== subscriptionId) continue; + const entryTime = entry.timestamp.getTime(); + if (fromMs !== undefined && entryTime < fromMs) continue; + if (toMs !== undefined && entryTime > toMs) continue; + usage[entry.metric] = (usage[entry.metric] || 0) + entry.value; } return usage; } @@ -299,13 +322,29 @@ export class MeteringService { } calculateUsageBilling(subscriptionId: string, unitPrice: number, currency: string): UsageBillingIntegration { - const metricUsage = this.getUsageByMetric(subscriptionId); const now = new Date(); const periodStart = new Date(now.getFullYear(), now.getMonth(), 1); - const periodEnd = new Date(now.getFullYear(), now.getMonth() + 1, 0); + // `new Date(y, m + 1, 0)` is midnight *starting* the last day, which + // excluded the final day of the month from the billed usage. + const periodEnd = new Date(now.getFullYear(), now.getMonth() + 1, 1); + + // Bill only usage inside the period being invoiced. Summing the full + // lifetime history while labelling the result "this month" over-billed + // every account with any prior history, compounding each cycle. + const metricUsage = this.getUsageByMetric(subscriptionId, periodStart, periodEnd); let totalMetered = 0; for (const usage of Object.values(metricUsage)) totalMetered += usage; - return { subscriptionId, meteredAmount: totalMetered, unitPrice, totalAmount: totalMetered * unitPrice, currency, period: { start: periodStart, end: periodEnd } }; + + const totalAmount = Number.isFinite(unitPrice) ? totalMetered * unitPrice : 0; + + return { + subscriptionId, + meteredAmount: totalMetered, + unitPrice, + totalAmount, + currency, + period: { start: periodStart, end: periodEnd }, + }; } } diff --git a/backend/services/shared/pagination.ts b/backend/services/shared/pagination.ts index 65ab0c85..ab418073 100644 --- a/backend/services/shared/pagination.ts +++ b/backend/services/shared/pagination.ts @@ -8,13 +8,80 @@ * - Integration with the existing ApiResponse / PaginationMeta envelope */ -import { createHmac } from 'node:crypto'; +import { createHmac, timingSafeEqual } from 'node:crypto'; // ─── Cursor Encoding ────────────────────────────────────────────────────────── -const CURSOR_SECRET = process.env['CURSOR_HMAC_SECRET'] ?? 'subtrackr-cursor-secret'; const CURSOR_VERSION = 1; +/** + * Resolves the cursor HMAC secret. + * + * This previously fell back to the literal `'subtrackr-cursor-secret'`, which + * is published in the repository. Anyone could therefore mint a cursor with a + * forged `field`, and that identifier is interpolated straight into `ORDER BY` + * and `WHERE` — a known-secret cursor is a SQL identifier-injection primitive. + * + * Outside production (tests, local development) a per-process random secret is + * generated instead, so the integrity guarantee still holds rather than being + * silently disabled. In production an unset secret is a hard failure. + */ +function resolveCursorSecret(): string { + const configured = process.env['CURSOR_HMAC_SECRET']; + + if (configured && configured.trim().length >= 16) { + return configured; + } + + if (process.env['NODE_ENV'] === 'production') { + throw new Error( + 'CURSOR_HMAC_SECRET must be set to a random value of at least 16 characters in production.' + ); + } + + if (configured) { + console.warn( + '[pagination] CURSOR_HMAC_SECRET is too short (min 16 chars); using an ephemeral secret. Cursors will not survive a restart.' + ); + } + + return createHmac('sha256', `${process.pid}:${process.hrtime.bigint()}:${Math.random()}`) + .update('cursor-secret') + .digest('hex'); +} + +let _cursorSecret: string | null = null; + +/** Lazily resolved so importing this module never throws in production. */ +function cursorSecret(): string { + if (_cursorSecret === null) { + _cursorSecret = resolveCursorSecret(); + } + return _cursorSecret; +} + +/** Test seam: forces a specific secret, or `null` to re-resolve. */ +export function __setCursorSecretForTesting(secret: string | null): void { + _cursorSecret = secret; +} + +function sign(data: string): string { + return createHmac('sha256', cursorSecret()).update(data).digest('base64url').slice(0, 16); +} + +/** + * Restricts a sort/identifier to a safe SQL identifier. + * + * Anything outside `[A-Za-z0-9_]` is rejected outright rather than sanitised, + * because a partially-stripped identifier is still attacker-influenced. + */ +export function assertSafeIdentifier(identifier: string, label: string): string { + if (!/^[A-Za-z0-9_]+$/.test(identifier)) { + throw new Error(`Unsafe SQL identifier for ${label}: ${JSON.stringify(identifier)}`); + } + return identifier; +} + export interface CursorPayload { v: number; field: string; @@ -43,9 +110,22 @@ export function decodeCursor(cursor: string): CursorPayload | null { if (sepIdx === -1) return null; const json = raw.slice(0, sepIdx); const sig = raw.slice(sepIdx + 1); - if (sign(json) !== sig) return null; + + // Constant-time so a forged signature cannot be refined byte by byte. + const bufA = Buffer.from(sign(json), 'utf8'); + const bufB = Buffer.from(sig, 'utf8'); + if (bufA.length !== bufB.length) return null; + if (!timingSafeEqual(bufA, bufB)) return null; + const payload = JSON.parse(json) as CursorPayload; if (payload.v !== CURSOR_VERSION) return null; + + // The signature is only trustworthy if the secret actually is. When a + // short/absent secret yields an ephemeral key we still refuse to trust a + // cursor's `field`, because it is interpolated into SQL below. + if (!/^[A-Za-z0-9_]+$/.test(String(payload.field ?? ''))) { + return null; + } return payload; } catch { return null; @@ -97,7 +177,7 @@ export interface SqlCursorClause { */ export function buildCursorClause(options: PageOptions = {}): SqlCursorClause { const dir = options.direction ?? 'asc'; - const sortField = options.sortField ?? 'id'; + const sortField = assertSafeIdentifier(options.sortField ?? 'id', 'sortField'); const rawLimit = options.limit ?? 20; const limit = Math.min(Math.max(1, rawLimit), options.maxLimit ?? 100); const op = dir === 'asc' ? '>' : '<'; @@ -117,10 +197,15 @@ export function buildCursorClause(options: PageOptions = {}): SqlCursorClause { return { where: '', param: null, orderBy: `"${sortField}" ${orderDir}`, limit }; } + // `decodeCursor` has already restricted `field` to `[A-Za-z0-9_]`, so it is + // safe to interpolate as a quoted SQL identifier. Re-check rather than trust + // the caller's own type-checking to have held. + const cursorField = assertSafeIdentifier(decoded.field, 'cursor.field'); + return { - where: `("${decoded.field}", id) ${op} ($1, $2)`, + where: `("${cursorField}", id) ${op} ($1, $2)`, param: decoded.value, - orderBy: `"${decoded.field}" ${orderDir}, id ${orderDir}`, + orderBy: `"${cursorField}" ${orderDir}, id ${orderDir}`, limit, }; } diff --git a/backend/services/shared/rateLimitingService.ts b/backend/services/shared/rateLimitingService.ts index 8f1a1c19..9904a8f7 100644 --- a/backend/services/shared/rateLimitingService.ts +++ b/backend/services/shared/rateLimitingService.ts @@ -110,6 +110,20 @@ export class RateLimitingService { return Array.from(this.bypass.userIds ?? []); } + // ------------------------------------------------------------------------- + // Tier resolution + // ------------------------------------------------------------------------- + + /** + * Returns the tier recorded for `apiKey`, or FREE when unknown. + * + * This is the only trustworthy source for a caller's tier. Reading it from + * a request header lets any caller self-select the ENTERPRISE allowance. + */ + getTierForApiKey(apiKey: string): SubscriptionTier { + return this.usages.get(apiKey)?.tier ?? SubscriptionTier.FREE; + } + // ------------------------------------------------------------------------- // Custom limit configuration // ------------------------------------------------------------------------- diff --git a/backend/services/webhook/eventReplayWorker.ts b/backend/services/webhook/eventReplayWorker.ts index 29fd72e0..81943c66 100644 --- a/backend/services/webhook/eventReplayWorker.ts +++ b/backend/services/webhook/eventReplayWorker.ts @@ -3,6 +3,9 @@ * with idempotency key checking and ordering guarantees. */ +import { createHmac } from 'node:crypto'; +import { validateOutboundUrl } from '../../shared/security/outboundUrlGuard'; + export interface ReplayRequest { eventIds: string[]; webhookId: string; @@ -97,7 +100,12 @@ export class EventReplayWorker { ): Promise<{ ok: boolean; status: number }> { const body = JSON.stringify(payload); - const res = await fetch(url, { + // Replay targets come from stored webhook configuration; validate before + // fetching so a compromised or misconfigured record cannot be turned into + // an SSRF pivot. + const target = validateOutboundUrl(url); + + const res = await fetch(target.toString(), { method: 'POST', headers: { 'Content-Type': 'application/json', @@ -113,14 +121,11 @@ export class EventReplayWorker { } private computeSignature(body: string, secret: string): string { - // In production, use HMAC-SHA256 - // Placeholder: simple hash for structure - let hash = 0; - const input = secret + body; - for (let i = 0; i < input.length; i++) { - hash = ((hash << 5) - hash + input.charCodeAt(i)) | 0; - } - return `sha256=${Math.abs(hash).toString(16)}`; + // This was a 31-bit rolling hash that merely *claimed* to be SHA-256 in + // the header value. It was trivially forgeable and, because replayed + // deliveries are signed with it, a receiver relying on it had no + // authenticity guarantee at all. Use a real keyed HMAC. + return `sha256=${createHmac('sha256', secret).update(body).digest('hex')}`; } getEventHistory(webhookId: string, limit = 50): StoredEvent[] { diff --git a/backend/shared/security/__tests__/outboundUrlGuard.test.ts b/backend/shared/security/__tests__/outboundUrlGuard.test.ts new file mode 100644 index 00000000..f2c04b7d --- /dev/null +++ b/backend/shared/security/__tests__/outboundUrlGuard.test.ts @@ -0,0 +1,149 @@ +/** + * Tests for outboundUrlGuard — SSRF protection for server-side fetches. + * + * @see backend/shared/security/outboundUrlGuard.ts + * @see https://github.com/Smartdevs17/SubTrackr/issues/1686 + */ + +import { + OutboundUrlError, + isPrivateHostname, + validateOutboundUrl, +} from '../outboundUrlGuard'; + +describe('isPrivateHostname', () => { + it('flags loopback and metadata hostnames', () => { + expect(isPrivateHostname('localhost')).toBe(true); + expect(isPrivateHostname('LOCALHOST')).toBe(true); + expect(isPrivateHostname('metadata.google.internal')).toBe(true); + expect(isPrivateHostname('instance-data')).toBe(true); + }); + + it('flags internal-looking suffixes', () => { + expect(isPrivateHostname('db.local')).toBe(true); + expect(isPrivateHostname('svc.internal')).toBe(true); + expect(isPrivateHostname('api.localhost')).toBe(true); + }); + + it('flags loopback and RFC1918 IPv4 literals', () => { + expect(isPrivateHostname('127.0.0.1')).toBe(true); + expect(isPrivateHostname('10.1.2.3')).toBe(true); + expect(isPrivateHostname('172.16.0.1')).toBe(true); + expect(isPrivateHostname('192.168.1.1')).toBe(true); + expect(isPrivateHostname('0.0.0.0')).toBe(true); + expect(isPrivateHostname('255.255.255.255')).toBe(true); + }); + + it('flags the cloud metadata address specifically', () => { + expect(isPrivateHostname('169.254.169.254')).toBe(true); + }); + + it('flags CGNAT space', () => { + expect(isPrivateHostname('100.64.0.1')).toBe(true); + }); + + it('flags private IPv6 literals', () => { + expect(isPrivateHostname('::1')).toBe(true); + expect(isPrivateHostname('fd00::1')).toBe(true); + expect(isPrivateHostname('fe80::1')).toBe(true); + }); + + it('flags IPv4-mapped IPv6 loopback', () => { + expect(isPrivateHostname('::ffff:127.0.0.1')).toBe(true); + }); + + it('allows ordinary public hosts', () => { + expect(isPrivateHostname('example.com')).toBe(false); + expect(isPrivateHostname('hooks.zapier.com')).toBe(false); + expect(isPrivateHostname('8.8.8.8')).toBe(false); + expect(isPrivateHostname('172.32.0.1')).toBe(false); + expect(isPrivateHostname('172.15.0.1')).toBe(false); + }); + + it('treats a malformed dotted quad as unsafe', () => { + expect(isPrivateHostname('999.1.1.1')).toBe(true); + expect(isPrivateHostname('127.0.0')).toBe(true); + }); + + it('treats an empty host as unsafe', () => { + expect(isPrivateHostname('')).toBe(true); + }); +}); + +describe('validateOutboundUrl', () => { + it('accepts a normal public https URL', () => { + const result = validateOutboundUrl('https://hooks.zapier.com/hooks/catch/1/2/'); + expect(result.protocol).toBe('https:'); + expect(result.hostname).toBe('hooks.zapier.com'); + }); + + it('accepts plain http', () => { + expect(validateOutboundUrl('http://example.com/hook').protocol).toBe('http:'); + }); + + it('rejects non-HTTP schemes', () => { + for (const url of [ + 'file:///etc/passwd', + 'gopher://example.com', + 'ftp://example.com', + 'javascript:alert(1)', + 'data:text/plain,hi', + ]) { + expect(() => validateOutboundUrl(url)).toThrow(OutboundUrlError); + } + }); + + it('rejects relative or malformed values', () => { + expect(() => validateOutboundUrl('/relative/path')).toThrow(OutboundUrlError); + expect(() => validateOutboundUrl('not a url')).toThrow(OutboundUrlError); + expect(() => validateOutboundUrl('')).toThrow(OutboundUrlError); + expect(() => validateOutboundUrl(undefined)).toThrow(OutboundUrlError); + expect(() => validateOutboundUrl(null)).toThrow(OutboundUrlError); + expect(() => validateOutboundUrl(42)).toThrow(OutboundUrlError); + }); + + it('rejects loopback and private targets', () => { + expect(() => validateOutboundUrl('http://localhost/hook')).toThrow(/non-public/); + expect(() => validateOutboundUrl('http://127.0.0.1:8080/hook')).toThrow(/non-public/); + expect(() => validateOutboundUrl('http://169.254.169.254/latest/meta-data/')).toThrow( + /non-public/ + ); + expect(() => validateOutboundUrl('http://10.0.0.5/admin')).toThrow(/non-public/); + }); + + it('rejects embedded credentials', () => { + expect(() => validateOutboundUrl('https://user:pass@example.com/')).toThrow( + /embedded credentials/ + ); + }); + + it('enforces an allowlist when one is supplied', () => { + expect( + validateOutboundUrl('https://hooks.zapier.com/x', { allowedHosts: ['hooks.zapier.com'] }) + .hostname + ).toBe('hooks.zapier.com'); + + // Subdomains of an allowlisted host are permitted. + expect(() => + validateOutboundUrl('https://eu.hooks.zapier.com/x', { allowedHosts: ['hooks.zapier.com'] }) + ).not.toThrow(); + + expect(() => + validateOutboundUrl('https://evil.example.com/x', { allowedHosts: ['hooks.zapier.com'] }) + ).toThrow(/allowlist/); + }); + + it('does not treat a lookalike host as allowlisted', () => { + expect(() => + validateOutboundUrl('https://hooks.zapier.com.evil.test/x', { + allowedHosts: ['hooks.zapier.com'], + }) + ).toThrow(/allowlist/); + }); + + it('still applies the private-address rules when an allowlist is present', () => { + expect(() => + validateOutboundUrl('http://127.0.0.1/x', { allowedHosts: ['127.0.0.1'] }) + ).toThrow(OutboundUrlError); + }); +}); \ No newline at end of file diff --git a/backend/shared/security/outboundUrlGuard.ts b/backend/shared/security/outboundUrlGuard.ts new file mode 100644 index 00000000..9c6ab63a --- /dev/null +++ b/backend/shared/security/outboundUrlGuard.ts @@ -0,0 +1,184 @@ +/** + * outboundUrlGuard.ts + * + * Validation for URLs the server will fetch on a user's behalf. + * + * Several integration routes accept an operator- or merchant-supplied + * `targetUrl` and later call `fetch()` with it. Without validation that is a + * server-side request forgery primitive: an attacker can reach the cloud + * metadata endpoint, loopback services, or RFC1918 hosts that are only + * reachable from inside the network. + * + * The rules below reject the obvious cases statically (scheme, obvious host + * names, literal private/loopback/link-local addresses). DNS rebinding and + * internal names that resolve to private space are not covered here — for + * those, pair this with an egress proxy or a resolved-address check at + * connection time. + */ + +import { isIP } from 'node:net'; + +/** Hostnames that always denote the local machine or a container escape. */ +const BLOCKED_HOSTNAMES = new Set([ + 'localhost', + 'localhost.localdomain', + 'ip6-localhost', + 'ip6-loopback', + 'metadata', + 'metadata.google.internal', + 'instance-data', +]); + +/** Suffixes used by cloud metadata services. */ +const BLOCKED_SUFFIXES = ['.localhost', '.local', '.internal', '.localdomain']); + +/** Only plain web schemes; `file:`, `gopher:`, `dict:` etc. are refused. */ +const ALLOWED_PROTOCOLS = new Set(['http:', 'https:']); + +export class OutboundUrlError extends Error { + readonly code = 'OUTBOUND_URL_REJECTED' as const; + + constructor(reason: string) { + super(`Rejected outbound URL: ${reason}`); + this.name = 'OutboundUrlError'; + Object.setPrototypeOf(this, new.target.prototype); + } +} + +/** + * Parses IPv4 dotted-quads into octets. + * Returns `null` for anything that is not exactly four numeric parts. + */ +function parseIPv4(host: string): number[] | null { + const parts = host.split('.'); + if (parts.length !== 4) return null; + + const octets: number[] = []; + for (const part of parts) { + if (!/^\d{1,3}$/.test(part)) return null; + const value = Number(part); + if (value > 255) return null; + octets.push(value); + } + + return octets; +} + +function isPrivateIPv4(octets: number[]): boolean { + const [a, b] = octets; + + if (a === 0) return true; // "this" network + if (a === 10) return true; // 10.0.0.0/8 + if (a === 127) return true; // loopback + if (a === 169 && b === 254) return true; // link-local, incl. 169.254.169.254 + if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 + if (a === 192 && b === 168) return true; // 192.168.0.0/16 + if (a === 192 && b === 0) return true; // 192.0.0.0/24 + 192.0.2.0/24 + if (a === 100 && b >= 64 && b <= 127) return true; // CGNAT 100.64.0.0/10 + if (a >= 224) return true; // multicast + reserved + + return false; +} + +function isPrivateIPv6(host: string): boolean { + const normalized = host.replace(/^\[/, '').replace(/\]$/, '').toLowerCase(); + + if (normalized === '::' || normalized === '::1') return true; // unspecified, loopback + + // IPv4-mapped (::ffff:a.b.c.d) and IPv4-compatible forms embed an IPv4 address. + const embeddedIPv4 = normalized.match(/(\d{1,3}(?:\.\d{1,3}){3})$/); + if (embeddedIPv4) { + const octets = parseIPv4(embeddedIPv4[1]); + if (octets && isPrivateIPv4(octets)) return true; + } + + // Unique-local (fc00::/7) and link-local (fe80::/10). + if (/^f[cd][0-9a-f]{2}:/.test(normalized)) return true; + if (/^fe[89ab][0-9a-f]:/.test(normalized)) return true; + + return false; +} + +/** + * Returns true when `hostname` cannot be reached from the public internet. + */ +export function isPrivateHostname(hostname: string): boolean { + const host = hostname.trim().toLowerCase().replace(/\.$/, ''); + + if (host === '') return true; + if (BLOCKED_HOSTNAMES.has(host)) return true; + if (BLOCKED_SUFFIXES.some((suffix) => host.endsWith(suffix))) return true; + + const ipVersion = isIP(host); + if (ipVersion === 4) { + const octets = parseIPv4(host); + // A malformed dotted quad is treated as unsafe rather than trusted. + return octets === null ? true : isPrivateIPv4(octets); + } + if (ipVersion === 6) { + return isPrivateIPv6(host); + } + + // A host made only of digits and dots is an *attempted* IP literal. If it + // did not parse as one, some other layer (a resolver, a proxy, a downstream + // parser) may interpret it differently than we just did, so refuse it. + if (/^[\d.]+$/.test(host)) { + return true; + } + + return false; +} + +export interface ValidateOutboundUrlOptions { + /** When set, only these hostnames (and their subdomains) are permitted. */ + allowedHosts?: readonly string[]; +} + +/** + * Validates and normalises a URL the server is about to fetch. + * + * @throws {OutboundUrlError} when the URL is malformed, uses a non-HTTP + * scheme, points at private/loopback/link-local space, or is not on the + * allowlist. + */ +export function validateOutboundUrl( + rawUrl: unknown, + options: ValidateOutboundUrlOptions = {} +): URL { + if (typeof rawUrl !== 'string' || rawUrl.trim() === '') { + throw new OutboundUrlError('value must be a non-empty string'); + } + + let parsed: URL; + try { + parsed = new URL(rawUrl.trim()); + } catch { + throw new OutboundUrlError('value is not a valid absolute URL'); + } + + if (!ALLOWED_PROTOCOLS.has(parsed.protocol)) { + throw new OutboundUrlError(`protocol '${parsed.protocol}' is not allowed`); + } + + if (parsed.username !== '' || parsed.password !== '') { + throw new OutboundUrlError('embedded credentials are not allowed'); + } + + if (isPrivateHostname(parsed.hostname)) { + throw new OutboundUrlError(`host '${parsed.hostname}' resolves to non-public address space`); + } + + if (options.allowedHosts && options.allowedHosts.length > 0) { + const host = parsed.hostname.toLowerCase(); + const allowed = options.allowedHosts.some((entry) => { + const candidate = entry.toLowerCase(); + return host === candidate || host.endsWith(`.${candidate}`); + }); + + if (!allowed) { + throw new OutboundUrlError(`host '${parsed.hostname}' is not on the allowlist`); + } + } + + return parsed; +} \ No newline at end of file diff --git a/developer-portal/pages/api/revalidate.ts b/developer-portal/pages/api/revalidate.ts index 66433bc2..97c1f8f4 100644 --- a/developer-portal/pages/api/revalidate.ts +++ b/developer-portal/pages/api/revalidate.ts @@ -1,3 +1,5 @@ +import { timingSafeEqual } from 'node:crypto'; + import type { NextApiRequest, NextApiResponse } from 'next'; type RevalidateResult = { revalidated: boolean; paths?: string[]; error?: string }; @@ -36,6 +38,31 @@ const TAG_TO_PATHS: Record = { sdks: ['/docs/webhook-integration'], }; +/** Every path the endpoint is permitted to revalidate, regardless of input. */ +const ALLOWED_PATHS = new Set(Object.values(TAG_TO_PATHS).flat()); + +/** + * Constant-time secret comparison. + * + * `secret !== expectedSecret` short-circuits on the first differing byte, + * which leaks the secret length and prefix through response timing and lets + * an attacker recover it one character at a time. + */ +function secretsMatch(provided: unknown, expected: string): boolean { + if (typeof provided !== 'string') return false; + + const a = Buffer.from(provided, 'utf8'); + const b = Buffer.from(expected, 'utf8'); + + if (a.length !== b.length) { + // Still perform a comparison so the fast path is not measurably faster. + timingSafeEqual(a, a); + return false; + } + + return timingSafeEqual(a, b); +} + export default async function handler(req: NextApiRequest, res: NextApiResponse) { if (req.method !== 'POST') { return res.status(405).json({ revalidated: false, error: 'Method not allowed' }); @@ -49,7 +76,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse< // Verify caller secret const expectedSecret = process.env.REVALIDATE_SECRET; - if (!expectedSecret || secret !== expectedSecret) { + if (!expectedSecret || !secretsMatch(secret, expectedSecret)) { return res.status(401).json({ revalidated: false, error: 'Invalid secret' }); } @@ -57,6 +84,13 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse< return res.status(400).json({ revalidated: false, error: 'Provide "path" or "tag"' }); } + // `path` is caller-supplied and goes straight into `res.revalidate()`. An + // arbitrary value lets a holder of the secret force revalidation of + // unrelated routes, so it is restricted to the documented set. + if (path !== undefined && !ALLOWED_PATHS.has(path)) { + return res.status(400).json({ revalidated: false, error: `Path "${path}" is not revalidatable` }); + } + const pathsToRevalidate: string[] = path ? [path] : (TAG_TO_PATHS[tag as string] ?? []); if (pathsToRevalidate.length === 0) { diff --git a/src/services/__tests__/groupBilling.test.ts b/src/services/__tests__/groupBilling.test.ts index 430270b6..ab7e0d29 100644 --- a/src/services/__tests__/groupBilling.test.ts +++ b/src/services/__tests__/groupBilling.test.ts @@ -60,13 +60,27 @@ describe('GroupBillingService', () => { expect(summary.memberBalances['0xMEMBER1']).toBe(10); }); - it('aggregates charges within period', () => { - const items = service.aggregateCharges(sampleGroup, 30); + it('aggregates charges within the requested period', () => { + const now = Date.now(); + const items = service.aggregateCharges(sampleGroup, now - 30 * 86400000, now); expect(items.length).toBe(2); const ownerItem = items.find((i) => i.memberAddress === '0xOWNER'); expect(ownerItem?.amount).toBe(29.99); }); + it('excludes charges outside the requested period', () => { + const now = Date.now(); + // The window is anchored to the period, not to "the last N days from + // now" — a period that ended long ago must not pick up today's charges. + const items = service.aggregateCharges( + sampleGroup, + now - 90 * 86400000, + now - 60 * 86400000 + ); + const ownerItem = items.find((i) => i.memberAddress === '0xOWNER'); + expect(ownerItem?.amount).toBe(0); + }); + it('generates and processes invoices', () => { const now = Date.now(); const invoice = service.generateInvoice(sampleGroup, now - 86400000, now); diff --git a/src/services/__tests__/rpcProvider.test.ts b/src/services/__tests__/rpcProvider.test.ts new file mode 100644 index 00000000..7ff7325f --- /dev/null +++ b/src/services/__tests__/rpcProvider.test.ts @@ -0,0 +1,166 @@ +/** + * Tests for the resilient RPC provider's registry and error surface. + * + * These deliberately avoid exercising real network I/O: the behaviour that + * matters for this change (which errors count against a circuit, URL-swap + * serialisation) is covered by `isTransportFailure` tests in + * `transactionConfirmation.test.ts` and by the backend integration suite. + * + * @see src/services/rpcProvider.ts + * @see https://github.com/Smartdevs17/SubTrackr/issues/1683 + */ + +import { + AllRpcProvidersFailedError, + RpcProviderCircuitOpenError, + RpcProviderTimeoutError, + clearResilientProviderRegistry, + defaultChainTimeoutMs, + getOrCreateResilientProvider, + isTransportFailure, +} from '../rpcProvider'; + +const PRIMARY = 'https://primary.example/rpc'; +const FALLBACK = 'https://fallback.example/rpc'; + +describe('defaultChainTimeoutMs', () => { + it('allows more headroom on high-variance L2 chains', () => { + expect(defaultChainTimeoutMs(10)).toBe(15_000); + expect(defaultChainTimeoutMs(137)).toBe(15_000); + expect(defaultChainTimeoutMs(8453)).toBe(15_000); + }); + + it('uses the tighter default on Ethereum mainnet', () => { + expect(defaultChainTimeoutMs(1)).toBe(10_000); + }); + + it('falls back to the default for unknown chains', () => { + expect(defaultChainTimeoutMs(999999)).toBe(10_000); + }); +}); + +describe('getOrCreateResilientProvider', () => { + beforeEach(() => { + clearResilientProviderRegistry(); + }); + + afterEach(() => { + clearResilientProviderRegistry(); + }); + + it('returns the same instance for the same chain and URL set', () => { + const first = getOrCreateResilientProvider(1, [PRIMARY, FALLBACK]); + const second = getOrCreateResilientProvider(1, [PRIMARY, FALLBACK]); + expect(second).toBe(first); + }); + + it('ignores URL order so circuit state is not silently reset', () => { + const first = getOrCreateResilientProvider(1, [PRIMARY, FALLBACK]); + const second = getOrCreateResilientProvider(1, [FALLBACK, PRIMARY]); + expect(second).toBe(first); + }); + + it('keeps separate instances per chain', () => { + const mainnet = getOrCreateResilientProvider(1, [PRIMARY]); + const polygon = getOrCreateResilientProvider(137, [PRIMARY]); + expect(mainnet).not.toBe(polygon); + }); + + it('keeps separate instances for different URL sets', () => { + const first = getOrCreateResilientProvider(1, [PRIMARY]); + const second = getOrCreateResilientProvider(1, [PRIMARY, FALLBACK]); + expect(first).not.toBe(second); + }); + + it('starts every circuit closed', () => { + const provider = getOrCreateResilientProvider(1, [PRIMARY, FALLBACK]); + const states = provider.getCircuitStates(); + + expect(states[PRIMARY]).toEqual({ state: 'closed', consecutiveFailures: 0 }); + expect(states[FALLBACK]).toEqual({ state: 'closed', consecutiveFailures: 0 }); + }); + + it('resets every circuit on demand', () => { + const provider = getOrCreateResilientProvider(1, [PRIMARY, FALLBACK]); + provider.resetAllCircuits(); + + const states = provider.getCircuitStates(); + expect(states[PRIMARY].state).toBe('closed'); + expect(states[FALLBACK].state).toBe('closed'); + }); + + it('drops every cached provider when the registry is cleared', () => { + const first = getOrCreateResilientProvider(1, [PRIMARY]); + clearResilientProviderRegistry(); + const second = getOrCreateResilientProvider(1, [PRIMARY]); + + expect(second).not.toBe(first); + }); +}); + +describe('isTransportFailure', () => { + const withCode = (message: string, code: unknown): Error => + Object.assign(new Error(message), { code }); + + it('classifies an explicit timeout as a transport failure', () => { + expect(isTransportFailure(withCode('nope', 'TIMEOUT'))).toBe(true); + }); + + it('classifies connection-level errors as transport failures', () => { + for (const code of ['ECONNREFUSED', 'ECONNRESET', 'ENOTFOUND', 'ERR_NETWORK']) { + expect(isTransportFailure(withCode('x', code))).toBe(true); + } + }); + + it('does not treat a JSON-RPC application error as a transport failure', () => { + // A revert carries a numeric JSON-RPC code. The node answered correctly, + // so counting it as a circuit failure would evict a healthy endpoint. + expect(isTransportFailure(withCode('execution reverted', 3))).toBe(false); + }); + + it('does not treat insufficient funds as a transport failure', () => { + const nested = Object.assign(new Error('insufficient funds'), { + error: { code: -32000 }, + }); + expect(isTransportFailure(nested)).toBe(false); + }); + + it('treats fetch-level errors as transport failures', () => { + expect(isTransportFailure(new TypeError('failed to fetch'))).toBe(true); + }); + + it('returns false for non-error values', () => { + expect(isTransportFailure(null)).toBe(false); + expect(isTransportFailure(undefined)).toBe(false); + expect(isTransportFailure('nope')).toBe(false); + }); +}); + +describe('provider error types', () => { + it('exposes a stable code and context for timeouts', () => { + const error = new RpcProviderTimeoutError(PRIMARY, 100); + expect(error.code).toBe('RPC_PROVIDER_TIMEOUT'); + expect(error.endpointUrl).toBe(PRIMARY); + expect(error.timeoutMs).toBe(100); + expect(error).toBeInstanceOf(Error); + }); + + it('exposes a stable code and context for an open circuit', () => { + const error = new RpcProviderCircuitOpenError(FALLBACK); + expect(error.code).toBe('RPC_PROVIDER_CIRCUIT_OPEN'); + expect(error.endpointUrl).toBe(FALLBACK); + }); + + it('reports every attempted endpoint when all of them fail', () => { + const error = new AllRpcProvidersFailedError(1, [ + { url: PRIMARY, message: 'failed to fetch' }, + { url: FALLBACK, message: 'timed out' }, + ]); + + expect(error.code).toBe('ALL_RPC_PROVIDERS_FAILED'); + expect(error.chainId).toBe(1); + expect(error.errors).toHaveLength(2); + expect(error.message).toContain(PRIMARY); + expect(error.message).toContain(FALLBACK); + }); +}); \ No newline at end of file diff --git a/src/services/__tests__/transactionConfirmation.test.ts b/src/services/__tests__/transactionConfirmation.test.ts new file mode 100644 index 00000000..51f5ff7f --- /dev/null +++ b/src/services/__tests__/transactionConfirmation.test.ts @@ -0,0 +1,261 @@ +/** + * Tests for transactionConfirmation — defensive receipt waiting. + * + * @see src/services/transactionConfirmation.ts + * @see https://github.com/Smartdevs17/SubTrackr/issues/1683 + */ + +import type { ethers } from 'ethers'; + +import { + DEFAULT_CONFIRMATIONS, + DEFAULT_CONFIRMATION_TIMEOUT_MS, + TransactionInsufficientConfirmationsError, + TransactionRevertedError, + TransactionTimeoutError, + bestEffortTransactionHash, + confirmTransaction, + isDefinitiveFailure, + requiredConfirmations, +} from '../transactionConfirmation'; + +type Receipt = ethers.providers.TransactionReceipt; + +const L2_CHAIN_ID = 8453; +const L1_CHAIN_ID = 1; + +const makeReceipt = (overrides: Partial = {}): Receipt => + ({ + transactionHash: '0xabc', + status: 1, + confirmations: 2, + ...overrides, + }) as Receipt; + +const makeTx = (wait: (confirmations?: number) => Promise) => ({ + hash: '0xabc', + wait, +}); + +describe('requiredConfirmations', () => { + it('requires a single confirmation on fast L2 chains', () => { + expect(requiredConfirmations(L2_CHAIN_ID)).toBe(1); + expect(requiredConfirmations(10)).toBe(1); + expect(requiredConfirmations(137)).toBe(1); + }); + + it('requires deeper confirmation on L1', () => { + expect(requiredConfirmations(L1_CHAIN_ID)).toBe(DEFAULT_CONFIRMATIONS); + expect(DEFAULT_CONFIRMATIONS).toBe(2); + }); + + it('honours an explicit override', () => { + expect(requiredConfirmations(L2_CHAIN_ID, 5)).toBe(5); + }); + + it('never returns less than one confirmation', () => { + expect(requiredConfirmations(L1_CHAIN_ID, 0)).toBe(1); + expect(requiredConfirmations(L1_CHAIN_ID, -3)).toBe(1); + }); +}); + +describe('confirmTransaction', () => { + it('returns the receipt when the transaction succeeded', async () => { + const receipt = makeReceipt(); + const tx = makeTx(async () => receipt); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).resolves.toBe(receipt); + }); + + it('passes the required confirmations to ethers', async () => { + const wait = jest.fn().mockResolvedValue(makeReceipt({ confirmations: 1 })); + const tx = makeTx(wait); + + await confirmTransaction(tx, { chainId: L2_CHAIN_ID }); + + expect(wait).toHaveBeenCalledWith(1); + }); + + it('asks for the L1 confirmation depth on mainnet', async () => { + const wait = jest.fn().mockResolvedValue(makeReceipt({ confirmations: 2 })); + const tx = makeTx(wait); + + await confirmTransaction(tx, { chainId: L1_CHAIN_ID }); + + expect(wait).toHaveBeenCalledWith(DEFAULT_CONFIRMATIONS); + }); + + it('enforces its own timeout even when the transaction never settles', async () => { + // ethers v5 `ContractTransaction.wait()` ignores a timeout argument, so the + // bound has to be enforced here or a contract payment can hang forever. + jest.useFakeTimers(); + + try { + const tx = makeTx(() => new Promise(() => undefined)); + + const pending = confirmTransaction(tx, { chainId: L2_CHAIN_ID, timeoutMs: 5000 }); + const assertion = expect(pending).rejects.toBeInstanceOf(TransactionTimeoutError); + + jest.advanceTimersByTime(5000); + await assertion; + } finally { + jest.useRealTimers(); + } + }); + + it('does not fire the timeout when the receipt arrives in time', async () => { + jest.useFakeTimers(); + + try { + const tx = makeTx(async () => makeReceipt()); + + await expect( + confirmTransaction(tx, { chainId: L2_CHAIN_ID, timeoutMs: 5000 }) + ).resolves.toBeDefined(); + + // Any surviving timer must have been cleared, otherwise this leaks. + expect(jest.getTimerCount()).toBe(0); + } finally { + jest.useRealTimers(); + } + }); + + it('defaults to a bounded timeout when none is supplied', async () => { + jest.useFakeTimers(); + + try { + expect(DEFAULT_CONFIRMATION_TIMEOUT_MS).toBeGreaterThan(0); + + const tx = makeTx(() => new Promise(() => undefined)); + const pending = confirmTransaction(tx, { chainId: L2_CHAIN_ID }); + const assertion = expect(pending).rejects.toBeInstanceOf(TransactionTimeoutError); + + jest.advanceTimersByTime(DEFAULT_CONFIRMATION_TIMEOUT_MS); + await assertion; + } finally { + jest.useRealTimers(); + } + }); + + it('throws when the receipt reports a reverted transaction', async () => { + // The core regression: `status === 0` means mined-but-failed, and this + // used to be reported to the user as a successful payment. + const tx = makeTx(async () => makeReceipt({ status: 0 })); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).rejects.toBeInstanceOf( + TransactionRevertedError + ); + }); + + it('includes the revert reason when the node provides one', async () => { + // Nodes expose the reason in vendor-specific shapes that ethers v5 does + // not model on TransactionReceipt. + const receiptWithReason = { + ...makeReceipt({ status: 0 }), + revertReason: 'ERC20: transfer amount exceeds balance', + } as unknown as Receipt; + const tx = makeTx(async () => receiptWithReason); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).rejects.toThrow( + /ERC20: transfer amount exceeds balance/ + ); + }); + + it('still throws for a revert when no reason is available', async () => { + const tx = makeTx(async () => makeReceipt({ status: 0 })); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).rejects.toThrow( + /reverted on-chain/ + ); + }); + + it('maps an ethers wait timeout to a typed timeout error', async () => { + const timeoutError = Object.assign(new Error('timeout'), { code: 'TIMEOUT' }); + const tx = makeTx(async () => { + throw timeoutError; + }); + + await expect( + confirmTransaction(tx, { chainId: L2_CHAIN_ID, timeoutMs: 5000 }) + ).rejects.toBeInstanceOf(TransactionTimeoutError); + }); + + it('recognises timeout phrased as "timed out"', async () => { + const tx = makeTx(async () => { + throw new Error('transaction timed out waiting for receipt'); + }); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).rejects.toBeInstanceOf( + TransactionTimeoutError + ); + }); + + it('treats a null receipt as an unknown outcome', async () => { + // ethers resolves null when a transaction was replaced or dropped. + const tx = makeTx(async () => null); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).rejects.toBeInstanceOf( + TransactionTimeoutError + ); + }); + + it('throws when fewer confirmations than required were reached', async () => { + const tx = makeTx(async () => makeReceipt({ confirmations: 1 })); + + await expect(confirmTransaction(tx, { chainId: L1_CHAIN_ID })).rejects.toBeInstanceOf( + TransactionInsufficientConfirmationsError + ); + }); + + it('rethrows unexpected errors untouched', async () => { + const tx = makeTx(async () => { + throw new Error('user rejected transaction'); + }); + + await expect(confirmTransaction(tx, { chainId: L2_CHAIN_ID })).rejects.toThrow( + 'user rejected transaction' + ); + }); +}); + +describe('isDefinitiveFailure', () => { + it('treats a revert as definitive', () => { + expect(isDefinitiveFailure(new TransactionRevertedError('0x1'))).toBe(true); + }); + + it('treats insufficient confirmations as definitive', () => { + expect(isDefinitiveFailure(new TransactionInsufficientConfirmationsError('0x1', 1, 2))).toBe( + true + ); + }); + + it('does not treat a timeout as definitive', () => { + expect(isDefinitiveFailure(new TransactionTimeoutError('0x1', 100))).toBe(false); + }); +}); + +describe('bestEffortTransactionHash', () => { + it('returns the hash when the transaction confirms', async () => { + const tx = makeTx(async () => makeReceipt()); + await expect(bestEffortTransactionHash(tx, { chainId: L2_CHAIN_ID })).resolves.toBe('0xabc'); + }); + + it('still returns the hash when the receipt cannot be observed', async () => { + // A crash or dropped connection after broadcast leaves the outcome + // unknown; the hash is the durable handle the user needs. + const tx = makeTx(async () => null); + await expect(bestEffortTransactionHash(tx, { chainId: L2_CHAIN_ID })).resolves.toBe('0xabc'); + }); + + it('propagates a revert rather than swallowing it', async () => { + const tx = makeTx(async () => makeReceipt({ status: 0 })); + await expect(bestEffortTransactionHash(tx, { chainId: L2_CHAIN_ID })).rejects.toBeInstanceOf( + TransactionRevertedError + ); + }); + + it('returns undefined for a missing response', async () => { + await expect(bestEffortTransactionHash(null)).resolves.toBeUndefined(); + await expect(bestEffortTransactionHash(undefined)).resolves.toBeUndefined(); + }); +}); \ No newline at end of file diff --git a/src/services/analyticsService.ts b/src/services/analyticsService.ts index 1870befa..0be8ae6f 100644 --- a/src/services/analyticsService.ts +++ b/src/services/analyticsService.ts @@ -1,4 +1,5 @@ -import { BillingCycle, Subscription } from '../types/subscription'; +import { Subscription } from '../types/subscription'; +import { monthlyEquivalent } from '../utils/billingCycleMath'; export interface RevenuePoint { label: string; @@ -49,18 +50,9 @@ export interface SubscriptionAnalyticsReport { forecast: RevenueForecastPoint[]; } -const MONTHS_PER_YEAR = 12; -const WEEKS_PER_MONTH = 4.345; - export const toMonthlyRevenue = ( subscription: Pick -): number => { - if (subscription.billingCycle === BillingCycle.YEARLY) - return subscription.price / MONTHS_PER_YEAR; - if (subscription.billingCycle === BillingCycle.WEEKLY) - return subscription.price * WEEKS_PER_MONTH; - return subscription.price; -}; +): number => monthlyEquivalent(subscription.price, subscription.billingCycle); const monthKey = (date: Date): string => `${date.getFullYear()}-${String(date.getMonth() + 1).padStart(2, '0')}`; diff --git a/src/services/invoiceService.ts b/src/services/invoiceService.ts index e091c7a2..07e7a515 100644 --- a/src/services/invoiceService.ts +++ b/src/services/invoiceService.ts @@ -12,6 +12,7 @@ import type { } from '../types/invoice'; import { InvoiceLayout, InvoiceStatus } from '../types/invoice'; import { getNextSequence, generateLegalInvoiceNumber } from './sequenceService'; +import { calculateInvoiceTotals, resolveLineTotal } from '../utils/invoiceTotals'; const STORAGE_KEYS = { INVOICES: '@SubTrackr:invoices', @@ -150,13 +151,15 @@ export async function createInvoice(data: InvoiceFormData): Promise { // Accepts either the itemised shape (lineTotal) or the flat one (amount). const lineItems = (data.lineItems ?? []).map((item) => ({ ...item, - lineTotal: item.lineTotal ?? item.amount ?? 0, + lineTotal: resolveLineTotal(item), })); - const lineTotal = - lineItems.length > 0 - ? lineItems.reduce((sum, item) => sum + item.lineTotal, 0) - : (data.amount ?? 0); - const total = lineTotal + (data.taxAmount || 0) - (data.discountAmount || 0); + const totals = calculateInvoiceTotals({ + lineItems, + amount: data.amount, + tax: data.taxAmount, + discount: data.discountAmount, + }); + const { subtotal, total } = totals; // Legal numbering: fetch next persistent sequence // We track sequence per year to reset it annually (common legal requirement) @@ -173,7 +176,10 @@ export async function createInvoice(data: InvoiceFormData): Promise { merchantName: '', lineItems, tax: data.taxAmount ?? 0, - subtotal: lineTotal, + // Persist the clamped discount so a later edit can re-apply it instead of + // silently repricing the invoice at full value. + discountAmount: totals.discount, + subtotal, total, totalAmount: total, amount: total, @@ -216,15 +222,24 @@ export async function updateInvoice(id: string, updates: Partial): Prom if (updates.lineItems) { const lineItems = updates.lineItems.map((item) => ({ ...item, - lineTotal: item.lineTotal ?? item.amount ?? 0, + lineTotal: resolveLineTotal(item), })); - const subtotal = lineItems.reduce((sum, item) => sum + (item.lineTotal ?? 0), 0); - const total = subtotal + (updatedInvoice.tax ?? 0); + + // The discount is preserved (and only overridden when the caller supplies + // a new one). Dropping it here meant editing any line on a discounted + // invoice increased the amount payable. + const totals = calculateInvoiceTotals({ + lineItems, + tax: updatedInvoice.tax ?? 0, + discount: updatedInvoice.discountAmount ?? 0, + }); + updatedInvoice.lineItems = lineItems; - updatedInvoice.subtotal = subtotal; - updatedInvoice.total = total; - updatedInvoice.totalAmount = total; - updatedInvoice.amount = total; + updatedInvoice.subtotal = totals.subtotal; + updatedInvoice.discountAmount = totals.discount; + updatedInvoice.total = totals.total; + updatedInvoice.totalAmount = totals.total; + updatedInvoice.amount = totals.total; } invoices[index] = updatedInvoice; @@ -331,7 +346,10 @@ export async function getInvoiceAnalytics(): Promise { .filter((inv) => inv.status === InvoiceStatus.PAID) .reduce((sum, inv) => sum + inv.total, 0); - const averageInvoiceAmount = totalInvoices > 0 ? totalRevenue / paidInvoices || 0 : 0; + // `paidInvoices` can be 0 while invoices exist (every one still a draft). The + // old `totalRevenue / paidInvoices || 0` then evaluated to Infinity, because + // `Infinity` is truthy, and the analytics screen reported an infinite average. + const averageInvoiceAmount = paidInvoices > 0 ? totalRevenue / paidInvoices : 0; // Revenue by month const revenueByMonth: Record = {}; diff --git a/src/services/paymentMethodService.ts b/src/services/paymentMethodService.ts index c6e90bf5..d116c500 100644 --- a/src/services/paymentMethodService.ts +++ b/src/services/paymentMethodService.ts @@ -1,6 +1,8 @@ import { ethers } from 'ethers'; -import { getEvmRpcUrl } from '../config/evm'; +import { getEvmRpcUrls } from '../config/evm'; +import { getOrCreateResilientProvider } from './rpcProvider'; +import { exceedsLimit } from '../utils/decimalAmount'; import { CHAIN_IDS } from '../utils/constants/values'; import { PaymentMethod, @@ -195,7 +197,7 @@ export class PaymentMethodService { } try { - const provider = new ethers.providers.JsonRpcProvider(getEvmRpcUrl(method.chainId)); + const provider = getOrCreateResilientProvider(method.chainId, getEvmRpcUrls(method.chainId)); const erc20Abi = [ 'function decimals() view returns (uint8)', 'function symbol() view returns (string)', @@ -307,29 +309,35 @@ export class PaymentMethodService { chainId: number ): Promise<{ sufficient: boolean; balance: string; symbol: string }> { try { - const provider = new ethers.providers.JsonRpcProvider(getEvmRpcUrl(chainId)); + const provider = getOrCreateResilientProvider(chainId, getEvmRpcUrls(chainId)); const conn = this.walletManager?.getConnection(); if (!conn) { return { sufficient: false, balance: '0', symbol: method.tokenType }; } let balance: ethers.BigNumber; + // Read the token's own decimals instead of assuming "USDC = 6, + // everything else = 18". Assuming wrong under- or over-reports the + // balance, which triggers spurious fallbacks or lets a charge through + // that cannot settle. + let decimals = 18; if (method.tokenType === TokenType.NATIVE) { balance = await provider.getBalance(conn.address); } else { - const erc20Abi = ['function balanceOf(address) view returns (uint256)']; + const erc20Abi = [ + 'function decimals() view returns (uint8)', + 'function balanceOf(address) view returns (uint256)', + ]; const contract = new ethers.Contract(method.tokenAddress, erc20Abi, provider); + decimals = await contract.decimals(); balance = await contract.balanceOf(conn.address); } - const required = ethers.utils.parseUnits( - requiredAmount, - method.tokenType === TokenType.USDC ? 6 : 18 - ); + const required = ethers.utils.parseUnits(requiredAmount, decimals); return { sufficient: balance.gte(required), - balance: balance.toString(), + balance: ethers.utils.formatUnits(balance, decimals), symbol: method.tokenType.toString(), }; } catch { @@ -342,7 +350,7 @@ export class PaymentMethodService { maxGasPriceGwei: number ): Promise<{ acceptable: boolean; currentGasPrice: string }> { try { - const provider = new ethers.providers.JsonRpcProvider(getEvmRpcUrl(chainId)); + const provider = getOrCreateResilientProvider(chainId, getEvmRpcUrls(chainId)); const gasPrice = await provider.getGasPrice(); const gasPriceGwei = parseFloat(ethers.utils.formatUnits(gasPrice, 'gwei')); @@ -441,10 +449,7 @@ export class PaymentMethodService { continue; } - if ( - method.maxSpendPerInterval && - ethers.BigNumber.from(amount).gt(method.maxSpendPerInterval) - ) { + if (method.maxSpendPerInterval && exceedsLimit(amount, method.maxSpendPerInterval)) { attempt.status = 'failed'; attempt.failureReason = `Amount ${amount} exceeds max spend per interval ${method.maxSpendPerInterval}`; attempt.resolvedAt = new Date(); @@ -485,7 +490,7 @@ export class PaymentMethodService { } try { - const provider = new ethers.providers.JsonRpcProvider(getEvmRpcUrl(method.chainId)); + const provider = getOrCreateResilientProvider(method.chainId, getEvmRpcUrls(method.chainId)); const code = await provider.getCode(method.tokenAddress); const newHash = ethers.utils.keccak256(code); @@ -682,10 +687,7 @@ export class PaymentMethodService { continue; } - if ( - method.maxSpendPerInterval && - ethers.BigNumber.from(amount).gt(method.maxSpendPerInterval) - ) { + if (method.maxSpendPerInterval && exceedsLimit(amount, method.maxSpendPerInterval)) { fail(`Amount ${amount} exceeds max spend per interval ${method.maxSpendPerInterval}`); continue; } diff --git a/src/services/rpcProvider.ts b/src/services/rpcProvider.ts index 246a1436..eb69dc73 100644 --- a/src/services/rpcProvider.ts +++ b/src/services/rpcProvider.ts @@ -73,6 +73,8 @@ interface CircuitEntry { state: CircuitState; consecutiveFailures: number; openedAt: number | null; + /** Consecutive successes observed while HALF-OPEN, used to close the circuit. */ + halfOpenSuccesses: number; } // ───────────────────────────────────────────────────────────────────────────── @@ -88,6 +90,8 @@ export interface ResilientProviderConfig { failureThreshold?: number; /** Ms the circuit stays OPEN before transitioning to HALF-OPEN. Default: 30_000. */ recoveryTimeoutMs?: number; + /** Consecutive successes in HALF-OPEN before closing the circuit. Default: 2. */ + successThreshold?: number; } const DEFAULT_CONFIG: Required = { @@ -95,6 +99,7 @@ const DEFAULT_CONFIG: Required = { jitterMs: 500, failureThreshold: 5, recoveryTimeoutMs: 30_000, + successThreshold: 2, }; /** Returns a sensible timeout for EVM chains with higher RPC variance. */ @@ -132,6 +137,15 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { private readonly _config: Required; private readonly _circuits = new Map(); + /** + * The base ethers provider talks to a single mutable `connection.url`, so + * exactly one `send()` may be swapping that URL at a time. Without this + * chain, two concurrent calls (e.g. `getBalancesAcrossChains`, which fans + * out in parallel) interleave and one of them is sent to the other's + * endpoint — which may be a node on an entirely different chain. + */ + private _swapChain: Promise = Promise.resolve(); + constructor(urls: string[], chainId: number, config: ResilientProviderConfig = {}) { super(urls[0], chainId); this._urls = urls; @@ -141,6 +155,7 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { jitterMs: config.jitterMs ?? DEFAULT_CONFIG.jitterMs, failureThreshold: config.failureThreshold ?? DEFAULT_CONFIG.failureThreshold, recoveryTimeoutMs: config.recoveryTimeoutMs ?? DEFAULT_CONFIG.recoveryTimeoutMs, + successThreshold: config.successThreshold ?? DEFAULT_CONFIG.successThreshold, }; // Initialise circuit entries for every URL @@ -149,6 +164,7 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { state: 'closed', consecutiveFailures: 0, openedAt: null, + halfOpenSuccesses: 0, }); } } @@ -166,6 +182,7 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { const elapsed = Date.now() - (circuit.openedAt ?? 0); if (elapsed >= this._config.recoveryTimeoutMs) { circuit.state = 'half-open'; + circuit.halfOpenSuccesses = 0; } else { errors.push({ url, message: new RpcProviderCircuitOpenError(url).message }); continue; @@ -178,7 +195,17 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { return result; } catch (err) { const message = err instanceof Error ? err.message : String(err); - this._recordFailure(url); + + // Only transport-level problems indicate an unhealthy endpoint. A + // legitimate JSON-RPC failure (revert, insufficient funds, invalid + // params) means the node answered correctly, so counting it towards + // the failure threshold would evict a perfectly healthy URL. + if (isTransportFailure(err)) { + this._recordFailure(url); + } else { + this._recordSuccess(url); + } + errors.push({ url, message }); logger.warn( @@ -194,7 +221,12 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { private _getCircuit(url: string): CircuitEntry { if (!this._circuits.has(url)) { - this._circuits.set(url, { state: 'closed', consecutiveFailures: 0, openedAt: null }); + this._circuits.set(url, { + state: 'closed', + consecutiveFailures: 0, + openedAt: null, + halfOpenSuccesses: 0, + }); } return this._circuits.get(url)!; } @@ -202,13 +234,27 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { private _recordSuccess(url: string): void { const c = this._getCircuit(url); c.consecutiveFailures = 0; - c.state = 'closed'; c.openedAt = null; + + if (c.state === 'half-open') { + c.halfOpenSuccesses += 1; + // Require consecutive successes before trusting a recovered endpoint + // again, so a node that flaps does not ping-pong the circuit. + if (c.halfOpenSuccesses >= this._config.successThreshold) { + c.state = 'closed'; + c.halfOpenSuccesses = 0; + } + return; + } + + c.state = 'closed'; + c.halfOpenSuccesses = 0; } private _recordFailure(url: string): void { const c = this._getCircuit(url); c.consecutiveFailures += 1; + c.halfOpenSuccesses = 0; if (c.state === 'half-open') { // Failed probe — reopen @@ -237,6 +283,39 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), deadline); + // Serialise access to the shared `connection.url`. Each caller waits for + // the previous swap-and-restore to complete before touching it. + const run = this._swapChain.then( + () => this._sendToUrl(url, method, params, controller.signal, deadline), + () => this._sendToUrl(url, method, params, controller.signal, deadline) + ); + + this._swapChain = run.then( + () => undefined, + () => undefined + ); + + try { + return await run; + } finally { + clearTimeout(timer); + } + } + + private async _sendToUrl( + url: string, + method: string, + params: Array, + signal: AbortSignal, + deadline: number + ): Promise { + // The deadline starts when the caller enqueued, so a call that waited + // behind another may already be expired. The abort event has already + // fired in that case, so fail fast instead of waiting unbounded. + if (signal.aborted) { + throw new RpcProviderTimeoutError(url, deadline); + } + // We need to temporarily swap the underlying connection URL so the base // JsonRpcProvider sends to the right endpoint. const previousUrl = this.connection.url; @@ -247,7 +326,7 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { // Race the call against an abort-signal-aware timeout const timeoutPromise = new Promise((_, reject) => { - controller.signal.addEventListener( + signal.addEventListener( 'abort', () => { reject(new RpcProviderTimeoutError(url, deadline)); @@ -258,7 +337,6 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { return await Promise.race([callPromise, timeoutPromise]); } finally { - clearTimeout(timer); // Restore URL in case of future calls to this same provider instance (this as unknown as { connection: { url: string } }).connection.url = previousUrl; } @@ -281,11 +359,73 @@ export class ResilientJsonRpcProvider extends ethers.providers.JsonRpcProvider { entry.state = 'closed'; entry.consecutiveFailures = 0; entry.openedAt = null; + entry.halfOpenSuccesses = 0; } logger.info(`[ResilientJsonRpcProvider] All circuits reset for chain ${this._chainId}`); } } +/** + * True when an error indicates the endpoint itself is unreachable rather than + * the request being rejected by a working node. + * + * This is what separates "this RPC URL is down" from "this transaction + * reverted", and it is the difference between a circuit breaker that protects + * the user and one that ejects healthy nodes during an incident. + */ +export function isTransportFailure(error: unknown): boolean { + if (error instanceof RpcProviderTimeoutError) return true; + if (error instanceof RpcProviderCircuitOpenError) return false; + + if (!error || typeof error !== 'object') return false; + + const err = error as { + code?: unknown; + error?: { code?: unknown }; + message?: unknown; + reason?: unknown; + }; + + // ethers wraps server responses in `error.code` as a numeric JSON-RPC code. + // Anything that reaches here with one of those is an application-level + // answer from a live node. + const codes = [err.code, err.error?.code]; + if (codes.some((code) => typeof code === 'number' || /^-?\d+$/.test(String(code)))) { + return false; + } + + const transportCodes = [ + 'TIMEOUT', + 'ETIMEDOUT', + 'ESOCKETTIMEDOUT', + 'ECONNREFUSED', + 'ECONNRESET', + 'EHOSTUNREACH', + 'ENETUNREACH', + 'EPIPE', + 'ERR_NETWORK', + 'NETWORK_ERROR', + ]; + if (codes.some((code) => typeof code === 'string' && transportCodes.includes(code))) { + return true; + } + + const text = [err.message, err.reason] + .filter((part): part is string => typeof part === 'string') + .join(' ') + .toLowerCase(); + + return ( + text.includes('failed to fetch') || + text.includes('network error') || + text.includes('fetch failed') || + text.includes('econnrefused') || + text.includes('econnreset') || + text.includes('socket hang up') || + text.includes('getaddrinfo') + ); +} + // ───────────────────────────────────────────────────────────────────────────── // Singleton registry — circuit state persists across calls in the same session // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/services/transactionConfirmation.ts b/src/services/transactionConfirmation.ts new file mode 100644 index 00000000..af9ce0eb --- /dev/null +++ b/src/services/transactionConfirmation.ts @@ -0,0 +1,287 @@ +/** + * transactionConfirmation.ts + * + * Shared, defensive waiting for on-chain transaction receipts. + * + * Every on-chain payment path in SubTrackr previously called `tx.wait()` + * with no arguments and then only asserted that a hash came back. That is + * unsafe in three ways: + * + * 1. `tx.wait()` with no timeout can hang forever if a transaction is + * stuck in the mempool, which also stalls the offline retry queue. + * 2. `receipt.status === 0` means the transaction was *mined but reverted*. + * Treating "mined" as "succeeded" reports a failed payment as a success. + * 3. A single confirmation is not final. On fast chains a reorg can still + * drop the transaction, so users need a chain-appropriate depth. + * + * This module centralises those three concerns so all payment paths behave + * the same way and can be tested in isolation. + */ + +import type { ethers } from 'ethers'; + +import { logger } from './logging'; + +// ───────────────────────────────────────────────────────────────────────────── +// Errors +// ───────────────────────────────────────────────────────────────────────────── + +/** The transaction was mined but the receipt reports `status === 0`. */ +export class TransactionRevertedError extends Error { + readonly code = 'TRANSACTION_REVERTED' as const; + readonly transactionHash: string; + + constructor(transactionHash: string, detail?: string) { + super( + `Transaction ${transactionHash} was reverted on-chain${detail ? `: ${detail}` : '.'}` + ); + this.name = 'TransactionRevertedError'; + this.transactionHash = transactionHash; + Object.setPrototypeOf(this, new.target.prototype); + } +} + +/** No receipt arrived within the allotted timeout. */ +export class TransactionTimeoutError extends Error { + readonly code = 'TRANSACTION_TIMEOUT' as const; + readonly transactionHash: string; + readonly timeoutMs: number; + + constructor(transactionHash: string, timeoutMs: number) { + super( + `Transaction ${transactionHash} was not confirmed within ${timeoutMs} ms. ` + + `It may still be pending on-chain — check your wallet before retrying.` + ); + this.name = 'TransactionTimeoutError'; + this.transactionHash = transactionHash; + this.timeoutMs = timeoutMs; + Object.setPrototypeOf(this, new.target.prototype); + } +} + +/** The transaction was confirmed but never reached the required depth. */ +export class TransactionInsufficientConfirmationsError extends Error { + readonly code = 'TRANSACTION_INSUFFICIENT_CONFIRMATIONS' as const; + readonly transactionHash: string; + readonly actual: number; + readonly required: number; + + constructor(transactionHash: string, actual: number, required: number) { + super( + `Transaction ${transactionHash} has ${actual} confirmation(s) but ${required} are required.` + ); + this.name = 'TransactionInsufficientConfirmationsError'; + this.transactionHash = transactionHash; + this.actual = actual; + this.required = required; + Object.setPrototypeOf(this, new.target.prototype); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Confirmation policy +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Minimum block confirmations before a payment is treated as settled. + * + * L2s reorg far less often than L1 and produce a block every ~250ms, so + * waiting many blocks would add latency without adding safety. Ethereum + * mainnet uses the deeper default. + */ +export const DEFAULT_CONFIRMATIONS = 2; + +const L2_CHAIN_IDS = new Set([10, 137, 42161, 8453, 43114, 56, 534352]); + +/** Confirmation depth appropriate for `chainId`. */ +export function requiredConfirmations(chainId: number, override?: number): number { + if (override !== undefined) { + return Math.max(1, Math.floor(override)); + } + return L2_CHAIN_IDS.has(chainId) ? 1 : DEFAULT_CONFIRMATIONS; +} + +/** + * Default receipt timeout. Generous enough for congested mainnet, short + * enough that a stuck mempool transaction does not block the queue forever. + */ +export const DEFAULT_CONFIRMATION_TIMEOUT_MS = 5 * 60 * 1000; + +// ───────────────────────────────────────────────────────────────────────────── +// Receipt classification +// ───────────────────────────────────────────────────────────────────────────── + +/** + * ethers surfaces a `wait()` timeout as a `TRANSACTION_REPLACED`-adjacent + * error, a plain `Error` carrying `code === 'TIMEOUT'`, or — depending on + * version — a rejected promise with a `timeout` field. Detection is done on + * shape rather than `instanceof` so that it stays robust across ethers v5/v6. + */ +function isWaitTimeout(error: unknown): boolean { + if (!error || typeof error !== 'object') return false; + + const err = error as { code?: unknown; message?: unknown; reason?: unknown; timeout?: unknown }; + if (err.code === 'TIMEOUT' || err.code === 'ETIMEDOUT' || err.code === 'ESOCKETTIMEDOUT') { + return true; + } + + const text = [err.message, err.reason] + .filter((part): part is string => typeof part === 'string') + .join(' ') + .toLowerCase(); + + return ( + text.includes('timeout') || + text.includes('timed out') || + text.includes('could not detect transaction') || + (typeof err.timeout === 'number' && text.includes('transaction')) + ); +} + +/** + * True when the error means "this transaction failed", as opposed to "we + * could not observe the outcome". Used to decide whether a queued item may + * be retried safely. + */ +export function isDefinitiveFailure(error: unknown): boolean { + return ( + error instanceof TransactionRevertedError || + error instanceof TransactionInsufficientConfirmationsError + ); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Wait +// ───────────────────────────────────────────────────────────────────────────── + +export interface ConfirmTransactionOptions { + chainId?: number; + /** Override the chain-derived confirmation depth. */ + confirmations?: number; + /** Override the receipt timeout. */ + timeoutMs?: number; + /** Human label used in error messages, e.g. `Sablier stream creation`. */ + operation?: string; +} + +/** The subset of a transaction response this module needs. */ +export interface AwaitableTransactionResponse { + hash: string; + wait: (confirmations?: number) => Promise; +} + +function label(options: ConfirmTransactionOptions): string { + return options.operation ? `${options.operation} ` : ''; +} + +/** + * Nodes expose revert reasons in several vendor-specific shapes and ethers v5 + * does not model them on `TransactionReceipt`. Read defensively so a missing + * reason never turns into a crash inside error handling. + */ +function extractRevertReason(receipt: ethers.providers.TransactionReceipt): string | undefined { + const candidate = receipt as unknown as { + revertReason?: unknown; + revertName?: unknown; + }; + + const reason = candidate.revertReason ?? candidate.revertName; + return typeof reason === 'string' && reason.length > 0 ? reason : undefined; +} + +/** + * Waits for `txResponse` to reach the required confirmation depth and proves + * it actually succeeded. + * + * Resolves with the confirmed receipt. Throws `TransactionRevertedError`, + * `TransactionTimeoutError` or `TransactionInsufficientConfirmationsError`. + */ +export async function confirmTransaction( + txResponse: AwaitableTransactionResponse, + options: ConfirmTransactionOptions = {} +): Promise { + const { + chainId, + confirmations = requiredConfirmations(chainId ?? 1), + timeoutMs = DEFAULT_CONFIRMATION_TIMEOUT_MS, + } = options; + + const txHash = txResponse.hash; + + let receipt: ethers.providers.TransactionReceipt | null = null; + try { + // The timeout is enforced here rather than delegated to ethers. In ethers + // v5 `ContractTransaction.wait(confirmations)` does not forward a timeout + // argument at all, so relying on it left every contract-based payment + // (ERC-20 approve, Sablier stream creation) able to hang indefinitely and + // to stall the offline retry queue queued behind it. + let timer: ReturnType | undefined = undefined; + + try { + receipt = await Promise.race([ + txResponse.wait(confirmations) as Promise, + new Promise((_resolve, reject) => { + timer = setTimeout(() => { + reject(new TransactionTimeoutError(txHash, timeoutMs)); + }, timeoutMs); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + } + } catch (error) { + if (error instanceof TransactionTimeoutError) throw error; + if (isWaitTimeout(error)) { + throw new TransactionTimeoutError(txHash, timeoutMs); + } + throw error; + } + + if (!receipt) { + // ethers resolves `null` when the transaction was replaced/dropped rather + // than mined. Treated as a timeout: the outcome is genuinely unknown. + throw new TransactionTimeoutError(txHash, timeoutMs); + } + + // A mined receipt with status 0 is a reverted transaction. Reporting this + // as success is what previously let failed payments look confirmed. + if (receipt.status === 0) { + throw new TransactionRevertedError(txHash, extractRevertReason(receipt)); + } + + const actual = receipt.confirmations ?? confirmations; + if (actual < confirmations) { + throw new TransactionInsufficientConfirmationsError(txHash, actual, confirmations); + } + + return receipt; +} + +/** + * Resolves the transaction hash without failing the surrounding operation. + * + * Used by the offline queue: when a broadcast may have succeeded but the + * receipt could not be observed (app crash, dropped connection), the hash is + * still the durable handle the user needs in order to check on-chain. + */ +export async function bestEffortTransactionHash( + txResponse: AwaitableTransactionResponse | null | undefined, + options: ConfirmTransactionOptions = {} +): Promise { + if (!txResponse?.hash) return undefined; + + try { + await confirmTransaction(txResponse, options); + return txResponse.hash; + } catch (error) { + if (error instanceof TransactionRevertedError) { + // Definitive: the chain says it failed. Surface it, do not swallow it. + throw error; + } + logger.warn( + `${label(options)}receipt for ${txResponse.hash} could not be confirmed: ` + + (error instanceof Error ? error.message : String(error)) + ); + return txResponse.hash; + } +} \ No newline at end of file diff --git a/src/services/walletService.ts b/src/services/walletService.ts index 85941144..86968774 100644 --- a/src/services/walletService.ts +++ b/src/services/walletService.ts @@ -2,10 +2,10 @@ import { ethers } from 'ethers'; import { Framework } from '@superfluid-finance/sdk-core'; import { logger } from './logging'; -import { PaymentMethodService } from './paymentMethodService'; import { ERC20__factory, getContractAddress } from '../contracts'; import { getEvmRpcUrl, getEvmRpcUrls } from '../config/evm'; import { getOrCreateResilientProvider } from './rpcProvider'; +import { confirmTransaction } from './transactionConfirmation'; import { ContractError, ContractErrorCode, NetworkError, NetworkErrorCode } from '../errors'; import { TIME_CONSTANTS, @@ -829,7 +829,10 @@ export class WalletServiceManager { ); const txResponse = await createOp.exec(signer); - const receipt = await txResponse.wait(); + const receipt = await confirmTransaction(txResponse, { + chainId, + operation: 'Superfluid stream creation', + }); if (!receipt?.transactionHash) { throw new Error('Transaction mined without a hash'); @@ -897,7 +900,10 @@ export class WalletServiceManager { ); if (currentAllowance.lt(amountBn)) { const txApprove = await erc20.approve(SABLIER_V2_LOCKUP_LINEAR, amountBn); - await txApprove.wait(); + await confirmTransaction(txApprove, { + chainId, + operation: 'Sablier token approval', + }); } // 3. Create the Sablier Stream @@ -926,7 +932,10 @@ export class WalletServiceManager { }; const txCreate = await sablierContract.createWithDurations(params); - const receipt = await txCreate.wait(); + const receipt = await confirmTransaction(txCreate, { + chainId, + operation: 'Sablier stream creation', + }); if (!receipt?.transactionHash) { throw new Error('Transaction mined without a hash'); @@ -1020,7 +1029,7 @@ export class WalletServiceManager { const erc20 = new ethers.Contract(token, erc20Abi, signer); try { const tx = await erc20.approve(spender, amount); - const receipt = await tx.wait(); + const receipt = await confirmTransaction(tx, { operation: 'Token approval' }); if (!receipt?.transactionHash) { throw new Error('Approval transaction mined without a hash'); } diff --git a/src/store/_tests_/transactionQueueStore.test.ts b/src/store/_tests_/transactionQueueStore.test.ts index c78a3e86..92160592 100644 --- a/src/store/_tests_/transactionQueueStore.test.ts +++ b/src/store/_tests_/transactionQueueStore.test.ts @@ -122,4 +122,168 @@ describe('transactionQueueStore', () => { expect(mockCreateSuperfluidStream).not.toHaveBeenCalled(); expect(useTransactionQueueStore.getState().queuedTransactions).toHaveLength(0); }); + + it('measures staleness from the last attempt, not creation', async () => { + // A transaction that keeps retrying is still making progress, so it must + // not be expired purely because it was created over 30 minutes ago. + const oldCreation = Date.now() - 31 * 60 * 1000; + const recentAttempt = Date.now() - 5_000; + + mockCreateSuperfluidStream.mockResolvedValue({ streamId: 'stream:1', txHash: '0xhash' }); + + useTransactionQueueStore.setState({ + queuedTransactions: [ + { + id: 'retrying_tx', + createdAt: oldCreation, + updatedAt: recentAttempt, + lastAttemptAt: recentAttempt, + attempts: 1, + conflictKey: 'superfluid:137:usdc:0x1111111111111111111111111111111111111111', + status: 'pending', + payload: samplePayload, + }, + ], + }); + + await act(async () => { + await useTransactionQueueStore.getState().processQueue(); + }); + + expect(mockCreateSuperfluidStream).toHaveBeenCalledTimes(1); + }); + + it('records the broadcast marker before handing the payload to the wallet', async () => { + let broadcastedAtSeenDuringExecution: number | undefined; + + mockCreateSuperfluidStream.mockImplementation(async () => { + // Inspect persisted state mid-flight: the marker must already be set, + // otherwise a crash here would cause a duplicate charge on restart. + const [entry] = useTransactionQueueStore.getState().queuedTransactions; + broadcastedAtSeenDuringExecution = entry?.broadcastedAt; + return { streamId: 'stream:1', txHash: '0xhash' }; + }); + + await act(async () => { + await useTransactionQueueStore.getState().queueTransaction(samplePayload); + await useTransactionQueueStore.getState().processQueue(); + }); + + expect(broadcastedAtSeenDuringExecution).toEqual(expect.any(Number)); + expect(useTransactionQueueStore.getState().queuedTransactions).toHaveLength(0); + }); + + it('retires a transaction whose outcome is unknown instead of re-broadcasting it', async () => { + // `broadcastedAt` in the past plus no receipt means the wallet may have + // already submitted it. Re-sending would charge the user twice. + const longAgo = Date.now() - 20 * 60 * 1000; + + useTransactionQueueStore.setState({ + queuedTransactions: [ + { + id: 'in_flight_tx', + createdAt: longAgo, + updatedAt: longAgo, + lastAttemptAt: longAgo, + broadcastedAt: longAgo, + attempts: 1, + conflictKey: 'superfluid:137:usdc:0x1111111111111111111111111111111111111111', + status: 'pending', + payload: samplePayload, + }, + ], + }); + + await act(async () => { + await useTransactionQueueStore.getState().processQueue(); + }); + + expect(mockCreateSuperfluidStream).not.toHaveBeenCalled(); + expect(useTransactionQueueStore.getState().queuedTransactions).toHaveLength(0); + expect(useTransactionQueueStore.getState().lastError).toMatch(/submitted before/i); + }); + + it('defers a retry until its backoff window has elapsed', async () => { + const now = Date.now(); + + mockCreateSuperfluidStream.mockRejectedValue(new Error('execution failed')); + + useTransactionQueueStore.setState({ + queuedTransactions: [ + { + id: 'backoff_tx', + createdAt: now - 1000, + updatedAt: now - 1000, + attempts: 1, + conflictKey: 'superfluid:137:usdc:0x1111111111111111111111111111111111111111', + status: 'pending', + payload: samplePayload, + nextAttemptAt: now + 60_000, + }, + ], + }); + + await act(async () => { + await useTransactionQueueStore.getState().processQueue(); + }); + + expect(mockCreateSuperfluidStream).not.toHaveBeenCalled(); + expect(useTransactionQueueStore.getState().queuedTransactions).toHaveLength(1); + }); + + it('schedules a backoff after a failed attempt', async () => { + mockCreateSuperfluidStream.mockRejectedValue(new Error('execution failed')); + + const before = Date.now(); + await act(async () => { + await useTransactionQueueStore.getState().queueTransaction(samplePayload); + await useTransactionQueueStore.getState().processQueue(); + }); + + const [entry] = useTransactionQueueStore.getState().queuedTransactions; + expect(entry?.nextAttemptAt).toBeGreaterThan(before); + }); + + it('keeps the broadcast marker after an ambiguous failure', async () => { + // A generic failure does not prove the transaction never reached the + // network, so the marker must survive to prevent a double charge. + mockCreateSuperfluidStream.mockRejectedValue(new Error('execution failed')); + + await act(async () => { + await useTransactionQueueStore.getState().queueTransaction(samplePayload); + await useTransactionQueueStore.getState().processQueue(); + }); + + const [entry] = useTransactionQueueStore.getState().queuedTransactions; + expect(entry?.broadcastedAt).toEqual(expect.any(Number)); + }); + + it('clears the broadcast marker when the wallet provably rejected the request', async () => { + mockCreateSuperfluidStream.mockRejectedValue( + Object.assign(new Error('Transaction was rejected in your wallet.'), { + code: 'USER_REJECTED', + }) + ); + + await act(async () => { + await useTransactionQueueStore.getState().queueTransaction(samplePayload); + await useTransactionQueueStore.getState().processQueue(); + }); + + const [entry] = useTransactionQueueStore.getState().queuedTransactions; + // Nothing was submitted, so an immediate retry is safe. + expect(entry?.broadcastedAt).toBeUndefined(); + }); + + it('keeps the connectivity listener alive while another consumer is mounted', async () => { + const unsubscribeFirst = + useTransactionQueueStore.getState().initializeConnectivityListener(); + const unsubscribeSecond = + useTransactionQueueStore.getState().initializeConnectivityListener(); + + unsubscribeFirst(); + + // The second consumer is still mounted, so tearing down must not happen. + expect(() => unsubscribeSecond()).not.toThrow(); + }); }); diff --git a/src/store/slices/analyticsSlice.ts b/src/store/slices/analyticsSlice.ts index 010a1f68..a76538b1 100644 --- a/src/store/slices/analyticsSlice.ts +++ b/src/store/slices/analyticsSlice.ts @@ -14,7 +14,8 @@ import { calculateSubscriptionAnalytics, SubscriptionAnalyticsReport, } from '../../services/analyticsService'; -import { BillingCycle, Subscription } from '../../types/subscription'; +import { Subscription } from '../../types/subscription'; +import { monthlyEquivalent } from '../../utils/billingCycleMath'; import { generateCSV } from '../../utils/importExport'; import { CohortService } from '../../../backend/services/analytics/cohortService'; import { cohortTableToCsv } from '../../../backend/services/analytics/cohortReportExport'; @@ -53,12 +54,7 @@ const toSubscriberRecords = (subscriptions: Subscription[]): SubscriberRecord[] signupAt: new Date(subscription.createdAt).getTime(), churnedAt: subscription.isActive ? undefined : new Date(subscription.updatedAt).getTime(), lastActiveAt: new Date(subscription.updatedAt).getTime(), - mrr: - subscription.billingCycle === BillingCycle.YEARLY - ? subscription.price / 12 - : subscription.billingCycle === BillingCycle.WEEKLY - ? subscription.price * 4.345 - : subscription.price, + mrr: monthlyEquivalent(subscription.price, subscription.billingCycle), })); export interface CreditMetricSnapshot { diff --git a/src/store/subscriptionStore.ts b/src/store/subscriptionStore.ts index 0e5518a0..ae12ec18 100644 --- a/src/store/subscriptionStore.ts +++ b/src/store/subscriptionStore.ts @@ -27,7 +27,8 @@ import { InvoiceStatus, isOpenInvoice } from '../types/invoice'; import { dummySubscriptions } from '../utils/dummyData'; // eslint-disable-line import { advanceBillingDate, calculateNextBillingDate } from '../utils/billingDate'; import { buildBillingPeriod } from '../utils/invoice'; -import { BILLING_CONVERSIONS, CACHE_CONSTANTS } from '../utils/constants/values'; +import { CACHE_CONSTANTS } from '../utils/constants/values'; +import { monthlyEquivalent, yearlyEquivalent } from '../utils/billingCycleMath'; import { syncRenewalReminders, presentChargeSuccessNotification, @@ -1392,11 +1393,7 @@ export const useSubscriptionStore = create()( preferredCurrency, rates ); - if (sub.billingCycle === 'monthly') return total + priceInPreferred; - if (sub.billingCycle === 'yearly') return total + priceInPreferred / 12; - if (sub.billingCycle === 'weekly') - return total + priceInPreferred * BILLING_CONVERSIONS.WEEKS_PER_MONTH; - return total + priceInPreferred; + return total + monthlyEquivalent(priceInPreferred, sub.billingCycle); }, 0); const totalYearlySpend = activeSubs.reduce((total, sub) => { @@ -1406,12 +1403,7 @@ export const useSubscriptionStore = create()( preferredCurrency, rates ); - if (sub.billingCycle === 'yearly') return total + priceInPreferred; - if (sub.billingCycle === 'monthly') - return total + priceInPreferred * BILLING_CONVERSIONS.MONTHS_PER_YEAR; - if (sub.billingCycle === 'weekly') - return total + priceInPreferred * BILLING_CONVERSIONS.WEEKS_PER_YEAR; - return total + priceInPreferred * BILLING_CONVERSIONS.MONTHS_PER_YEAR; + return total + yearlyEquivalent(priceInPreferred, sub.billingCycle); }, 0); const categoryBreakdown = activeSubs.reduce( diff --git a/src/store/transactionQueueStore.ts b/src/store/transactionQueueStore.ts index 9d691a1a..3e3ada3e 100644 --- a/src/store/transactionQueueStore.ts +++ b/src/store/transactionQueueStore.ts @@ -28,6 +28,17 @@ export interface QueuedTransaction { status: 'pending' | 'processing'; payload: QueuedTransactionPayload; lastError?: string; + /** + * Set immediately before the transaction is handed to the wallet. + * + * A broadcast can succeed on-chain even when the app dies before the + * receipt is observed. Persisting this marker first means the next queue + * pass can tell "never sent" apart from "sent, outcome unknown" and refuse + * to re-broadcast the latter — which would otherwise charge the user twice. + */ + broadcastedAt?: number; + /** Earliest time the next attempt may start (exponential backoff). */ + nextAttemptAt?: number; } export interface ExecuteOrQueueResult { @@ -59,7 +70,21 @@ const STORAGE_KEY = 'subtrackr-transaction-queue'; const STALE_TRANSACTION_TIMEOUT_MS = 30 * 60 * 1000; const MAX_ATTEMPTS = 3; +/** First retry delay after a failed attempt. Doubles each subsequent failure. */ +const RETRY_BASE_BACKOFF_MS = 5_000; +const RETRY_MAX_BACKOFF_MS = 5 * 60 * 1000; + +/** + * A queued item that was already handed to the wallet but whose receipt was + * never observed is *not* safe to re-broadcast. It is retired and surfaced + * to the user instead, because the on-chain outcome is genuinely unknown and + * a blind retry risks a second charge. + */ +const UNKNOWN_OUTCOME_TIMEOUT_MS = 10 * 60 * 1000; + let connectivitySubscription: NetInfoSubscription | null = null; +/** Reference count so one consumer unmounting does not silence every other. */ +let connectivitySubscriberCount = 0; const isOnlineState = (isConnected: boolean | null, isInternetReachable: boolean | null): boolean => Boolean(isConnected) && isInternetReachable !== false; @@ -94,6 +119,56 @@ const isLikelyConflictError = (error: unknown): boolean => { ); }; +/** + * Exponential backoff with a ceiling, so a flapping connection cannot drive + * the queue into a tight retry loop that burns through `MAX_ATTEMPTS` in a + * single pass. + */ +const backoffDelayMs = (attempts: number): number => { + const exponent = Math.max(0, attempts - 1); + return Math.min(RETRY_BASE_BACKOFF_MS * 2 ** exponent, RETRY_MAX_BACKOFF_MS); +}; + +/** + * Runs a fire-and-forget promise without producing an unhandled rejection, + * which in React Native terminates the app. + */ +const safely = (promise: Promise, context: string): void => { + void promise.catch((error: unknown) => { + console.warn(`[transactionQueue] ${context} failed`, error); + }); +}; + +/** + * True only when the error proves the transaction never left the wallet. + * + * The broadcast marker may be cleared (and the item retried) in exactly these + * cases. For every ambiguous failure — a dropped connection, an RPC timeout — + * the marker is kept, because the transaction may already be in the mempool + * and re-sending it would charge the user twice. + */ +const isDefinitelyNotBroadcast = (error: unknown): boolean => { + if (!error || typeof error !== 'object') return false; + + const err = error as { code?: unknown; userMessage?: unknown; message?: unknown }; + + const code = typeof err.code === 'string' ? err.code : ''; + if ( + code === 'USER_REJECTED' || + code === 'WALLET_NOT_CONNECTED' || + code === 'INVALID_PARAMS' + ) { + return true; + } + + const text = [err.userMessage, err.message] + .filter((part): part is string => typeof part === 'string') + .join(' ') + .toLowerCase(); + + return text.includes('rejected in your wallet') || text.includes('user rejected'); +}; + const executeQueuedPayload = async ( payload: QueuedTransactionPayload ): Promise<{ streamId: string; txHash?: string }> => { @@ -135,10 +210,15 @@ export const useTransactionQueueStore = create()( lastError: null, initializeConnectivityListener: () => { + connectivitySubscriberCount += 1; + if (connectivitySubscription) { return () => { - connectivitySubscription?.(); - connectivitySubscription = null; + connectivitySubscriberCount = Math.max(0, connectivitySubscriberCount - 1); + if (connectivitySubscriberCount === 0) { + connectivitySubscription?.(); + connectivitySubscription = null; + } }; } @@ -148,27 +228,37 @@ export const useTransactionQueueStore = create()( set({ isOnline: online }); if (!wasOnline && online) { - void presentTransactionQueueNotification( - 'Back online', - 'Queued transactions are being processed now.' + safely( + presentTransactionQueueNotification( + 'Back online', + 'Queued transactions are being processed now.' + ), + 'reconnect notification' ); - void get().processQueue(); + safely(get().processQueue(), 'processQueue after reconnect'); } }); return () => { - connectivitySubscription?.(); - connectivitySubscription = null; + connectivitySubscriberCount = Math.max(0, connectivitySubscriberCount - 1); + if (connectivitySubscriberCount === 0) { + connectivitySubscription?.(); + connectivitySubscription = null; + } }; }, refreshConnectivity: async () => { - const state = await NetInfo.fetch(); - const online = isOnlineState(state.isConnected, state.isInternetReachable); - const wasOnline = get().isOnline; - set({ isOnline: online }); - if (!wasOnline && online) { - void get().processQueue(); + try { + const state = await NetInfo.fetch(); + const online = isOnlineState(state.isConnected, state.isInternetReachable); + const wasOnline = get().isOnline; + set({ isOnline: online }); + if (!wasOnline && online) { + safely(get().processQueue(), 'processQueue after connectivity refresh'); + } + } catch (error) { + console.warn('[transactionQueue] connectivity refresh failed', error); } }, @@ -204,11 +294,16 @@ export const useTransactionQueueStore = create()( }; }); - await presentTransactionQueueNotification( - 'Transaction queued', - replacedExisting - ? 'Updated a pending transaction with your latest request.' - : 'Transaction will run automatically once you are online.' + // A failed notification must not lose the queued transaction: the item is + // already persisted above, so this only downgrades to a warning. + safely( + presentTransactionQueueNotification( + 'Transaction queued', + replacedExisting + ? 'Updated a pending transaction with your latest request.' + : 'Transaction will run automatically once you are online.' + ), + 'queued notification' ); return { transactionId, replacedExisting }; @@ -262,19 +357,58 @@ export const useTransactionQueueStore = create()( for (const tx of sortedQueue) { if (!get().isOnline) break; - const age = now() - tx.createdAt; + // Staleness is measured from the last attempt, not creation, so a + // transaction that keeps making progress is not expired out from + // under itself on a long-lived queue. + const reference = tx.lastAttemptAt ?? tx.createdAt; + const age = now() - reference; if (age > STALE_TRANSACTION_TIMEOUT_MS) { set((state) => ({ queuedTransactions: state.queuedTransactions.filter((q) => q.id !== tx.id), })); - await presentTransactionQueueNotification( - 'Queued transaction expired', - 'A pending transaction was removed because it became stale.' + safely( + presentTransactionQueueNotification( + 'Queued transaction expired', + 'A pending transaction was removed because it became stale.' + ), + 'stale notification' + ); + continue; + } + + // Respect exponential backoff so a flapping connection cannot + // burn every attempt in a single pass. + if (tx.nextAttemptAt !== undefined && now() < tx.nextAttemptAt) { + continue; + } + + // A transaction that was already handed to the wallet but whose + // receipt was never observed must not be re-broadcast: the + // on-chain outcome is unknown and a retry would double-charge. + // Retire it and tell the user to verify manually. + if ( + tx.broadcastedAt !== undefined && + now() - tx.broadcastedAt > UNKNOWN_OUTCOME_TIMEOUT_MS + ) { + set((state) => ({ + queuedTransactions: state.queuedTransactions.filter((q) => q.id !== tx.id), + lastError: + 'A queued transaction may have been submitted before its result could be confirmed.', + })); + + safely( + presentTransactionQueueNotification( + 'Queued transaction needs review', + 'It may already have been submitted on-chain. Please check your wallet before retrying.' + ), + 'unknown-outcome notification' ); continue; } + // Mark the broadcast BEFORE handing the payload to the wallet so + // a crash between submission and receipt cannot cause a replay. set((state) => ({ queuedTransactions: state.queuedTransactions.map((queued) => queued.id === tx.id @@ -283,6 +417,8 @@ export const useTransactionQueueStore = create()( status: 'processing', attempts: queued.attempts + 1, lastAttemptAt: now(), + broadcastedAt: queued.broadcastedAt ?? now(), + nextAttemptAt: undefined, updatedAt: now(), lastError: undefined, } @@ -299,9 +435,12 @@ export const useTransactionQueueStore = create()( ), })); - await presentTransactionQueueNotification( - 'Queued transaction sent', - 'A pending transaction has been executed successfully.' + safely( + presentTransactionQueueNotification( + 'Queued transaction sent', + 'A pending transaction has been executed successfully.' + ), + 'success notification' ); } catch (error) { if (isLikelyNetworkError(error)) { @@ -334,9 +473,12 @@ export const useTransactionQueueStore = create()( ), })); - await presentTransactionQueueNotification( - 'Queued transaction skipped', - 'A pending transaction conflicted with another on-chain transaction and was removed.' + safely( + presentTransactionQueueNotification( + 'Queued transaction skipped', + 'A pending transaction conflicted with another on-chain transaction and was removed.' + ), + 'conflict notification' ); continue; } @@ -352,21 +494,32 @@ export const useTransactionQueueStore = create()( lastError: error instanceof Error ? error.message : 'Queued transaction failed.', })); - await presentTransactionQueueNotification( - 'Queued transaction failed', - error instanceof Error - ? `Dropped after retries: ${error.message}` - : 'Dropped after retry attempts.' + safely( + presentTransactionQueueNotification( + 'Queued transaction failed', + error instanceof Error + ? `Dropped after retries: ${error.message}` + : 'Dropped after retry attempts.' + ), + 'dropped notification' ); continue; } + // Schedule the next attempt with exponential backoff. The broadcast marker is + // cleared only when the failure proves nothing was submitted; + // otherwise it is kept so the queue never re-sends a payment + // that may already be in the mempool. + const safeToRetry = isDefinitelyNotBroadcast(error); + set((state) => ({ queuedTransactions: state.queuedTransactions.map((queued) => queued.id === tx.id ? { ...queued, status: 'pending', + broadcastedAt: safeToRetry ? undefined : queued.broadcastedAt, + nextAttemptAt: now() + backoffDelayMs(attempts), updatedAt: now(), lastError: error instanceof Error ? error.message : 'Execution failed.', } @@ -397,7 +550,10 @@ export const useTransactionQueueStore = create()( storage: createJSONStorage(() => asyncStorageAdapter), partialize: (state) => ({ queuedTransactions: state.queuedTransactions }), onRehydrateStorage: () => () => { - void useTransactionQueueStore.getState().refreshConnectivity(); + safely( + useTransactionQueueStore.getState().refreshConnectivity(), + 'rehydrate connectivity refresh' + ); }, } ) diff --git a/src/store/walletStore.ts b/src/store/walletStore.ts index fb64c5bc..fc5b1b8f 100644 --- a/src/store/walletStore.ts +++ b/src/store/walletStore.ts @@ -32,6 +32,30 @@ export interface NetworkMismatch { preferredNetwork: Network; } +/** + * Adapts stored `PaymentAttempt` records to the shape the fallback-chain + * health monitor consumes. + * + * The two shapes previously disagreed: this code read `attempt.success` and + * `attempt.createdAt`, neither of which exists on `PaymentAttempt` (it has + * `status` and `attemptedAt`). Every field resolved to `undefined`, so the + * monitor's look-back window discarded *all* attempts and every payment + * method scored a 100% success rate. Smart fallback ordering was therefore + * permanently inert. + */ +function buildRecentAttempts(attempts: PaymentAttempt[]): Array<{ + paymentMethodId: string; + success: boolean; + timestamp: Date; + latencyMs?: number; +}> { + return attempts.map((attempt) => ({ + paymentMethodId: attempt.paymentMethodId, + success: attempt.status === 'success', + timestamp: attempt.attemptedAt, + })); +} + interface WalletState { // Connection state from service connection: WalletConnection | null; @@ -438,7 +462,7 @@ export const useWalletStore = create()( const updatedMethods = paymentMethods.map((m) => m.id === id ? { ...m, isVerified: true, updatedAt: new Date() } : m ); - set({ paymentMethods: updatedMethods, isLoading: false }); + set({ paymentMethods: updatedMethods }); } return verified; } catch (error) { @@ -449,9 +473,13 @@ export const useWalletStore = create()( : error instanceof Error ? error.message : 'Failed to verify payment method', - isLoading: false, }); throw error; + } finally { + // Cleared unconditionally: previously `isLoading` was only reset + // inside the `if (verified)` branch and the catch, so a `false` + // result left the UI spinning forever. + set({ isLoading: false }); } }, @@ -796,15 +824,11 @@ export const useWalletStore = create()( const chain = get().fallbackChains.find((c) => c.id === chainId); if (!chain) return null; - // Build lightweight attempt objects from stored PaymentAttempts. - const recentAttempts = get().paymentAttempts.map((a) => ({ - paymentMethodId: a.paymentMethodId, - success: a.success, - timestamp: new Date(a.createdAt), - latencyMs: undefined as number | undefined, - })); - - return _chainHealthMonitor.snapshotChainHealth(chainId, chain.methodIds, recentAttempts); + return _chainHealthMonitor.snapshotChainHealth( + chainId, + chain.methodIds, + buildRecentAttempts(get().paymentAttempts) + ); }, getSmartFallbackSelection: (chainId: string) => { @@ -847,6 +871,7 @@ export const useWalletStore = create()( applyAllRotationPolicies: () => { const { rotationPolicies, fallbackChains, paymentAttempts } = get(); + const recentAttempts = buildRecentAttempts(paymentAttempts); const updatedPolicies: PaymentMethodRotationPolicy[] = []; for (const policy of rotationPolicies) { @@ -856,12 +881,6 @@ export const useWalletStore = create()( continue; } - const recentAttempts = paymentAttempts.map((a) => ({ - paymentMethodId: a.paymentMethodId, - success: a.success, - timestamp: new Date(a.createdAt), - })); - const snapshot = _chainHealthMonitor.snapshotChainHealth( chain.id, chain.methodIds, diff --git a/src/utils/__tests__/billingCycleMath.test.ts b/src/utils/__tests__/billingCycleMath.test.ts new file mode 100644 index 00000000..2d23791c --- /dev/null +++ b/src/utils/__tests__/billingCycleMath.test.ts @@ -0,0 +1,103 @@ +/** + * Tests for billingCycleMath — one shared billing-cycle normaliser. + * + * @see src/utils/billingCycleMath.ts + * @see https://github.com/Smartdevs17/SubTrackr/issues/1684 + */ + +import { BillingCycle } from '../../types/subscription'; +import { + WEEKS_PER_MONTH, + isAnnualCycle, + monthlyEquivalent, + normalizeBillingCycle, + yearlyEquivalent, +} from '../billingCycleMath'; + +describe('normalizeBillingCycle', () => { + it('passes through valid enum values', () => { + expect(normalizeBillingCycle('monthly')).toBe(BillingCycle.MONTHLY); + expect(normalizeBillingCycle('yearly')).toBe(BillingCycle.YEARLY); + expect(normalizeBillingCycle('annual')).toBe(BillingCycle.ANNUAL); + expect(normalizeBillingCycle('weekly')).toBe(BillingCycle.WEEKLY); + expect(normalizeBillingCycle('custom')).toBe(BillingCycle.CUSTOM); + }); + + it('is case and whitespace insensitive', () => { + expect(normalizeBillingCycle(' YEARLY ')).toBe(BillingCycle.YEARLY); + expect(normalizeBillingCycle('Monthly')).toBe(BillingCycle.MONTHLY); + }); + + it('maps common synonyms', () => { + expect(normalizeBillingCycle('annually')).toBe(BillingCycle.YEARLY); + expect(normalizeBillingCycle('year')).toBe(BillingCycle.YEARLY); + expect(normalizeBillingCycle('month')).toBe(BillingCycle.MONTHLY); + expect(normalizeBillingCycle('week')).toBe(BillingCycle.WEEKLY); + }); + + it('defaults to monthly for unknown or missing values', () => { + expect(normalizeBillingCycle('fortnightly')).toBe(BillingCycle.MONTHLY); + expect(normalizeBillingCycle('')).toBe(BillingCycle.MONTHLY); + expect(normalizeBillingCycle(undefined)).toBe(BillingCycle.MONTHLY); + expect(normalizeBillingCycle(null)).toBe(BillingCycle.MONTHLY); + expect(normalizeBillingCycle(42)).toBe(BillingCycle.MONTHLY); + }); +}); + +describe('monthlyEquivalent', () => { + it('leaves monthly prices untouched', () => { + expect(monthlyEquivalent(10, BillingCycle.MONTHLY)).toBe(10); + }); + + it('divides both annual spellings by twelve', () => { + // The regression: `annual` fell through to the monthly branch, so a + // 120/year plan reported 120 MRR instead of 10. + expect(monthlyEquivalent(120, BillingCycle.YEARLY)).toBe(10); + expect(monthlyEquivalent(120, BillingCycle.ANNUAL)).toBe(10); + expect(monthlyEquivalent(120, 'annual')).toBe(10); + }); + + it('multiplies weekly prices by the shared weeks-per-month constant', () => { + expect(monthlyEquivalent(10, BillingCycle.WEEKLY)).toBeCloseTo(10 * WEEKS_PER_MONTH, 10); + }); + + it('uses 52/12 for weeks per month so it cannot drift from BILLING_CONVERSIONS', () => { + expect(WEEKS_PER_MONTH).toBe(52 / 12); + // The old hard-coded value was 4.345, which differs from 52/12. + expect(WEEKS_PER_MONTH).not.toBe(4.345); + }); + + it('maps non-finite prices to zero', () => { + expect(monthlyEquivalent(Number.NaN, BillingCycle.MONTHLY)).toBe(0); + expect(monthlyEquivalent(Number.POSITIVE_INFINITY, BillingCycle.YEARLY)).toBe(0); + }); +}); + +describe('yearlyEquivalent', () => { + it('leaves annual prices untouched', () => { + expect(yearlyEquivalent(120, BillingCycle.YEARLY)).toBe(120); + expect(yearlyEquivalent(120, BillingCycle.ANNUAL)).toBe(120); + }); + + it('multiplies monthly prices by twelve', () => { + expect(yearlyEquivalent(10, BillingCycle.MONTHLY)).toBe(120); + }); + + it('multiplies weekly prices by fifty-two', () => { + expect(yearlyEquivalent(10, BillingCycle.WEEKLY)).toBe(520); + }); +}); + +describe('isAnnualCycle', () => { + it('recognises both annual spellings', () => { + expect(isAnnualCycle(BillingCycle.YEARLY)).toBe(true); + expect(isAnnualCycle(BillingCycle.ANNUAL)).toBe(true); + expect(isAnnualCycle('annual')).toBe(true); + }); + + it('rejects non-annual cycles', () => { + expect(isAnnualCycle(BillingCycle.MONTHLY)).toBe(false); + expect(isAnnualCycle(BillingCycle.WEEKLY)).toBe(false); + expect(isAnnualCycle(undefined)).toBe(false); + }); +}); \ No newline at end of file diff --git a/src/utils/__tests__/decimalAmount.test.ts b/src/utils/__tests__/decimalAmount.test.ts new file mode 100644 index 00000000..b640c29c --- /dev/null +++ b/src/utils/__tests__/decimalAmount.test.ts @@ -0,0 +1,110 @@ +/** + * Tests for decimalAmount — exact fixed-point comparison for crypto amounts. + * + * @see src/utils/decimalAmount.ts + * @see https://github.com/Smartdevs17/SubTrackr/issues/1683 + */ + +import { + compareDecimalAmounts, + exceedsLimit, + normalizeDecimal, +} from '../decimalAmount'; + +describe('normalizeDecimal', () => { + it('splits a decimal string into sign, digits and scale', () => { + expect(normalizeDecimal('12.5')).toEqual({ negative: false, digits: '125', scale: 1 }); + expect(normalizeDecimal('120')).toEqual({ negative: false, digits: '120', scale: 0 }); + }); + + it('strips leading zeros without changing the value', () => { + expect(normalizeDecimal('00.5')).toEqual({ negative: false, digits: '5', scale: 1 }); + expect(normalizeDecimal('0007')).toEqual({ negative: false, digits: '7', scale: 0 }); + }); + + it('accepts a leading decimal point', () => { + expect(normalizeDecimal('.5')).toEqual({ negative: false, digits: '5', scale: 1 }); + }); + + it('normalises negative zero to zero', () => { + expect(normalizeDecimal('-0')?.negative).toBe(false); + expect(normalizeDecimal('-0.00')?.negative).toBe(false); + }); + + it('preserves the sign for non-zero values', () => { + expect(normalizeDecimal('-5')?.negative).toBe(true); + expect(normalizeDecimal('-0.01')?.negative).toBe(true); + }); + + it('accepts finite numbers', () => { + expect(normalizeDecimal(1.5)).toEqual({ negative: false, digits: '15', scale: 1 }); + }); + + it('rejects anything that is not a finite decimal', () => { + expect(normalizeDecimal('')).toBeNull(); + expect(normalizeDecimal(' ')).toBeNull(); + expect(normalizeDecimal('abc')).toBeNull(); + expect(normalizeDecimal('.')).toBeNull(); + expect(normalizeDecimal('-')).toBeNull(); + expect(normalizeDecimal('1e18')).toBeNull(); + expect(normalizeDecimal('1,000')).toBeNull(); + expect(normalizeDecimal(null)).toBeNull(); + expect(normalizeDecimal(undefined)).toBeNull(); + expect(normalizeDecimal(Number.NaN)).toBeNull(); + expect(normalizeDecimal(Number.POSITIVE_INFINITY)).toBeNull(); + }); +}); + +describe('compareDecimalAmounts', () => { + it('compares equal values written at different scales', () => { + expect(compareDecimalAmounts('0.1', '0.10')).toBe(0); + expect(compareDecimalAmounts('5', '5.000')).toBe(0); + expect(compareDecimalAmounts('1.0', '1')).toBe(0); + }); + + it('orders fractional amounts correctly', () => { + expect(compareDecimalAmounts('12.5', '10')).toBe(1); + expect(compareDecimalAmounts('10', '12.5')).toBe(-1); + expect(compareDecimalAmounts('1.005', '1.004')).toBe(1); + expect(compareDecimalAmounts('1.004', '1.005')).toBe(-1); + }); + + it('does not lose precision on values that float maths gets wrong', () => { + // 0.1 + 0.2 !== 0.3 in binary floating point. + expect(compareDecimalAmounts('0.3', '0.1')).toBe(1); + expect(compareDecimalAmounts('0.3', '0.2')).toBe(1); + expect(compareDecimalAmounts('0.30000000000000004', '0.3')).toBe(1); + }); + + it('handles negatives', () => { + expect(compareDecimalAmounts('-5', '3')).toBe(-1); + expect(compareDecimalAmounts('3', '-5')).toBe(1); + expect(compareDecimalAmounts('-5', '-3')).toBe(-1); + expect(compareDecimalAmounts('-0.5', '-0.5')).toBe(0); + }); + + it('orders values of differing digit length', () => { + expect(compareDecimalAmounts('100', '99.99')).toBe(1); + expect(compareDecimalAmounts('0.001', '0.0009')).toBe(1); + }); + + it('throws on non-decimal input rather than guessing', () => { + expect(() => compareDecimalAmounts('abc', '1')).toThrow(/non-decimal/); + expect(() => compareDecimalAmounts('1', undefined)).toThrow(/non-decimal/); + }); +}); + +describe('exceedsLimit', () => { + it('is true only when the amount is strictly greater than the limit', () => { + expect(exceedsLimit('12.5', '10')).toBe(true); + expect(exceedsLimit('10', '10')).toBe(false); + expect(exceedsLimit('9.99', '10')).toBe(false); + }); + + it('works on fractional limits, which BigNumber.from would reject', () => { + // `ethers.BigNumber.from('0.1')` throws; this path must not. + expect(() => exceedsLimit('0.2', '0.1')).not.toThrow(); + expect(exceedsLimit('0.2', '0.1')).toBe(true); + expect(exceedsLimit('0.1', '0.1')).toBe(false); + }); +}); \ No newline at end of file diff --git a/src/utils/__tests__/invoiceTotals.test.ts b/src/utils/__tests__/invoiceTotals.test.ts new file mode 100644 index 00000000..e7ff924f --- /dev/null +++ b/src/utils/__tests__/invoiceTotals.test.ts @@ -0,0 +1,140 @@ +/** + * Tests for invoiceTotals — the single invoice arithmetic implementation. + * + * @see src/utils/invoiceTotals.ts + * @see https://github.com/Smartdevs17/SubTrackr/issues/1684 + */ + +import { + calculateInvoiceTotals, + resolveLineTotal, + roundMoney, +} from '../invoiceTotals'; + +describe('roundMoney', () => { + it('rounds half away from zero at two decimal places', () => { + expect(roundMoney(1.005)).toBe(1.01); + expect(roundMoney(1.004)).toBe(1); + expect(roundMoney(2.675)).toBe(2.68); + }); + + it('rounds negative values symmetrically', () => { + expect(roundMoney(-1.005)).toBe(-1.01); + }); + + it('honours a custom precision', () => { + expect(roundMoney(1.2345, 3)).toBe(1.235); + expect(roundMoney(1.2345, 0)).toBe(1); + }); + + it('maps non-finite input to zero', () => { + expect(roundMoney(Number.NaN)).toBe(0); + expect(roundMoney(Number.POSITIVE_INFINITY)).toBe(0); + }); + + it('never returns negative zero', () => { + expect(Object.is(roundMoney(-0), 0)).toBe(true); + expect(Object.is(roundMoney(-0.001), 0)).toBe(true); + }); +}); + +describe('resolveLineTotal', () => { + it('prefers an explicit lineTotal', () => { + expect(resolveLineTotal({ lineTotal: 12, amount: 99, unitPrice: 5, quantity: 2 })).toBe(12); + }); + + it('falls back to the flat amount alias', () => { + expect(resolveLineTotal({ amount: 7.5, unitPrice: 5, quantity: 2 })).toBe(7.5); + }); + + it('derives unit price times quantity as a last resort', () => { + expect(resolveLineTotal({ unitPrice: 5, quantity: 3 })).toBe(15); + }); + + it('returns zero for an unusable line', () => { + expect(resolveLineTotal({})).toBe(0); + expect(resolveLineTotal({ lineTotal: Number.NaN })).toBe(0); + }); +}); + +describe('calculateInvoiceTotals', () => { + it('sums line items into a subtotal', () => { + const result = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 10 }, { lineTotal: 5 }], + }); + expect(result.subtotal).toBe(15); + expect(result.tax).toBe(0); + expect(result.discount).toBe(0); + expect(result.total).toBe(15); + }); + + it('uses the flat amount when there are no line items', () => { + expect(calculateInvoiceTotals({ amount: 42 }).total).toBe(42); + }); + + it('prefers line items over the flat amount', () => { + expect(calculateInvoiceTotals({ lineItems: [{ lineTotal: 10 }], amount: 999 }).total).toBe(10); + }); + + it('adds tax and subtracts discount', () => { + const result = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 100 }], + tax: 20, + discount: 15, + }); + expect(result.subtotal).toBe(100); + expect(result.tax).toBe(20); + expect(result.discount).toBe(15); + expect(result.total).toBe(105); + }); + + it('clamps a discount larger than the subtotal so the total never goes negative', () => { + const result = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 100 }], + discount: 150, + }); + expect(result.discount).toBe(100); + expect(result.total).toBe(0); + }); + + it('ignores a negative discount rather than inflating the bill', () => { + const result = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 100 }], + discount: -25, + }); + expect(result.discount).toBe(0); + expect(result.total).toBe(100); + }); + + it('rounds each component to minor units', () => { + const result = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 10.005 }, { lineTotal: 10.005 }], + }); + // Raw sum is 20.01, and rounding the subtotal (not each line) gives 20.01. + expect(result.subtotal).toBe(20.01); + expect(result.total).toBe(20.01); + }); + + it('treats an empty invoice as zero', () => { + expect(calculateInvoiceTotals({}).total).toBe(0); + }); + + it('produces identical totals whether created or recomputed on update', () => { + const created = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 50 }, { lineTotal: 30 }], + tax: 8, + discount: 10, + }); + + // Editing one line re-derives totals from line items + the stored tax and + // discount; the payable amount must not change. + const updated = calculateInvoiceTotals({ + lineItems: [{ lineTotal: 50 }, { lineTotal: 30 }], + tax: 8, + discount: 10, + }); + + expect(updated).toEqual(created); + expect(updated.total).toBe(78); + }); +}); \ No newline at end of file diff --git a/src/utils/billingCycleMath.ts b/src/utils/billingCycleMath.ts new file mode 100644 index 00000000..887d8866 --- /dev/null +++ b/src/utils/billingCycleMath.ts @@ -0,0 +1,98 @@ +/** + * billingCycleMath.ts + * + * One place that turns a subscription's price and billing cycle into a + * comparable per-month figure. + * + * This logic was previously duplicated in four places that disagreed with one + * another, which produced inconsistent spend and MRR figures depending on + * which screen the user looked at: + * + * - `subscriptionStore` compared raw strings and omitted `annual` entirely, + * so an annual plan was billed as a *monthly* one and inflated MRR 12×. + * - `analyticsService` and `analyticsSlice` handled `yearly` but not + * `annual`. + * - Those two used a hard-coded `4.345` weeks/month while + * `BILLING_CONVERSIONS.WEEKS_PER_MONTH` is `52 / 12`. + */ + +import { BillingCycle } from '../types/subscription'; +import { BILLING_CONVERSIONS } from './constants/values'; + +/** Canonical weeks-per-month used for every weekly normalisation. */ +export const WEEKS_PER_MONTH = BILLING_CONVERSIONS.WEEKS_PER_MONTH; + +/** + * Coerces loose input (`'monthly'`, `'Monthly'`, a missing value) into a + * `BillingCycle`, defaulting to MONTHLY. + */ +export function normalizeBillingCycle(value: unknown): BillingCycle { + if (typeof value !== 'string') return BillingCycle.MONTHLY; + + const candidate = value.trim().toLowerCase(); + if (candidate === '') return BillingCycle.MONTHLY; + + const known = Object.values(BillingCycle) as string[]; + if (known.includes(candidate)) { + return candidate as BillingCycle; + } + + // Tolerate common synonyms used by imports and older records. + switch (candidate) { + case 'annually': + case 'yearly': + case 'annual': + case 'year': + return BillingCycle.YEARLY; + case 'week': + case 'weekly': + return BillingCycle.WEEKLY; + case 'month': + case 'monthly': + return BillingCycle.MONTHLY; + default: + return BillingCycle.MONTHLY; + } +} + +/** + * Monthly equivalent of `price` charged on `cycle`. + * + * `yearly` and `annual` are both billed once per year, so both normalise to + * one twelfth of the quoted price. `weekly` uses the shared weeks-per-month + * constant so it cannot drift away from the rest of the codebase. + */ +export function monthlyEquivalent(price: number, cycle: unknown): number { + if (!Number.isFinite(price)) return 0; + + switch (normalizeBillingCycle(cycle)) { + case BillingCycle.YEARLY: + case BillingCycle.ANNUAL: + return price / BILLING_CONVERSIONS.MONTHS_PER_YEAR; + case BillingCycle.WEEKLY: + return price * WEEKS_PER_MONTH; + default: + return price; + } +} + +/** Yearly equivalent of `price` charged on `cycle`. */ +export function yearlyEquivalent(price: number, cycle: unknown): number { + if (!Number.isFinite(price)) return 0; + + switch (normalizeBillingCycle(cycle)) { + case BillingCycle.YEARLY: + case BillingCycle.ANNUAL: + return price; + case BillingCycle.WEEKLY: + return price * BILLING_CONVERSIONS.WEEKS_PER_YEAR; + default: + return price * BILLING_CONVERSIONS.MONTHS_PER_YEAR; + } +} + +/** True for cycles billed once per year. */ +export function isAnnualCycle(cycle: unknown): boolean { + const normalized = normalizeBillingCycle(cycle); + return normalized === BillingCycle.YEARLY || normalized === BillingCycle.ANNUAL; +} \ No newline at end of file diff --git a/src/utils/decimalAmount.ts b/src/utils/decimalAmount.ts new file mode 100644 index 00000000..a06a3938 --- /dev/null +++ b/src/utils/decimalAmount.ts @@ -0,0 +1,105 @@ +/** + * decimalAmount.ts + * + * Exact comparison for fixed-point decimal amounts expressed as strings. + * + * Crypto amounts are user-entered decimals like `"12.5"`. `ethers.BigNumber` + * only accepts integer base units, so `BigNumber.from("12.5")` throws. That + * throw used to be swallowed by the surrounding `catch`, which meant *every* + * fractional amount failed the per-method spend cap and was reported as a + * generic payment error. + * + * Comparing via `parseFloat` instead would reintroduce float error, so the + * comparison below is done on normalised integer coefficient/exponent pairs. + */ + +/** Splits a decimal string into a sign, a digit string and a decimal scale. */ +interface NormalizedAmount { + negative: boolean; + /** Digits with the decimal point removed. */ + digits: string; + /** Number of implied decimal places in `digits`. */ + scale: number; +} + +const DECIMAL_PATTERN = /^[+-]?(\d+(\.\d*)?|\.\d+)$/; + +/** + * Parses a decimal string, or returns `null` when it is not a finite decimal. + * Rejects exponents (`1e18`), blanks, and anything non-numeric. + */ +export function normalizeDecimal( + value: string | number | null | undefined +): NormalizedAmount | null { + if (typeof value === 'number') { + if (!Number.isFinite(value)) return null; + value = value.toString(); + } + + if (typeof value !== 'string') return null; + + const trimmed = value.trim(); + if (trimmed === '' || !DECIMAL_PATTERN.test(trimmed)) return null; + + const negative = trimmed.startsWith('-'); + const unsigned = trimmed.replace(/^[+-]/, ''); + const dotIndex = unsigned.indexOf('.'); + + const whole = dotIndex === -1 ? unsigned : unsigned.slice(0, dotIndex); + const fraction = dotIndex === -1 ? '' : unsigned.slice(dotIndex + 1); + const rawDigits = `${whole}${fraction}`.replace(/^0+(?=\d)/, ''); + + if (rawDigits === '') return null; + + return { + negative: negative && !/^0+$/.test(rawDigits) ? true : false, + digits: rawDigits, + scale: fraction.length, + }; +} + +/** + * Returns -1, 0 or 1 as `a` is less than, equal to, or greater than `b`. + * + * @throws when either side is not a finite decimal string. + */ +export function compareDecimalAmounts( + a: string | number | null | undefined, + b: string | number | null | undefined +): number { + const left = normalizeDecimal(a); + const right = normalizeDecimal(b); + + if (!left || !right) { + throw new Error( + `Cannot compare non-decimal amounts: ${JSON.stringify(a)} vs ${JSON.stringify(b)}` + ); + } + + if (left.negative !== right.negative) { + return left.negative ? -1 : 1; + } + + // Align both sides to the same scale so a plain lexicographic compare works. + const scale = Math.max(left.scale, right.scale); + const leftAligned = left.digits.padEnd(left.digits.length + (scale - left.scale), '0'); + const rightAligned = right.digits.padEnd(right.digits.length + (scale - right.scale), '0'); + + const magnitude = compareDigits(leftAligned, rightAligned); + + return left.negative ? -magnitude : magnitude; +} + +function compareDigits(a: string, b: string): number { + if (a.length !== b.length) return a.length < b.length ? -1 : 1; + if (a === b) return 0; + return a < b ? -1 : 1; +} + +/** True when `amount` exceeds `limit`. Both must be finite decimals. */ +export function exceedsLimit( + amount: string | number | null | undefined, + limit: string | number | null | undefined +): boolean { + return compareDecimalAmounts(amount, limit) > 0; +} \ No newline at end of file diff --git a/src/utils/invoiceTotals.ts b/src/utils/invoiceTotals.ts new file mode 100644 index 00000000..2b3ca5f5 --- /dev/null +++ b/src/utils/invoiceTotals.ts @@ -0,0 +1,92 @@ +/** + * invoiceTotals.ts + * + * Single source of truth for turning line items into an invoice total. + * + * `createInvoice` and `updateInvoice` previously each rolled their own + * arithmetic, and they did not agree: `createInvoice` subtracted the + * discount, `updateInvoice` did not. Editing one line on a discounted + * invoice therefore silently increased the amount payable, and because the + * discount was never persisted on the record there was nothing left to + * subtract on the second pass. + */ + +/** Rounds to `decimals` places using half-up, avoiding binary float drift. */ +export function roundMoney(amount: number, decimals = 2): number { + if (!Number.isFinite(amount)) return 0; + + const factor = 10 ** decimals; + // The epsilon nudge keeps values such as 1.005 from rounding down because + // their binary representation is fractionally below the decimal value. + // The trailing `+ 0` normalises -0 to 0 so callers never see a negative zero. + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * factor) / factor + 0; +} + +export interface LineTotalSource { + lineTotal?: number; + amount?: number; + unitPrice?: number; + quantity?: number; +} + +/** + * Resolves a single line's total, tolerating both the itemised (`lineTotal`) + * and flat (`amount`) shapes. + */ +export function resolveLineTotal(item: LineTotalSource): number { + const derived = + item.quantity !== undefined ? (item.unitPrice ?? 0) * item.quantity : undefined; + const candidates = [item.lineTotal, item.amount, derived]; + + for (const candidate of candidates) { + if (typeof candidate === 'number' && Number.isFinite(candidate)) { + return candidate; + } + } + + return 0; +} + +export interface InvoiceTotalsInput { + lineItems?: LineTotalSource[]; + /** Used when the invoice has no line items. */ + amount?: number; + tax?: number; + discount?: number; + currencyDecimals?: number; +} + +export interface InvoiceTotals { + subtotal: number; + tax: number; + discount: number; + total: number; +} + +/** + * Computes invoice totals with every component rounded to minor units and a + * non-negative payable amount. + * + * The clamp matters: a discount larger than the subtotal otherwise produces a + * negative tax base and a negative total, which downstream code then treats as + * a credit owed to the customer rather than an over-discounted invoice. + */ +export function calculateInvoiceTotals(input: InvoiceTotalsInput): InvoiceTotals { + const decimals = input.currencyDecimals ?? 2; + + const hasLineItems = (input.lineItems?.length ?? 0) > 0; + const rawSubtotal = hasLineItems + ? (input.lineItems ?? []).reduce((sum, item) => sum + resolveLineTotal(item), 0) + : (input.amount ?? 0); + + const subtotal = roundMoney(rawSubtotal, decimals); + const tax = roundMoney(input.tax ?? 0, decimals); + + // A discount may not exceed what is being discounted. + const requestedDiscount = roundMoney(input.discount ?? 0, decimals); + const discount = roundMoney(Math.min(Math.max(requestedDiscount, 0), subtotal), decimals); + + const total = roundMoney(Math.max(subtotal + tax - discount, 0), decimals); + + return { subtotal, tax, discount, total }; +} \ No newline at end of file