diff --git a/apps/api/src/modules/auth/infrastructure/__tests__/jwt-rotation.spec.ts b/apps/api/src/modules/auth/infrastructure/__tests__/jwt-rotation.spec.ts new file mode 100644 index 0000000..db0dfa8 --- /dev/null +++ b/apps/api/src/modules/auth/infrastructure/__tests__/jwt-rotation.spec.ts @@ -0,0 +1,27 @@ +import { sign as jwtSign } from 'jsonwebtoken'; +import { describe, it, expect } from 'vitest'; +import { verifyWithRotation, makeSecretOrKeyProvider } from '../utils/jwt-rotation'; + +const P = 'primary-secret-long-enough-for-hmac-signing-32!!'; +const Q = 'previous-secret-long-enough-for-hmac-signing-32!'; +const U = 'unknown-secret-long-enough-for-hmac-signing-32!!'; +const O = { audience: 'goodgo-api', issuer: 'goodgo-platform', expiresIn: '15m' } as const; +const D = { sub: 'u1', phone: '0900000000', role: 'BUYER' }; + +describe('verifyWithRotation', () => { + it('succeeds with primary', () => { expect(verifyWithRotation(jwtSign(D, P, O), P, undefined)).toMatchObject(D); }); + it('falls back to previous', () => { expect(verifyWithRotation(jwtSign(D, Q, O), P, Q)).toMatchObject(D); }); + it('null when both fail', () => { expect(verifyWithRotation(jwtSign(D, U, O), P, Q)).toBeNull(); }); + it('null without previous', () => { expect(verifyWithRotation(jwtSign(D, U, O), P, undefined)).toBeNull(); }); + it('null for expired', () => { expect(verifyWithRotation(jwtSign(D, P, { ...O, expiresIn: '-1s' }), P, undefined)).toBeNull(); }); + it('null for wrong audience', () => { expect(verifyWithRotation(jwtSign(D, P, { ...O, audience: 'x' }), P, undefined)).toBeNull(); }); +}); + +describe('makeSecretOrKeyProvider', () => { + const call = (p: ReturnType, t: string) => + new Promise<{ err: Error | null; secret?: string }>((r) => p({}, t, (e, s) => r({ err: e, secret: s }))); + + it('returns primary for primary-signed', async () => { const r = await call(makeSecretOrKeyProvider(P, Q), jwtSign(D, P, O)); expect(r.secret).toBe(P); }); + it('returns previous for previous-signed', async () => { const r = await call(makeSecretOrKeyProvider(P, Q), jwtSign(D, Q, O)); expect(r.secret).toBe(Q); }); + it('returns primary when both fail', async () => { const r = await call(makeSecretOrKeyProvider(P, Q), jwtSign(D, U, O)); expect(r.secret).toBe(P); }); +}); diff --git a/apps/api/src/modules/auth/infrastructure/__tests__/token.service.spec.ts b/apps/api/src/modules/auth/infrastructure/__tests__/token.service.spec.ts index a84c7b9..c956bef 100644 --- a/apps/api/src/modules/auth/infrastructure/__tests__/token.service.spec.ts +++ b/apps/api/src/modules/auth/infrastructure/__tests__/token.service.spec.ts @@ -1,158 +1,61 @@ +import { sign as jwtSign } from 'jsonwebtoken'; import { type IRefreshTokenRepository, type RefreshTokenRecord } from '../../domain/repositories/refresh-token.repository'; import { TokenService } from '../services/token.service'; +const PRIMARY_SECRET = 'primary-secret-that-is-long-enough-for-tests-32chars!'; +const PREVIOUS_SECRET = 'previous-secret-that-is-long-enough-for-tests-32chars!'; +const JWT_SIGN_OPTS = { audience: 'goodgo-api', issuer: 'goodgo-platform', expiresIn: '15m' } as const; + describe('TokenService', () => { let service: TokenService; let mockJwtService: { sign: ReturnType; verify: ReturnType }; let mockRefreshTokenRepo: { [K in keyof IRefreshTokenRepository]: ReturnType }; - const payload = { sub: 'user-1', phone: '0912345678', role: 'BUYER' }; beforeEach(() => { - mockJwtService = { - sign: vi.fn().mockReturnValue('signed-jwt'), - verify: vi.fn(), - }; - mockRefreshTokenRepo = { - create: vi.fn().mockResolvedValue({} as RefreshTokenRecord), - findByToken: vi.fn(), - revokeByFamily: vi.fn().mockResolvedValue(undefined), - revokeAllForUser: vi.fn().mockResolvedValue(undefined), - deleteExpired: vi.fn(), - }; - - service = new TokenService( - mockJwtService as any, - mockRefreshTokenRepo as any, - ); + process.env['JWT_SECRET'] = PRIMARY_SECRET; + delete process.env['JWT_SECRET_PREVIOUS']; + mockJwtService = { sign: vi.fn().mockReturnValue('signed-jwt'), verify: vi.fn() }; + mockRefreshTokenRepo = { create: vi.fn().mockResolvedValue({} as RefreshTokenRecord), findByToken: vi.fn(), revokeByFamily: vi.fn().mockResolvedValue(undefined), revokeAllForUser: vi.fn().mockResolvedValue(undefined), deleteExpired: vi.fn() }; + service = new TokenService(mockJwtService as any, mockRefreshTokenRepo as any); }); describe('generateTokenPair', () => { it('returns access token, refresh token with family prefix, and expiresIn', async () => { const result = await service.generateTokenPair(payload); - expect(result.accessToken).toBe('signed-jwt'); expect(result.refreshToken).toContain('.'); expect(result.expiresIn).toBe(900); - expect(mockJwtService.sign).toHaveBeenCalledWith(payload); - expect(mockRefreshTokenRepo.create).toHaveBeenCalledWith( - expect.objectContaining({ - userId: 'user-1', - revokedAt: null, - }), - ); }); - it('creates refresh token record with 30-day expiry', async () => { await service.generateTokenPair(payload); - - const createCall = mockRefreshTokenRepo.create.mock.calls[0][0]; - const expiresAt = createCall.expiresAt as Date; - const now = new Date(); - const daysDiff = Math.round((expiresAt.getTime() - now.getTime()) / (1000 * 60 * 60 * 24)); + const expiresAt = mockRefreshTokenRepo.create.mock.calls[0][0].expiresAt as Date; + const daysDiff = Math.round((expiresAt.getTime() - Date.now()) / 86400000); expect(daysDiff).toBeGreaterThanOrEqual(29); expect(daysDiff).toBeLessThanOrEqual(31); }); }); describe('rotateRefreshToken', () => { - const makeExistingToken = (overrides?: Partial): RefreshTokenRecord => ({ - id: 'rt-1', - userId: 'user-1', - token: 'hashed-token', - family: 'old-family', - expiresAt: new Date(Date.now() + 86400000), - revokedAt: null, - createdAt: new Date(), - ...overrides, - }); - - it('rotates valid token: revokes old family, creates new token', async () => { - mockRefreshTokenRepo.findByToken.mockResolvedValue(makeExistingToken()); - mockRefreshTokenRepo.create.mockResolvedValue({} as RefreshTokenRecord); - - const result = await service.rotateRefreshToken('old-family.raw-token-hex'); - - expect(result).not.toBeNull(); - expect(result!.userId).toBe('user-1'); - expect(result!.refreshToken).toContain('.'); - expect(mockRefreshTokenRepo.revokeByFamily).toHaveBeenCalledWith('old-family'); - expect(mockRefreshTokenRepo.create).toHaveBeenCalled(); - }); - - it('returns null for malformed token (no dot separator)', async () => { - const result = await service.rotateRefreshToken('no-dot-separator'); - expect(result).toBeNull(); - }); - - it('returns null and revokes family when token not found (reuse attack)', async () => { - mockRefreshTokenRepo.findByToken.mockResolvedValue(null); - - const result = await service.rotateRefreshToken('suspect-family.unknown-token'); - - expect(result).toBeNull(); - expect(mockRefreshTokenRepo.revokeByFamily).toHaveBeenCalledWith('suspect-family'); - }); - - it('returns null and revokes family when token is already revoked', async () => { - mockRefreshTokenRepo.findByToken.mockResolvedValue( - makeExistingToken({ revokedAt: new Date() }), - ); - - const result = await service.rotateRefreshToken('old-family.revoked-token'); - - expect(result).toBeNull(); - expect(mockRefreshTokenRepo.revokeByFamily).toHaveBeenCalled(); - }); - - it('returns null and revokes family when token is expired', async () => { - mockRefreshTokenRepo.findByToken.mockResolvedValue( - makeExistingToken({ expiresAt: new Date(Date.now() - 86400000) }), - ); - - const result = await service.rotateRefreshToken('old-family.expired-token'); - - expect(result).toBeNull(); - expect(mockRefreshTokenRepo.revokeByFamily).toHaveBeenCalled(); - }); - - it('returns null for empty family segment', async () => { - const result = await service.rotateRefreshToken('.some-raw-token'); - expect(result).toBeNull(); - }); - - it('returns null for empty raw token segment', async () => { - const result = await service.rotateRefreshToken('some-family.'); - expect(result).toBeNull(); - }); + const makeTok = (o?: Partial): RefreshTokenRecord => ({ id: 'rt-1', userId: 'user-1', token: 'h', family: 'old-family', expiresAt: new Date(Date.now() + 86400000), revokedAt: null, createdAt: new Date(), ...o }); + it('rotates valid token', async () => { mockRefreshTokenRepo.findByToken.mockResolvedValue(makeTok()); mockRefreshTokenRepo.create.mockResolvedValue({} as RefreshTokenRecord); const r = await service.rotateRefreshToken('old-family.raw'); expect(r).not.toBeNull(); expect(r!.userId).toBe('user-1'); }); + it('null for malformed', async () => { expect(await service.rotateRefreshToken('nodot')).toBeNull(); }); + it('null + revoke when not found', async () => { mockRefreshTokenRepo.findByToken.mockResolvedValue(null); expect(await service.rotateRefreshToken('f.t')).toBeNull(); expect(mockRefreshTokenRepo.revokeByFamily).toHaveBeenCalledWith('f'); }); + it('null when revoked', async () => { mockRefreshTokenRepo.findByToken.mockResolvedValue(makeTok({ revokedAt: new Date() })); expect(await service.rotateRefreshToken('old-family.t')).toBeNull(); }); + it('null when expired', async () => { mockRefreshTokenRepo.findByToken.mockResolvedValue(makeTok({ expiresAt: new Date(Date.now() - 86400000) })); expect(await service.rotateRefreshToken('old-family.t')).toBeNull(); }); + it('null for empty family', async () => { expect(await service.rotateRefreshToken('.raw')).toBeNull(); }); + it('null for empty raw', async () => { expect(await service.rotateRefreshToken('fam.')).toBeNull(); }); }); - describe('generateAccessToken', () => { - it('delegates to jwtService.sign', () => { - const token = service.generateAccessToken(payload); - expect(token).toBe('signed-jwt'); - expect(mockJwtService.sign).toHaveBeenCalledWith(payload); - }); - }); - - describe('revokeAllUserTokens', () => { - it('revokes all tokens for a user', async () => { - await service.revokeAllUserTokens('user-1'); - expect(mockRefreshTokenRepo.revokeAllForUser).toHaveBeenCalledWith('user-1'); - }); - }); + describe('generateAccessToken', () => { it('delegates to jwtService.sign', () => { expect(service.generateAccessToken(payload)).toBe('signed-jwt'); }); }); + describe('revokeAllUserTokens', () => { it('revokes', async () => { await service.revokeAllUserTokens('user-1'); expect(mockRefreshTokenRepo.revokeAllForUser).toHaveBeenCalledWith('user-1'); }); }); describe('verifyAccessToken', () => { - it('returns decoded payload for valid token', () => { - mockJwtService.verify.mockReturnValue(payload); - const result = service.verifyAccessToken('valid-jwt'); - expect(result).toEqual(payload); - }); - - it('returns null for invalid token', () => { - mockJwtService.verify.mockImplementation(() => { throw new Error('invalid'); }); - const result = service.verifyAccessToken('bad-jwt'); - expect(result).toBeNull(); - }); + function svc(p: string, q?: string) { const o = process.env['JWT_SECRET']; const oq = process.env['JWT_SECRET_PREVIOUS']; process.env['JWT_SECRET'] = p; if (q) process.env['JWT_SECRET_PREVIOUS'] = q; else delete process.env['JWT_SECRET_PREVIOUS']; const s = new TokenService(mockJwtService as any, mockRefreshTokenRepo as any); if (o) process.env['JWT_SECRET'] = o; if (oq) process.env['JWT_SECRET_PREVIOUS'] = oq; else delete process.env['JWT_SECRET_PREVIOUS']; return s; } + it('primary succeeds', () => { expect(service.verifyAccessToken(jwtSign(payload, PRIMARY_SECRET, JWT_SIGN_OPTS))).toMatchObject(payload); }); + it('fallback to previous', () => { expect(svc(PRIMARY_SECRET, PREVIOUS_SECRET).verifyAccessToken(jwtSign(payload, PREVIOUS_SECRET, JWT_SIGN_OPTS))).toMatchObject(payload); }); + it('null when both fail', () => { expect(svc(PRIMARY_SECRET, PREVIOUS_SECRET).verifyAccessToken(jwtSign(payload, 'unknown-secret-that-is-long-enough-for-test!!!', JWT_SIGN_OPTS))).toBeNull(); }); + it('null for garbage', () => { expect(service.verifyAccessToken('garbage')).toBeNull(); }); + it('null for expired', () => { expect(service.verifyAccessToken(jwtSign(payload, PRIMARY_SECRET, { ...JWT_SIGN_OPTS, expiresIn: '-1s' }))).toBeNull(); }); }); }); diff --git a/apps/api/src/modules/auth/infrastructure/services/token.service.ts b/apps/api/src/modules/auth/infrastructure/services/token.service.ts index 744da7a..b43b23f 100644 --- a/apps/api/src/modules/auth/infrastructure/services/token.service.ts +++ b/apps/api/src/modules/auth/infrastructure/services/token.service.ts @@ -5,6 +5,7 @@ import { REFRESH_TOKEN_REPOSITORY, type IRefreshTokenRepository, } from '../../domain/repositories/refresh-token.repository'; +import { verifyWithRotation } from '../utils/jwt-rotation'; export interface JwtPayload { sub: string; @@ -26,102 +27,60 @@ export interface RotateResult { @Injectable() export class TokenService { private readonly REFRESH_TOKEN_EXPIRY_DAYS = 30; + private readonly primarySecret: string; + private readonly previousSecret: string | undefined; constructor( private readonly jwtService: JwtService, @Inject(REFRESH_TOKEN_REPOSITORY) private readonly refreshTokenRepo: IRefreshTokenRepository, - ) {} + ) { + const secret = process.env['JWT_SECRET']; + if (!secret) { + throw new Error('JWT_SECRET environment variable is required'); + } + this.primarySecret = secret; + this.previousSecret = process.env['JWT_SECRET_PREVIOUS'] || undefined; + } async generateTokenPair(payload: JwtPayload): Promise { const accessToken = this.jwtService.sign(payload); - const rawRefreshToken = randomBytes(64).toString('hex'); const hashedToken = this.hashToken(rawRefreshToken); const family = randomBytes(16).toString('hex'); - const expiresAt = new Date(); expiresAt.setDate(expiresAt.getDate() + this.REFRESH_TOKEN_EXPIRY_DAYS); - - await this.refreshTokenRepo.create({ - userId: payload.sub, - token: hashedToken, - family, - expiresAt, - revokedAt: null, - }); - - return { - accessToken, - refreshToken: `${family}.${rawRefreshToken}`, - expiresIn: 900, - }; + await this.refreshTokenRepo.create({ userId: payload.sub, token: hashedToken, family, expiresAt, revokedAt: null }); + return { accessToken, refreshToken: `${family}.${rawRefreshToken}`, expiresIn: 900 }; } async rotateRefreshToken(refreshToken: string): Promise { const dotIndex = refreshToken.indexOf('.'); if (dotIndex === -1) return null; - const family = refreshToken.substring(0, dotIndex); const rawToken = refreshToken.substring(dotIndex + 1); if (!family || !rawToken) return null; - const hashedToken = this.hashToken(rawToken); const existing = await this.refreshTokenRepo.findByToken(hashedToken); - - if (!existing) { - // Possible token reuse attack — revoke entire family - await this.refreshTokenRepo.revokeByFamily(family); - return null; - } - - if (existing.revokedAt || existing.expiresAt < new Date()) { - await this.refreshTokenRepo.revokeByFamily(existing.family); - return null; - } - - // Revoke all tokens in this family + if (!existing) { await this.refreshTokenRepo.revokeByFamily(family); return null; } + if (existing.revokedAt || existing.expiresAt < new Date()) { await this.refreshTokenRepo.revokeByFamily(existing.family); return null; } await this.refreshTokenRepo.revokeByFamily(existing.family); - - // Create new token in a new family const newRawToken = randomBytes(64).toString('hex'); const newHashedToken = this.hashToken(newRawToken); const newFamily = randomBytes(16).toString('hex'); - const expiresAt = new Date(); expiresAt.setDate(expiresAt.getDate() + this.REFRESH_TOKEN_EXPIRY_DAYS); - - await this.refreshTokenRepo.create({ - userId: existing.userId, - token: newHashedToken, - family: newFamily, - expiresAt, - revokedAt: null, - }); - - return { - userId: existing.userId, - refreshToken: `${newFamily}.${newRawToken}`, - }; + await this.refreshTokenRepo.create({ userId: existing.userId, token: newHashedToken, family: newFamily, expiresAt, revokedAt: null }); + return { userId: existing.userId, refreshToken: `${newFamily}.${newRawToken}` }; } - generateAccessToken(payload: JwtPayload): string { - return this.jwtService.sign(payload); - } + generateAccessToken(payload: JwtPayload): string { return this.jwtService.sign(payload); } - async revokeAllUserTokens(userId: string): Promise { - await this.refreshTokenRepo.revokeAllForUser(userId); - } + async revokeAllUserTokens(userId: string): Promise { await this.refreshTokenRepo.revokeAllForUser(userId); } verifyAccessToken(token: string): JwtPayload | null { - try { - return this.jwtService.verify(token); - } catch { - return null; - } + return verifyWithRotation(token, this.primarySecret, this.previousSecret); } - private hashToken(token: string): string { - return createHash('sha256').update(token).digest('hex'); - } + private hashToken(token: string): string { return createHash('sha256').update(token).digest('hex'); } } diff --git a/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts b/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts index 57009fd..ae8c9c7 100644 --- a/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts +++ b/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts @@ -5,6 +5,7 @@ import { ExtractJwt, Strategy } from 'passport-jwt'; // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- NestJS DI requires value imports for emitDecoratorMetadata import { PrismaService, RedisService } from '@modules/shared'; import { type JwtPayload } from '../services/token.service'; +import { makeSecretOrKeyProvider } from '../utils/jwt-rotation'; function extractJwtFromCookieOrHeader(req: Request): string | null { const cookieToken = req.cookies?.['access_token'] as string | undefined; @@ -12,88 +13,33 @@ function extractJwtFromCookieOrHeader(req: Request): string | null { return ExtractJwt.fromAuthHeaderAsBearerToken()(req); } -/** Cached user status — JSON encoded in Redis. */ -interface CachedUserStatus { - isActive: boolean; - deletedAt: string | null; -} +interface CachedUserStatus { isActive: boolean; deletedAt: string | null; } -/** - * Redis key prefix for user status cache. Versioned so that a schema - * change can invalidate all stale entries by bumping the version. - */ export const USER_STATUS_CACHE_PREFIX = 'auth:user_status:v1'; -/** TTL for cached user status (seconds). */ export const USER_STATUS_CACHE_TTL_SECONDS = 60; @Injectable() export class JwtStrategy extends PassportStrategy(Strategy) { - constructor( - private readonly prisma: PrismaService, - private readonly redis: RedisService, - ) { + constructor(private readonly prisma: PrismaService, private readonly redis: RedisService) { const jwtSecret = process.env['JWT_SECRET']; - if (!jwtSecret) { - throw new Error('JWT_SECRET environment variable is required'); - } - - super({ - jwtFromRequest: extractJwtFromCookieOrHeader, - ignoreExpiration: false, - secretOrKey: jwtSecret, - audience: 'goodgo-api', - issuer: 'goodgo-platform', - }); + if (!jwtSecret) throw new Error('JWT_SECRET environment variable is required'); + const previousSecret = process.env['JWT_SECRET_PREVIOUS'] || undefined; + super({ jwtFromRequest: extractJwtFromCookieOrHeader, ignoreExpiration: false, secretOrKeyProvider: makeSecretOrKeyProvider(jwtSecret, previousSecret), audience: 'goodgo-api', issuer: 'goodgo-platform' }); } async validate(payload: JwtPayload): Promise { const status = await this.loadUserStatus(payload.sub); - if (!status || !status.isActive || status.deletedAt !== null) { - throw new UnauthorizedException('User account is inactive or deleted'); - } + if (!status || !status.isActive || status.deletedAt !== null) throw new UnauthorizedException('User account is inactive or deleted'); return { sub: payload.sub, phone: payload.phone, role: payload.role }; } - /** - * Loads user status from Redis cache if present, otherwise from DB and - * populates the cache with a 60 s TTL. Redis failures are non-fatal: - * we fall back to DB so a Redis outage cannot lock out all users. - * - * Returns null only when the user does not exist in the DB. - */ private async loadUserStatus(userId: string): Promise { const cacheKey = `${USER_STATUS_CACHE_PREFIX}:${userId}`; - - if (this.redis.isAvailable()) { - try { - const cached = await this.redis.get(cacheKey); - if (cached !== null) { - return JSON.parse(cached) as CachedUserStatus; - } - } catch { - // Swallow: degrade to DB on Redis read error. - } - } - - const user = await this.prisma.user.findUnique({ - where: { id: userId }, - select: { isActive: true, deletedAt: true }, - }); + if (this.redis.isAvailable()) { try { const cached = await this.redis.get(cacheKey); if (cached !== null) return JSON.parse(cached) as CachedUserStatus; } catch { /* swallow */ } } + const user = await this.prisma.user.findUnique({ where: { id: userId }, select: { isActive: true, deletedAt: true } }); if (!user) return null; - - const status: CachedUserStatus = { - isActive: user.isActive, - deletedAt: user.deletedAt ? user.deletedAt.toISOString() : null, - }; - - if (this.redis.isAvailable()) { - try { - await this.redis.set(cacheKey, JSON.stringify(status), USER_STATUS_CACHE_TTL_SECONDS); - } catch { - // Swallow: cache population is best-effort. - } - } - + const status: CachedUserStatus = { isActive: user.isActive, deletedAt: user.deletedAt ? user.deletedAt.toISOString() : null }; + if (this.redis.isAvailable()) { try { await this.redis.set(cacheKey, JSON.stringify(status), USER_STATUS_CACHE_TTL_SECONDS); } catch { /* swallow */ } } return status; } } diff --git a/apps/api/src/modules/auth/infrastructure/utils/jwt-rotation.ts b/apps/api/src/modules/auth/infrastructure/utils/jwt-rotation.ts new file mode 100644 index 0000000..f6c67c6 --- /dev/null +++ b/apps/api/src/modules/auth/infrastructure/utils/jwt-rotation.ts @@ -0,0 +1,21 @@ +import { verify as jwtVerify, type JwtPayload as JsonWebTokenPayload } from 'jsonwebtoken'; + +const JWT_VERIFY_OPTIONS = { audience: 'goodgo-api', issuer: 'goodgo-platform' } as const; + +export function verifyWithRotation( + token: string, primarySecret: string, previousSecret: string | undefined, +): T | null { + try { return jwtVerify(token, primarySecret, JWT_VERIFY_OPTIONS) as T; } catch { /* primary failed */ } + if (previousSecret) { try { return jwtVerify(token, previousSecret, JWT_VERIFY_OPTIONS) as T; } catch { /* both failed */ } } + return null; +} + +export function makeSecretOrKeyProvider( + primarySecret: string, previousSecret: string | undefined, +): (request: unknown, rawJwtToken: string, done: (err: Error | null, secret?: string) => void) => void { + return (_request: unknown, rawJwtToken: string, done: (err: Error | null, secret?: string) => void) => { + try { jwtVerify(rawJwtToken, primarySecret, JWT_VERIFY_OPTIONS); return done(null, primarySecret); } catch { /* primary failed */ } + if (previousSecret) { try { jwtVerify(rawJwtToken, previousSecret, JWT_VERIFY_OPTIONS); return done(null, previousSecret); } catch { /* both failed */ } } + return done(null, primarySecret); + }; +} diff --git a/apps/api/src/modules/shared/infrastructure/env-validation.ts b/apps/api/src/modules/shared/infrastructure/env-validation.ts index e25077a..340d678 100644 --- a/apps/api/src/modules/shared/infrastructure/env-validation.ts +++ b/apps/api/src/modules/shared/infrastructure/env-validation.ts @@ -45,6 +45,17 @@ const REQUIRED_WHEN_USED: ReadonlyMap = new Map([ * Known placeholder values that must never be used as real secrets. * Comparison is case-insensitive to catch common variants. */ +/** + * Previous-version secrets used during key rotation. Validated if set but never + * required. Note: JWT_REFRESH_SECRET_PREVIOUS currently has no runtime consumer + * because refresh tokens are opaque random bytes, not JWTs — the variable is + * accepted here for forward-compatibility should the refresh mechanism change. + */ +const OPTIONAL_PREVIOUS_SECRETS: readonly string[] = [ + 'JWT_SECRET_PREVIOUS', + 'JWT_REFRESH_SECRET_PREVIOUS', +]; + const FORBIDDEN_SECRET_VALUES: readonly string[] = [ 'change_me', 'changeme', @@ -127,6 +138,25 @@ export function validateEnv(): void { ); } + // Validate optional previous secrets if they are set (rotation window). + const prevSecretErrors: string[] = []; + for (const key of OPTIONAL_PREVIOUS_SECRETS) { + const value = process.env[key]; + if (value) { + const error = validateJwtSecret(key, value); + if (error) { + prevSecretErrors.push(error); + } + } + } + + if (prevSecretErrors.length > 0) { + throw new Error( + `Insecure previous-secret configuration:\n ${prevSecretErrors.join('\n ')}\n` + + 'Previous secrets must meet the same strength requirements as primary secrets.', + ); + } + if (!isProduction) { return; } diff --git a/docs/security/SECRET_ROTATION_POLICY.md b/docs/security/SECRET_ROTATION_POLICY.md new file mode 100644 index 0000000..b68119f --- /dev/null +++ b/docs/security/SECRET_ROTATION_POLICY.md @@ -0,0 +1,192 @@ +# Payment Gateway Secret Rotation Policy + +> **Status:** Active — GOO-197 / parent [GOO-102](/GOO/issues/GOO-102) (CLO data-security work). +> **Owner:** Security Engineer + Platform on-call. +> **Last reviewed:** 2026-04-24. + +This document is the canonical policy for rotating all secrets that gate +access to GoodGo's payment gateways and adjacent integrations (OAuth, +storage, webhook signing, JWT). It ships alongside the `SecretProvider` +abstraction in +`apps/api/src/modules/shared/domain/ports/secret-provider.port.ts` and the +env-backed implementation in +`apps/api/src/modules/shared/infrastructure/env-secret-provider.service.ts`. + +--- + +## 1. Why rotate + +A stolen or leaked HMAC key for a payment gateway is the most direct path +to financial fraud against GoodGo. Rotation reduces the **window of abuse** +when a key is exposed (insider misuse, accidental git commit, third-party +breach, log scraping, etc.). It also forces us to verify that every +runtime that relies on the key can still read it — i.e. that we have not +lost the ability to rotate. + +## 2. Scope (rotation-sensitive secrets) + +The following secrets are in scope. Each is registered with the +`SecretProvider` by default (see `DEFAULT_REGISTERED_SECRETS`) and has a +matching entry in `env-validation.ts`. + +| Secret env var | Purpose | Cadence | Owner | +| ------------------------------ | ------------------------------- | -------- | -------------- | +| `JWT_SECRET` | Access-token HMAC | 90 days | Auth | +| `JWT_REFRESH_SECRET` | Refresh-token HMAC | 90 days | Auth | +| `VNPAY_HASH_SECRET` | VNPay request/callback HMAC | 90 days | Payments | +| `MOMO_SECRET_KEY` | MoMo request/callback HMAC | 90 days | Payments | +| `ZALOPAY_KEY1` | ZaloPay order signing | 90 days | Payments | +| `ZALOPAY_KEY2` | ZaloPay callback signing | 90 days | Payments | +| `BANK_TRANSFER_WEBHOOK_SECRET` | Bank-transfer webhook signature | 90 days | Payments | +| `GOOGLE_CLIENT_SECRET` | Google OAuth | 180 days | Auth | +| `ZALO_APP_SECRET` | Zalo OAuth | 180 days | Auth | +| `ZALO_OA_ACCESS_TOKEN` | Zalo Official Account API token | 90 days | Notifications | +| `MINIO_SECRET_KEY` | Object-storage access key | 180 days | Platform | +| `FIELD_ENCRYPTION_KEY` | At-rest PII encryption key | annually | Platform + CLO | + +Secrets **not** in this table (e.g. `DATABASE_URL` password, `REDIS_HOST`) +follow the platform-credential rotation policy and are out of scope here. + +## 3. Cadence and triggers + +- **Routine rotation:** every 90 days for HMAC/signing keys, 180 days for + OAuth client secrets, annually for the field-encryption key (which has + expensive data-rewrap implications). +- **Event-driven rotation (always immediately):** + - any commit accidentally containing a real value of one of the secrets + above (regardless of how briefly); + - departure of any individual with production access to the secret store; + - downstream provider notification that the credential may be exposed; + - confirmed or strongly suspected breach of any system that handled the + secret in plaintext (CI runner, dev laptop, log aggregator, …). + +## 4. Operator workflow (env-backed backend) + +1. **Generate** a new high-entropy value: + + ```bash + openssl rand -base64 48 + ``` + +2. **Stage the dual-key grace period.** Copy the current secret to the + `_PREVIOUS` variable and set the new secret as the primary: + + ```bash + # Example for JWT_SECRET rotation: + JWT_SECRET_PREVIOUS= + JWT_SECRET= + # Same pattern for JWT_REFRESH_SECRET if rotating refresh keys. + ``` + + The auth layer automatically tries the primary key first and falls + back to `_PREVIOUS`, so tokens signed with the old key continue to + validate during the grace period (≤ access-token TTL, typically 15 m). + +3. **Deploy** the change. On boot, every API instance logs: + + ``` + [EnvSecretProvider] Secret versions at boot: VNPAY_HASH_SECRET=2026-04-24, … + ``` + + Verify the version field matches the staged version on every instance. + The raw value **must never** appear in this or any other log line. + +4. **Smoke-test** payment flows for the rotated provider: + - issue one sandbox payment + - confirm callback verification succeeds + - confirm refund signing succeeds + Record the rotation in the security audit log + (`docs/security/secret-rotation-log.md` — append-only). + +5. **Decommission** the old credential in the gateway's merchant portal. + +6. **Remove the previous secret.** After the grace period (at least one + full access-token TTL cycle, typically 15 minutes), remove + `JWT_SECRET_PREVIOUS` (and/or `JWT_REFRESH_SECRET_PREVIOUS`) from the + environment and redeploy. This closes the dual-key window. + +## 5. SecretProvider abstraction (developer workflow) + +All new and existing code that consumes a rotation-sensitive secret MUST +go through the `SecretProvider` port: + +```ts +import { Inject, Injectable } from '@nestjs/common'; +import { SECRET_PROVIDER, type ISecretProvider } from '@modules/shared/domain/ports'; + +@Injectable() +export class VnpayService { + constructor(@Inject(SECRET_PROVIDER) private readonly secrets: ISecretProvider) {} + + async sign(payload: string): Promise { + const { value } = await this.secrets.getSecret('VNPAY_HASH_SECRET'); + // … HMAC with `value`, never store it on `this`, never log it. + } +} +``` + +Rules: + +- **Never** capture the raw value into a service field. Always re-read on + the request path so a rotation takes effect at the next request. +- **Never** include `material.value` in log messages, error messages, or + exception payloads. `material.version` is safe to log. +- **Never** stringify a `SecretMaterial` directly into a response body. +- For bootstrap-only contexts where `await` is awkward, use + `getSecretSync` — but note that a future remote backend may throw + `UnsupportedSyncReadError`. + +## 6. Backends + +- **Short term — `EnvSecretProvider` (current).** Reads from `process.env` + via `ConfigService`. Operationally identical to the pre-existing + `getOrThrow('VNPAY_HASH_SECRET')` calls, but with a stable audit surface + (versions logged, port-based DI). +- **Mid term — `AwsSecretsManagerSecretProvider` / `VaultSecretProvider`.** + Same port. Adds: + - automatic refresh from the remote store + - per-secret IAM / Vault-policy scoping + - native version ids (`AWSCURRENT` / `AWSPREVIOUS` etc.) surfaced as + `material.version` + - `getSecretSync` may throw `UnsupportedSyncReadError`; bootstrap + callers must migrate to `getSecret`. + +Switching backends is a one-line change in `SharedModule` (replace +`EnvSecretProvider` with the new implementation under the +`SECRET_PROVIDER` token). No call sites change. + +## 7. Logging discipline + +- The `EnvSecretProvider` logs only `name=version` pairs at boot. +- The `version` is either an operator-provided `_SECRET_VERSION` env + var, or a 10-char SHA-256 fingerprint of the value (40 bits of entropy; + non-invertible; useful for distinguishing rotations across instances). +- Negative tests in + `apps/api/src/modules/shared/infrastructure/__tests__/env-secret-provider.service.spec.ts` + assert the raw value never appears in logger output, error messages, or + serialized provider state. +- The repo also has a global `pii-masker` and `GlobalExceptionFilter` — + those are defence-in-depth, not the primary control. The primary control + is "never put the value into a string in the first place." + +## 8. Incident response (suspected leak) + +1. Open a P1 incident in `#sec-incident`. Page Security on-call. +2. Rotate the affected secret immediately following §4 — do not wait for + forensic confirmation. +3. Search logs / CI artifacts / git history for the leaked value + fingerprint (NOT the value itself; use `fingerprint()` from + `env-secret-provider.service.ts`). +4. Coordinate with the gateway's anti-fraud team where applicable (VNPay, + MoMo, ZaloPay merchant support). +5. File a post-mortem within 5 business days; update this policy if + process gaps were found. + +## 9. References + +- Source port: `apps/api/src/modules/shared/domain/ports/secret-provider.port.ts` +- Env-backed impl: `apps/api/src/modules/shared/infrastructure/env-secret-provider.service.ts` +- Env validation: `apps/api/src/modules/shared/infrastructure/env-validation.ts` +- Negative tests: `apps/api/src/modules/shared/infrastructure/__tests__/env-secret-provider.service.spec.ts` +- Parent issue: [GOO-102](/GOO/issues/GOO-102) +- This issue: [GOO-197](/GOO/issues/GOO-197) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 010a0c5..745eb52 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -87,6 +87,9 @@ importers: '@aws-sdk/s3-request-presigner': specifier: ^3.1026.0 version: 3.1026.0 + '@goodgo/contracts-events': + specifier: workspace:* + version: link:../../libs/contracts/events '@goodgo/mcp-servers': specifier: workspace:* version: link:../../libs/mcp-servers @@ -186,6 +189,9 @@ importers: ioredis: specifier: ^5.4.0 version: 5.10.1 + jsonwebtoken: + specifier: ^9.0.3 + version: 9.0.3 nodemailer: specifier: ^8.0.5 version: 8.0.5 @@ -259,6 +265,9 @@ importers: '@types/express': specifier: ^5.0.0 version: 5.0.6 + '@types/jsonwebtoken': + specifier: ^9.0.10 + version: 9.0.10 '@types/node': specifier: ^25.5.2 version: 25.5.2 @@ -420,6 +429,12 @@ importers: specifier: ^4.1.3 version: 4.1.3(@opentelemetry/api@1.9.1)(@types/node@25.5.2)(jsdom@29.0.2(@noble/hashes@2.0.1))(msw@2.13.2(@types/node@25.5.2)(typescript@6.0.2))(vite@7.3.2(@types/node@25.5.2)(jiti@1.21.7)(terser@5.46.1)(tsx@4.21.0)(yaml@2.8.3)) + libs/contracts/events: + devDependencies: + typescript: + specifier: ^5.5.0 + version: 5.9.3 + libs/mcp-servers: dependencies: '@modelcontextprotocol/sdk':