From 499232db3c576a989a8402b7bceca5c56a114293 Mon Sep 17 00:00:00 2001 From: chitrank2050 Date: Wed, 27 May 2026 18:53:28 +0530 Subject: [PATCH 1/2] feat: add phase 5.5 documentation detailing security hardening and audit remediations --- docs/phases/phase-5.5.md | 64 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 docs/phases/phase-5.5.md diff --git a/docs/phases/phase-5.5.md b/docs/phases/phase-5.5.md new file mode 100644 index 0000000..46a78c5 --- /dev/null +++ b/docs/phases/phase-5.5.md @@ -0,0 +1,64 @@ +# Phase 5.5 - Security Hardening and Audit Fixes + +**Goal:** Fix all security vulnerabilities and technical debt identified in the security audit. This is not a feature phase - it's a quality gate. + +**Status:** ✅ Complete + +--- + +## TLDR + +11 issues resolved across authentication, authorization, billing, rate limiting, and code quality. Every finding from the Security Audit v2 (2026-05-20) has been addressed. + +## What was fixed + +### Critical (pre-production blockers) + +**#140 - Tenant :id param vs x-tenant-id header mismatch.** An OWNER of Tenant A could modify Tenant B's data by setting `x-tenant-id: TenantA` but putting Tenant B's UUID in the URL. Fixed with controller-level comparison of `@Param('id')` against `@TenantId()`, returning 403 on mismatch. Applied to `findById`, `update`, and `remove` in `TenantsController`. + +**#141 - User update endpoint allows any user to modify any profile.** Any JWT holder could PATCH any user's `firstName`, `lastName`, or `isActive`. Fixed with authorization logic: self-update allowed (except `isActive`), admin-update requires `x-tenant-id` + OWNER/ADMIN role + target must be a member of that tenant, `isActive` changes require OWNER. + +**#142 - No PlatformAdminGuard for catalog mutations.** Any registered user could create plans, modify features, and change pricing. Added `isPlatformAdmin` boolean to User model, created `PlatformAdminGuard`, applied to all mutating endpoints on plans, features, entitlements, and plan-prices controllers. Read-only endpoints remain public/JWT-only. + +**#143 - No rate limiting anywhere.** Auth brute force, usage flooding, and billing DoS were trivial. Implemented Redis-based rate limiting with `RateLimitGuard` (global, 200/min per user default) and `@RateLimit()` decorator with `RATE_LIMITS` presets for tiered limits. Auth: 10/min per IP. Usage: 100/min per tenant. Invoice generation: 5/min per tenant. Standard 429 with `Retry-After` and `X-RateLimit-*` headers. Fail-open on Redis errors. + +### High priority + +**#144 - Refresh token findUniqueOrThrow fails open on purged tokens.** Purged tokens caused a Prisma `NotFoundError` that bubbled up as 500 instead of 401. Replaced with `findUnique` + explicit null check throwing `UnauthorizedException`. + +### Medium priority + +**#145 - Cancel endpoint doesn't set status to CANCELLED.** `cancel()` set `cancelledAt` but left `status: ACTIVE`. Cancelled subscriptions continued passing entitlement checks and generating invoices. Added `status: SubscriptionStatus.CANCELLED` to the update. + +**#146 - Consume endpoint doesn't persist usage events.** Redis counter was ephemeral - flushed Redis means lost usage, never billed. Added transaction writing `usage_event` + `outbox_event` after Redis increment. Events flow through the existing pipeline for billing durability. + +**#147 - Audit log shallow sanitization + no size limit.** `sanitize()` only stripped top-level keys. Nested `{ user: { password: "secret" } }` persisted in plaintext. Replaced with recursive sanitization via `WeakSet` cycle detection, `SENSITIVE_KEYS` set with lowercase comparison, `toJSON()` delegation for Date/Decimal/custom classes, BigInt/Symbol/Function handling, and 10KB payload size limit with `_truncated` flag. + +**#148 - Billing ledger endpoints missing RolesGuard.** Any tenant member including DEVELOPER could view financial data. Added `RolesGuard` with `@Roles(OWNER, ADMIN, BILLING)` to all four billing endpoints. + +### Low priority + +**#149 - Invoices controller has direct Prisma queries.** Created `InvoicesService` with `findAll`, `findById`, `findLineItems`, `generateForTenant`. Controller now delegates everything. Also fixed `generate()` returning inline `{ statusCode: 404 }` instead of throwing `NotFoundException`. + +**#150 - Misc: process.env.PORT, pagination, duplicate comment.** Replaced `process.env.PORT` with `ConfigService.get('PORT')`. Added pagination to subscriptions and API keys list endpoints. Removed duplicate comment in `usage-aggregation.consumer.ts`. + +## Key decisions + +| Decision | Why | +| ----------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | +| Controller-level tenant ID check, not middleware | Only affects routes with both `:id` param and `x-tenant-id`. Middleware would run on every request. | +| `@Headers('x-tenant-id')` on user update, not `@TenantId()` | The decorator assumes header is required (paired with TenantGuard). User self-update doesn't need tenant context. | +| `isPlatformAdmin` in DB, not env var | Database-driven is more flexible. Can grant/revoke without restart. | +| Fixed-window rate limiting, not sliding window | Simpler, sufficient for current scale. Sliding window can be swapped in later - same decorator interface. | +| Fail-open rate limiting on Redis errors | Availability over security for non-auth endpoints. Auth endpoints should arguably fail-closed - future enhancement. | +| Recursive sanitize with `toJSON()` delegation | Handles Date, Decimal, any custom class automatically. Future-proof. | +| 10KB audit payload limit | Prevents OOM on massive request bodies. `_truncated` flag preserves auditability. | + +## Schema changes + +- Added `is_platform_admin` boolean to `users` table (default false) +- Migration: `add-is-platform-admin` + +## Seed changes + +- `alice@meterplex.dev` marked as `isPlatformAdmin: true` From 4a9a32405391a437451fd9275247d92c83a56ed4 Mon Sep 17 00:00:00 2001 From: chitrank2050 Date: Wed, 27 May 2026 18:56:06 +0530 Subject: [PATCH 2/2] docs(phase-5.5): security audit resolution log and phase documentation - Phase log: docs/phases/phase-5.5.md with all 11 fixes documented - Audit tracker: docs/audits/security-audit-v2-resolution.md with resolution table - Verification checklist confirming all findings addressed - Security posture summary with known limitations - mkdocs.yml: added Audits section and Phase 5.5 to roadmap Closes #151 --- .../2026-05-20-security-audit-resolution.md | 75 +++++++++++++++++++ mkdocs.yml | 3 + 2 files changed, 78 insertions(+) create mode 100644 docs/audits/2026-05-20-security-audit-resolution.md diff --git a/docs/audits/2026-05-20-security-audit-resolution.md b/docs/audits/2026-05-20-security-audit-resolution.md new file mode 100644 index 0000000..5d1dd7e --- /dev/null +++ b/docs/audits/2026-05-20-security-audit-resolution.md @@ -0,0 +1,75 @@ +# Security Audit - Resolution Tracker + +**Audit date:** 2026-05-20 +**Resolution completed:** 2026-05-23 +**Auditor:** Internal review +**Phase:** 5.5 + +--- + +## Summary + +| Severity | Total | Resolved | Remaining | +| --------- | ------ | -------- | --------- | +| Critical | 4 | 4 | 0 | +| High | 1 | 1 | 0 | +| Medium | 4 | 4 | 0 | +| Low | 2 | 2 | 0 | +| **Total** | **11** | **11** | **0** | + +--- + +## Resolution details + +| # | Finding | Severity | Issue | Status | +| --- | ---------------------------------------------------------------- | -------- | ----- | -------- | +| 1 | Tenant :id param vs x-tenant-id mismatch - cross-tenant mutation | Critical | #140 | ✅ Fixed | +| 2 | User update endpoint allows any user to modify any profile | Critical | #141 | ✅ Fixed | +| 3 | No PlatformAdminGuard - any user can mutate billing catalog | Critical | #142 | ✅ Fixed | +| 4 | No rate limiting - auth brute force and DoS trivial | Critical | #143 | ✅ Fixed | +| 5 | Refresh token findUniqueOrThrow fails open on purged tokens | High | #144 | ✅ Fixed | +| 6 | Cancel endpoint doesn't update status to CANCELLED | Medium | #145 | ✅ Fixed | +| 7 | Consume endpoint doesn't persist usage events | Medium | #146 | ✅ Fixed | +| 8 | Audit log shallow sanitization + no size limit | Medium | #147 | ✅ Fixed | +| 9 | Billing ledger endpoints missing RolesGuard | Medium | #148 | ✅ Fixed | +| 10 | Invoices controller direct Prisma queries | Low | #149 | ✅ Fixed | +| 11 | process.env.PORT, missing pagination, duplicate comment | Low | #150 | ✅ Fixed | + +--- + +## Verification checklist + +- [x] Cross-tenant mutation: PATCH tenant with mismatched header/param returns 403 +- [x] User update: DEVELOPER cannot update other users, self-update works, isActive requires OWNER +- [x] Platform admin: non-admin gets 403 on plan/feature/entitlement/price mutations +- [x] Rate limiting: 11th login attempt returns 429 with Retry-After header +- [x] Refresh token: purged token returns 401, not 500 +- [x] Subscription cancel: status changes to CANCELLED, not just cancelledAt +- [x] Consume persistence: usage_event row created with source=consume-endpoint +- [x] Audit sanitization: nested passwords redacted, Date fields serialize as ISO strings +- [x] Billing roles: DEVELOPER gets 403 on ledger/balance/payments endpoints +- [x] Invoice service: controller has no PrismaService import, generate throws NotFoundException +- [x] Misc: app uses ConfigService for PORT, subscriptions/api-keys have pagination + +--- + +## Security posture after Phase 5.5 + +### What's protected + +- **Tenant isolation:** URL param validated against header on tenant mutations +- **User authorization:** self-update vs admin-update with role checks +- **Catalog integrity:** only platform admins can mutate plans/features/entitlements/prices +- **Brute force prevention:** IP-based rate limiting on auth endpoints +- **Financial data access:** OWNER/ADMIN/BILLING roles required for billing endpoints +- **Audit integrity:** recursive sanitization, size limits, no sensitive data in logs +- **Billing correctness:** cancelled subscriptions stop generating invoices, consumed usage is durable + +### Known limitations (future phases) + +- Rate limiting is fixed-window, not sliding window (2x burst at boundaries) +- Auth rate limit fails open on Redis errors (should fail closed) +- No IP blocklist or ban after repeated failures +- No CSRF protection (API-only, no browser forms) +- No request body size limit at application level (rely on nginx/reverse proxy) +- Webhook signature validation depends on provider adapter correctness diff --git a/mkdocs.yml b/mkdocs.yml index 1297293..47f4e1d 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -92,6 +92,8 @@ nav: - Plans: api/plans.md - API Keys: api/api-keys.md - Billing: api/billing.md + - Audits: + - '2026-05-20 Security Audit': audits/2026-05-20-security-audit-resolution.md - Roadmap: - Build Phases: - 'Phase 0: Project Foundations': phases/phase-0.md @@ -100,6 +102,7 @@ nav: - 'Phase 3: Usage Pipeline': phases/phase-3.md - 'Phase 4: Billing & Invoices': phases/phase-4.md - 'Phase 5: Payments & Webhooks': phases/phase-5.md + - 'Phase 5.5: Security Hardening': phases/phase-5.5.md - Maintenance: - CI/CD & Automation: maintenance/ci-cd.md - Community: