Inject CORS headers even when server-side errors occur

[taisey]

Motivation:

There is an issue where CORS headers are not added when exceptions occur while using CorsService.
CorsService does not inject CORS headers into error responses

Modifications:

Created CorsServerErrorHandler to inject CORS headers upon exceptions. Created CorsHeaderUtil and refactored CorsService, CorsPolicy.

Result:

CORS headers will be added to responses even if exceptions occur.

[github-actions]

🔍 Build Scan® (commit: cee6554)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/aadlbx6trvej2
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/bqm62v7gwyjzi
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/rv6ou6ap2txau
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/g4zgsg4skdjau
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/o5lxatzgcvbug
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/a4moiahkdplbc
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/wcj6t7zn7ley6
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/mampdkdjjeofa

[trustin]

Looks like the core logic is well done! Left some coding style comments. 🙇

[trustin]

Could you also make sure to fix all line errors, reported by Checkstyle?

[ikhoon]

Question)
In #5493, CORS headers weren't injected with .mapHeaders() because the HttpResponse was created from an exception or was aborted. HttpResponse.mapHeaders() can be invoked if an HttpResponse was successfully subscribed by HttpServerHandler.

It looks good to use ServerErrorHandler to inject CORS headers. However, for that, the CORS code is refactored quite a lot. Instead, there is a straightforward way to inject CORS headers in CorsService even under error responses. That is by using ServiceRequestContext.addAdditionalResonseHeaders() and its variants.

ctx.mutateAdditionalResponseHeaders(headers -> {
   setCorsResponseHeaders(ctx, req, headers);
});

I was wondering what is the advantage when using CorsExeptionHandler over using ctx.mutateAdditionalResponseHeaders? Maybe I'm missing something.

[trustin]

I was wondering what is the advantage when using CorsExeptionHandler over using ctx.mutateAdditionalResponseHeaders? Maybe I'm missing something.

It can inject CORS headers even when a decorator throws an exception before CorsService.serve() is even invoked. Do you think it's acceptable not to inject CORS headers in such cases?

[ikhoon]

Do you think it's acceptable not to inject CORS headers in such cases?

Other frameworks seem to use Filter pattern to inject CORS headers.
https://github.com/micronaut-projects/micronaut-core/blob/02a8378b7dd39c79e2b32313f78232d09caa8a45/http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java#L140-L146
That means a former filter does not call the next chain in an error situation, CORS may not be injected.

I thought that we could use the same approach. If the decorator in front intercepts the request and returns an exception, CORS headers may not be necessary.

That said, I think the current approach is also good.

[jrhee17]

I originally also thought of an approach similar to @ikhoon 's idea mainly due to the ease of implementation.

Do you think it's acceptable not to inject CORS headers in such cases?

I thought of this case as analogous to the success case.
e.g. If a decorator before CorsService returns with a HttpResponse(statusCode) early, then CORS headers won't be set.

Having said this, I agree with the current approach users have to worry less about the order of the CorsService decorator.
I think I'm fine with either approach.

[trustin]

@taisey Gentle ping - I think the general consensus here is that the current approache is good and we can continue cleaning up the PR and get it reviewed by other folks. Could you continue working on this PR?

[taisey]

@trustin Thank you for reaching out to me. If possible, I would like to continue working on this pull request.

[trustin]

@taisey Yes, please! Shall we continue addressing the review comments? We also need to fill the PR description following our commit message template here.

[minwoox]

Thanks! Left a few minor suggestions. 😉

[ikhoon]

We can't simply find a CorsService added by ServerBuilder.decorate() with .as() operation.

All decorators would be searched from Router instead of using .as(). A possible workaround is to add a helper method to InitialDispatcherService.

public static class InitialDispatcherService extends SimpleDecoratingHttpService {

  @Nullable
  public <T extends HttpService> T findService(ServiceRequestContext ctx, Class<? extends T> serviceClass) {
    final List<Routed<RouteDecoratingService>> serviceChain = router.findAll(ctx.routingContext());
    if (serviceChain.isEmpty()) {
        return unwrap().as(serviceClass);
    }
    
    for (Routed<RouteDecoratingService> routed : serviceChain) {
        if (routed.isPresent()) {
            final HttpService service = routed.value().decorator();
            final T targetService = service.as(serviceClass);
            if (targetService != null) {
                return targetService;
            }
        }
    }
    return null;
}
}

[ikhoon]

Created #5670
If the PR is merged, we can the code with:

final CorsService corsService = ctx.findService(CorsService.class);

[trustin]

Let me add a follow-up commit once it's merged. 😄

[trustin]

[jrhee17]

Changes look straightforward! Thanks @taisey 👍 🙇 👍

[minwoox]

Looks great! Left a couple of nits. 😉

[minwoox]

@taisey 👏 👏 👏 👍 👍 👍