Fix/18552 cors wildcard credentials#19020
Fix/18552 cors wildcard credentials#19020Bellambharath wants to merge 4 commits intoOrchardCMS:mainfrom
Conversation
|
Could you please share a GIF screenshot, before I test it myself |
|
If I understand this PR correctly, the same check done by the editor UI is now also done by the |
|
That's what I understand, but I will check the PR |
There was a problem hiding this comment.
Pull request overview
Updates OrchardCore CORS handling so that a literal "*" in AllowedOrigins is treated as “any origin” for validation/loading, and prevents persisting insecure policies that combine “any origin” with credentials (fixing the lock-out scenario described in #18552).
Changes:
- Detect
"*"(with trimming) inAllowedOriginsand treat it asAllowAnyOriginduring CORS policy loading. - Block saving CORS policies that effectively allow any origin (including
"*") whileAllowCredentialsis enabled. - Make “any origin” detection null-safe for
AllowedOrigins.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/OrchardCore.Modules/OrchardCore.Cors/Services/CorsOptionsConfiguration.cs | Adds wildcard-origin detection during policy loading and skips insecure policies. |
| src/OrchardCore.Modules/OrchardCore.Cors/Controllers/AdminController.cs | Updates “any origin” validation to also treat "*" (trimmed) as any origin and avoids null refs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if (corsPolicy.ExposedHeaders?.Length > 0) | ||
| { | ||
| configurePolicy.WithExposedHeaders(corsPolicy.ExposedHeaders); | ||
| } | ||
| }); | ||
|
|
||
| if (corsPolicy.IsDefaultPolicy) | ||
| { | ||
| options.DefaultPolicyName = corsPolicy.Name; | ||
| } | ||
| } | ||
|
|
||
| options.DefaultPolicyName ??= corsSettings.Policies.FirstOrDefault()?.Name; | ||
| } | ||
| options.DefaultPolicyName ??= corsSettings.Policies.FirstOrDefault()?.Name; | ||
| } |
There was a problem hiding this comment.
options.DefaultPolicyName ??= corsSettings.Policies.FirstOrDefault()?.Name; can set the default policy name to a policy that was skipped above (e.g., invalid AllowCredentials + any-origin). That leaves DefaultPolicyName pointing at a policy that was never added via AddPolicy, which can cause runtime failures when the pipeline calls UseCors() without a policy name. Prefer selecting the first successfully-added policy name (track it during the loop), or leave DefaultPolicyName unset when no policies were loaded.
| if (corsPolicy.AllowCredentials && allowAnyOrigin) | ||
| { | ||
| _logger.LogWarning( | ||
| "Using AllowCredentials and AllowAnyOrigin at the same time is considered a security risk, the {PolicyName} policy will not be loaded.", |
There was a problem hiding this comment.
The warning text mentions "AllowAnyOrigin" but this branch is now also triggered when AllowedOrigins contains the wildcard "" (even if AllowAnyOrigin is false). Consider rewording to refer to "any origin (including '')" so logs accurately reflect the configuration being rejected.
| "Using AllowCredentials and AllowAnyOrigin at the same time is considered a security risk, the {PolicyName} policy will not be loaded.", | |
| "Using AllowCredentials with any origin (including '*') is considered a security risk, the {PolicyName} policy will not be loaded.", |
| public void Configure(CorsOptions options) | ||
| { | ||
| var corsSettings = _corsService.GetSettingsAsync().GetAwaiter().GetResult(); | ||
| if (corsSettings?.Policies == null || !corsSettings.Policies.Any()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| foreach (var corsPolicy in corsSettings.Policies) | ||
| { | ||
| 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; | ||
| } | ||
|
|
||
| options.AddPolicy(corsPolicy.Name, configurePolicy => | ||
| { | ||
| if (corsPolicy.AllowAnyHeader) | ||
| { | ||
| configurePolicy.AllowAnyHeader(); | ||
| } | ||
| else | ||
| { | ||
| configurePolicy.WithHeaders(corsPolicy.AllowedHeaders); | ||
| } | ||
| { | ||
| var corsSettings = _corsService.GetSettingsAsync().GetAwaiter().GetResult(); | ||
| if (corsSettings?.Policies == null || !corsSettings.Policies.Any()) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Indentation for the method opening brace/body is inconsistent with the surrounding 4-space indentation in this file (e.g., the { under Configure is currently offset). Please align formatting to match the rest of the module to avoid noisy diffs going forward.
| var allowAnyOrigin = corsPolicy.AllowAnyOrigin | ||
| || corsPolicy.AllowedOrigins?.Any(origin => | ||
| string.Equals(origin?.Trim(), CorsConstants.AnyOrigin, StringComparison.Ordinal)) == true; |
There was a problem hiding this comment.
The "any origin" detection logic is duplicated here and in AdminController.IsAnyOriginAllowed. To avoid the two drifting (e.g., different trimming/comparison rules), consider extracting a shared helper (e.g., on the settings model or a small internal utility) and reusing it in both places.
| private static bool IsAnyOriginAllowed(CorsPolicyViewModel corsPolicyViewModel) | ||
| => corsPolicyViewModel.AllowAnyOrigin || corsPolicyViewModel.AllowedOrigins.Any(origin => origin == CorsConstants.AnyOrigin); | ||
| => corsPolicyViewModel.AllowAnyOrigin | ||
| || corsPolicyViewModel.AllowedOrigins?.Any(origin => | ||
| string.Equals(origin?.Trim(), CorsConstants.AnyOrigin, StringComparison.Ordinal)) == true; |
There was a problem hiding this comment.
This method now treats an origin entry of "" (including whitespace-trimmed) as "any origin". Since the UI warning shown on save is framed in terms of "AllowAnyOrigin", it can become confusing when the checkbox is off but "" is present in the list. Consider adjusting the user-facing warning text to reference "any origin / '*'" rather than only the checkbox name.
|
@hishamco Thanks for checking! Let me know if any changes are needed. If everything looks good, could you please approve the PR? |
|
Seems Copilot suggested something :) |
Summary
Testing
Fixes #18552