helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.52k stars 564 forks source link

CORS config in SE is confusing #8670

Open tjquinno opened 7 months ago

tjquinno commented 7 months ago

Environment Details


Problem Description

With CORS now implemented as a webserver feature, CORS configuration in 4.x SE is very confusing.

(See also #7978)

To make CORS work, users need to set up two separate parts of config. This example is similar to what's in our quickstart SE app:

server:
  port: 8080
  host: 0.0.0.0
  features:
    cors:

This part enables the CORS feature in the web server. You don't even need anything else under server.features.cors; enabled is true by default if the cors config node is present.

Users also need the following (at the top level of config):

cors: 
 paths:
   - path-pattern: /greet
     allow-origins: ["http://foo.com", "http://there.com", "http://other.com"]
     allow-methods: ["PUT", "DELETE", "GET"]
   - path-pattern: /observe/health
     allow-origins: [ "http://trusted.org" ]
     allow-methods: [ "GET" ]

This sets up the behavior of the CORS logic itself.

Omitting the first part of config leaves CORS disabled, whether the second part is present or not.

Omitting the second part uses default permissive CORS settings. Helidon currently ignores it if you move the second part under the server.features.cors section.


At a minimum, this is a doc issue. The CORS SE page largely addresses programmatic set-up for CORS. With CORS now a webserver feature that's no longer necessary (although it's available if the developer wants to use it). The doc page needs to also describe automatic discovery and config.

Beyond that, do we want to allow users to put all the CORS config under the server.features.cors section instead of having it split? If we add that ability we'd need to support both, at least for a while, to avoid breaking existing working projects.

tjquinno commented 6 months ago

Attaching a reproducer. See the README.md for instructions. cors-test.zip

tjquinno commented 6 months ago

Note - #7990 might be related.