feat(security): JWT secret hardening + dynamic CORS + sliding-window rate limiting#24
Conversation
… limiting JWT secret management: - Fail fast on startup when JWT_SECRET is missing, still a placeholder (dev-secret/change-me-in-production), or shorter than 32 chars in production - Support zero-downtime rotation via JWT_SECRET_PREVIOUS: tokens signed with the current OR previous secret verify; previous-secret tokens are flagged and can be re-issued with the current secret via POST /auth/refresh CORS + security headers: - Read allowed origins from CORS_ALLOWED_ORIGINS (comma-separated); default http://localhost:3000 in dev, required in production (crash if unset) - Dynamic CORS echoes the matching Origin (never *), sets the documented methods/headers/max-age, and short-circuits preflight OPTIONS with 204 - Public route GET /v1/checkout/sessions/:id accepts any origin - Add Strict-Transport-Security, X-Content-Type-Options, X-Frame-Options Sliding-window rate limiting (Redis sorted sets, atomic Lua): - Per-endpoint limits keyed by IP or API key (auth 5/min, register 3/min, checkout create 100/min per key, checkout get 60/min, default 60/min) - X-RateLimit-Limit/Remaining/Reset headers; 429 + Retry-After on breach - Graceful degradation to in-memory limiting at 50% when Redis is down - Exempt /health and /metrics Tests: - Unit tests for JWT secret validation, rotation, CORS, rate-limit rules/service (ioredis-mock) and Redis-failure fallback - Integration test (supertest) asserting 429 + headers under limit breach - Optional artillery load-test config (no new deps; install via npx) Closes OrbitStream#5
oomokaro1
left a comment
There was a problem hiding this comment.
Thanks for the thorough implementation — the JWT hardening and rate limiting are solid and match the issue spec exactly.
However, there's a critical conflict that needs to be resolved before merging:
CORS changes conflict with already-merged PR #22
PR #22 (merged to main) already implemented a comprehensive dynamic CORS system:
src/middleware/dynamic-cors.middleware.ts— Route-grouped CORS with per-merchant origin allowlisting (DB-backed + Redis-cached)src/middleware/cors-origins-cache.service.ts— Redis cache with 5-min TTL + per-merchant invalidationsrc/middleware/security-headers.middleware.ts— HSTS, nosniff, DENY framing, CSP, etc.src/app.module.ts— Both middlewares registered globally viaNestModule.configure()src/db/schema.ts—cors_originsJSONB column on merchants tableGET/PUT /merchants/me/corsendpoints for merchant origin management
This PR's CORS changes (src/api/middleware/security.middleware.ts, src/api/middleware/cors.config.ts) introduce a parallel, static CORS system that:
- Conflicts in
app.module.ts— PR #22'sDynamicCorsMiddleware+SecurityHeadersMiddlewareare still registered; your PR doesn't remove them, so both layers run - Conflicts in
main.ts— Yourapp.use(createSecurityMiddleware())runs alongside PR #22's middleware, creating unpredictable behavior - Downgrades functionality — PR #22 supports per-merchant dynamic origins (stored in DB, cached in Redis); your static
CORS_ALLOWED_ORIGINSenv var approach replaces this with a flat list
Requested change
Remove the CORS-related files and keep only JWT + rate limiting:
- Remove
src/api/middleware/cors.config.tsandsrc/api/middleware/cors.config.spec.ts - Remove
src/api/middleware/security.middleware.tsandsrc/api/middleware/security.middleware.spec.ts - Remove the CORS changes from
src/main.ts(thecreateSecurityMiddleware()/resolveAllowedOrigins()calls) - Remove
CORS_ALLOWED_ORIGINSfrom.env.example(already handled by PR #22's per-merchant model) - The
X-Frame-Options/X-Content-Type-Options/Strict-Transport-Securityheaders are already covered by PR #22'sSecurityHeadersMiddleware
The JWT hardening (src/config/jwt-secret.config.ts, src/auth/jwt.strategy.ts, /auth/refresh) and rate limiting (src/api/middleware/rate-limit.*) are clean, well-tested, and have no conflicts. Those can merge as-is once the CORS overlap is removed.
Happy to help resolve the app.module.ts merge conflict as well once the CORS changes are stripped out.
…eam#22) Addresses the change request on OrbitStream#24: PR OrbitStream#22 landed dynamic per-merchant CORS + security headers on main, so this branch's parallel static CORS layer is removed. JWT hardening and sliding-window rate limiting are unchanged. - Remove src/api/middleware/cors.config.ts + security.middleware.ts (+ specs) - main.ts: drop createSecurityMiddleware()/resolveAllowedOrigins(); keep resolveJwtSecrets() and the trust-proxy setting (needed for rate limiting) - Remove CORS_ALLOWED_ORIGINS from .env.example - Keep PR OrbitStream#22's DynamicCorsMiddleware + SecurityHeadersMiddleware in app.module.ts alongside RateLimitModule - Scope the rate-limit integration test to rate limiting only
|
Thanks for the detailed review, @oomokaro1 — you're right, PR #22's dynamic per-merchant CORS landed after this branch was opened. I've rebased onto current Changes made:
|
Summary
Implements the three security hardening areas from #5: JWT secret management with rotation, dynamic CORS with security headers, and Redis-backed sliding-window rate limiting with graceful degradation. New code matches the existing Nest module/service conventions and lives under
src/config/andsrc/api/middleware/.Changes
1. JWT secret management (
src/config/jwt-secret.config.ts,src/auth/*)JWT_SECRETis missing, still a placeholder (dev-secret/change-me-in-production), or shorter than 32 characters. Length is enforced in every environment.JWT_SECRET_PREVIOUS: tokens signed with the current or previous secret verify. Previous-secret tokens are flagged (viaPreviousSecret) and can be exchanged for a current-secret token via the newPOST /auth/refresh(JWT-guarded). All logins already sign with the current secret.JwtModulenow resolves the validated secret viaregisterAsync; the Passport strategy uses asecretOrKeyProviderto pick the matching key.2. CORS + security headers (
src/api/middleware/security.middleware.ts,cors.config.ts)CORS_ALLOWED_ORIGINS(comma-separated). Defaults tohttp://localhost:3000in dev; required in production (startup crash if unset).Origin(never*), setsAccess-Control-Allow-Methods: GET, POST, PATCH, DELETE, OPTIONS,Access-Control-Allow-Headers: Content-Type, Authorization, X-Request-Id,Access-Control-Max-Age: 86400, and short-circuits preflightOPTIONSwith204.GET /v1/checkout/sessions/:idis public and accepts any origin.Strict-Transport-Security,X-Content-Type-Options: nosniff,X-Frame-Options: DENYto every response.app.enableCors()inmain.ts.3. Sliding-window rate limiting (
src/api/middleware/rate-limit.*)/auth/login|/auth/verify5/min per IP;/merchants/register3/min per IP;POST /v1/checkout/sessions100/min per API key;GET /v1/checkout/sessions/:id60/min per IP; all others 60/min per IP.X-RateLimit-Limit/-Remaining/-Resetheaders; on breach429withRetry-After./healthand/metricsare exempt. Applied globally viaRateLimitModule.Env
.env.exampleupdated with explanatory comments forJWT_SECRET,JWT_SECRET_PREVIOUS, andCORS_ALLOWED_ORIGINS.Test plan
npm run build(tsc) passes429+Retry-Afterreturned once the limit is exceeded;/healthexempt; CORS/security headers present; preflight204load-tests/rate-limit.artillery.yml, left as a follow-up (see notes)Notes / out of scope
load-tests/rate-limit.artillery.yml) runnable vianpx artillery— no new dependency added per the issue guidance. Full load verification is left as a follow-up.webhook-controller,webhook-module,app.smoke) fail becausesrc/db/index.tsthrowsDATABASE_URL is requiredat import time when no DB env is set. This is unrelated to this PR and predates it; the new code does not import the DB layer.Closes #5