Take regexps into account when setting access-control-allow-credentials

[gsmet]

We used to only consider exact matches which looks like an oversight.

Fixes #43736

[gsmet]

Failures are unrelated (Docker rate limiting in action).

[michalvavrik]

Changes, code and expected behavior makes sense to me, but I'll leave it to @sberyozkin because I didn't change this file before IIRC.

[sberyozkin]

Thanks Guillaume @gsmet for starting looking at it, I suggested a minor optimization.
I think the intention was to avoid loose matches with the wildcard or regexes for this specific property, but since we do allow regex for the actual Origin match, it is indeed unexpected that it has no impact on setting the allowed credentials headers.

I think we should either update the origins configuration property to recommend users to avoid wildcard or regular expressions in prod and list precise Origin values, or keep insisting on the precise Origin match only... I'm leaning toward retaining the current status quo, but it can become difficult to manage when a lot of origins are involved...

How about adding a dedicated property on the allowed credentials header, we retain the current status quo by default, only if a precise Origin match is achieved, this header is returned. If users say access-control-allow-credentials-strict-match=false, it is a permission to Quarkus to allow it in the response even if someone configured allowed-origins=*.

It may be one more property too many, but it will require users double-confirm their intention that they really want to allow access-control-allow-credentials from the JavaScript....

[sberyozkin]

Hi @gsmet FYI, https://stackoverflow.com/questions/79175348/response-to-preflight-request-doesnt-pass-access-control-check-no-access-con, it is a bit related

[sberyozkin]

@gsmet You are right that having regexs to support various subdomains with otherwise rather complete URLs in the expressions is reasonable, I'd only like to exclude expressions like \.*\ to minimize non-intentional allowing of JavaScript accessing credentials. I'll be happy enough if we can specifically check if \.*\ was not used as a regex source, are you OK with it ?

[gsmet]

@sberyozkin I think I addressed your concern in the second commit. I let you check all is good for you as I'm not exactly familiar with this code :).

[sberyozkin]

Thanks very much @gsmet, let me do a minor tweak with a separate commit to try to restrict it a bit further

[gsmet]

Can you clarify what you're trying to do? I would like to avoid ending up with a Frankenstein monster and something that is hard to understand?

[sberyozkin]

@gsmet Sure, I'd like to make sure that if it is a wildcard (regex or not), then Access-Control-Allow-Credentials is null.

Currently you have:

boolean allowsOrigin = wildcardOrigin;
boolean originMatches = corsConfig.origins.isPresent() &&
        (corsConfig.origins.get().contains(origin) || isOriginAllowedByRegex(allowedOriginsRegex, origin));

Right now, we'll have both allowsOrigin and originMatches set to true if the origin contains /.*/ and therefore the access to credentials will be allowed.

I'd like to add only !wildcardOrigin to:

boolean allowsOrigin = wildcardOrigin;
boolean originMatches = !wildcardOrigin && corsConfig.origins.isPresent() &&
        (corsConfig.origins.get().contains(origin) || isOriginAllowedByRegex(allowedOriginsRegex, origin));

It will avoid allowing credential access, and also do a tiny optimization, if the origin is set to /.*/ then isOriginAllowedByRegex won't run because allowsOrigin is already set to true with your latest update...

I've just realized only now once I started typing that this is all I wanted to do, I was not sure what exactly to suggest...

I just wanted to save you some time as opposed to keeping adding some vague comments :-), but please add this one extra boolean check and it will be good to go

[gsmet]

Ah yes, you're right, it should be fixed, thanks for catching it.

[sberyozkin]

@gsmet If you are busy with backports, I can do it, just let me know

[sberyozkin]

Hey @gsmet I did that minor test and added a test, should be ready to go... FYI, the allow credentials header is null if the Origin is wrong, but false it if it is not allowed but with the valid Origin...

Let me approve - and if you happy yourself with the latest update - please merge

Cheers !

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit cd28056.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[gsmet]

Thanks!