Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ data class UnreadCountResponse(

@Serializable
data class NotificationPreferenceRequest(
val userId: String,
val userId: String? = null,
val eventType: String,
val channels: List<DeliveryChannel>,
)
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,20 @@ fun Application.configureRouting(prometheusRegistry: PrometheusMeterRegistry) {
}

put {
val userId = call.request.headers["X-User-ID"]
if (userId.isNullOrBlank()) {
call.respond(HttpStatusCode.Unauthorized, ErrorResponse("X-User-ID header is required"))
return@put
}
Comment on lines +155 to +159

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟥 Direct requests forge preference ownership

A direct client can set X-User-ID and overwrite that user's preferences. The public service ingress bypasses gateway JWT validation.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


val request = call.receive<NotificationPreferenceRequest>()
if (request.userId != null && request.userId != userId) {
call.respond(HttpStatusCode.Forbidden, ErrorResponse("Cannot update preferences for another user"))
return@put
}

notificationService.updatePreferences(
userId = request.userId,
userId = userId,
eventType = request.eventType,
channels = request.channels,
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package com.otterworks.notification.routes

import com.otterworks.notification.model.DeliveryChannel
import com.otterworks.notification.service.NotificationService
import com.otterworks.notification.websocket.WebSocketManager
import io.ktor.client.request.header
import io.ktor.client.request.put
import io.ktor.client.request.setBody
import io.ktor.http.ContentType
import io.ktor.http.HttpStatusCode
import io.ktor.http.contentType
import io.ktor.serialization.kotlinx.json.json
import io.ktor.server.application.install
import io.ktor.server.plugins.contentnegotiation.ContentNegotiation
import io.ktor.server.testing.ApplicationTestBuilder
import io.ktor.server.testing.testApplication
import io.ktor.server.websocket.WebSockets
import io.micrometer.prometheus.PrometheusConfig
import io.micrometer.prometheus.PrometheusMeterRegistry
import io.mockk.coVerify
import io.mockk.mockk
import kotlinx.serialization.json.Json
import org.koin.core.context.stopKoin
import org.koin.dsl.module
import org.koin.ktor.plugin.Koin
import kotlin.test.AfterTest
import kotlin.test.Test
import kotlin.test.assertEquals

class PreferencesRouteTest {

private val notificationService = mockk<NotificationService>(relaxed = true)
private val webSocketManager = mockk<WebSocketManager>(relaxed = true)

@AfterTest
fun tearDown() {
stopKoin()
}

private fun ApplicationTestBuilder.setupApp() {
application {
install(ContentNegotiation) { json(Json { ignoreUnknownKeys = true }) }
install(WebSockets)
install(Koin) {
modules(
module {
single { notificationService }
single { webSocketManager }
}
)
}
configureRouting(PrometheusMeterRegistry(PrometheusConfig.DEFAULT))
}
}

@Test
fun `PUT preferences uses the authenticated X-User-ID, not the body userId`() = testApplication {
setupApp()

val response = client.put("/api/v1/preferences") {
header("X-User-ID", "alice")
contentType(ContentType.Application.Json)
setBody("""{"eventType":"file_shared","channels":["IN_APP"]}""")
}

assertEquals(HttpStatusCode.NoContent, response.status)
coVerify(exactly = 1) { notificationService.updatePreferences("alice", "file_shared", listOf(DeliveryChannel.IN_APP)) }
}

@Test
fun `PUT preferences rejects a body userId that differs from the caller`() = testApplication {
setupApp()

val response = client.put("/api/v1/preferences") {
header("X-User-ID", "attacker")
contentType(ContentType.Application.Json)
setBody("""{"userId":"victim","eventType":"file_shared","channels":[]}""")
}

assertEquals(HttpStatusCode.Forbidden, response.status)
coVerify(exactly = 0) { notificationService.updatePreferences(any(), any(), any()) }
}

@Test
fun `PUT preferences accepts a body userId that matches the caller`() = testApplication {
setupApp()

val response = client.put("/api/v1/preferences") {
header("X-User-ID", "alice")
contentType(ContentType.Application.Json)
setBody("""{"userId":"alice","eventType":"comment_added","channels":["EMAIL"]}""")
}

assertEquals(HttpStatusCode.NoContent, response.status)
coVerify(exactly = 1) { notificationService.updatePreferences("alice", "comment_added", listOf(DeliveryChannel.EMAIL)) }
}

@Test
fun `PUT preferences without an authenticated identity is rejected`() = testApplication {
setupApp()

val response = client.put("/api/v1/preferences") {
contentType(ContentType.Application.Json)
setBody("""{"userId":"victim","eventType":"file_shared","channels":[]}""")
}

assertEquals(HttpStatusCode.Unauthorized, response.status)
coVerify(exactly = 0) { notificationService.updatePreferences(any(), any(), any()) }
}
}
Loading