quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.8k stars 2.68k forks source link

CORS broken? #34669

Closed edeandrea closed 1 year ago

edeandrea commented 1 year ago

Describe the bug

I'm not sure how far this goes back, but I started noticing it in Quarkus 3.1.x and it still exists in 3.2.x. There seems to be something broken with CORS.

Expected behavior

I would expect since rest-fights has

quarkus.http.cors=true
quarkus.http.cors.origins=*

in it's application.properties that all requests would pass CORS, but that doesn't seem to be the case.

Actual behavior

In the Quarkus superheroes app if I deploy all the apps in the same namespace everything is fine, but if I move the UI app out to another namespace or to a different cluster, the Angular app can no longer communicate with the rest-fights app.

@holly-cummins and I originally thought it was something we/she did when we moved the UI image from Node.js to Quarkus Quinoa, but I've heard reports of others seeing similar issues.

The UI gets a 500 error back from rest-fights with this:

ui-super-heroes-supes.apps.cluster-rpfcn.rpfcn.sandbox1370.opentlc.com/:1  Access to XMLHttpRequest at 'http://rest-fights-supes.apps.cluster-rpfcn.rpfcn.sandbox1370.opentlc.com/api/fights/randomfighters' from origin 'http://ui-super-heroes-supes.apps.cluster-rpfcn.rpfcn.sandbox1370.opentlc.com' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

(That cluster with those URLs are long gone - I just saved the error from the last time I saw it)

the rest-fights app has this config in it's application.properties:

quarkus.http.cors=true
quarkus.http.cors.origins=*

How to Reproduce?

Deploy the Quarkus Superheroes somewhere where the UI app is on a different host than the rest-fights app and configure the UI URL accordingly.

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

3.1.x, but not sure if this existed in previous versions.

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

No response

Additional information

No response

edeandrea commented 1 year ago

@cescoffier ^^^

sberyozkin commented 1 year ago

@edeandrea https://quarkus.io/guides/http-reference#cors-filter says that for the value be treated as a regular expression it has to be surrounded by forward slashes. Example is given below in https://quarkus.io/guides/http-reference#support-all-origins-in-devmode

edeandrea commented 1 year ago

@sberyozkin That doesn't work either. I tried that as well.

I tried setting quarkus.http.cors.origins=/.*/ in rest-fights/src/main/resources/application.properties

sberyozkin commented 1 year ago

@edeandrea, Hmm, we have a test I believe for /.*/, let me try http://ui-super-heroes-supes.apps.cluster-rpfcn.rpfcn.sandbox1370.opentlc.com against this configuration, thanks

edeandrea commented 1 year ago

That cluster is gone unfortunately. That was from a week or so ago.

sberyozkin commented 1 year ago

@edeandrea I'd like to have a test confirming it works in principle, a regular expression supporting any origin, and that Access-Control-Allow-Origin is set. Let me handle #34659 very quickly as it is being reviewed right now, and I will be prioritizing on this issue. Sorry about the hassle, we'll sort it out

cescoffier commented 1 year ago

@sberyozkin why are we recommending a regexp equivalent to ? The regexp evaluation is more expensive than comparing with .

sberyozkin commented 1 year ago

@cescoffier Hi, * is only one example, and the pattern is calculated at the build time. I'm not sure we need to have a dedicated support for *, in prod it is not a good idea to use a wildcard, so the performance cost of the wildcard pattern match is really small

edeandrea commented 1 year ago

It's fixed at build time? Not according to the docs.....

Also, I understand having * in prod is not a good idea, but we should absolutely not disallow it. That would completely break the superheroes sample app. There is no possible way to know the possible urls ahead of time and the user should not have to manually configure anything to deploy and run it, so yes we still need a way to allow all

sberyozkin commented 1 year ago

@edeandrea

Also, I understand having * in prod is not a good idea, but we should absolutely not disallow it.

We don't disallow it, we let users set it. Quarkus can't be taking responsibility for enabling a wildcard out of the box, we've already been affected once.

edeandrea commented 1 year ago

We don't disallow it, we let users set it

Ok good. For some reason I must have mis-read your comment saying that wildcards would not be able to be done.

sberyozkin commented 1 year ago

@edeandrea

I'm just preparing a test, sorry. This wildcard issue must be fixed, no doubt :-)

sberyozkin commented 1 year ago

Very strange, CORS filter has a dedicated support for * (without a regex), and with a regex, several tests in vertx-http/deployment use a * configuration.

sberyozkin commented 1 year ago

@edeandrea Is it possible to set up Quarkus Superheroes without going through the whole workshop coding and configuration set up ? (I do look forward to catching up but would like to reproduce fast), FYI, I've downloaded ZIP from the workshop site but it fails to build, assembly.xml is missing

holly-cummins commented 1 year ago

@sberyozkin the workshop and the sample that @edeandrea is talking about two different projects. It's the same theme, but the code is different, and the deployment logic is quite different. You want https://github.com/quarkusio/quarkus-super-heroes.

sberyozkin commented 1 year ago

@holly-cummins Can I try https://github.com/quarkusio/quarkus-super-heroes#running-locally-via-docker-compose ? I'm not sure deploying to OpenShift will make it any easier to trace what is going on, I may need to to trace HTTP traffic etc

sberyozkin commented 1 year ago

I was hoping to reproduce it locally, as long as services run on different ports, it should be reproducible

edeandrea commented 1 year ago

The problem is not reproducable (I've found at least) if the apps are running on the same host or domain name. They have to have different domains (& therefore truly cross-origin).

You could spin up each app in dev mode (ui-super-heroes and rest-fights) an just hit http://localhost:8080, but I'm not sure you would actually see the problem there.

sberyozkin commented 1 year ago

Hey @edeandrea @holly-cummins I've deployed to OpenShift, oc get routes gives me:

[sberyozkin@fedora k8s]$ oc get routes
NAME                HOST/PORT                                                                  PATH   SERVICES            PORT    TERMINATION   WILDCARD
apicurio            apicurio-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com                   apicurio            8080                  None
event-statistics    event-statistics-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com           event-statistics    http                  None
rest-fights         rest-fights-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com                rest-fights         http                  None
rest-heroes         rest-heroes-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com                rest-heroes         http                  None
rest-villains       rest-villains-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com              rest-villains       http                  None
ui-super-heroes     ui-super-heroes-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com            ui-super-heroes     http                  None

All looks good, and I can access http://ui-super-heroes-sbiarozk-dev.apps.sandbox-m3.1530.p1.openshiftapps.com/. It is super nice what you have done, lets discuss at some point wiring in Keycloak/SSO ?

In any case, what do I need to press in UI to reproduce ?

sberyozkin commented 1 year ago

Someone called Cell has won. No CORS problem in the WebDevTools...

sberyozkin commented 1 year ago

Screenshot from 2023-07-11 15-24-50

edeandrea commented 1 year ago

When everything is deployed in the same namespace things are ok. You'd have to deploy the UI and the fights service into separate clusters/namespaces for it to truly be cross origin.

Just trying to load the UI was causing the error, which is kind of weird because it's a GET, which shouldn't need a pre-flight request.

sberyozkin commented 1 year ago

@edeandrea Unfortunately I can't create a new project in the OpenShift Dev Sandbox I got allocated. Can you recreate this multi-cluster setup if you've done it before ?

Just trying to load the UI was causing the error, which is kind of weird because it's a GET, which shouldn't need a pre-flight request.

Such requests may still provide Origin header and expect Access-Control-Allow-Origin.

edeandrea commented 1 year ago

I can't right now (I'm at a conference) but I can try later or in a day or 2.

sberyozkin commented 1 year ago

@edeandrea

I can't right now (I'm at a conference)

Enjoy

but I can try later or in a day or 2.

It would be appreciated

sberyozkin commented 1 year ago

@edeandrea Hey, one more comment re:

Just trying to load the UI was causing the error, which is kind of weird because it's a GET, which shouldn't need a pre-flight request.

Do you recall if it was failing with you configuring a wildcard or with only CORS enabled ? If this GET was same origin request, then CORS filter should handle it even without one having to enable a wildcard - but it works only if the CORS filter detects if the Origin header matches what RoutingContext says about the current request. So in OpenShift, it might not match if Origin is the the exposed route URL and RoutingContexts shows an internal URL. Having a wildcard enabled should work though

edeandrea commented 1 year ago

It was a wildcard (*). It's been a wildcard for many versions now.

sberyozkin commented 1 year ago

@edeandrea Can you link me please to where it is configured in Quarkus Heroes repo ?

sberyozkin commented 1 year ago

I wonder, if you supply it via the config map for example if it is somehow gets escaped or something like that

edeandrea commented 1 year ago

No its set directly in the application.properties

https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-fights/src/main/resources/application.properties#L15_L17

I’m on a plane today and the Wi-Fi is terrible. I’m back in the office tomorrow and will work on setting up a reproducer.

sberyozkin commented 1 year ago

Sorry @edeandrea I could've checked, just found it, yes, it looks fine.

edeandrea commented 1 year ago

I did just check myself to make sure it wasn’t overridden anywhere in any of the ConfigMaps, and verified that it is not.

sberyozkin commented 1 year ago

Thanks @edeandrea

FYI,

https://github.com/quarkusio/quarkus/blob/3.2.0.Final/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/cors/CORSFilter.java#L43

checks if it is a wildcard only, see

https://github.com/quarkusio/quarkus/blob/3.2.0.Final/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/cors/CORSFilter.java#L87

and then:

https://github.com/quarkusio/quarkus/blob/3.2.0.Final/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/cors/CORSFilter.java#L143

holly-cummins commented 1 year ago

I wonder if this is an area where our test coverage isn't what it could be, because it's really &*!&(! hard to set up a test rig where we have services on different domains talking to each other, with browsers thrown into the mix too. As @sberyozkin is discovering, even just trying to reproduce manually takes a lot of effort.

edeandrea commented 1 year ago

Hi @sberyozkin I've spent some time today trying to re-create this issue using 2 separate OpenShift clusters, deploying the UI on one cluster and the rest of the services on another cluster. I can't seem to reproduce it. Things seem to work fine.

Maybe there was some network hiccup or something weird when I tried to do this originally? I'm not sure.

@holly-cummins I was also able to get JKube remote dev to work as well, even in this multi-cluster setup, so I think we can close this issue? If I come across it again I'll go ahead and re-open it.

Thank you @sberyozkin for diving into it so quickly!

sberyozkin commented 1 year ago

Hey @edeandrea Glad to hear it, np at all, good things are working now. P.S. Will ping you and Holly to discuss how SSO can be wired in Cheers

edeandrea commented 1 year ago

There's an open issue for it thats been around for some time...

https://github.com/quarkusio/quarkus-super-heroes/issues/3

t-shaguy commented 1 year ago

@edeandrea and All

Many thanks, this. quarkus.http.cors.origins=/.*/ worked for me