diff --git a/core/src/main/kotlin/com/lightningkite/lightningserver/http/HttpInterceptor.kt b/core/src/main/kotlin/com/lightningkite/lightningserver/http/HttpInterceptor.kt index 6bb94c96e..137d18bf0 100644 --- a/core/src/main/kotlin/com/lightningkite/lightningserver/http/HttpInterceptor.kt +++ b/core/src/main/kotlin/com/lightningkite/lightningserver/http/HttpInterceptor.kt @@ -1,7 +1,9 @@ package com.lightningkite.lightningserver.http +import com.lightningkite.lightningserver.pathing.PathSpec import com.lightningkite.lightningserver.runtime.ServerRuntime import com.lightningkite.lightningserver.runtime.instrument +import kotlinx.coroutines.CancellationException /** * Interface for intercepting and modifying HTTP requests and responses. @@ -72,10 +74,26 @@ public fun interface HttpInterceptor { } /** - * Wraps the intercept call with instrumentation for performance monitoring. + * Wraps the intercept call with instrumentation for performance monitoring, and recovers from + * exceptions thrown by this interceptor (or anything nested inside it) by converting them to a + * response via the configured exception handler. * * This is used internally to track the time spent in each interceptor. * + * ## Why recover here + * Every interceptor in the chain is composed via nested calls to this function (see + * [compileAndInstrument]), so recovering at this single point means an exception thrown by *any* + * interceptor - not just the terminal handler - is turned into a response before it unwinds past + * the interceptors that wrap it. Those outer interceptors then see a normal return value from + * their own continuation call and still get to post-process it (e.g. CORS still adds + * `Access-Control-Allow-Origin` to a response produced by a rate limiter's own thrown exception). + * Without this, only exceptions from the innermost handler were guaranteed interceptor + * post-processing; an interceptor throwing directly (as [com.lightningkite.lightningserver.cors.CorsInterceptor] + * and rate limiters do) would still skip every interceptor wrapping it. + * + * [CancellationException] is rethrown unchanged - it signals coroutine cancellation (e.g. a client + * disconnect), not a request-level failure, and must not be swallowed into a fabricated response. + * * @param request The HTTP request to intercept * @param action The continuation function * @return The HTTP response @@ -85,8 +103,19 @@ public suspend inline fun HttpInterceptor.interceptInstrumented( request: HttpRequest<*>, noinline action: suspend ServerRuntime.(HttpRequest<*>) -> HttpResponse, ): HttpResponse { - return instrument(name) { - intercept(request, action) + return try { + instrument(name) { + intercept(request, action) + } + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + try { + @Suppress("UNCHECKED_CAST") + server.server.exceptionHandler.handle(request as HttpRequest, e) + } catch (_: Exception) { + HttpResponse(status = HttpStatus.InternalServerError) + } } } diff --git a/core/src/main/kotlin/com/lightningkite/lightningserver/runtime/implementationHelpers.kt b/core/src/main/kotlin/com/lightningkite/lightningserver/runtime/implementationHelpers.kt index a95dd1dab..bafac7e7f 100644 --- a/core/src/main/kotlin/com/lightningkite/lightningserver/runtime/implementationHelpers.kt +++ b/core/src/main/kotlin/com/lightningkite/lightningserver/runtime/implementationHelpers.kt @@ -60,9 +60,28 @@ private val errorType = TelemetryKey.OfString("error.type") */ public suspend fun ServerRuntime.handle(request: HttpRequest): HttpResponse = instrumentHttpRequest(request) { var errorType: String? = null + + // Turns a thrown error into an HTTP response via the configured exception handler, + // recording the error type for instrumentation; falls back to a bare 500 if the handler + // itself throws. Applied INSIDE the interceptor chain (below) so error responses still + // pass back through the interceptors — most importantly CORS. Otherwise error responses + // ship without CORS headers and browsers misreport every 4xx/5xx as a CORS failure. + suspend fun handleError(e: Exception, label: String? = e::class.simpleName): HttpResponse { + errorType = label + return try { + instrument("exceptionHandler") { server.exceptionHandler.handle(request, e) } + } catch (_: Exception) { + errorType = "unhandled_exception" + HttpResponse(status = HttpStatus.InternalServerError) + } + } + val response = try { server.compiledHttpInterceptors.intercept(request) { req -> this.logger.info { "${request.path} accessed by ${request.sourceIp}" } + // Map handler/route/compression exceptions to responses in-place so the surrounding + // interceptors (CORS, etc.) still post-process error responses. + try { val result = try { // Route resolution must live inside this try so that a RouteNotFoundException (e.g. a HEAD // request with no HEAD handler, or a missing trailing slash) is caught below and recovered @@ -173,37 +192,33 @@ public suspend fun ServerRuntime.handle(request: HttpRequest): HttpRes } else result.headers, body = TypedData(newData, result.body.mediaType) ) - } - } catch (timeout: TimeoutCancellationException) { - // A handler exceeded its HttpHandler.timeout. Map to 408 through the normal exception handler so the - // error body is formatted consistently. (Other CancellationExceptions — e.g. client disconnect — are - // NOT caught here and fall through unchanged.) - errorType = "timeout" - this.logger.warn { "Request to ${request.path} exceeded its handler timeout." } - instrument("exceptionHandler") { - server.exceptionHandler.handle( - request, - HttpStatusException( - status = HttpStatus.RequestTimeout, - detail = "timeout", - message = "The request handler exceeded its timeout.", - ), - ) - } - } catch (e: Exception) { - errorType = e::class.simpleName - try { - this.logger.error(e) { "Exception in HTTP" } - instrument("exceptionHandler") { - server.exceptionHandler.handle( - request, - e + } catch (timeout: TimeoutCancellationException) { + // A handler exceeded its HttpHandler.timeout. Map to 408 through the normal exception handler so + // the error body is formatted consistently. (Other CancellationExceptions — e.g. client + // disconnect — are handled by the generic catch below, matching prior behavior.) + this.logger.warn { "Request to ${request.path} exceeded its handler timeout." } + handleError( + HttpStatusException( + status = HttpStatus.RequestTimeout, + detail = "timeout", + message = "The request handler exceeded its timeout.", + ), + label = "timeout", ) + } catch (e: Exception) { + this.logger.error(e) { "Exception in HTTP" } + handleError(e) } - } catch (_: Exception) { - errorType = "unhandled_exception" - HttpResponse(status = HttpStatus.InternalServerError) } + } catch (e: Exception) { + // Last-resort safety net. Exceptions thrown by an interceptor itself are now recovered + // inside HttpInterceptor.interceptInstrumented, at the point each interceptor is invoked, + // so outer interceptors (e.g. CORS) still get to post-process the resulting response. This + // catch only fires when there are no interceptors installed (compiledHttpInterceptors is + // HttpInterceptor.NoOp, which bypasses interceptInstrumented) or some other exception + // escapes the chain machinery itself — headers from would-be interceptors are absent here. + this.logger.error(e) { "Exception in HTTP interceptor chain" } + handleError(e) } HttpInstrumentationResult(response, response.status.code, errorType) } diff --git a/core/src/test/kotlin/com/lightningkite/lightningserver/runtime/ImplementationHelpersHandleTest.kt b/core/src/test/kotlin/com/lightningkite/lightningserver/runtime/ImplementationHelpersHandleTest.kt index 5ab06a287..a81034714 100644 --- a/core/src/test/kotlin/com/lightningkite/lightningserver/runtime/ImplementationHelpersHandleTest.kt +++ b/core/src/test/kotlin/com/lightningkite/lightningserver/runtime/ImplementationHelpersHandleTest.kt @@ -1,6 +1,8 @@ package com.lightningkite.lightningserver.runtime import com.lightningkite.lightningserver.HttpMethod +import com.lightningkite.lightningserver.HttpStatusException +import com.lightningkite.lightningserver.NotFoundException import com.lightningkite.lightningserver.definition.builder.ServerBuilder import com.lightningkite.lightningserver.definition.loggingSettings import com.lightningkite.lightningserver.http.* @@ -88,6 +90,11 @@ class ImplementationHelpersHandleTest { HttpResponse.plainText("quick") } + // Always throws, to prove error responses still receive interceptor post-processing. + val boom = path.path("boom").get bind HttpHandler { + throw NotFoundException(detail = "boom", message = "Boom.") + } + init { registerBasicMediaTypeCoders() } @@ -390,6 +397,84 @@ class ImplementationHelpersHandleTest { } } + @Test + fun error_response_still_receives_cors_headers() { + // Regression: a handler that throws must still get CORS headers. The exception is now + // mapped to a response INSIDE the interceptor chain, so CORS post-processes it. Without + // this, the browser masks every 4xx/5xx as a CORS failure and the real error (here, a + // 404) is invisible to client JS. + TestServer.test(settings = {}) { + runBlocking { + val resp = serverRuntime.handle( + HttpRequest( + path = RawHttpEndpoint(asString = "/boom", method = HttpMethod.GET), + queryParameters = QueryParameters.EMPTY, + headers = HttpHeaders { add(HttpHeader.Origin, "https://example.com") }, + domain = "example.com", + protocol = "https", + sourceIp = "local", + ) + ) + assertEquals(HttpStatus.NotFound, resp.status) + assertEquals( + "https://example.com", + resp.headers[HttpHeader.AccessControlAllowOrigin]?.root, + "error responses must carry the CORS allow-origin header", + ) + } + } + } + + // A minimal server whose second (innermost) interceptor always throws before calling its + // continuation - simulating a rate limiter or auth interceptor rejecting a request. CORS is + // installed first (outermost) so this proves outer interceptors still post-process a response + // that resulted from an *interceptor's own* thrown exception, not just a handler's. + object InterceptorFailureTestServer : ServerBuilder() { + val cors = com.lightningkite.lightningserver.cors.CorsSettings( + limitToDomains = listOf("example.com"), + limitToMethods = listOf("*"), + ) + + init { + install(com.lightningkite.lightningserver.cors.CorsInterceptor(setting("cors", cors))) + install(HttpInterceptor { _, _ -> + throw HttpStatusException( + status = HttpStatus.TooManyRequests, + detail = "boom-interceptor", + message = "Simulated interceptor failure.", + ) + }) + registerBasicMediaTypeCoders() + } + } + + @Test + fun error_thrown_by_interceptor_itself_still_receives_outer_post_processing() { + // Regression: an interceptor that throws directly (not the handler) must still be + // recovered close enough to the throw site that interceptors wrapping it - here, CORS - + // see a normal response back from their continuation and still post-process it. + InterceptorFailureTestServer.test(settings = {}) { + runBlocking { + val resp = serverRuntime.handle( + HttpRequest( + path = RawHttpEndpoint(asString = "/anything", method = HttpMethod.GET), + queryParameters = QueryParameters.EMPTY, + headers = HttpHeaders { add(HttpHeader.Origin, "https://example.com") }, + domain = "example.com", + protocol = "https", + sourceIp = "local", + ) + ) + assertEquals(HttpStatus.TooManyRequests, resp.status) + assertEquals( + "https://example.com", + resp.headers[HttpHeader.AccessControlAllowOrigin]?.root, + "CORS (an outer interceptor) must still post-process a response produced by an inner interceptor's own thrown exception", + ) + } + } + } + @Test fun fast_handler_completes_within_its_timeout() { TestServer.test(settings = {}) {