From 7bdd183f922b456128c5c5aa9b223749deb3f999 Mon Sep 17 00:00:00 2001 From: Demian Date: Sat, 11 Apr 2026 01:15:17 -0400 Subject: [PATCH 1/9] feat: add scheduled refresh token and password reset cleanup job Add TokenCleanupService to the auth module with a @Cron job that runs daily at 3am (configurable via REFRESH_TOKEN_CLEANUP_CRON) and deletes all refresh_tokens rows where revoked=true or expires_at < now, and all password_resets rows where used=true or expires_at < now. - Job skips early when NODE_ENV=test - Logs row count and duration on success, error stack on failure - Does not rethrow on failure so job errors cannot crash the process - ScheduleModule is already conditionally excluded in test env (AppModule) Closes #98 --- backend/.env.example | 3 +++ backend/src/modules/auth/auth.module.ts | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/backend/.env.example b/backend/.env.example index 9e03ac1..dd893dc 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -17,6 +17,9 @@ USE_REDIS_CACHE=true PORT=3001 APP_NAME=STATION BACKEND +# Token Cleanup Cron (default: 3am daily) +REFRESH_TOKEN_CLEANUP_CRON="0 3 * * *" + # UEX Sync Configuration UEX_SYNC_ENABLED=true UEX_CATEGORIES_SYNC_ENABLED=true diff --git a/backend/src/modules/auth/auth.module.ts b/backend/src/modules/auth/auth.module.ts index fb2778c..cc82483 100644 --- a/backend/src/modules/auth/auth.module.ts +++ b/backend/src/modules/auth/auth.module.ts @@ -4,6 +4,7 @@ import { JwtModule } from '@nestjs/jwt'; import { TypeOrmModule } from '@nestjs/typeorm'; import { AuthController } from './auth.controller'; import { AuthService } from './auth.service'; +import { TokenCleanupService } from './token-cleanup.service'; import { LocalStrategy } from './local.strategy'; import { JwtStrategy } from './jwt.strategy'; import { RefreshTokenStrategy } from './refresh-token.strategy'; @@ -27,7 +28,13 @@ import { ConfigModule, ConfigService } from '@nestjs/config'; }), ], controllers: [AuthController], - providers: [AuthService, LocalStrategy, JwtStrategy, RefreshTokenStrategy], + providers: [ + AuthService, + TokenCleanupService, + LocalStrategy, + JwtStrategy, + RefreshTokenStrategy, + ], exports: [AuthService], }) export class AuthModule {} From 0507031a27240e5abd29189bdde8d1a2c42156e0 Mon Sep 17 00:00:00 2001 From: Demian Date: Sat, 11 Apr 2026 01:15:57 -0400 Subject: [PATCH 2/9] feat: add TokenCleanupService implementation Co-authored-with: ISSUE-98 --- .../src/modules/auth/token-cleanup.service.ts | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 backend/src/modules/auth/token-cleanup.service.ts diff --git a/backend/src/modules/auth/token-cleanup.service.ts b/backend/src/modules/auth/token-cleanup.service.ts new file mode 100644 index 0000000..d1a0748 --- /dev/null +++ b/backend/src/modules/auth/token-cleanup.service.ts @@ -0,0 +1,64 @@ +import { Injectable, Logger } from '@nestjs/common'; +import { Cron } from '@nestjs/schedule'; +import { InjectRepository } from '@nestjs/typeorm'; +import { Repository } from 'typeorm'; +import { ConfigService } from '@nestjs/config'; +import { RefreshToken } from './refresh-token.entity'; +import { PasswordReset } from './password-reset.entity'; + +@Injectable() +export class TokenCleanupService { + private readonly logger = new Logger(TokenCleanupService.name); + + constructor( + @InjectRepository(RefreshToken) + private readonly refreshTokenRepository: Repository, + @InjectRepository(PasswordReset) + private readonly passwordResetRepository: Repository, + private readonly configService: ConfigService, + ) {} + + @Cron( + // Default: 3am daily — override with REFRESH_TOKEN_CLEANUP_CRON env var + process.env['REFRESH_TOKEN_CLEANUP_CRON'] ?? '0 3 * * *', + ) + async cleanupExpiredTokens(): Promise { + if (process.env['NODE_ENV'] === 'test') { + return; + } + + const start = Date.now(); + this.logger.log('Starting expired/revoked token cleanup'); + + try { + const now = new Date(); + + const { affected: refreshDeleted } = await this.refreshTokenRepository + .createQueryBuilder() + .delete() + .where('revoked = :revoked OR expires_at < :now', { + revoked: true, + now, + }) + .execute(); + + const { affected: resetDeleted } = await this.passwordResetRepository + .createQueryBuilder() + .delete() + .where('used = :used OR expires_at < :now', { used: true, now }) + .execute(); + + const duration = Date.now() - start; + this.logger.log( + `Token cleanup complete in ${duration}ms — ` + + `deleted ${refreshDeleted ?? 0} refresh token(s), ` + + `${resetDeleted ?? 0} password reset(s)`, + ); + } catch (error) { + this.logger.error( + 'Token cleanup job failed', + error instanceof Error ? error.stack : String(error), + ); + } + } +} From c1c364763b3e4b5c3a9d5659629b5dbb4fc0b43e Mon Sep 17 00:00:00 2001 From: Demian Date: Mon, 13 Apr 2026 10:42:40 -0400 Subject: [PATCH 3/9] fix: correct column name in cleanup queries, remove unused ConfigService, add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace expires_at with "expiresAt" in both QueryBuilder WHERE clauses — TypeORM quotes identifiers so the DB column is case-sensitive "expiresAt", not expires_at, which would always error and silently skip cleanup - Remove ConfigService import and constructor injection — it was never used; @Cron() is evaluated at module-load time before DI so ConfigService cannot supply the cron expression regardless - Add TokenCleanupService unit tests covering early return in test env, correct WHERE clauses for both delete operations, and error handling --- .../auth/token-cleanup.service.spec.ts | 103 ++++++++++++++++++ .../src/modules/auth/token-cleanup.service.ts | 6 +- 2 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 backend/src/modules/auth/token-cleanup.service.spec.ts diff --git a/backend/src/modules/auth/token-cleanup.service.spec.ts b/backend/src/modules/auth/token-cleanup.service.spec.ts new file mode 100644 index 0000000..8c97053 --- /dev/null +++ b/backend/src/modules/auth/token-cleanup.service.spec.ts @@ -0,0 +1,103 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { TokenCleanupService } from './token-cleanup.service'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { RefreshToken } from './refresh-token.entity'; +import { PasswordReset } from './password-reset.entity'; + +describe('TokenCleanupService', () => { + let service: TokenCleanupService; + + const mockQueryBuilder = { + delete: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + execute: jest.fn(), + }; + + const mockRefreshTokenRepository = { + createQueryBuilder: jest.fn().mockReturnValue(mockQueryBuilder), + }; + + const mockPasswordResetRepository = { + createQueryBuilder: jest.fn().mockReturnValue(mockQueryBuilder), + }; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + TokenCleanupService, + { + provide: getRepositoryToken(RefreshToken), + useValue: mockRefreshTokenRepository, + }, + { + provide: getRepositoryToken(PasswordReset), + useValue: mockPasswordResetRepository, + }, + ], + }).compile(); + + service = module.get(TokenCleanupService); + jest.clearAllMocks(); + }); + + describe('cleanupExpiredTokens', () => { + it('should return early in test environment without touching the database', async () => { + // Jest always sets NODE_ENV=test, so no override needed + await service.cleanupExpiredTokens(); + + expect( + mockRefreshTokenRepository.createQueryBuilder, + ).not.toHaveBeenCalled(); + expect( + mockPasswordResetRepository.createQueryBuilder, + ).not.toHaveBeenCalled(); + }); + + describe('in a non-test environment', () => { + const originalEnv = process.env['NODE_ENV']; + + beforeEach(() => { + process.env['NODE_ENV'] = 'development'; + // Re-wire execute to return distinct values per call + mockQueryBuilder.execute + .mockResolvedValueOnce({ affected: 3 }) // refresh tokens + .mockResolvedValueOnce({ affected: 1 }); // password resets + }); + + afterEach(() => { + process.env['NODE_ENV'] = originalEnv; + }); + + it('should delete revoked and expired refresh tokens', async () => { + await service.cleanupExpiredTokens(); + + expect( + mockRefreshTokenRepository.createQueryBuilder, + ).toHaveBeenCalled(); + const [whereClause, params] = mockQueryBuilder.where.mock.calls[0]; + expect(whereClause).toContain('revoked'); + expect(whereClause).toContain('"expiresAt"'); + expect(params).toMatchObject({ revoked: true, now: expect.any(Date) }); + }); + + it('should delete used and expired password resets', async () => { + await service.cleanupExpiredTokens(); + + expect( + mockPasswordResetRepository.createQueryBuilder, + ).toHaveBeenCalled(); + const [whereClause, params] = mockQueryBuilder.where.mock.calls[1]; + expect(whereClause).toContain('used'); + expect(whereClause).toContain('"expiresAt"'); + expect(params).toMatchObject({ used: true, now: expect.any(Date) }); + }); + + it('should not throw when a query fails', async () => { + mockQueryBuilder.execute.mockReset(); + mockQueryBuilder.execute.mockRejectedValueOnce(new Error('DB failure')); + + await expect(service.cleanupExpiredTokens()).resolves.toBeUndefined(); + }); + }); + }); +}); diff --git a/backend/src/modules/auth/token-cleanup.service.ts b/backend/src/modules/auth/token-cleanup.service.ts index d1a0748..39d3e4d 100644 --- a/backend/src/modules/auth/token-cleanup.service.ts +++ b/backend/src/modules/auth/token-cleanup.service.ts @@ -2,7 +2,6 @@ import { Injectable, Logger } from '@nestjs/common'; import { Cron } from '@nestjs/schedule'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { ConfigService } from '@nestjs/config'; import { RefreshToken } from './refresh-token.entity'; import { PasswordReset } from './password-reset.entity'; @@ -15,7 +14,6 @@ export class TokenCleanupService { private readonly refreshTokenRepository: Repository, @InjectRepository(PasswordReset) private readonly passwordResetRepository: Repository, - private readonly configService: ConfigService, ) {} @Cron( @@ -36,7 +34,7 @@ export class TokenCleanupService { const { affected: refreshDeleted } = await this.refreshTokenRepository .createQueryBuilder() .delete() - .where('revoked = :revoked OR expires_at < :now', { + .where('revoked = :revoked OR "expiresAt" < :now', { revoked: true, now, }) @@ -45,7 +43,7 @@ export class TokenCleanupService { const { affected: resetDeleted } = await this.passwordResetRepository .createQueryBuilder() .delete() - .where('used = :used OR expires_at < :now', { used: true, now }) + .where('used = :used OR "expiresAt" < :now', { used: true, now }) .execute(); const duration = Date.now() - start; From 8c2fc195bdc8abc8dda8232394d7bb2e7f92d085 Mon Sep 17 00:00:00 2001 From: Demian Date: Mon, 13 Apr 2026 11:07:39 -0400 Subject: [PATCH 4/9] fix: register cleanup cron at runtime via SchedulerRegistry, add JEST_WORKER_ID guard @Cron() expressions are evaluated at module-load time in Node.js CJS, before dotenv runs, so REFRESH_TOKEN_CLEANUP_CRON from .env was never read. Switching to OnApplicationBootstrap + SchedulerRegistry means the cron is registered after ConfigModule has fully loaded all env vars. - Remove @Cron() decorator; implement OnApplicationBootstrap - Register cron via SchedulerRegistry using ConfigService.get() so .env values are honoured; falls back to '0 3 * * *' if unset - Add JEST_WORKER_ID guard to onApplicationBootstrap() so cron is never registered in Jest worker processes even if NODE_ENV is not 'test' - Add cron as a direct dependency (was transitive via @nestjs/schedule) - Update spec: explicit NODE_ENV mock in cleanupExpiredTokens early-return test for determinism; add onApplicationBootstrap() coverage --- backend/package.json | 1 + .../auth/token-cleanup.service.spec.ts | 97 ++++++++++++++++--- .../src/modules/auth/token-cleanup.service.ts | 41 ++++++-- pnpm-lock.yaml | 6 +- 4 files changed, 125 insertions(+), 20 deletions(-) diff --git a/backend/package.json b/backend/package.json index 76649e9..8d12962 100644 --- a/backend/package.json +++ b/backend/package.json @@ -50,6 +50,7 @@ "cache-manager-redis-yet": "^5.1.5", "class-transformer": "^0.5.1", "class-validator": "^0.14.2", + "cron": "^4.3.3", "dotenv": "^16.4.5", "figlet": "^1.8.0", "passport": "^0.7.0", diff --git a/backend/src/modules/auth/token-cleanup.service.spec.ts b/backend/src/modules/auth/token-cleanup.service.spec.ts index 8c97053..9efa7c7 100644 --- a/backend/src/modules/auth/token-cleanup.service.spec.ts +++ b/backend/src/modules/auth/token-cleanup.service.spec.ts @@ -3,6 +3,8 @@ import { TokenCleanupService } from './token-cleanup.service'; import { getRepositoryToken } from '@nestjs/typeorm'; import { RefreshToken } from './refresh-token.entity'; import { PasswordReset } from './password-reset.entity'; +import { SchedulerRegistry } from '@nestjs/schedule'; +import { ConfigService } from '@nestjs/config'; describe('TokenCleanupService', () => { let service: TokenCleanupService; @@ -21,6 +23,14 @@ describe('TokenCleanupService', () => { createQueryBuilder: jest.fn().mockReturnValue(mockQueryBuilder), }; + const mockSchedulerRegistry = { + addCronJob: jest.fn(), + }; + + const mockConfigService = { + get: jest.fn(), + }; + beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -33,6 +43,14 @@ describe('TokenCleanupService', () => { provide: getRepositoryToken(PasswordReset), useValue: mockPasswordResetRepository, }, + { + provide: SchedulerRegistry, + useValue: mockSchedulerRegistry, + }, + { + provide: ConfigService, + useValue: mockConfigService, + }, ], }).compile(); @@ -40,9 +58,71 @@ describe('TokenCleanupService', () => { jest.clearAllMocks(); }); + describe('onApplicationBootstrap', () => { + it('should not register cron when NODE_ENV is test', () => { + mockConfigService.get.mockImplementation((key: string) => + key === 'NODE_ENV' ? 'test' : undefined, + ); + + service.onApplicationBootstrap(); + + expect(mockSchedulerRegistry.addCronJob).not.toHaveBeenCalled(); + }); + + it('should not register cron when JEST_WORKER_ID is set', () => { + const originalWorker = process.env['JEST_WORKER_ID']; + process.env['JEST_WORKER_ID'] = '1'; + mockConfigService.get.mockReturnValue('development'); + + service.onApplicationBootstrap(); + + expect(mockSchedulerRegistry.addCronJob).not.toHaveBeenCalled(); + process.env['JEST_WORKER_ID'] = originalWorker; + }); + + it('should register cron with default expression in non-test env', () => { + const originalWorker = process.env['JEST_WORKER_ID']; + delete process.env['JEST_WORKER_ID']; + mockConfigService.get.mockImplementation((key: string) => { + if (key === 'NODE_ENV') return 'production'; + return undefined; // REFRESH_TOKEN_CLEANUP_CRON not set → fallback + }); + + service.onApplicationBootstrap(); + + expect(mockSchedulerRegistry.addCronJob).toHaveBeenCalledWith( + 'tokenCleanup', + expect.any(Object), + ); + process.env['JEST_WORKER_ID'] = originalWorker; + }); + + it('should register cron with custom expression from config', () => { + const originalWorker = process.env['JEST_WORKER_ID']; + delete process.env['JEST_WORKER_ID']; + mockConfigService.get.mockImplementation((key: string) => { + if (key === 'NODE_ENV') return 'production'; + if (key === 'REFRESH_TOKEN_CLEANUP_CRON') return '0 2 * * *'; + return undefined; + }); + + service.onApplicationBootstrap(); + + expect(mockSchedulerRegistry.addCronJob).toHaveBeenCalledWith( + 'tokenCleanup', + expect.any(Object), + ); + process.env['JEST_WORKER_ID'] = originalWorker; + }); + }); + describe('cleanupExpiredTokens', () => { it('should return early in test environment without touching the database', async () => { - // Jest always sets NODE_ENV=test, so no override needed + // Explicitly set NODE_ENV to 'test' for a deterministic guard check + mockConfigService.get.mockImplementation((key: string) => + key === 'NODE_ENV' ? 'test' : undefined, + ); + await service.cleanupExpiredTokens(); expect( @@ -54,18 +134,13 @@ describe('TokenCleanupService', () => { }); describe('in a non-test environment', () => { - const originalEnv = process.env['NODE_ENV']; - beforeEach(() => { - process.env['NODE_ENV'] = 'development'; - // Re-wire execute to return distinct values per call + mockConfigService.get.mockImplementation((key: string) => + key === 'NODE_ENV' ? 'development' : undefined, + ); mockQueryBuilder.execute - .mockResolvedValueOnce({ affected: 3 }) // refresh tokens - .mockResolvedValueOnce({ affected: 1 }); // password resets - }); - - afterEach(() => { - process.env['NODE_ENV'] = originalEnv; + .mockResolvedValueOnce({ affected: 3 }) + .mockResolvedValueOnce({ affected: 1 }); }); it('should delete revoked and expired refresh tokens', async () => { diff --git a/backend/src/modules/auth/token-cleanup.service.ts b/backend/src/modules/auth/token-cleanup.service.ts index 39d3e4d..f289769 100644 --- a/backend/src/modules/auth/token-cleanup.service.ts +++ b/backend/src/modules/auth/token-cleanup.service.ts @@ -1,12 +1,14 @@ -import { Injectable, Logger } from '@nestjs/common'; -import { Cron } from '@nestjs/schedule'; +import { Injectable, Logger, OnApplicationBootstrap } from '@nestjs/common'; +import { SchedulerRegistry } from '@nestjs/schedule'; +import { CronJob } from 'cron'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; +import { ConfigService } from '@nestjs/config'; import { RefreshToken } from './refresh-token.entity'; import { PasswordReset } from './password-reset.entity'; @Injectable() -export class TokenCleanupService { +export class TokenCleanupService implements OnApplicationBootstrap { private readonly logger = new Logger(TokenCleanupService.name); constructor( @@ -14,14 +16,37 @@ export class TokenCleanupService { private readonly refreshTokenRepository: Repository, @InjectRepository(PasswordReset) private readonly passwordResetRepository: Repository, + private readonly configService: ConfigService, + private readonly schedulerRegistry: SchedulerRegistry, ) {} - @Cron( - // Default: 3am daily — override with REFRESH_TOKEN_CLEANUP_CRON env var - process.env['REFRESH_TOKEN_CLEANUP_CRON'] ?? '0 3 * * *', - ) + onApplicationBootstrap(): void { + // Skip cron registration in test environments. JEST_WORKER_ID is set by + // Jest's worker processes even when NODE_ENV is not explicitly 'test'. + if ( + this.configService.get('NODE_ENV') === 'test' || + process.env['JEST_WORKER_ID'] !== undefined + ) { + return; + } + + // Read the cron expression here via ConfigService so that .env values + // loaded by ConfigModule.forRoot() are available — unlike @Cron() which + // evaluates at module-load time before dotenv runs. + const cronExpression = + this.configService.get('REFRESH_TOKEN_CLEANUP_CRON') ?? + '0 3 * * *'; + + const job = new CronJob(cronExpression, () => { + void this.cleanupExpiredTokens(); + }); + this.schedulerRegistry.addCronJob('tokenCleanup', job); + job.start(); + this.logger.log(`Token cleanup cron registered: ${cronExpression}`); + } + async cleanupExpiredTokens(): Promise { - if (process.env['NODE_ENV'] === 'test') { + if (this.configService.get('NODE_ENV') === 'test') { return; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f6c590f..1958eba 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -77,6 +77,9 @@ importers: class-validator: specifier: ^0.14.2 version: 0.14.2 + cron: + specifier: ^4.3.3 + version: 4.3.3 dotenv: specifier: ^16.4.5 version: 16.6.1 @@ -2609,11 +2612,12 @@ packages: glob@10.4.5: resolution: {integrity: sha512-7Bv8RF0k6xjo7d4A/PxYLbUCfb6c+Vpd2/mB2yRDlew7Jb5hEXiCD9ibfO7wpk8i4sevK6DFny9h7EYbM3/sHg==} + deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me hasBin: true glob@7.2.3: resolution: {integrity: sha512-nFR0zLpU2YCaRxwoCJvL6UvCH2JFyFVIvwTLsIf21AuHlMskA1hhTdk+LlYJtOlYt9v6dvszD2BGRqBL+iQK9Q==} - deprecated: Glob versions prior to v9 are no longer supported + deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me globals@13.24.0: resolution: {integrity: sha512-AhO5QUcj8llrbG09iWhPU2B204J1xnPeL8kQmVorSsy+Sjj1sk8gIyh6cUocGmH4L0UuhAJy+hJMRA4mgA4mFQ==} From f50b7d07a881c5eb44ff8592b9107a54de499f0e Mon Sep 17 00:00:00 2001 From: Demian Date: Mon, 13 Apr 2026 16:27:23 -0400 Subject: [PATCH 5/9] fix: address PR #114 review comments on token cleanup job - Add @Optional() to SchedulerRegistry injection so service can be instantiated in test environments where ScheduleModule is excluded - Validate cron expression at runtime; fall back to '0 3 * * *' on invalid value - Treat blank/whitespace REFRESH_TOKEN_CLEANUP_CRON as unset (|| not ??) - Upgrade Dockerfile base image from node:14 to node:18-alpine - Add migration for token cleanup indexes (partial + range indexes on revoked, expiresAt for refresh_tokens and used, expiresAt for password_resets) - Expand spec: test @Optional() path, invalid cron fallback, blank env var --- backend/Dockerfile | 2 +- .../1765038000000-AddTokenCleanupIndexes.ts | 56 ++++++++++++++++++ .../auth/token-cleanup.service.spec.ts | 57 +++++++++++++++++++ .../src/modules/auth/token-cleanup.service.ts | 48 +++++++++++----- 4 files changed, 148 insertions(+), 15 deletions(-) create mode 100644 backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts diff --git a/backend/Dockerfile b/backend/Dockerfile index 378706c..fd543b0 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -1,4 +1,4 @@ -FROM node:14 +FROM node:18-alpine # Create app directory WORKDIR /usr/src/app diff --git a/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts b/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts new file mode 100644 index 0000000..4d17f96 --- /dev/null +++ b/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts @@ -0,0 +1,56 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Adds indexes to support efficient token cleanup queries. + * + * The cleanup job deletes rows matching: + * refresh_tokens: revoked = true OR "expiresAt" < now + * password_resets: used = true OR "expiresAt" < now + * + * Partial indexes on the boolean flags keep index size small by only + * covering the rows that will actually be deleted. + */ +export class AddTokenCleanupIndexes1765038000000 implements MigrationInterface { + name = 'AddTokenCleanupIndexes1765038000000'; + + public async up(queryRunner: QueryRunner): Promise { + // Partial index on revoked refresh tokens (WHERE clause mirrors cleanup query) + await queryRunner.query(` + CREATE INDEX IF NOT EXISTS "IDX_refresh_tokens_revoked" + ON "refresh_tokens" ("revoked") + WHERE "revoked" = TRUE + `); + + // Range index for expiry-based cleanup of refresh tokens + await queryRunner.query(` + CREATE INDEX IF NOT EXISTS "IDX_refresh_tokens_expiresAt" + ON "refresh_tokens" ("expiresAt") + `); + + // Partial index on used password resets (WHERE clause mirrors cleanup query) + await queryRunner.query(` + CREATE INDEX IF NOT EXISTS "IDX_password_resets_used" + ON "password_resets" ("used") + WHERE "used" = TRUE + `); + + // Range index for expiry-based cleanup of password resets + await queryRunner.query(` + CREATE INDEX IF NOT EXISTS "IDX_password_resets_expiresAt" + ON "password_resets" ("expiresAt") + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `DROP INDEX IF EXISTS "IDX_password_resets_expiresAt"`, + ); + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_password_resets_used"`); + await queryRunner.query( + `DROP INDEX IF EXISTS "IDX_refresh_tokens_expiresAt"`, + ); + await queryRunner.query( + `DROP INDEX IF EXISTS "IDX_refresh_tokens_revoked"`, + ); + } +} diff --git a/backend/src/modules/auth/token-cleanup.service.spec.ts b/backend/src/modules/auth/token-cleanup.service.spec.ts index 9efa7c7..32981c7 100644 --- a/backend/src/modules/auth/token-cleanup.service.spec.ts +++ b/backend/src/modules/auth/token-cleanup.service.spec.ts @@ -80,6 +80,27 @@ describe('TokenCleanupService', () => { process.env['JEST_WORKER_ID'] = originalWorker; }); + it('should not register cron when schedulerRegistry is absent', () => { + // Simulate the @Optional() case: schedulerRegistry is undefined + const serviceWithoutRegistry = new TokenCleanupService( + mockRefreshTokenRepository as any, + mockPasswordResetRepository as any, + mockConfigService as any, + undefined, + ); + + const originalWorker = process.env['JEST_WORKER_ID']; + delete process.env['JEST_WORKER_ID']; + mockConfigService.get.mockImplementation((key: string) => + key === 'NODE_ENV' ? 'production' : undefined, + ); + + serviceWithoutRegistry.onApplicationBootstrap(); + + expect(mockSchedulerRegistry.addCronJob).not.toHaveBeenCalled(); + process.env['JEST_WORKER_ID'] = originalWorker; + }); + it('should register cron with default expression in non-test env', () => { const originalWorker = process.env['JEST_WORKER_ID']; delete process.env['JEST_WORKER_ID']; @@ -114,6 +135,42 @@ describe('TokenCleanupService', () => { ); process.env['JEST_WORKER_ID'] = originalWorker; }); + + it('should fall back to default cron expression when config value is invalid', () => { + const originalWorker = process.env['JEST_WORKER_ID']; + delete process.env['JEST_WORKER_ID']; + mockConfigService.get.mockImplementation((key: string) => { + if (key === 'NODE_ENV') return 'production'; + if (key === 'REFRESH_TOKEN_CLEANUP_CRON') return 'not-a-valid-cron'; + return undefined; + }); + + // Should not throw, and should still register the job with the fallback + expect(() => service.onApplicationBootstrap()).not.toThrow(); + expect(mockSchedulerRegistry.addCronJob).toHaveBeenCalledWith( + 'tokenCleanup', + expect.any(Object), + ); + process.env['JEST_WORKER_ID'] = originalWorker; + }); + + it('should treat blank REFRESH_TOKEN_CLEANUP_CRON as unset and use default', () => { + const originalWorker = process.env['JEST_WORKER_ID']; + delete process.env['JEST_WORKER_ID']; + mockConfigService.get.mockImplementation((key: string) => { + if (key === 'NODE_ENV') return 'production'; + if (key === 'REFRESH_TOKEN_CLEANUP_CRON') return ' '; // blank/whitespace + return undefined; + }); + + service.onApplicationBootstrap(); + + expect(mockSchedulerRegistry.addCronJob).toHaveBeenCalledWith( + 'tokenCleanup', + expect.any(Object), + ); + process.env['JEST_WORKER_ID'] = originalWorker; + }); }); describe('cleanupExpiredTokens', () => { diff --git a/backend/src/modules/auth/token-cleanup.service.ts b/backend/src/modules/auth/token-cleanup.service.ts index f289769..191ef10 100644 --- a/backend/src/modules/auth/token-cleanup.service.ts +++ b/backend/src/modules/auth/token-cleanup.service.ts @@ -1,4 +1,9 @@ -import { Injectable, Logger, OnApplicationBootstrap } from '@nestjs/common'; +import { + Injectable, + Logger, + OnApplicationBootstrap, + Optional, +} from '@nestjs/common'; import { SchedulerRegistry } from '@nestjs/schedule'; import { CronJob } from 'cron'; import { InjectRepository } from '@nestjs/typeorm'; @@ -17,29 +22,44 @@ export class TokenCleanupService implements OnApplicationBootstrap { @InjectRepository(PasswordReset) private readonly passwordResetRepository: Repository, private readonly configService: ConfigService, - private readonly schedulerRegistry: SchedulerRegistry, + // Optional: ScheduleModule is intentionally excluded in test environments. + // @Optional() allows the service to be instantiated without it and + // onApplicationBootstrap() guards against a missing registry. + @Optional() private readonly schedulerRegistry?: SchedulerRegistry, ) {} onApplicationBootstrap(): void { - // Skip cron registration in test environments. JEST_WORKER_ID is set by - // Jest's worker processes even when NODE_ENV is not explicitly 'test'. if ( this.configService.get('NODE_ENV') === 'test' || - process.env['JEST_WORKER_ID'] !== undefined + process.env['JEST_WORKER_ID'] !== undefined || + !this.schedulerRegistry ) { return; } - // Read the cron expression here via ConfigService so that .env values - // loaded by ConfigModule.forRoot() are available — unlike @Cron() which - // evaluates at module-load time before dotenv runs. - const cronExpression = - this.configService.get('REFRESH_TOKEN_CLEANUP_CRON') ?? - '0 3 * * *'; + // Read via ConfigService so .env values loaded by ConfigModule.forRoot() + // are honoured — unlike @Cron() which evaluates before dotenv runs. + // Use || so a blank/whitespace-only value is treated the same as unset. + const rawExpression = this.configService + .get('REFRESH_TOKEN_CLEANUP_CRON') + ?.trim(); + const cronExpression = rawExpression || '0 3 * * *'; + + let job: CronJob; + try { + job = new CronJob(cronExpression, () => { + void this.cleanupExpiredTokens(); + }); + } catch { + this.logger.warn( + `Invalid REFRESH_TOKEN_CLEANUP_CRON value "${cronExpression}", ` + + `falling back to default "0 3 * * *"`, + ); + job = new CronJob('0 3 * * *', () => { + void this.cleanupExpiredTokens(); + }); + } - const job = new CronJob(cronExpression, () => { - void this.cleanupExpiredTokens(); - }); this.schedulerRegistry.addCronJob('tokenCleanup', job); job.start(); this.logger.log(`Token cleanup cron registered: ${cronExpression}`); From af8662b8a4fc68540b1010246b1c1dd41e1921ae Mon Sep 17 00:00:00 2001 From: Demian Date: Wed, 15 Apr 2026 19:33:11 -0400 Subject: [PATCH 6/9] fix: clarify partial index comments in token cleanup migration The previous comments said the WHERE clause 'mirrors cleanup query', which implied full coverage of the OR condition. Each partial index only covers one predicate (revoked = true / used = true); the range index on expiresAt handles the other side. Updated comments to describe what each index actually covers. --- .../migrations/1765038000000-AddTokenCleanupIndexes.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts b/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts index 4d17f96..e07485e 100644 --- a/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts +++ b/backend/src/migrations/1765038000000-AddTokenCleanupIndexes.ts @@ -14,7 +14,9 @@ export class AddTokenCleanupIndexes1765038000000 implements MigrationInterface { name = 'AddTokenCleanupIndexes1765038000000'; public async up(queryRunner: QueryRunner): Promise { - // Partial index on revoked refresh tokens (WHERE clause mirrors cleanup query) + // Partial index on revoked refresh tokens — covers the `revoked = true` + // predicate of the cleanup DELETE's OR condition, keeping the index small + // by only including rows that will eventually be deleted. await queryRunner.query(` CREATE INDEX IF NOT EXISTS "IDX_refresh_tokens_revoked" ON "refresh_tokens" ("revoked") @@ -27,7 +29,9 @@ export class AddTokenCleanupIndexes1765038000000 implements MigrationInterface { ON "refresh_tokens" ("expiresAt") `); - // Partial index on used password resets (WHERE clause mirrors cleanup query) + // Partial index on used password resets — covers the `used = true` + // predicate of the cleanup DELETE's OR condition, keeping the index small + // by only including rows that will eventually be deleted. await queryRunner.query(` CREATE INDEX IF NOT EXISTS "IDX_password_resets_used" ON "password_resets" ("used") From 778a96b7c9bf4ff94ac679e204525b3f8c371e7c Mon Sep 17 00:00:00 2001 From: Demian Date: Wed, 15 Apr 2026 22:26:38 -0400 Subject: [PATCH 7/9] chore: bump Dockerfile base image to node:20-slim Node 18 reached EOL; CI already runs on Node 20. Switching to node:20-slim (Debian/glibc) keeps the runtime in sync with CI and avoids the musl libc issues that alpine would introduce for bcrypt's native prebuilds. --- backend/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/Dockerfile b/backend/Dockerfile index 10d6263..23dda20 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -1,4 +1,4 @@ -FROM node:18-slim +FROM node:20-slim # Create app directory WORKDIR /usr/src/app From af5077a97af381252e399a699980ea4212297ad1 Mon Sep 17 00:00:00 2001 From: Demian Date: Wed, 15 Apr 2026 22:48:29 -0400 Subject: [PATCH 8/9] fix: harden TokenCleanupService and its spec Service: - Separate the schedulerRegistry guard from the test/JEST_WORKER_ID guard so a missing registry in a non-test environment emits a warn log rather than silently no-oping; production misconfiguration is now visible in logs - Split the single try/catch around both DELETE queries into two independent blocks so a failure in the refresh token cleanup does not prevent the password reset cleanup from running; each table logs its own error Spec: - Extract restoreWorker() helper that calls delete instead of assignment when the original JEST_WORKER_ID value was undefined; assigning undefined to process.env coerces it to the string 'undefined', which would leave the guard permanently set and contaminate later tests - Add test: 'should still clean up password resets when refresh token cleanup fails' to assert the new per-table resilience --- .../auth/token-cleanup.service.spec.ts | 44 +++++++++++++---- .../src/modules/auth/token-cleanup.service.ts | 47 +++++++++++++------ 2 files changed, 69 insertions(+), 22 deletions(-) diff --git a/backend/src/modules/auth/token-cleanup.service.spec.ts b/backend/src/modules/auth/token-cleanup.service.spec.ts index 1515f0e..a315c3c 100644 --- a/backend/src/modules/auth/token-cleanup.service.spec.ts +++ b/backend/src/modules/auth/token-cleanup.service.spec.ts @@ -6,6 +6,18 @@ import { PasswordReset } from './password-reset.entity'; import { SchedulerRegistry } from '@nestjs/schedule'; import { ConfigService } from '@nestjs/config'; +// Helper that mirrors the safe JEST_WORKER_ID restore pattern used throughout +// this file: deletes the variable when the original value was undefined rather +// than assigning the string 'undefined' (Node coerces undefined to 'undefined' +// in process.env, which would leave the guard permanently set). +function restoreWorker(original: string | undefined): void { + if (original === undefined) { + delete process.env['JEST_WORKER_ID']; + } else { + process.env['JEST_WORKER_ID'] = original; + } +} + describe('TokenCleanupService', () => { let service: TokenCleanupService; @@ -77,7 +89,7 @@ describe('TokenCleanupService', () => { service.onApplicationBootstrap(); expect(mockSchedulerRegistry.addCronJob).not.toHaveBeenCalled(); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); it('should not register cron when schedulerRegistry is absent', () => { @@ -98,7 +110,7 @@ describe('TokenCleanupService', () => { serviceWithoutRegistry.onApplicationBootstrap(); expect(mockSchedulerRegistry.addCronJob).not.toHaveBeenCalled(); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); it('should register cron with default expression in non-test env', () => { @@ -115,7 +127,7 @@ describe('TokenCleanupService', () => { 'tokenCleanup', expect.any(Object), ); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); it('should register cron with custom expression from config', () => { @@ -133,7 +145,7 @@ describe('TokenCleanupService', () => { 'tokenCleanup', expect.any(Object), ); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); it('should fall back to default cron expression when config value is invalid', () => { @@ -151,7 +163,7 @@ describe('TokenCleanupService', () => { 'tokenCleanup', expect.any(Object), ); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); it('should treat blank REFRESH_TOKEN_CLEANUP_CRON as unset and use default', () => { @@ -169,7 +181,7 @@ describe('TokenCleanupService', () => { 'tokenCleanup', expect.any(Object), ); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); }); @@ -205,7 +217,7 @@ describe('TokenCleanupService', () => { expect( mockPasswordResetRepository.createQueryBuilder, ).not.toHaveBeenCalled(); - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); describe('in a non-test environment', () => { @@ -223,7 +235,7 @@ describe('TokenCleanupService', () => { }); afterEach(() => { - process.env['JEST_WORKER_ID'] = originalWorker; + restoreWorker(originalWorker); }); it('should delete revoked and expired refresh tokens', async () => { @@ -256,6 +268,22 @@ describe('TokenCleanupService', () => { await expect(service.cleanupExpiredTokens()).resolves.toBeUndefined(); }); + + it('should still clean up password resets when refresh token cleanup fails', async () => { + mockQueryBuilder.execute.mockReset(); + mockQueryBuilder.execute + .mockRejectedValueOnce(new Error('refresh token DB failure')) + .mockResolvedValueOnce({ affected: 2 }); + + await expect(service.cleanupExpiredTokens()).resolves.toBeUndefined(); + // Both query builders should have been invoked despite the first failure + expect( + mockRefreshTokenRepository.createQueryBuilder, + ).toHaveBeenCalled(); + expect( + mockPasswordResetRepository.createQueryBuilder, + ).toHaveBeenCalled(); + }); }); }); }); diff --git a/backend/src/modules/auth/token-cleanup.service.ts b/backend/src/modules/auth/token-cleanup.service.ts index c010f30..882d9ff 100644 --- a/backend/src/modules/auth/token-cleanup.service.ts +++ b/backend/src/modules/auth/token-cleanup.service.ts @@ -31,12 +31,19 @@ export class TokenCleanupService implements OnApplicationBootstrap { onApplicationBootstrap(): void { if ( this.configService.get('NODE_ENV') === 'test' || - process.env['JEST_WORKER_ID'] !== undefined || - !this.schedulerRegistry + process.env['JEST_WORKER_ID'] !== undefined ) { return; } + if (!this.schedulerRegistry) { + this.logger.warn( + 'SchedulerRegistry is not available — token cleanup cron will not run. ' + + 'Ensure ScheduleModule is imported in AppModule.', + ); + return; + } + // Read via ConfigService so .env values loaded by ConfigModule.forRoot() // are honoured — unlike @Cron() which evaluates before dotenv runs. // Use || so a blank/whitespace-only value is treated the same as unset. @@ -78,11 +85,13 @@ export class TokenCleanupService implements OnApplicationBootstrap { const start = Date.now(); this.logger.log('Starting expired/revoked token cleanup'); + const now = new Date(); + // Each table is cleaned independently so a failure in one does not prevent + // the other from running — both errors are logged separately. + let refreshDeleted = 0; try { - const now = new Date(); - - const { affected: refreshDeleted } = await this.refreshTokenRepository + const { affected } = await this.refreshTokenRepository .createQueryBuilder() .delete() .where('revoked = :revoked OR "expiresAt" < :now', { @@ -90,24 +99,34 @@ export class TokenCleanupService implements OnApplicationBootstrap { now, }) .execute(); + refreshDeleted = affected ?? 0; + } catch (error) { + this.logger.error( + 'Refresh token cleanup failed', + error instanceof Error ? error.stack : String(error), + ); + } - const { affected: resetDeleted } = await this.passwordResetRepository + let resetDeleted = 0; + try { + const { affected } = await this.passwordResetRepository .createQueryBuilder() .delete() .where('used = :used OR "expiresAt" < :now', { used: true, now }) .execute(); - - const duration = Date.now() - start; - this.logger.log( - `Token cleanup complete in ${duration}ms — ` + - `deleted ${refreshDeleted ?? 0} refresh token(s), ` + - `${resetDeleted ?? 0} password reset(s)`, - ); + resetDeleted = affected ?? 0; } catch (error) { this.logger.error( - 'Token cleanup job failed', + 'Password reset cleanup failed', error instanceof Error ? error.stack : String(error), ); } + + const duration = Date.now() - start; + this.logger.log( + `Token cleanup complete in ${duration}ms — ` + + `deleted ${refreshDeleted} refresh token(s), ` + + `${resetDeleted} password reset(s)`, + ); } } From 58cb583b8646ccfee3f1cb38a77d9f290602d91e Mon Sep 17 00:00:00 2001 From: Demian Date: Wed, 15 Apr 2026 23:05:53 -0400 Subject: [PATCH 9/9] fix: clarify onApplicationBootstrap comment and deduplicate default cron - Replace the inaccurate comment ('evaluates before dotenv runs') with the actual constraint: @Cron() decorator arguments are evaluated at class- definition time before DI runs, so ConfigService cannot be used in them - Define DEFAULT_CRON = '0 3 * * *' once and reference it throughout, eliminating the duplicated string that could diverge on future edits --- .../src/modules/auth/token-cleanup.service.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/backend/src/modules/auth/token-cleanup.service.ts b/backend/src/modules/auth/token-cleanup.service.ts index 882d9ff..126deef 100644 --- a/backend/src/modules/auth/token-cleanup.service.ts +++ b/backend/src/modules/auth/token-cleanup.service.ts @@ -44,15 +44,17 @@ export class TokenCleanupService implements OnApplicationBootstrap { return; } - // Read via ConfigService so .env values loaded by ConfigModule.forRoot() - // are honoured — unlike @Cron() which evaluates before dotenv runs. + // Use ConfigService rather than @Cron() because decorator arguments are + // evaluated at class-definition time (before DI runs), so there is no way + // to inject ConfigService into a @Cron() expression. Reading from + // ConfigService here gives us the fully-loaded, validated config value. // Use || so a blank/whitespace-only value is treated the same as unset. + const DEFAULT_CRON = '0 3 * * *'; const rawExpression = this.configService .get('REFRESH_TOKEN_CLEANUP_CRON') ?.trim(); - const cronExpression = rawExpression || '0 3 * * *'; + const cronExpression = rawExpression || DEFAULT_CRON; - const defaultExpression = '0 3 * * *'; let effectiveExpression = cronExpression; let job: CronJob; try { @@ -62,10 +64,10 @@ export class TokenCleanupService implements OnApplicationBootstrap { } catch { this.logger.warn( `Invalid REFRESH_TOKEN_CLEANUP_CRON value "${cronExpression}", ` + - `falling back to default "${defaultExpression}"`, + `falling back to default "${DEFAULT_CRON}"`, ); - effectiveExpression = defaultExpression; - job = new CronJob(defaultExpression, () => { + effectiveExpression = DEFAULT_CRON; + job = new CronJob(DEFAULT_CRON, () => { void this.cleanupExpiredTokens(); }); }