notification-service: stop cross-user overwrite of notification preferences (IDOR) - #1559
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
… X-User-ID The handler took the target userId from the request body, so any caller could overwrite another user's notification preferences (IDOR, CWE-639). Use the gateway-supplied X-User-ID identity instead; reject a mismatching body userId with 403 and a missing identity with 401.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| val userId = call.request.headers["X-User-ID"] | ||
| if (userId.isNullOrBlank()) { | ||
| call.respond(HttpStatusCode.Unauthorized, ErrorResponse("X-User-ID header is required")) | ||
| return@put | ||
| } |
There was a problem hiding this comment.
Acknowledged, but this is the platform's existing trust model rather than something this PR introduces: every identity-scoped route in notification-service (GET /notifications, /unread-count, /read-all, GET /preferences) and the other services already trust the gateway-injected X-User-ID, and the gateway (api-gateway/internal/proxy/router.go) overwrites that header from the validated JWT so a client cannot forge it through the supported entry point. The finding being fixed here (sfind-229db8f8) is specifically that this one write handler ignored that identity and trusted the request body instead, which was exploitable even through the gateway.
Hardening the per-service ingress (network policy / JWT validation inside each service) is a separate, cross-service change and out of scope for this fix. Leaving this thread open for the reviewer to decide.
Summary
Fixes code-scan finding
sfind-229db8f8bd194120803af3c1f27811d6.Vulnerability: Insecure Direct Object Reference / missing authorization (CWE-639, CWE-862). Severity: medium.
Trust boundary: any authenticated user behind the API gateway (or anyone reaching the notification-service ingress directly) vs. every other user's notification-delivery settings.
Before:
PUT /api/v1/preferencesinRoutes.ktusedrequest.userIdfrom the JSON body as the target user. The gateway injects the caller's identity asX-User-ID(seeapi-gateway/internal/proxy/router.go), but this handler never read it, so{"userId":"<victim>","eventType":"file_shared","channels":[]}silently disabled a victim's channels.After: the handler uses the gateway identity and treats the body field as an optional consistency check only:
put { val userId = call.request.headers["X-User-ID"] // 401 if missing val request = call.receive<NotificationPreferenceRequest>() if (request.userId != null && request.userId != userId) // 403 on mismatch ... notificationService.updatePreferences(userId = userId, ...) // never the body value }NotificationPreferenceRequest.userIdbecomesString? = nullso clients that omit it keep working; clients that send their own id keep working; a body id for anyone else is rejected. Unlike the GET routes, the write path does not fall back to auser_idquery parameter, since that would just be another caller-chosen id.Regression test:
PreferencesRouteTest(KtortestApplication+ mockedNotificationService). Three of its four cases fail against the old handler (body id used, no 403, no 401) and pass with this change. Full./gradlew testfor notification-service is green.Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/c735d78cd3ed402d834aebfb72f052f2
Open in Devin Desktop: https://partner-workshops.devinenterprise.com/desktop/session/c735d78cd3ed402d834aebfb72f052f2?variant=devin
Requested by: @mbatchelor81