Skip to content

feat: subscription billing correctness (#1684) - #1882

Merged
github-actions[bot] merged 1 commit into
Smartdevs17:mainfrom
akansnoro:feat/1684-subscription-billing-correctness
Oct 22, 2026
Merged

github-actions[bot] merged 1 commit into
Smartdevs17:mainfrom
akansnoro:feat/1684-subscription-billing-correctness

Conversation

@akansnoro

Copy link
Copy Markdown
Contributor

Context

This is the second of four pull requests that re-land the contents of the squash-merged PR #1703 one issue at a time.

#1703 was merged as a single commit (5afe754) covering four unrelated issues. The companion pull request (the revert) undoes that squash so each issue can be merged on its own. This pull request restores only the part of #1703 that belongs to:

  • #1684 — subscription billing correctness

What this changes

One place that computes invoice totals (src/utils/invoiceTotals.ts)

createInvoice and updateInvoice each rolled their own arithmetic and the two 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.

calculateInvoiceTotals() is now the single source of truth:

  • every component is rounded to minor units (half-up, no binary float drift);
  • discount line items (kind: 'discount') are kept out of the subtotal and summed into discount instead, so an itemised discount is subtracted exactly once;
  • the discount is clamped to the subtotal. Without the 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.

Invoice service (src/services/invoiceService.ts)

Both the create and the update path go through the shared totals function, and the discount is persisted on the record so a second pass subtracts the same amount.

Billing cycle maths (src/utils/billingCycleMath.ts, new)

monthlyEquivalent, yearlyEquivalent, isAnnualCycle and normalizeBillingCycle replace the ad-hoc cycle conversions that were scattered across the store and the analytics service, which disagreed about what a weekly or yearly cycle was worth.

Revenue recognition (backend/billing/jobs/monthlyRevenueRecognitionJob.ts, backend/billing/jobs/index.ts)

The job now tracks cumulative per-entry recognition progress (recordEntryProgress, recognisedToDate). Previously each run re-recognised the whole elapsed portion of an entry, so recognised revenue was inflated on every run and the deferred balance grew without bound.

Metering and group billing (backend/services/billing/meteringService.ts, groupBilling.ts, interfaces.ts)

  • Metered billing aggregates over the requested window instead of the last N days from "now", so a period that ended long ago no longer picks up today's usage.
  • aggregateCharges(group, from, to) takes an explicit window.
  • Amounts are rounded to minor units on the way out.

Analytics (src/services/analyticsService.ts, src/store/slices/analyticsSlice.ts, src/store/subscriptionStore.ts, backend/services/analytics/subscriptionAnalyticsService.ts)

Monthly and yearly equivalents now come from the shared billing-cycle helpers, so the dashboard, the store and the backend report the same numbers.

Batch charges (backend/services/batchChargeService.ts)

Uses the shared rounding helper.

Test (src/services/__tests__/groupBilling.test.ts)

Updated for the explicit-window aggregateCharges signature.

Merge order

This is the third of five pull requests in the split of #1703. It is based on main after the revert pull request (#1880) and the #1683 pull request (#1881) were merged, so its diff contains the #1684 work and nothing else.

The remaining two pull requests (#1686, #1685) are opened on top of this one, in that order, and must be merged after it.

The issues referenced above (#1683–#1686) have been deleted from the tracker, so they are referenced as plain text rather than with closing keywords.

Re-lands the part of the reverted Smartdevs17#1703 squash that belongs to issue
1684 only. The remainder of the squash is re-landed by the other
branches of the split, so this diff contains this issue's work exclusively.
@github-actions
github-actions Bot merged commit babb5b5 into Smartdevs17:main Oct 22, 2026
10 of 26 checks passed
github-actions Bot pushed a commit that referenced this pull request Oct 22, 2026
## Context

This is the third of four pull requests that re-land the contents of the squash-merged PR #1703 one issue at a time.

#1703 was merged as a single commit (`5afe754`) covering four unrelated issues. The companion pull request (the revert) undoes that squash so each issue can be merged on its own. This pull request restores **only** the part of #1703 that belongs to:

- #1686 — platform security

## What this changes

### Rate-limit tier cannot be spoofed (`backend/server.ts`, `backend/services/shared/rateLimitingService.ts`)

The caller's tier was read from 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 (`rateLimitingService.getTierForApiKey`). The header is honoured only when the explicit `TRUST_TIER_HEADER=true` opt-in is configured, which suits deployments where an authenticating gateway sits in front of this process and strips the header from inbound requests.

### Outbound URL guard (`backend/shared/security/outboundUrlGuard.ts`, new)

`validateOutboundUrl()` rejects URLs that resolve to private, loopback, link-local or otherwise non-routable addresses, including DNS names that resolve to them. `isPrivateHostname()` and `assertSafeIdentifier()` are the building blocks.

Used by:

- `backend/integrations/zapier/ZapierIntegrationService.ts` and `zapierRouter.ts` — the Zapier integration fetches a user-supplied URL, which was a server-side request forgery vector.
- `backend/services/webhook/eventReplayWorker.ts` — the replay worker re-delivers to a stored webhook target.

### Cursor pagination (`backend/services/shared/pagination.ts`, new)

`encodeCursor` / `decodeCursor` sign the cursor payload with a secret, so a client cannot craft one; `buildCursorClause` and `buildPage` provide keyset pagination, and `parseFieldSelection` / `selectFields` let a caller restrict the returned columns.

### API key rotation (`backend/services/auth/domain/ApiKeyRotationService.ts`)

Key rotation records the previous key's state so it can be revoked and audited.

### Webhook body limits and raw payload handling (`backend/server/createApiServer.ts`)

The two provider webhook registrations now cap the raw body size (`limit: MAX_JSON_BODY`), so a webhook POST cannot be used to exhaust memory. The registrations remain body-parser-only — no handler, no state change — and the HMAC verification still happens inside the Stripe and Paddle routers, which fail closed with 503 when unconfigured and 401 on a bad signature.

### Revalidation allow-list (`developer-portal/pages/api/revalidate.ts`)

`path` is caller-supplied and goes straight into `res.revalidate()`, so an arbitrary value let a holder of the secret force revalidation of unrelated routes. Only the documented set of paths is accepted now, and the secret is compared in constant time.

### Gateway (`backend/gateway/index.ts`) and configuration (`.env.example`)

The new toggles (`TRUST_TIER_HEADER`, the pagination cursor secret, the webhook body limit) are documented in `.env.example`.

## Merge order

This is the fourth of five pull requests in the split of #1703. It is based on `main` **after** the revert pull request ([#1880](#1880)) and the #1683 ([#1881](#1881)) and #1684 ([#1882](#1882)) pull requests were merged, so its diff contains the #1686 work and nothing else.

The last pull request (#1685) is opened on top of this one and must be merged after it.

The issues referenced above (#1683–#1686) have been deleted from the tracker, so they are referenced as plain text rather than with closing keywords.
github-actions Bot pushed a commit that referenced this pull request Oct 22, 2026
## Context

This is the last of four pull requests that re-land the contents of the squash-merged PR #1703 one issue at a time.

#1703 was merged as a single commit (`5afe754`) covering four unrelated issues. The companion pull request (the revert) undoes that squash so each issue can be merged on its own. This pull request restores **only** the part of #1703 that belongs to:

- #1685 — automated test coverage

## What this changes

Seven new test suites that cover the behaviour introduced by the other three issues:

| Suite | Covers |
| --- | --- |
| `src/utils/__tests__/decimalAmount.test.ts` | decimal-safe normalisation and comparison, including values exactly at a limit |
| `src/utils/__tests__/billingCycleMath.test.ts` | `monthlyEquivalent` / `yearlyEquivalent` for weekly, monthly and yearly cycles |
| `src/utils/__tests__/invoiceTotals.test.ts` | `calculateInvoiceTotals`: rounding, an itemised discount subtracted exactly once, and the discount clamp |
| `src/services/__tests__/transactionConfirmation.test.ts` | confirmation thresholds, timeouts, reverts and the definitive-failure classification |
| `src/services/__tests__/rpcProvider.test.ts` | circuit-breaker transitions, the half-open success threshold and endpoint failover |
| `src/store/_tests_/transactionQueueStore.test.ts` | the broadcast marker, exponential backoff and the refusal to re-broadcast an unknown-outcome transaction |
| `backend/shared/security/__tests__/outboundUrlGuard.test.ts` | rejection of private, loopback and link-local outbound targets |

## Merge order

This is the fifth and last pull request in the split of #1703. It is based on `main` **after** the revert pull request ([#1880](#1880)) and the #1683 ([#1881](#1881)), #1684 ([#1882](#1882)) and #1686 ([#1883](#1883)) pull requests were merged, so its diff contains the #1685 work and nothing else.

The suites exercise the modules added by the #1683, #1684 and #1686 pull requests, so this pull request has to come last.

Merging it completes the split. After all five pull requests are merged, `main` is identical to its state before the split.

The issues referenced above (#1683–#1686) have been deleted from the tracker, so they are referenced as plain text rather than with closing keywords.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant