Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORS broken? #34669

Closed
edeandrea opened this issue Jul 11, 2023 · 38 comments
Closed

CORS broken? #34669

edeandrea opened this issue Jul 11, 2023 · 38 comments
Assignees
Labels

Comments

@edeandrea
Copy link
Contributor

edeandrea commented Jul 11, 2023

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 edeandrea added the kind/bug Something isn't working label Jul 11, 2023
@edeandrea
Copy link
Contributor Author

@cescoffier ^^^

@sberyozkin
Copy link
Member

@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
Copy link
Contributor Author

edeandrea commented Jul 11, 2023

@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
Copy link
Member

@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
Copy link
Contributor Author

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

@sberyozkin
Copy link
Member

@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
Copy link
Member

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

@sberyozkin
Copy link
Member

@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
Copy link
Contributor Author

edeandrea commented Jul 11, 2023

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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

@edeandrea

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

@sberyozkin
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

holly-cummins commented Jul 11, 2023

@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
Copy link
Member

@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
Copy link
Member

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

@edeandrea
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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

@sberyozkin
Copy link
Member

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

@edeandrea
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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

@sberyozkin
Copy link
Member

sberyozkin commented Jul 11, 2023

@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
Copy link
Member

@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
Copy link
Contributor Author

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

@sberyozkin
Copy link
Member

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

@sberyozkin
Copy link
Member

sberyozkin commented Jul 13, 2023

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

@edeandrea
Copy link
Contributor Author

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
Copy link
Member

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

@edeandrea
Copy link
Contributor Author

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

@holly-cummins
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

quarkusio/quarkus-super-heroes#3

@t-shaguy
Copy link

@edeandrea and All

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

5 participants