Skip to content

Commit 45f47ce

Browse files
committed
fix: correct HTTP status codes and add StatusPages safety net
- Change 500→403 for share code race conditions (consumed/disappeared) - Change 401→403 for account mismatch (authenticated but not authorized) - Add StatusPages with handlers for BadRequest, PayloadTooLarge, and unhandled exceptions as a global safety net - Rethrow JVM Errors in Throwable handler to avoid masking OOM etc. - Add ErrorHandlingFlowTest for StatusPages behavior (413, 404)
1 parent fe5fc2b commit 45f47ce

6 files changed

Lines changed: 66 additions & 6 deletions

File tree

build.gradle.kts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ dependencies {
2525
implementation("io.ktor:ktor-serialization-kotlinx-json:$ktorVersion")
2626
implementation("io.ktor:ktor-server-content-negotiation:$ktorVersion")
2727
implementation("io.ktor:ktor-server-body-limit:$ktorVersion")
28+
implementation("io.ktor:ktor-server-status-pages:$ktorVersion")
2829
testImplementation("io.ktor:ktor-server-test-host:$ktorVersion")
2930
testImplementation("io.ktor:ktor-client-content-negotiation:$ktorVersion")
3031

src/main/kotlin/eu/darken/octi/kserver/Server.kt

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@ package eu.darken.octi.kserver
22

33
import eu.darken.octi.kserver.account.AccountRoute
44
import eu.darken.octi.kserver.account.share.ShareRoute
5+
import eu.darken.octi.kserver.common.debug.logging.Logging.Priority.ERROR
56
import eu.darken.octi.kserver.common.debug.logging.Logging.Priority.INFO
67
import eu.darken.octi.kserver.common.debug.logging.Logging.Priority.WARN
8+
import eu.darken.octi.kserver.common.debug.logging.asLog
79
import eu.darken.octi.kserver.common.debug.logging.log
810
import eu.darken.octi.kserver.common.debug.logging.logTag
911
import eu.darken.octi.kserver.common.installCallLogging
@@ -13,12 +15,17 @@ import eu.darken.octi.kserver.device.DeviceRoute
1315
import eu.darken.octi.kserver.module.ModuleRoute
1416
import eu.darken.octi.kserver.myip.MyIpRoute
1517
import eu.darken.octi.kserver.status.StatusRoute
18+
import io.ktor.http.*
1619
import io.ktor.serialization.kotlinx.json.*
1720
import io.ktor.server.application.*
1821
import io.ktor.server.engine.*
1922
import io.ktor.server.netty.*
2023
import io.ktor.server.plugins.contentnegotiation.*
24+
import io.ktor.server.plugins.*
25+
import io.ktor.server.plugins.statuspages.*
26+
import io.ktor.server.response.*
2127
import io.ktor.server.routing.*
28+
import kotlin.coroutines.cancellation.CancellationException
2229
import kotlinx.serialization.json.Json
2330
import kotlinx.serialization.modules.SerializersModule
2431
import javax.inject.Inject
@@ -46,6 +53,28 @@ class Server @Inject constructor(
4653
})
4754
}
4855

56+
install(StatusPages) {
57+
exception<CancellationException> { _, cause -> throw cause }
58+
exception<BadRequestException> { call, cause ->
59+
log(TAG, WARN) { "Bad request: ${cause.message}" }
60+
if (!call.response.isCommitted) {
61+
call.respond(HttpStatusCode.BadRequest, "Bad request")
62+
}
63+
}
64+
exception<PayloadTooLargeException> { call, _ ->
65+
if (!call.response.isCommitted) {
66+
call.respond(HttpStatusCode.PayloadTooLarge)
67+
}
68+
}
69+
exception<Throwable> { call, cause ->
70+
if (cause is Error) throw cause
71+
log(TAG, ERROR) { "Unhandled exception: ${cause.asLog()}" }
72+
if (!call.response.isCommitted) {
73+
call.respond(HttpStatusCode.InternalServerError, "Internal server error")
74+
}
75+
}
76+
}
77+
4978
config.rateLimit
5079
?.let { installRateLimit(it) }
5180
?: log(TAG, WARN) { "rateLimit is not configured" }

src/main/kotlin/eu/darken/octi/kserver/account/AccountRoute.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,15 @@ class AccountRoute @Inject constructor(
8787
val account = if (share != null) {
8888
if (!shareRepo.consumeShare(shareCode!!)) {
8989
log(TAG, ERROR) { "create($callInfo): Failed to consume Share" }
90-
call.respond(HttpStatusCode.InternalServerError, "ShareCode was already consumed")
90+
call.respond(HttpStatusCode.Forbidden, "ShareCode was already consumed")
9191
return
9292
}
9393
log(TAG, INFO) { "create($callInfo): Share was valid, let's add the device" }
9494
val resolved = accountRepo.getAccount(share.accountId)
9595
if (resolved == null) {
9696
log(TAG, ERROR) { "create($callInfo): Account ${share.accountId} disappeared, restoring share" }
9797
shareRepo.restoreShare(share)
98-
call.respond(HttpStatusCode.InternalServerError, "Account no longer exists")
98+
call.respond(HttpStatusCode.Forbidden, "Account no longer exists")
9999
return
100100
}
101101
resolved

src/main/kotlin/eu/darken/octi/kserver/module/ModuleRoute.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ class ModuleRoute @Inject constructor(
8080

8181
if (callerDevice.accountId != target.accountId) {
8282
log(TAG, ERROR) { "Devices don't share the same account: $callerDevice and $target" }
83-
call.respond(HttpStatusCode.Unauthorized, "Devices don't share the same account")
83+
call.respond(HttpStatusCode.Forbidden, "Devices don't share the same account")
8484
return null
8585
}
8686

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package eu.darken.octi.kserver.common
2+
3+
import eu.darken.octi.*
4+
import io.kotest.matchers.shouldBe
5+
import io.ktor.client.request.*
6+
import io.ktor.http.*
7+
import org.junit.jupiter.api.Test
8+
9+
class ErrorHandlingFlowTest : TestRunner() {
10+
11+
@Test
12+
fun `oversized payload returns 413`() = runTest2 {
13+
val creds = createDevice()
14+
http.post {
15+
url { takeFrom("/v1/account") }
16+
addDeviceId(creds.deviceId)
17+
contentType(ContentType.Application.OctetStream)
18+
setBody("a".repeat((128 * 1024) + 1))
19+
}.apply {
20+
status shouldBe HttpStatusCode.PayloadTooLarge
21+
}
22+
}
23+
24+
@Test
25+
fun `unknown route returns 404`() = runTest2 {
26+
http.get("/v1/nonexistent").apply {
27+
status shouldBe HttpStatusCode.NotFound
28+
}
29+
}
30+
}

src/test/kotlin/eu/darken/octi/kserver/module/ModuleFlowTest.kt

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class ModuleFlowTest : TestRunner() {
7171
val creds1 = createDevice()
7272
val creds2 = createDevice()
7373
readModuleRaw(creds1, "abc", creds2.deviceId).apply {
74-
status shouldBe HttpStatusCode.Unauthorized
74+
status shouldBe HttpStatusCode.Forbidden
7575
bodyAsText() shouldBe "Devices don't share the same account"
7676
}
7777
}
@@ -133,7 +133,7 @@ class ModuleFlowTest : TestRunner() {
133133
val creds1 = createDevice()
134134
val creds2 = createDevice()
135135
writeModule(creds2, "abc", creds1.deviceId, "test").apply {
136-
status shouldBe HttpStatusCode.Unauthorized
136+
status shouldBe HttpStatusCode.Forbidden
137137
bodyAsText() shouldBe "Devices don't share the same account"
138138
}
139139
writeModule(creds2, "abc", creds1.deviceId, "test")
@@ -191,7 +191,7 @@ class ModuleFlowTest : TestRunner() {
191191
val creds1 = createDevice()
192192
val creds2 = createDevice()
193193
deleteModuleRaw(creds2, "abc", creds1.deviceId).apply {
194-
status shouldBe HttpStatusCode.Unauthorized
194+
status shouldBe HttpStatusCode.Forbidden
195195
bodyAsText() shouldBe "Devices don't share the same account"
196196
}
197197
}

0 commit comments

Comments
 (0)