CorsPlugin reflectClientOrigin doesn't work in Before handlers

[YaroslavYakymenko]

Actual behavior (the bug)
Hi, I use the CorsPlugin with the following configuration:

javalinConfig.registerPlugin(new CorsPlugin(cors -> cors.addRule(corsRule -> corsRule.reflectClientOrigin = true)));

It works fine for the Endpoint handlers like get, post, etc. However, it doesn't work in Before handlers.
The Access-Control-Allow-Origin header is not being set in the response.

Expected behavior
The Access-Control-Allow-Origin header is present in the Before handlers response.

To Reproduce
Implement a Before handler that will throw any exception, for example, BadRequestResponse.
Send the API request with the Origin header. The response will not include the Access-Control-Allow-Origin header.

Additional context
The issue causes problems with handling errors in the UI application because of missing Access-Control-Allow-Origin response header

[tipsy]

Could you have a look here @Playacem ?

[Playacem]

The following tests works without issues, can you maybe share your code or a minimal reproduction setup for this issue?

        @Test
        fun `works with throwing before`() = TestUtil.test(Javalin.create { config ->
            config.bundledPlugins.enableCors { cors ->
                cors.addRule { it.reflectClientOrigin = true }
            }
        }.get("/") { it.result("a") }.before { throw BadRequestResponse() }) { _, http ->
            assertThat(
                http.get("/", mapOf(ORIGIN to "https://some-origin")).header(ACCESS_CONTROL_ALLOW_ORIGIN)
            ).isEqualTo(
                "https://some-origin"
            )
        }

Edit: The registerPlugin version also works as expected. Do note that I am registering all handlers before starting Javalin. That might be something that you do not do.

[YaroslavYakymenko]

Hi @Playacem , with your example it works for me too.
However, here is a code example that breaks the things:
Server configuration:

public class Main {

  public static void main(String[] args) {

    Consumer<JavalinConfig> javalinConfig = configureRoutes();
    javalinConfig = javalinConfig.andThen(configureServer());

    var app = Javalin.create(javalinConfig)
        .start(7070);
  }

  private static Consumer<JavalinConfig> configureServer() {
    return config -> {
      config.registerPlugin(new CorsPlugin(cors -> {
        cors.addRule(corsRule -> corsRule.reflectClientOrigin = true);
      }));
    };
  }


  private static Consumer<JavalinConfig> configureRoutes() {
    return javalinConfig -> {
      javalinConfig.router.apiBuilder(() -> {
        path(
            "/",
            () -> {
              before("", ctx -> {
                throw new TooManyRequestsResponse("bad");
              });
              get("", ctx -> ctx.result("Hello World"));
            });
      });
    };
  }
}

Response without header:

curl -I -H "Origin: https://google.com" http://localhost:7070/
HTTP/1.1 429 Too Many Requests
Date: Thu, 23 May 2024 08:09:25 GMT
Content-Type: text/plain
Content-Length: 3

Please not that we chain configuration with javalinConfig = javalinConfig.andThen(configureServer());

[Playacem]

Okay, I found the reason why this happens. Basically we register the cors routes later than the user defined before handler.
A possible solution is to use the new beforeMatched handlers as those run after the normal before handlers.
Does this work for you, @YaroslavYakymenko ?

        @Test
        fun `gh-2246`() = TestUtil.test(Javalin.create { config ->
            config.registerPlugin(CorsPlugin { cors ->
                cors.addRule { rule -> rule.reflectClientOrigin = true }
            })
            config.router.mount {
                it.beforeMatched {
                    throw TooManyRequestsResponse()
                }
                it.get("/") {
                    it.result("a")
                }
            }
        }) { _, http ->
            assertThat(http.get("/", mapOf(ORIGIN to "https://some-origin")).header(ACCESS_CONTROL_ALLOW_ORIGIN)).isEqualTo("https://some-origin")
        }

[YaroslavYakymenko]

Hi, @Playacem looks like beforeMatched is working for the config.router.mount API only.
However, we are using config.router.apiBuilder and it seems not possible to use beforeMatched but only before handler in the EndpointGroup

[Playacem]

You can use both approaches and have the beforeMatched handler using the mount api. The api builder is more for those typical rest apis, leaving the rest to the mount api and the direct stuff on the Javalin instance.

[YaroslavYakymenko]

Yes, this is a workable option, but I'm afraid it doesn't work very well for us.
We have a complex EndpointGroup structure that holds all the configurations in one place.
For some of our applications, we have 43 before handlers, and moving them to the config.router.mount will lead to code duplication and worse readability.

For example:

      javalinConfig.router.apiBuilder(() -> {
        path(
            "/api",
            () -> {
              //before handlers common for entire /api path

              path(
                  "/clients",
                  () -> {
                    //before handlers and method handlers for the /api/clients endpoints
                  });

              path(
                  "/notifications",
                  () -> {
                    //before handlers and method handlers for the /api/notifications endpoints
                  });

              path(
                  "/users",
                  () -> {
                    //before handlers and method handlers for the /api/users endpoints
                  });

            //..... another endpoint groups
            });
      });

It would be nice to have beforeMatched available for the EndpointGroups or before handler to work properly and return Origin header.