KTOR-2872 Add check to prevent anyHost with allowCredentials

[hfhbd]

Subsystem
Server, core, CORS

Motivation
https://youtrack.jetbrains.com/issue/KTOR-2872
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSNotSupportingCredentials

Solution
Added check

[rsinukov]

Hi @hfhbd.
What do you thing about changing it from exception to a warning?

1. This combination can be useful in tests.
2. Adding extra restrictions can break exisitig clients. Even if it's for a right reasons, better to avoid such changes. Adding a warning and later promoting it to the error is a berret option.

[hfhbd]

@rsinukov Sure, I can change it to warning, but are you sure you can disable this behavior for tests?
There is no public configuration to disable it:

https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSNotSupportingCredentials
https://docs.microsoft.com/en-us/aspnet/core/security/cors?view=aspnetcore-5.0#credentials-in-cross-origin-requests-1
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#requests_with_credentials

Without disabling, I don't see a point downgrade the error to warning.

[rsinukov]

Ok, I think you are right. Can you update CORSTest, please? It fails since it uses this combination.

[hfhbd]

@rsinukov I removed 2 tests, because the used configuration is not possible:

• testSimpleStarCredentials same like testSimpleStar but with credentials
• testSimpleNullAllowCredentials should return the sent origin and not *, lines, but the case allowsAnyHost && allowCredentials is not possible after this fix. So it will always go to else, which is tested by other tests.

[rsinukov]

LGTM. Will merge after CI build

[raresvla]

@hfhbd , @rsinukov I don't think this fixes a problem, it rather creates one:

Before this PR was merged, you could allow cross-site requests from any host, even in combination with allowCredentials, since the plugin was not setting the Access-Control-Allow-Origin to the illegal value "*", but rather to the request origin itself. This trick shouldn't have triggered the problem referenced in the newly added error.

How can one obtain the same behavior now (i.e. essentially allow cross-domain requests from any host)?

[hfhbd]

@raresvla Sorry, I did't get your problem.

1. CORS is only supported in browsers.
2. The used combination allowCredentials and wildcard origins is illegal, according to MDN: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#preflighted_requests (the line above), https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSNotSupportingCredentials or https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#requests_with_credentials

You won't get any errors on the client side, if you are using a non-browser HTTP client, which does not require CORS at all.

[raresvla]

@hfhbd Thanks for looking into this. Let me be more specific on the context I'm referring at:

• I have a Ktor application that always receives cross-site requests
• this application also works with cookies (3rd party cookies)

Therefore, I'm currently configuring the CORS plugin as follows:

install(CORS) {
    allowCredentials = true
    anyHost()
    // .. more settings here
}

This setup worked before and was not triggering a browser complaint, given that the illegal "*" (wildcard) value for Access-Control-Allow-Origin was not set in this case.

Essentially, the CORS plugin was reflecting the caller (origin) host in the Access-Control-Allow-Origin, effectively allowing cross-site requests from any host -- something we actually expect. This behavior is also aligned with the standard recommendations

The server must not specify the "*" wildcard for the Access-Control-Allow-Origin response-header value, but must instead specify an explicit origin; for example: Access-Control-Allow-Origin: https://example.com

[hfhbd]

@raresvla
Okay, so you only want to revert this change? https://github.com/ktorio/ktor/pull/2536/files#diff-887f99f9b2aca204f8fe9a8f88fbbc2b0432265e41598de7109624ab125454a5L168-L171

If so, how can you apply the CORS plugin with the new init check here? https://github.com/hfhbd/ktor/blob/32882d108a406a3d415fe4593a6fe46b411ee33d/ktor-server/ktor-server-plugins/ktor-server-cors/jvm/src/io/ktor/server/plugins/CORS.kt#L80

Found the standard:
https://fetch.spec.whatwg.org/#cors-protocol-and-credentials

Request’s credentials mode	Access-Control-Allow-Origin	Access-Control-Allow-Credentials	Shared?	Notes
"include"	*	true	❌	If credentials mode is "include", then Access-Control-Allow-Origin cannot be *.

Maybe I read the table wrong, or still don't get your problem :)

[hfhbd]

@raresvla Or do you also allow other hosts in your config?
I have no problems to revert this PR at all: My motivation was to prevent CORS errors during fetching during runtime/production, by checking this combination when starting the Ktor server. I had this issue with a productive application and resolving it took some time, so by adding this check I wanted to prevent other running into this issue too.

[raresvla]

@hfhbd I actually have no explicit allowed hosts in the config, it was anyHost() that used to do the trick: when allowCredentials was also true, it was setting Access-Control-Allow-Origin to the request origin value (not "*"), as if every host would have been explicitly allowed via hosts.

I agree with you that it could be a bit confusing to have this dual behavior (i.e. depending on other settings, return either wildcard "*" or the origin host) -- but in my case, this is exactly what I need, since:

• I want to allow browser requests from (really) any origin
• I also want to support cookies, therefore I must use the allowCredentials.

Again, before this PR was merged, for a request issued by JS on https://foo.com the CORS plugin was returning:

Access-Control-Allow-Origin: https://foo.com
Access-Control-Allow-Credentials: true

[glasser]

I believe @raresvla is right here.

Browsers do not accept the combination of literally access-control-allow-origin: * and access-control-allow-credentials: true, but that's not what ktor did before this PR — it interpreted anyHost(); allowCredentials = true as "echo the given origin in access-control-allow-origin".

You might argue that the plugin's API is confusing because the things you type don't really map directly onto the headers it sets. But the previous behavior was useful and can't be replicated with the plugin any more.

[glasser]

Specifically I think the PR should be entirely reverted. Would it help if I opened a PR?

(Another alternative would be to explicitly have an option like allowAnyHostButDoItByEchoingTheOriginInsteadOfWithAStar() but I don't know if that would actually be an improvement.)

[hfhbd]

Thanks for the feedback @glasser @raresvla!
You are right, the name for the option allowAnyHost is wrong and mean allowAnyHostButDoItByEchoingTheOriginInsteadOfWithAStarIfAllowCredentialsIsTrue, which isn't an improvement at all.

I think it would make sense to revert this PR: See #2767