HTTP CORS headers not included.

[sjaakd]

Describe the bug

We have the following settings in our application properties

quarkus.http.cors=${QUARKUS_HTTP_CORS_ENABLED:true}
quarkus.http.cors.methods=${QUARKUS_HTTP_CORS_METHODS:GET,POST}
quarkus.http.cors.access-control-max-age=${QUARKUS_HTTP_CORS_ACCESS_CONTROL_MAX_AGE:24H}

Expected behavior

CORS headers included

Actual behavior

CORS headers not included.

However, changing the configuration (strangly enough) to

quarkus.http.cors=false

Or omitting this setting altogether will trigger the headers.

which seems to be in conflict to the documentation

How to Reproduce?

Service with quarkus.http.cors=false:
https://publiek.broservices.nl/gm/gmw-v1.1/openapi

Service with

quarkus.http.cors=${QUARKUS_HTTP_CORS_ENABLED:true}
quarkus.http.cors.methods=${QUARKUS_HTTP_CORS_METHODS:GET,POST}
quarkus.http.cors.access-control-max-age=${QUARKUS_HTTP_CORS_ACCESS_CONTROL_MAX_AGE:24H}

https://publiek.broservices.nl/gm/gmn-v1.0/openapi

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.3.1.Final

Build tool (ie. output of mvnw --version or gradlew --version)

mvn 3.6.3

Additional information

No response

[sberyozkin]

@sjaakd Thanks, what happens if you do:

quarkus.http.cors=true
quarkus.http.cors.methods=GET,POST
quarkus.http.cors.access-control-max-age:24H

? Would like to see if the system property replacement is a problem or not

[sjaakd]

@sjaakd Thanks, what happens if you do:

quarkus.http.cors=true
quarkus.http.cors.methods=GET,POST
quarkus.http.cors.access-control-max-age:24H

? Would like to see if the system property replacement is a problem or not

Just tried that locally.

No effect.

[sjaakd]

Furthermore, I do use 2 RouteFilter (vertex). To include the API-Version and to make sure that openapi supports json-by-extension.

    @RouteFilter
    public void addResponse(RoutingContext rc) {
        rc.response().headers().set( API_VERSION_HEADER,  provider.version  );
        rc.next();
    }

    @RouteFilter
    public void modifyAcceptHeader(RoutingContext rc) {
        String url = rc.normalizedPath();
        if ( url.endsWith( "openapi.json" ) ) {
            String acceptHeader = rc.request().headers().get( ACCEPT );
            if ( acceptHeader != null && !acceptHeader.contains( "yaml" ) ) {
                rc.request().headers().set( ACCEPT, "application/json" );
            }
            rc.reroute(  url.substring( 0, url.lastIndexOf( '.' ) ) );
            return;
        }
        else if ( url.endsWith( "openapi.yaml" ) ) {
            rc.reroute(  url.substring( 0, url.lastIndexOf( '.' ) ) );
            return;
        }
        rc.next();
    }

could that somewhat be related? (did not find the time yet to investigate further and making a minimum reproducer).

[sberyozkin]

@sjaakd Looks like the problem is due to the fact the browser is not indicating it requires the CORS headers - it does not include Origin as you probably do the same origin requests to test ?

[sjaakd]

@sjaakd Looks like the problem is due to the fact the browser is not indicating it requires the CORS headers - it does not include Origin as you probably do the same origin requests to test ?

That is true.. I test locally. But the headers are included if i change to quarkus.http.cors=false. Also locally. Moreover, the problem occurs also if you test with a remote browser (check my public reproducers).

And.. important: external verification fails on cors headers. Checkout: https://developer.overheid.nl/apis/minbzk-bro-grondwatermonitoringput-(gmw)-uitgifte/score-detail . Note: tonight this compliance test will (hopefully) pass again and I changed the application.properties this morning.

EDIT: I added /openapi to the test URLs. . the compliance is about openapi

[sberyozkin]

@sjaakd So looks like it is about quarkus-smallrye-openapi. Vert.x CORS filter just does not do any work at all if no Origin is included - and it itself is not included if the CORS is disabled.
@phillip-kruger hey Phillip, can you check it please ? I see OpenApiHandler checks quarkus.http.cors but not sure what are the expectations

[sjaakd]

An interesting observation: after a rerun of the test last night I noticed that another design rule is failing. The API-Version header (see @RouteFilter above) which seems suddenly gone missing. That suggests that the now included CORS headers remove the other response headers somehow (set by the @RouteFilter). Checkout: https://publiek.broservices.nl/gm/gmw-v1.1/openapi.json and this report (API-57).

[phillip-kruger]

Hi @sjaakd @sberyozkin - Happy to have a look. Please provide a small reproducer that I can run locally. Thanks :)

[sjaakd]

Hi @sjaakd @sberyozkin - Happy to have a look. Please provide a small reproducer that I can run locally. Thanks :)

Hi @phillip-kruger thanks.. I'll do my best to provide one as soon as possible (hopefully a bit later this week).

[sjaakd]

@phillip-kruger , @sberyozkin : the reproducer can be found here. Thanks for looking into this!.

[sjaakd]

I checked the latest release: 2.4.2.Final .. behavior is identical

[phillip-kruger]

@sjaakd I had a look and in terms of CORS, everything seems to be OK. One issue that exist, is that your custom (API version) header gets removed when OpenAPI adds the default CORS (i.e when you do not have any CORS config in application.properties)
(So I added a fix for that)

Here the results from my investigation:

When CORS is not configured:

wget -d --header="Origin: http://localhost:8080" http://localhost:8080/reproducer/q/openapi

gives this response headers:

API-Version: 1.0.0
access-control-allow-origin: *
access-control-allow-credentials: true
access-control-allow-methods: GET, HEAD, OPTIONS
access-control-allow-headers: Content-Type, Authorization
access-control-max-age: 86400
Content-Type: application/yaml;charset=UTF-8
content-length: 739

Before my fix the API-Version was not included. Another thing that we potentially do it to only included this default if the origin request header is present. In fact I am not even sure why we need to add default headers at all... ? (This pre-date my involvement)

When CORS config exist:

wget -d --header="Origin: http://localhost:8080" http://localhost:8080/reproducer/q/openapi

gives this response headers:

access-control-allow-origin: http://localhost:8080
access-control-allow-credentials: false
API-Version: 1.0.0
Content-Type: application/yaml;charset=UTF-8
content-length: 739

I'll do a PR for my fix soon, but that is not solving your issue. Please can you test the wget command on your localhost to see if you get the same result as I got.

[sjaakd]

I'll do a PR for my fix soon, but that is not solving your issue. Please can you test the wget command on your localhost to see if you get the same result as I got.

I can confirm. My observation is consistent with yours. I do get the headers this way.

Before my fix the API-Version was not included. Another thing that we potentially do it to only included this default if the origin request header is present.

That's interesting. It would be nice if the user would have some control over the headers regardless the origin request header. I need to investigate myself how this is supposed to work. The standard I try to implement states:

 Furthermore, the CORS policy for this URI must allow external domains to read the documentation from a browser environment.

I noticed, when the access-control-allow-* headers are included, the test passes. The test are run every night (not under my control), so I'll have a go at it and see if I can find out more on the desired behavior and the workings of CORS headers in relation to openapi

[phillip-kruger]

I do not think it makes sense to have the header in the response if no origin is available ? (cross ORIGIN being the thing we are solving here)

But, what I would suggest :

1. create an issue that ask for a feature that allows the .json and .yaml content negotiation in Quarkus for openapi (then you would not need your filter)
2. create an issue that ask for ability to add custom header in openapi (then you would not need your other filter)

If you need the CORS header to be included always, maybe we can add another config that, when set to true, does not check for origin ? (@sberyozkin ?)

[sjaakd]

@phillip-kruger .. Thanks for you quick answers. Just to clarify:

1. create an issue that ask for a feature that allows the .json and .yaml content negotiation in Quarkus for openapi (then you would not need your filter)

That would be great. I'm trying to do some reading up and found this on the swagger documentation. Are you suggestion a feature that CORS is handled automatically according this documentation? Does this (openapi) somehow deviates from the regular way that Quarkus supports CORS? You see: I'm a bit lost here on how this all fits together.

2. create an issue that ask for ability to add custom header in openapi (then you would not need your other filter)

Are you referring to
a. the '.json' and '.yaml' extensions routefilter: so support for that out -of-the box (since the "swagger" documentation seems to hint to that as well)
or
b. the api-version routefilter? So to add a custom header based on an application.properties parameter.

Anyway: both would be really cool.

[phillip-kruger]

Quarkus OpenAPI already support CORS (that is the default I refer too), but it can be set by the client too. There are two scenarios here:

1. Getting the schema document (either in yaml or json format). This is the scenario we discussed so far. CORS is only applicable when the origin is different from where the schema document is hosted. So for example if you would add the url of the schema document to (https://petstore.swagger.io/) then the origin would be petstore.swagger.io:

By default we make it very open (*), but you can override it by using the CORS config as defined in Quarkus: https://quarkus.io/guides/http-reference#cors-filter to narrow it to your domains.

2. CORS for Swagger UI (this is different from the schema document). By default Swagger UI is not included in non dev-mode, but you can included it. When you do, swagger ui need to access the above mentioned schema document (as described in 1) but by default swagger-ui and the schema document is available from the same host (so no cross origin). swagger-ui also need to access your actual endpoints, but again, by default they are all on the same host. If you need to proxy this or somehow host this differently you can use the CORS config to configure the requirement for your endpoints and swagger-ui.

The, w.r.t what I was referring to, both a and b :)
a) So for your .json and .yaml extension filter, we can enhance the extension to just support that out of the box. At the moment we only do content negotiation via headers, but it should be easy enough to add extension.
b) Here we can allow something in config yes.

[sjaakd]

1. Getting the schema document (either in yaml or json format). This is the scenario we discussed so far. CORS is only applicable when the origin is different from where the schema document is hosted. So for example if you would add the url of the schema document to (https://petstore.swagger.io/) then the origin would be petstore.swagger.io:

Now I finally get it 😄 . So If I were to host a combined swagger page (I've later on multiple of these REST services, the CORS headers would be applicable if I host that combined page in a different domain. That is obviously not the case here. I therefore think that the implementer of the validation website (developers at the dutch government) have understood this particular aspect the wrong way.

The, w.r.t what I was referring to, both a and b :)

Great! Do you want met to open 2 new feature requests for these (and refer to our dialogue here)? Is there something I can contribute?

[phillip-kruger]

@sjaakd - So what we maybe can consider (@sberyozkin to comment) is to, with some config flag, always add the CORS headers (regardless of the origin request header). I am not too familiar with that part of the code (hence @sberyozkin to comment) so I am not sure if it's even possible. But if we can, then you can set the flag, and should pass the validation.

Yes please open 2 issues as discussed, thanks :)

[sjaakd]

Yes please open 2 issues as discussed, thanks :)

Done.. My pleasure!

[sjaakd]

So what we maybe can consider (@sberyozkin to comment) is to, with some config flag, always add the CORS headers (regardless of the origin request header). I am not too familiar with that part of the code (hence @sberyozkin to comment) so I am not sure if it's even possible. But if we can, then you can set the flag, and should pass the validation.

That would solve my issue. But I can imagine this is not according the CORS specs. @sberyozkin to be the judge on that one.

[phillip-kruger]

Can we close this ?

[sjaakd]

@phillip-kruger I guess we did not receive a verdict from @sberyozkin on this matter. That's why I left it open.

[phillip-kruger]

Ok I leave this for @sberyozkin to close :)

[sberyozkin]

Hi @phillip-kruger @sjaakd Sorry, looks like I've badly missed commenting on this issue.

I've read through all of it and as far as I understand the reason it is still open is that an option of providing CORS headers without even the browsers requesting them with Origin.

I believe Quarkus should not do it, it would not be CORS spec compliant and it is unclear what side-effects it may have.

@sjaakd IMHO, the simplest option would be for your application to register a custom Vertx or even pre-match JAX-RS filter which would make sure the required CORS headers are reported even if they are not requested by the browser.

Hope it makes sense, let me close this issue. Please create another more specific issue if you'd like to continue discussing an option of the CORS headers being pushed by Quarkus without Origin

thanks

[sjaakd]

I believe Quarkus should not do it, it would not be CORS spec compliant and it is unclear what side-effects it may have.

I agree.

@sjaakd IMHO, the simplest option would be for your application to register a custom Vertx or even pre-match JAX-RS filter which would make sure the required CORS headers are reported even if they are not requested by the browser.

That will not be needed. The validation service reporting these CORS issues has been fixed. Thanks for your answer!