WAL journal mode + OpenAPI 3.1
- InitSchema migration now issues PRAGMA journal_mode = WAL after
PRAGMA foreign_keys = ON so fresh DBs are created in WAL mode.
- DatabaseInitService.init() also calls ensureWalMode() after
dataSource.initialize() so warm DBs (where migrations are skipped)
have WAL applied and persisted on every startup. Logs whether it
was already set or just enabled.
- main.ts now serves an OpenAPI 3.1 document at /api/docs and
/api/docs-json. A new toOpenApi31() utility converts the
NestJS-generated 3.0 document by setting openapi=3.1.0, normalising
the required info fields, and rewriting every legacy
'nullable: true' marker into a JSON Schema 2020-12 type union
('type: [<orig>, "null"]'). Recursion handles components, paths,
parameters, request bodies, response bodies, properties, items,
oneOf/anyOf/allOf, and parameter/request schemas.
- 7 new unit tests in tests/backend/openapi31.spec.ts lock the
conversion behaviour; total suite: 83 tests across 18 specs.
This commit is contained in:
OpenVelo Agent
@@ -1,152 +0,0 @@
|
||||
# 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:
|
||||
```ts
|
||||
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:
|
||||
```ts
|
||||
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:
|
||||
```ts
|
||||
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.
|
||||
Reference in New Issue
Block a user