dev · 4 files changed, 5 deletions · 9 annotated findings| 3 | import { ForbiddenException, UnauthorizedException } from "@/errors/error"; | |
| 4 | − | import { userRoleService } from "@/modules/access-control/role/service/user-role.service"; |
| 5 | import { jwtService } from "@/modules/jwt/service/jwt.service"; |
appliedinfo Imported but never referenced. Removing it cuts an unnecessary module-load edge in the auth hot path (this middleware runs on every authenticated request).
| 1 | − | import prisma from "@/config/prisma"; |
| 2 | − | |
| 3 | class AccessControlRepository { | |
| 4 | // Common access control methods if any |
appliedwarning Import removed, but the underlying smell remains: this is an empty stub class exported as a singleton. Either implement it or delete the file — empty placeholders are a maintainability tax reviewers notice.
| 1 | import prisma from "@/config/prisma"; | |
| 2 | − | import type { Prisma } from "@prisma/client"; |
| 3 |
appliedinfo Type-only import never used. Zero runtime impact; pure clarity win.
| 1 | − | import { ConflictException } from "@/errors/error"; |
| 2 | import { roleRepository } from "../repository/role.repository"; |
appliedwarning ConflictException was imported but never thrown — a hint that role create/update lacks duplicate-name conflict handling. Removing the import is correct now; the missing validation is logged as a follow-up.
| 12 | serializers: { | |
| 13 | req(req) { | |
| 20 | − | body: req.body, |
| 20 | + | // body removed from logs; see redact config below |
| + | + | // in pino(): redact:{ paths:["req.body.password","req.body.token","req.headers.authorization"], remove:true } |
critical Plaintext credential leakage (OWASP A09 / A02). The request serializer logs the entire req.body on every request — including POST /auth/login, /auth/register, and /auth/reset-password. Every login password is written to logs in cleartext. If logs ship anywhere (CloudWatch, files, a 3rd-party aggregator) you have a credential breach. Fix: remove body from the serializer or configure pino redact for sensitive paths.
| 11 | − | baseLogger.info(`Authenticating Leonardo webhook with headers: ${JSON.stringify(req.headers)}`); |
| .. | const token = receivedAuthorization?.startsWith("Bearer ") ? ... : ...; | |
| 23 | − | baseLogger.info(`Received Leonardo webhook bearer token: ${token}`); |
critical Secret-in-logs (OWASP A09). The webhook handler logs the full header set and the bearer token in cleartext on every call. Anyone with log read access can replay the webhook. The timingSafeEqual comparison further down is good practice — but it's undermined by logging the secret it's protecting. Fix: delete both log lines (or log only a SHA-256 prefix at debug level).
| 16 | − | if (signature !== configService.HOOK_SECRET_KEY) { // non-constant-time |
| 16 | + | if (!safeEqual(signature, configService.HOOK_SECRET_KEY)) { // crypto.timingSafeEqual |
| · | − | HOOK_SECRET_KEY: z.string().default('default_hook_secret_key') |
| · | + | HOOK_SECRET_KEY: z.string().min(32) // required, no public default |
warning Weak webhook auth (OWASP A07). Two issues: (1) the secret falls back to a hard-coded public default 'default_hook_secret_key' — if prod forgets to set it, the HLS callback is forgeable by anyone reading this repo; (2) the comparison is !== (non-constant-time, timing-leakable), inconsistent with the Leonardo hook which correctly uses timingSafeEqual. Fix: require the env var (no default) and use a constant-time compare. Better: HMAC the payload, not a static shared secret.
| 41 | − | app.use('/api/v1', apiLimiter, router); // single global 100/15min, in-memory |
| 41 | + | app.use('/api/v1/auth', authLimiter); // strict: e.g. 10/15min/IP, Redis-backed |
| 42 | + | app.use('/api/v1', apiLimiter, router); // store: new RedisStore({ client }) |
warning Brute-force exposure + broken under horizontal scale (OWASP A07). The limiter is express-rate-limit with the default in-memory store and one global rule for all routes. Two problems: (1) no stricter limit on /auth/*, so login/credential-stuffing only hits the generic 100/15min; (2) in-memory state is per-process and resets on restart — your compose runs api + worker and is meant to scale, so the limit is silently multiplied and inconsistent. Fix: Redis store + a dedicated tight auth limiter.
| 7 | fileFilter: (req, file, cb) => { | |
| 8 | − | if (!file.mimetype.startsWith('image/') && !file.mimetype.startsWith('video/')) |
| 8 | + | // mimetype is client-controlled — validate magic bytes after buffer load, |
| + | + | // enforce an extension allowlist, and rely on sharp/ffmpeg re-encode to reject fakes |
warning Upload content-type spoofing (OWASP A04/A08). Validation trusts the client-supplied file.mimetype, which is forgeable. A 20MB cap and downstream sharp re-encode mitigate image abuse, but videos pass to ffmpeg with no magic-byte check. Fix: verify real content type from the buffer (e.g. file-type), enforce an extension allowlist, and serve R2 objects from a non-executable path.
| · | @@index([userId, createdAt]) | |
| · | @@index([createdAt]) | |
| + | + | @@index([isDeleted, isHidden, visibility, createdAt, id]) // covers every feed query |
warning Missing composite index = table scans (performance, High). Every feed/timeline query filters isDeleted=false AND isHidden=false AND visibility=PUBLIC ordered by createdAt DESC, id DESC, but no single index covers that predicate+sort. At scale MySQL filters then filesorts. Fix: add the composite above and confirm with EXPLAIN before/after (great resume artifact).
| 136 | − | await followRepository.updateStatusByFollowId(existingFollow.id, false); |
| 137 | − | await userRepository.decrementFollowersCount(targetUserId); // separate tx |
| 136 | + | await transactionService.doInTransaction(async (tx) => { /* both writes atomic */ }); |
warning Denormalized counter drift (data integrity). Follow-state update and the followersCount increment/decrement run as two independent DB calls. A failure between them permanently desyncs the cached count. Post creation already uses doInTransaction correctly — apply the same here. Fix: wrap state-change + counter in one transaction.
| 4 | − | JWT_SECRET: z.string().min(1) |
| 4 | + | JWT_SECRET: z.string().min(32) |
| · | − | jwt.verify(token, privateKey, (err, decoded) => {...}) |
| · | + | jwt.verify(token, privateKey, { algorithms: ['HS256'] }, (err, decoded) => {...}) |
info JWT hardening (OWASP A02), Low. Secret length is only floor-checked at 1 char (allows a trivially brute-forceable secret), and verify doesn't pin the algorithm. Pinning algorithms:['HS256'] is defense-in-depth against algorithm-confusion. Fix: raise the min length and pass the algorithm allowlist explicitly.
good SQL injection — not present. Every $queryRaw/$executeRaw uses tagged templates or Prisma.sql with bound parameters (verified in post, statistics, user, topic, quest repos). No string concatenation into SQL.
good Error handler returns generic messages in production and only attaches stack traces when NODE_ENV==='development'. Prisma error codes mapped to clean HTTP statuses. No info leak.
good Secrets hygiene: .env / .env.prod are gitignored and untracked. helmet, compression, and a 1mb JSON body cap are wired in app.ts. Refresh tokens hashed in DB; access tokens blacklisted on logout.