feat: dynamic CORS + CSP + merchant origin allowlisting#22
Conversation
- Add cors_origins JSONB column to merchants table - Implement Redis-backed CorsOriginsCacheService with 5-min refresh - Add DynamicCorsMiddleware with route-grouped policies (public/merchant/dashboard) - Add SecurityHeadersMiddleware (HSTS, CSP, X-Frame-Options, etc.) - Add GET/PUT /merchants/me/cors endpoints (JWT auth) - Register both middlewares globally in AppModule - 24 passing unit tests covering all scenarios
PR Review: feat: dynamic CORS + CSP + merchant origin allowlistingCritical Issues1. The jest config is in 2. .set({ corsOrigins: origins } as any)This hides a type mismatch. The Drizzle schema likely expects 3. async invalidateMerchantCache(merchantId: string): Promise<void> {
await this.redis.getClient().del(`${CORS_CACHE_KEY}:${merchantId}`);
await this.refreshCache(); // Refreshes ALL merchants
}After invalidating a single merchant, you refresh the entire cache. This is wasteful. Just refresh that single merchant's key instead. Security Concerns4. Route classification uses hardcoded paths ( if (path.startsWith('/health') || path.startsWith('/metrics')) return 'public';
if (method === 'GET' && /^\/v1\/checkout\/sessions\/[^\/]+$/.test(path)) return 'public';
if (path.startsWith('/v1/checkout/')) return 'merchant_api';If routes change, this middleware can silently become misconfigured. Consider a more maintainable approach (e.g., decorators or route metadata). 5. Dashboard allows only 6. Code Quality7. const CSP_VALUE = process.env.CONTENT_SECURITY_POLICY ?? "default-src 'self'...";This means CSP can't be changed at runtime without restart. Acceptable for security headers, but document this. 8. a.endsWith('/*') && origin.startsWith(a.slice(0, -1))If someone passes 9. 10. Missing @IsArray()
@IsString({ each: true })
origins: string[];Consider adding URL validation to prevent invalid origins from being stored. Minor Nits
Suggestions for Improvement
VerdictRequest changes — The 3 critical issues (jest path, type casting, full cache refresh) should be fixed before merge. The security concerns are valid but don't block merge. |
9e3ee6f to
2bd3c15
Compare
|
Follow-up: The prettier/eslint fixes look good, but the 3 critical issues from my earlier review are still unresolved. Could you address these before merge? 1. .set({ corsOrigins: origins } as any)This hides a type mismatch. Fix the Drizzle typing instead of casting. 2. async invalidateMerchantCache(merchantId: string): Promise<void> {
await this.redis.getClient().del(`${CORS_CACHE_KEY}:${merchantId}`);
await this.refreshCache(); // Refreshes ALL merchants
}After invalidating a single merchant's key, this re-fetches every merchant from the DB. Just refresh that single merchant's cache key instead. 3. |
1. Keep Drizzle as any cast on set() (required by Drizzle v0.38 type inference for jsonb columns — consistent with all other update calls) 2. invalidateMerchantCache now refreshes single merchant key instead of re-fetching all merchants from DB 3. platformOrigin parsed once in DynamicCorsMiddleware constructor instead of on every request via new URL()
0c029d1 to
13977e7
Compare
oomokaro1
left a comment
There was a problem hiding this comment.
Thanks for the thorough implementation! The route-grouped CORS approach is clean and the Redis cache layer with per-merchant invalidation is well thought out. Great test coverage too.
LGTM — approved. 🎉
…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
Closes #15
Summary
Implements a comprehensive CORS and security headers system for OrbitStream. Replaces the permissive app.enableCors() with a route-aware DynamicCorsMiddleware that enforces origin allowlisting per merchant, plus a SecurityHeadersMiddleware that applies standard security headers to all responses.
Changes
Database
New Files
Modified Files
src/main.ts: Removed app.enableCors() — CORS is now handled entirely by the middleware
src/app.module.ts: Implements NestModule.configure() to register both middlewares globally
src/merchants/merchants.service.ts: Added getCorsOrigins(merchantId) and setCorsOrigins(merchantId, origins) methods with cache invalidation
src/merchants/merchants.controller.ts: Added two JWT-protected endpoints:
GET /merchants/me/cors — returns list of configured origins
PUT /merchants/me/cors — sets allowed origins, body: { "origins": ["https://..."] }
src/merchants/merchants.dto.ts: Added SetCorsOriginsDto with @isarray() + @IsString({ each: true }) validation
src/merchants/merchants.module.ts: Provides and exports CorsOriginsCacheService
jest.config.ts: Added setupFiles pointing to jest-setup.ts
jest-setup.ts: Sets test env vars (DATABASE_URL, REDIS_URL, JWT_SECRET, PLATFORM_DOMAIN)
Testing — 24 new tests (all passing)
Add cors_origins JSONB column to merchants table
Implement Redis-backed CorsOriginsCacheService with 5-min refresh
Add DynamicCorsMiddleware with route-grouped policies (public/merchant/dashboard)
Add SecurityHeadersMiddleware (HSTS, CSP, X-Frame-Options, etc.)
Add GET/PUT /merchants/me/cors endpoints (JWT auth)
Register both middlewares globally in AppModule
24 passing unit tests covering all scenarios