`CorsService` does not inject CORS headers to error responses

[Mina-1316]

I've created an error handler, and they won't affected by CorsDecorator I've attached...

Server.builder().apply {
        errorHandler(ServiceExceptionHandler())
        errorHandler(UniversalFailureHandler())
        errorHandler(UniversalIgnoreHandler())
        errorHandler(UniversalExceptionHandler())
        http(14000)

        decorator(corsDecorator)
}

In this situation, cors won't do their Jobs when error Handler catches the error.

I've digged some code and figured that decorators and errorHandlers are managed differently... Is there any way to decorate errorHandlers to follow the CORS rules?

[Mina-1316]

btw, sorry for empty issue at Initial! I've mistyped enter and github submitted the issue with empty.

[trustin]

Hey, thanks a lot for reporting the problem. If I understood correctly, you want the CorsService to inject the CORS response headers even to the error responses generated by ServerErrorHandler, which is not possible obviously.

I think this is not something CorsService can solve unfortunately at the moment, because CorsService is only capable of handling a successful HttpResponse.

Perhaps we should provide CorsServerErrorHandler so that a user specify it with errorHandler()..?

[trustin]

Here's the idea:

• Improve CorsConfig API so that a user can build it with a builder API, e.g.

  var myCorsCfg = CorsConfig.builder()....build();

• Allow a user to create a CorsService with a CorsConfig.

  CorsService.newDecorator(myCorsCfg);
  // and
  myCorsCfg.newDecorator();
  myCorsCfg.newService();

• Implement a CorsServerErrorHandler and allow a user to create it with a CorsConfig.

  CorsServerErrorHandler.of(myCorsCfg);
  // and
  myCorsCfg.newErrorHandler();

• (Stretch goal) Add a shortcut method to ServerBuilder and VirtualHostBuilder so that a user can add CorsService and CorsErrorHandler with a single call.

  Server.builder()
      .cors(myCorsCfg) // Adds CorsService and CorsErrorHandler on build() automatically.
      ..

[Mina-1316]

Thank you for the fast response!

Is it possible to create CorsServerErrorHandler to wrap and provide CORS header for another ServerErrorHandler's response which will actually return a value? As I checked, multiple ServerErrorHandler are associated by ServerErrorHandler.orElse() method, in below:

    default ServerErrorHandler orElse(ServerErrorHandler other) {
        requireNonNull(other, "other");
        return new ServerErrorHandler() {
            @Nullable
            @Override
            public HttpResponse onServiceException(ServiceRequestContext ctx, Throwable cause) {
                final HttpResponse response = ServerErrorHandler.this.onServiceException(ctx, cause);
                if (response != null) {
                    return response;
                }
                return other.onServiceException(ctx, cause);
            }
            /* skip others because of same approaches when association */
        };
    }

I don't know much about how ServiceExceptionHandler associated/interacts with others, but as I assume this won't make other handler can modify response, which is necessary on current case.

If it can, then it could be nice to document about how to decorate ServiceExceptionHandler's behavior, If not, maybe we should create new way to decorate ServiceExceptionHandler.

ps: For who need fast solution for this, I solved the issue by decorating handlers manually, like this:

// Create decorator which will decorate the value to 
fun corsExceptionHandlerDecorator(handler: ServerErrorHandler) = object : ServerErrorHandler {
    override fun onServiceException(ctx: ServiceRequestContext, cause: Throwable): HttpResponse? =
        handler.onServiceException(ctx, cause)?.mapHeaders { headers ->
            // Can be improved when CorsConfig API is injectable
            headers.withMutations { builder ->
                ctx.request().headers()["origin"]?.let { builder.add("access-control-allow-origin", it) }
                builder.add("access-control-allow-credentials", "true")
                builder.add("vary", "origin")
            }
        }
}

    // Create server
    private val server = Server.builder().apply {
        annotatedService("/metrics", MetricsController())

        listOf(
            ServiceExceptionHandler(),
            UniversalFailureHandler(),
            UniversalIgnoreHandler(),
            UniversalExceptionHandler(),
        ).forEach { errorHandler(corsExceptionHandlerDecorator(it)) }
        http(14000)

        decorator(corsDecorator)
    }.build()

[trustin]

Yeah, orElse() doesn't achieve what you want as you mentioned. The ability to decorate a ServerHandler with another is something we need to implement as well. For now, your Kotlin code is the correct solution.

[ikhoon]

A straightforward workaround is to set CORS headers using ServiceRequestContext.addAdditionalResponseHeader() in the CORS exception hander

[Mina-1316]

like this?

fun corsExceptionHandlerDecorator(handler: ServerErrorHandler) = object : ServerErrorHandler {
    override fun onServiceException(ctx: ServiceRequestContext, cause: Throwable): HttpResponse? {
        ctx.additionalResponseHeaders().withMutations { builder ->
            ctx.request().headers()["origin"]?.let { builder.add("access-control-allow-origin", it) }
            builder.add("access-control-allow-credentials", "true")
            builder.add("vary", "origin")
        }

        return handler.onServiceException(ctx, cause)
    }
}

As i tried, this won't add CORS headers to error handler's response..

[ikhoon]

ctx.additionalResponseHeaders().withMutations { builder ->

HttpHeaders is immutable so additionalResponseHeaders().withMutations() will create a new headers and the mutation won't affect the additional headers in ServiceRequestContext.

You may want to use one of them below. https://github.com/line/armeria/blob/7f302505a05d24e7704bcffc43fab6337344cb2c/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java#L553-L567

fun corsExceptionHandlerDecorator() = object : ServerErrorHandler {
    override fun onServiceException(ctx: ServiceRequestContext, cause: Throwable): HttpResponse? {
        ctx. mutateAdditionalResponseHeaders { builder ->
            ctx.request().headers()["origin"]?.let { builder.add("access-control-allow-origin", it) }
            builder.add("access-control-allow-credentials", "true")
            builder.add("vary", "origin")
        }
        // fallback to the next error handler
        return null
    }
}

[ikhoon]

@Mina-1316 It seems like there is no need to apply CORS headers when preflight requests fail because they don't reach the business logic. Do you want to set CORS headers even when preflight requests fail?