Improve OpenApi CORS message #28384
Improve OpenApi CORS message #28384
Conversation
There was a problem hiding this comment.
LGTM if we do need this log.
Still wondering if this shouldn't be moved to the vertx extension, though, since this is really not specific to openapi. But that's probably a separate issue.
Let's wait for @gsmet 's opinion though, as I'm sure he has one :)
The log is needed as it is doc-ed as a response to the Snyk SAST failure (in a separate analysis doc). While removing the default properties completely is a breaking change so it won't work as well.
This is specific to |
Ok, I thought those wildcard CORS properties were equivalent to simply no CORS properties, but apparently that's not true. So I assume those wildcard CORS properties are useful somehow. I assume those wildcard properties are also useful in development mode, probably because development setups would fail without those. In production though... I'm sorry if I'm being thickheaded, but I keep thinking there's something strange here... To sum up, we have a default that is insecure in some cases (which we cannot detect), and presumably useful in others (which we cannot detect either). And, whatever the case, we would rather have users pick their configuration explicitly. If we have an insecure default that we don't want people to rely on anyway, maybe we should reconsider that default, at least in the next minor or major, to be secure by default? The only "secure" option I'm seeing is simply not setting any CORS headers at all when they are not enabled explicitly (in production). From what I understand that will cause errors in the browser, but at least those errors mention the need to enable CORS in the REST API, and from there users can go configure their application correctly. Maybe that's already planned? I couldn't find a GitHub issue about that, though. |
|
Hey @yrodiere Np, thanks for trying to make sure all is clear. So this log message will only be shown in the production mode. Wildcard CORS properties are really never useful in the production mode - but we just all tend do it to get past the annoying CORS errors which might show up, it is just convenient, but Quarkus can't offer this convenience by default without saying anything as eventually someone will report CVE. |
It is in the product as this is why I've found about it via a product Snyk scan so we can't really remove these default properties easily. But deprecating them in a message would be even more annoying to the users who just would like to try it locally. So not sure what else we can do but make this message more informative as you have proposed |
Thanks for clarifying, I was suspecting as much but wasn't sure, given that's not really my area of expertise :)
Sure, it's still a good idea to keep and improve the message in the meantime.
Right, if it's really too disruptive, regardless of products, maybe that's not something we want to do in a minor. Perhaps we could plan this change for the next major of Quarkus? No idea when/if that will happen of course, but in a major version it's more reasonable to introduce disruptive changes, especially if they make Quarkus more secure by default. I created #28397, let's continue the discussion there :) @gsmet Summary of the discussion so you don't have to read it all:
|
|
@sberyozkin Fine by me, thanks! |
Fixes #28377.
Improving the message as proposed by Yoann but without a specific doc link.