🚨 ALL CI CHECKS MUST PASS
Your PR will not be reviewed or merged until every CI job is green. No exceptions.
Run these locally before you push:
npm run format:check # Formatting
npm run lint # Lint (--max-warnings=0)
npm run typecheck # TypeScript
npm test # Tests
npm run build # Build
npm run ci:app-boot # App boot (needs Postgres + Redis)
A red build is the single most common reason work stalls on this repo. If CI fails and you are stuck, say so in the PR — do not push a failing build and go quiet.
Also required: put Closes #<this issue number> in your PR description. Without it, GrantFox cannot link your PR to this issue.
What needs to be done
src/notifications/notifications.controller.ts exposes six routes with no guards at all and still hardcodes recipientId = 0 in three handlers. Give the module real authenticated user context and ownership enforcement.
Why it matters
This was reported before and closed as fixed. Issue #478 — "Notifications module has no authenticated user context — recipientId is hardcoded to 0" — was closed as completed via merged PR #483. The merge fixed one of four handlers.
Current state of the file:
| Line |
Handler |
Recipient source |
| 38 |
findAll() |
req.user.id ✅ fixed by #483 |
| 51 |
getUnreadCount() |
const recipientId = 0; ❌ |
| 80 |
create() |
const recipientId = 0; // This should come from authenticated context or request body ❌ |
| 104 |
markAllRead() |
const recipientId = 0; ❌ |
Three handlers still operate on user ID zero. markAllRead() marks notifications read for whatever user happens to own ID 0. getUnreadCount() reports a count that belongs to nobody. create() writes notifications attributed to a phantom account.
Worse, the controller has zero @UseGuards and there is no global authentication guard (only ThrottlerGuard is registered as APP_GUARD). So:
GET /notifications/:id (line 61) — takes an ID, returns that notification, no ownership check. Any caller reads any user's notifications by enumerating integer IDs. Textbook IDOR.PATCH /notifications/:id/read (line 88) — same, but mutates. Any caller marks any user's notification read.GET /notifications (line 30) reads req.user.id with no guard in front of it, so req.user is undefined for an unauthenticated request and the handler throws Cannot read properties of undefined.
Notifications carry order updates, dispute activity, and payment events — reading another user's stream leaks commercial and financial activity.
Technical context
src/notifications/notifications.controller.ts — all six routes; lines cited abovesrc/notifications/notifications-sse.controller.ts — the streaming endpoint; it has one @UseGuards, but verify the stream is scoped to the authenticated user and cannot be subscribed to on behalf of someone elsesrc/notifications/notifications.service.ts — findOne, markRead, create, markAllRead signatures likely need a recipient parameter so ownership is enforced in the service, not just the controllersrc/entities/ — the Notification entity and its recipientId relation
Enforce ownership in the service layer, scoped by recipient in the query (where: { id, recipientId }), rather than fetching and then comparing in the controller. A fetch-then-compare still lets a caller confirm an ID exists via timing or error differences.
Acceptance criteria
Out of scope
- Redesigning the notification delivery mechanism or adding channels
- The global authorization default — tracked separately; this issue must be safe to merge either before or after that one
Getting started
npm ci
grep -n "recipientId" src/notifications/notifications.controller.ts
npx jest src/notifications
Read PR #483 first to see what was already changed, so you extend that approach rather than introducing a second pattern.
What needs to be done
src/notifications/notifications.controller.tsexposes six routes with no guards at all and still hardcodesrecipientId = 0in three handlers. Give the module real authenticated user context and ownership enforcement.Why it matters
This was reported before and closed as fixed. Issue #478 — "Notifications module has no authenticated user context — recipientId is hardcoded to 0" — was closed as completed via merged PR #483. The merge fixed one of four handlers.
Current state of the file:
findAll()req.user.id✅ fixed by #483getUnreadCount()const recipientId = 0;❌create()const recipientId = 0; // This should come from authenticated context or request body❌markAllRead()const recipientId = 0;❌Three handlers still operate on user ID zero.
markAllRead()marks notifications read for whatever user happens to own ID 0.getUnreadCount()reports a count that belongs to nobody.create()writes notifications attributed to a phantom account.Worse, the controller has zero
@UseGuardsand there is no global authentication guard (onlyThrottlerGuardis registered asAPP_GUARD). So:GET /notifications/:id(line 61) — takes an ID, returns that notification, no ownership check. Any caller reads any user's notifications by enumerating integer IDs. Textbook IDOR.PATCH /notifications/:id/read(line 88) — same, but mutates. Any caller marks any user's notification read.GET /notifications(line 30) readsreq.user.idwith no guard in front of it, soreq.userisundefinedfor an unauthenticated request and the handler throwsCannot read properties of undefined.Notifications carry order updates, dispute activity, and payment events — reading another user's stream leaks commercial and financial activity.
Technical context
src/notifications/notifications.controller.ts— all six routes; lines cited abovesrc/notifications/notifications-sse.controller.ts— the streaming endpoint; it has one@UseGuards, but verify the stream is scoped to the authenticated user and cannot be subscribed to on behalf of someone elsesrc/notifications/notifications.service.ts—findOne,markRead,create,markAllReadsignatures likely need a recipient parameter so ownership is enforced in the service, not just the controllersrc/entities/— theNotificationentity and itsrecipientIdrelationEnforce ownership in the service layer, scoped by recipient in the query (
where: { id, recipientId }), rather than fetching and then comparing in the controller. A fetch-then-compare still lets a caller confirm an ID exists via timing or error differences.Acceptance criteria
const recipientId = 0;are gone; every handler derives the recipient from the authenticated userfindOneandmarkReadscope by recipient in the query, returning 404 — not 403 — for someone else's notification, so IDs are not enumerablecreate()cannot be used to write a notification attributed to another user; if internal services need that, it is a separate authenticated path with an explicit commentmarkAllReadaffects only the caller's rowsgrep -n "recipientId = 0" src/notificationsreturns nothingOut of scope
Getting started
npm ci grep -n "recipientId" src/notifications/notifications.controller.ts npx jest src/notificationsRead PR #483 first to see what was already changed, so you extend that approach rather than introducing a second pattern.