All documentation

Reviews & Audits

Module 18 — Security-Critical Test Hardening

Task #527 (Phase 25). The original 16-category checklist this task's title refers to predates the context window this work resumed in and isn't preserved verbatim anywhere in the repo (a casualty of this session's own compaction, not something silently dropped). Rather than guess at wording, this phase reconstructs 16 concrete, security-critical categories directly from what Module 18's own Phases 1-24 actually touched, checks each for real existing test coverage (Grep across every *.spec.ts in apps/api/src, not assumption), and adds targeted new tests only where that check found a genuine, previously-untested gap — consistent with this module's "disclose, don't fabricate" discipline applied to test coverage specifically.

Coverage checklist

  1. JWT authentication (JwtAuthGuard + auth use-cases) — EXISTS. 19 spec files under modules/auth/use-cases/ already cover login/MFA/refresh/OAuth/session lifecycle.
  2. API key authentication (ApiKeyAuthGuard) — GAP, FIXED THIS PHASE. This guard gates every Public API route (hash lookup, revocation, expiry, IP allowlist, rate limit) and had no dedicated test. Added guards/api-key-auth.guard.spec.ts — 11 cases covering the malformed-header/revoked/expired/IP-allowlist/rate-limit/success paths.
  3. Authorization / RBAC / IDOR — EXISTS. agent-authorization.util.spec.ts, ai-authorization.util.spec.ts, collaboration/permissions.util.spec.ts, and the extensive workspace-scoping checks already built into command/query specs throughout (e.g. findByIdForWorkspace patterns).
  4. Multi-tenant / workspace isolation — EXISTS. Covered by the M17 security review's own dedicated isolation tests (#488, #496) and realtime-hub.service.spec.ts's workspace-scoped broadcast checks.
  5. SSRF protection (ssrf-guard.util.ts) — EXISTS. common/utils/ssrf-guard.util.spec.ts.
  6. AI prompt injection (prompt-injection-guard.service.ts) — EXISTS. modules/ai/security/prompt-injection-guard.service.spec.ts, built specifically for Phase 13.
  7. File/upload security — EXISTS. modules/attachments/commands/upload-attachment.command.spec.ts.
  8. Public sharing (ShareLink token issuance/access) — EXISTS. modules/sharing/share-permission.util.spec.ts and commands/access-share-link.command.spec.ts.
  9. Extension/plugin permission enforcement — EXISTS. extension-permission-enforcer.util.spec.ts, plugin-signature.util.spec.ts, extension-invoke.commands.spec.ts.
  10. Rate limiting / abuse protection — PARTIAL GAP, FIXED THIS PHASE. The global ThrottlerGuard (NestJS's own, configured in app.module.ts) is third-party framework code this module reasonably doesn't re-test. The codebase's one hand-rolled rate limiter — ApiKeyAuthGuard's per-key sliding window — had no test; now covered by finding #2's new spec (its rate-limit test cases).
  11. Job queue idempotency / claim-race safety — GAP, FIXED THIS PHASE. ClaimNextJobsHandler's atomic conditional-updateMany claim (the mechanism that stops two workers from ever executing the same job) had no dedicated test despite being exactly the kind of logic where a subtle bug is a silent double-execution, not a loud crash. Added modules/distributed-jobs/commands/distributed-jobs.commands.spec.ts — 5 cases covering offline-worker/zero-capacity short-circuits, a race-loser being correctly skipped rather than double-counted, the claimed count never exceeding available concurrency, and currentLoad only advancing by jobs actually claimed.
  12. Audit log integrity — EXISTS structurally as of Phase 17 (#519); AuditService itself (modules/audit/audit.service.ts) is a two- method pass-through to AuditRepository.record/findRecentByUserId with no branching logic of its own — a test would only assert that a mock was called with its own arguments, which is not a meaningful regression test. Not added here; the actual integrity-relevant surface (every call site that decides what gets logged) is exercised indirectly by each of those call sites' own specs.
  13. Secrets / crypto utilities — EXISTS. notification-channel-config-crypto.util.spec.ts, oauth-exchange.util.spec.ts, aws-sigv4.util.spec.ts, plugin-signature.util.spec.ts.
  14. Session / MFA / account lifecycle — EXISTS. Covered by the same 19 auth use-case specs as #1 (mfa-setup, mfa-disable, mfa-challenge, mfa-confirm, list-sessions, revoke-session, delete-account).
  15. SSE / real-time auth & isolation — EXISTS. modules/realtime/realtime-hub.service.spec.ts and realtime-publish.handler.spec.ts, extended by Phase 15's own reconnect/cleanup work.
  16. Client-surface reliability (SDK/CLI/Desktop/Mobile) — covered by Phase 24 (task #526, docs/reviews/0016-m18-cli-desktop-mobile-sdk-reliability-audit.md) — not duplicated here; that phase's two mechanical fixes (SDK timeout/retry, CLI update-check timeout) are reliability, not security, but the CLI token-refresh gap it disclosed has a security-adjacent angle (stale-credential UX) already recorded there.

What this environment could not verify

Per this module's standing discipline (mcp__workspace__bash denied this session, confirmed repeatedly since Phase 22): the two new spec files added in this phase were written to match this codebase's established Jest/hand-rolled-mock conventions exactly (cross-checked field-for-field against the real ClaimNextJobsHandler, ApiKeyAuthGuard, isIpAllowlisted, and ApiKeysRepository/ PrismaService shapes they exercise), but could not actually be run in this sandbox. This is disclosed explicitly rather than claimed as a passing test run — actual execution is deferred to Phase 30's verification loop attempt, or to a real CI run once this repository is pushed.

Verdict

Of 16 reconstructed categories, 13 already had solid existing coverage (verified by direct Grep, not assumed), 2 had genuine gaps that were mechanically fixable and got new tests this phase (API key guard, distributed job claim race), and 1 (audit log integrity) was assessed and deliberately not given a low-value pass-through test. No category was found completely uncovered.