HIPCTF2/.kilo/plans/reviewer-followups-820-part2.md

Implementation Plan: Reviewer Follow-ups (Part 2)

0. Scope

Six discrete hardenings of the scaffold already on the branch:

1. Commit the actual scaffold to the branch (the working-tree only had README.md deleted in the previous commit).
2. Make main.ts explicitly await DB initialization (migrations + seeds) before app.listen().
3. The Angular authInterceptor should call auth.getAccessToken() (it already does — confirm wiring & ensure clones set Authorization: Bearer; also make sure it's registered BEFORE csrfInterceptor in the providers list).
4. Add Zod + class-validator DTOs for every admin body/path/query input, with @Body(new ZodValidationPipe(...)) etc.
5. Implement real upload endpoints using a configured per-route/global Multer fileSize limit and a safe filename strategy.
6. ThemeLoaderService must REQUIRE and validate the ten theme files at startup; validate the configured theme key; fall back with a warning if missing/invalid.

1. Architectural Reconnaissance

• Current state: all scaffold files are present in the working tree and staged; only README.md was deleted in the prior commit. git status shows the planned commit is already a no-op on tracked paths except the working-tree edits.
• Stack: NestJS 10 + TypeORM/better-sqlite3 backend, Angular 17 standalone frontend, Jest + supertest tests in /repo/tests/, single command npm test.
• Key existing modules: DatabaseModule (TypeORM async + migrationsRun: true), ThemeLoaderService (currently warn-and-fallback on missing themes), CommonModule (exports CsrfMiddleware), AuthModule, UsersModule, AdminModule, SystemModule, FrontendModule.
• Tests live in: /repo/tests/backend/ and /repo/tests/frontend/, runnable with npm test.
• Persistent data: /data/hipctf/db.sqlite and /data/hipctf/uploads/.

2. Impacted Files

To Create

• backend/src/modules/admin/dto/create-user.dto.ts (Zod schema)
• backend/src/modules/admin/dto/update-user-role.dto.ts
• backend/src/modules/admin/dto/list-users.query.dto.ts
• backend/src/modules/admin/dto/user-id.param.dto.ts
• backend/src/modules/uploads/uploads.module.ts
• backend/src/modules/uploads/uploads.controller.ts
• backend/src/modules/uploads/uploads.service.ts
• backend/src/modules/uploads/dto/upload-category-image.dto.ts
• tests/backend/admin-validation.spec.ts
• tests/backend/uploads.spec.ts
• tests/backend/theme-required.spec.ts

To Modify

• .gitignore — keep /data/ and node_modules/ ignored; keep the prior diff already staged.
• backend/src/main.ts — await dataSource.initialize() (or a dedicated DatabaseInitService.init()) BEFORE app.listen(); ensure SPA static and helmet/CORS ordering unchanged.
• backend/src/database/database.module.ts — implement OnApplicationBootstrap that awaits DataSource.initialize() and logs; export DatabaseInitService so main.ts can await it.
• backend/src/common/utils/theme-loader.service.ts — make the service REQUIRE all 10 themes; at the end of loadAll() compare loaded ids vs THEME_IDS; if any are missing OR if the configured themeKey setting does not match a loaded id, log a clear warning and fall back to default.
• backend/src/modules/admin/admin.controller.ts — replace untyped @Body()/@Param() with @Body(new ZodValidationPipe(...))/@Param(new ZodValidationPipe(...))/query DTOs; add class-validator metadata via @ApiProperty for OpenAPI.
• backend/src/common/utils/upload.ts — add a createMulterMiddleware(config, opts) factory returning a configured multer instance with limits.fileSize; add a safeFilename(original) helper that strips path traversal, lowercases, restricts charset, and deduplicates.
• frontend/src/main.ts — verify the providers order: [csrfInterceptor, authInterceptor]; ensure AuthService.getAccessToken() is wired and the clone sets Authorization: Bearer.

3. Proposed Changes

3.1 Commit the scaffold

The working tree is already correctly staged. Steps:

1. git add -A to capture any untracked files (.gitignore already modified).
2. git commit -m "Scaffold HIPCTF platform (NestJS + Angular)" on the current branch feature-820-1784637300954. (No amend; no force-push.)
3. Verify the new commit includes all scaffold files: git show --stat HEAD lists 50+ files across backend/, frontend/, tests/.

3.2 Await DB init in main.ts

1. Add DatabaseInitService exporting init(): Promise<void> that:
   • Resolves the DataSource token from app.get(DataSource).
   • Calls dataSource.initialize() (idempotent — TypeORM returns the existing connection if already initialized).
   • Awaits dataSource.runMigrations({ transaction: 'each' }) (TypeORM runs them in order; both our migrations are idempotent).
   • Logs the migration count and seed result.
2. Expose via DatabaseModule exports so main.ts can app.get(DatabaseInitService).init().
3. main.ts flow:

   const app = await NestFactory.create(AppModule);
   await app.get(DatabaseInitService).init();
   // ... existing helmet/cors/static/validation/swagger setup unchanged ...
   await app.listen(port, '0.0.0.0');
   

4. The existing migrationsRun: true flag in TypeOrmModule.forRootAsync already runs migrations automatically on DataSource.initialize(); the explicit init() step is for visibility, ordering guarantees, and logging. This satisfies "startup awaits database initialization/migrations/seeds before app.listen".
5. Test: add database-init.spec.ts that calls DatabaseInitService.init() against :memory: and asserts the schema + seeded categories/settings are present.

3.3 Angular auth interceptor (confirm and finalize)

1. frontend/src/main.ts registers providers in this order:

   provideHttpClient(withInterceptors([csrfInterceptor, authInterceptor]))
   

2. frontend/src/app/core/interceptors/auth.interceptor.ts already calls auth.getAccessToken() and clones with Authorization: Bearer ${token}. No code change needed — verify by a unit test:
   • tests/frontend/auth-interceptor.spec.ts uses provideHttpClientTesting() to capture outbound requests; sets a token on AuthService.setSession(...) and asserts the cloned request has the Authorization header.

3.4 Admin DTOs (Zod + class-validator)

1. Create backend/src/modules/admin/dto/ with:
   • create-user.dto.ts — Zod: { username: string.min(3).max(64), password: string.min(12), role: enum(['admin','player']) }.
   • update-user-role.dto.ts — Zod: { role: enum(['admin','player']) }.
   • user-id.param.dto.ts — Zod: { id: string.uuid() }.
   • list-users.query.dto.ts — Zod: { limit?: number.int().positive().max(200).default(50), cursor?: string.uuid().optional() }.
2. Apply via ZodValidationPipe on every handler. Also add @ApiProperty for Swagger metadata (the project already mixes Zod + Swagger; the Swagger annotations live alongside the DTOs).
3. Admin service: enforce password policy (delegate to validatePassword from common/utils/password-policy).
4. Tests: admin-validation.spec.ts — invalid body returns 400; missing path param returns 400; bad role returns 400.

3.5 Upload endpoints with safe filenames

1. backend/src/common/utils/upload.ts additions:

   export function safeFilename(original: string): string {
     // strip directory, lowercase, replace [^a-z0-9._-], collapse, dedupe
     const base = path.basename(original || 'file');
     const cleaned = base.toLowerCase()
       .replace(/[^a-z0-9._-]+/g, '-')
       .replace(/^-+|-+$/g, '')
       .slice(0, 120) || 'file';
     const ext = path.extname(cleaned);
     const stem = ext ? cleaned.slice(0, -ext.length) : cleaned;
     return `${stem}-${randomUUID().slice(0, 8)}${ext}`;
   }
   export function buildMulter(config: ConfigService) {
     const limits = buildUploadLimits(config);
     return multer({ storage: multer.diskStorage({
       destination: path.resolve(config.get('UPLOAD_DIR', '/data/hipctf/uploads')),
       filename: (_req, file, cb) => cb(null, safeFilename(file.originalname)),
     }), limits });
   }
   

2. New uploads.module/controller/service:
   • POST /api/v1/uploads/category-icon (admin-only via AdminGuard) — multer.single('file'), global fileSize limit applied via buildMulter(config). Returns { id, storedPath, originalFilename, size }.
   • POST /api/v1/uploads/challenge-file (admin-only) — same, but stored under <UPLOAD_DIR>/challenges/<id>/.
3. Serve via existing app.use('/uploads', express.static(uploadDir)) — already in main.ts. The static handler uses the SAME uploadDir, so files saved by multer are publicly served.
4. Tests: uploads.spec.ts — admin can upload a small file (200), rejects oversize with 413 (or 500 via Multer's LIMIT_FILE_SIZE error), rejects path-traversal filenames via safeFilename (unit-test the helper directly).

3.6 ThemeLoaderService — require + validate the 10 themes

1. After loadAll() populates this.themes:
   • Compute missing = THEME_IDS.filter(id => !this.themes.has(id)).
   • If missing.length > 0:
     • Log a warn listing missing ids.
     • For each missing id, copy the matching entry from BUILTIN_THEMES into this.themes so THEME_IDS.length === this.themes.size after init.
2. Read the configured theme key: await this.settings.get('themeKey', 'classic') (SettingsService is @Global() already).
3. If configuredKey is not in THEME_IDS OR not in this.themes, log a warning (Configured theme 'X' is unknown; falling back to 'classic') and set defaultThemeId = 'classic'.
4. Otherwise set defaultThemeId = configuredKey.
5. getTheme(id) behaviour unchanged but now guaranteed to find a theme (since all 10 are loaded).
6. Test: theme-required.spec.ts — theme-loader.spec.ts already covers warn-and-fallback; this new spec asserts that even with an empty THEMES_DIR, all 10 themes are available (because builtins are backfilled).

3.7 Tests added

• database-init.spec.ts — boots DatabaseInitService against :memory:, asserts schema present + 6 system categories + 8 settings rows.
• admin-validation.spec.ts — covers all four admin DTOs (create, update, param, query).
• uploads.spec.ts — admin upload happy-path + safeFilename unit + oversize rejection.
• theme-required.spec.ts — all 10 ids available even when THEMES_DIR points at empty dir; configured theme key fallback logs warn.
• auth-interceptor.spec.ts (frontend) — verifies Authorization: Bearer header is attached when AuthService.getAccessToken() returns a token.

4. Test Strategy

• New unit/integration specs listed above.
• No new heavyweight suites; all are pure-Jest with no extra containers.
• One small frontend test for the auth interceptor (jsdom + provideHttpClientTesting).
• All tests runnable with npm test from repo root.

5. Persistent /data Usage

No change to existing /data layout. Upload tests use UPLOAD_DIR=/tmp/... via env, mirroring the existing convention.

6. Definition of Done

• git log -1 --stat shows the new commit containing the full scaffold diff on the feature-820-1784637300954 branch.
• npm test passes (46 existing + new suites green).
• npm run build succeeds.
• Startup logging explicitly mentions migrations + seeds having completed before app.listen resolves.
• GET /api/v1/admin/users requires valid DTO inputs and returns 400 on bad UUIDs, bad roles, etc.
• POST /api/v1/uploads/category-icon saves files into /data/hipctf/uploads/ with safe filenames, rejecting oversize uploads.
• ThemeLoaderService.listThemes().length === 10 regardless of disk state; configured theme key validated.
• Manual E2E smoke: register admin → upload a 1kb icon → GET /uploads/<safe-name> returns the file.