"*" origin and AllowCredentials shouldn't set together

[hishamco]

Fixes #18552

[hishamco]

Seems we need to add another check if the Cors set from appsettings.json, @Piedone, I think logging a warning is enough in this case, or shall we throw an exception coz it will break the site

[hishamco]

FYI the current implementation shows

Specifying AllowAnyOrigin and AllowCredentias is an insecure configuration and can result in cross-site request forgery. The CORS service returns an invalid CORS response when an app is configured with both methods.
Affected policies: New policy

but still accepts the input and the exception throws in the next request

[hishamco]

@rjpowers10 could please confirm

[Piedone]

Please see #18552 (comment).

[hishamco]

it reverts." feature

Which feature do you refer to?

[Piedone]

[Piedone]

This displays a warning when the offending configuration is saved, but saves the values. Instead, it should add a validation error and prevent saving the value in the first place.

[sebastienros]

@rjpowers10 please comment on the PR

[rjpowers10]

I added a few minor comments but the validation change is what I was expecting.

[Piedone]

Did you submit your comments? Because nothing appears for me here.

[hishamco]

Is there anything else to add here, or shall we merge this?

[Piedone]

I'd appreciate if before these questions you'd please take a look at what the reviewer has written; it's a recurring theme that I come back with links to my prior comments. Because no, we shouldn't merge this until you address my comment here: #18554 (review). Please take a look.

[hishamco]

I got an approval from @rjpowers10; moreover, there's a reason that prevents me from achieving this easily, because the UI is controlled at the client-side. Let me revise this PR again

[hishamco]

@Piedone, as I said, the form relies on client-side, also the policies are not preserved until policyWarnings.Count == 0 satisfied

You could give it a try

[Piedone]

Well, then something needs to change because this is really confusing UX.

BTW the wrong configuration is the default when you add a new policy. Please also remove that footgun.

[Piedone]

I just noticed:

OrchardCore/src/OrchardCore.Modules/OrchardCore.Cors/Services/CorsOptionsConfiguration.cs

Lines 28 to 32 in 8bac37a

	if (corsPolicy.AllowCredentials && corsPolicy.AllowAnyOrigin)
	{
	_logger.LogWarning("Using AllowCredentials and AllowAnyOrigin at the same time is considered a security risk, the {PolicyName} policy will not be loaded.", corsPolicy.Name);
	continue;
	}

Apparently, this isn't working well enough. This is what needs to be fixed.

[hishamco]

Well, then something needs to change because this is really confusing UX.

That's why I show the warning on the UI to let the user know there's something that needs attention

BTW the wrong configuration is the default when you add a new policy. Please also remove that footgun.

Did you mean to check if there's a wrong configuration in the appsettings.json file? coz the UI will prevent this issue

[Piedone]

That's why I show the warning on the UI to let the user know there's something that needs attention

Saving (seemingly) the policy while showing a warning that it's wrong and you shouldn't do that is confusing UX.

Did you mean to check if there's a wrong configuration in the appsettings.json file? coz the UI will prevent this issue

Add a policy, leave everything on default, Save, notice warning. The defaults are wrong.