Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,13 @@ public async Task<IActionResult> RequestPasswordReset(CancellationToken cancella

// If this feature is switched off in configuration, the UI will be amended to not make the request to reset password available.
// So this is just a server-side secondary check.
// ApplicationUrlNotConfigured is also surfaced since it is a server-wide configuration issue, not user-specific.
// Regardless of other status values, it will just return Ok, so you can't use this endpoint to determine whether the email exists in the system.
return result.Result == UserOperationStatus.CannotPasswordReset
? BadRequest()
: Ok();
return result.Result switch
{
UserOperationStatus.CannotPasswordReset => BadRequest(),
UserOperationStatus.ApplicationUrlNotConfigured => UserOperationStatusResult(result.Result),
_ => Ok(),
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ protected IActionResult UserOperationStatusResult(UserOperationStatus status, Er
.WithTitle("Unknown failure")
.WithDetail(errorMessageResult?.Error?.ErrorMessage ?? "The error was unknown")
.Build()),
UserOperationStatus.ApplicationUrlNotConfigured => BadRequest(problemDetailsBuilder
.WithTitle("Application URL not configured")
.WithDetail("The application URL is not configured. Set Umbraco:CMS:WebRouting:UmbracoApplicationUrl in configuration, or change Umbraco:CMS:WebRouting:ApplicationUrlDetection to 'FirstRequest' or 'EveryRequest'.")
.Build()),
Comment thread
AndyButland marked this conversation as resolved.
_ => StatusCode(StatusCodes.Status500InternalServerError, problemDetailsBuilder
.WithTitle("Unknown user operation status.")
.Build()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@
.WithDetail("The target user type does not support this operation.")
.Build()),
UserOperationStatus.Forbidden => Forbidden(),
UserOperationStatus.ApplicationUrlNotConfigured => BadRequest(problemDetailsBuilder
.WithTitle("Application URL not configured")
.WithDetail("The application URL is not configured. Set Umbraco:CMS:WebRouting:UmbracoApplicationUrl in configuration, or change Umbraco:CMS:WebRouting:ApplicationUrlDetection to 'FirstRequest' or 'EveryRequest'.")
.Build()),

Check warning on line 146 in src/Umbraco.Cms.Api.Management/Controllers/User/UserOrCurrentUserControllerBase.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Large Method

UserOperationStatusResult increases from 133 to 137 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
Comment thread
AndyButland marked this conversation as resolved.
_ => StatusCode(StatusCodes.Status500InternalServerError, problemDetailsBuilder
.WithTitle("Unknown user operation status.")
.Build()),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Hosting;
using Umbraco.Cms.Core.Models.Membership;
using Umbraco.Cms.Core.Security;
Expand All @@ -16,7 +13,6 @@ namespace Umbraco.Cms.Api.Management.Security;
/// </summary>
public class ForgotPasswordUriProvider : IForgotPasswordUriProvider
{

private readonly ICoreBackOfficeUserManager _userManager;
private readonly IHostingEnvironment _hostingEnvironment;
private readonly IHttpContextAccessor _httpContextAccessor;
Expand All @@ -37,29 +33,34 @@ public ForgotPasswordUriProvider(
_httpContextAccessor = httpContextAccessor;
}

/// <inheritdoc/>
public async Task<Attempt<Uri, UserOperationStatus>> CreateForgotPasswordUriAsync(IUser user)
{
Attempt<string, UserOperationStatus> tokenAttempt = await _userManager.GeneratePasswordResetTokenAsync(user);
HttpRequest? request = _httpContextAccessor.HttpContext?.Request ?? throw new NotSupportedException("Needs a HttpContext");
Comment thread
AndyButland marked this conversation as resolved.
Outdated

if (tokenAttempt.Success is false)
Uri? appUrl = _hostingEnvironment.ApplicationMainUrl;
if (appUrl is null)
{
return Attempt.FailWithStatus(tokenAttempt.Status, new Uri(string.Empty));
return Attempt.FailWithStatus<Uri, UserOperationStatus>(UserOperationStatus.ApplicationUrlNotConfigured, default!);
}

HttpRequest? request = _httpContextAccessor.HttpContext?.Request;
if (request is null)
Attempt<string, UserOperationStatus> tokenAttempt = await _userManager.GeneratePasswordResetTokenAsync(user);

if (tokenAttempt.Success is false)
{
throw new NotSupportedException("Needs a HttpContext");
return Attempt.FailWithStatus(tokenAttempt.Status, new Uri(string.Empty));
Comment thread
AndyButland marked this conversation as resolved.
Outdated
}

var uriBuilder = new UriBuilder(_hostingEnvironment.ApplicationMainUrl);
uriBuilder.Path = BackOfficeLoginController.LoginPath;
uriBuilder.Query = QueryString.Create(new KeyValuePair<string, string?>[]
var uriBuilder = new UriBuilder(appUrl)
{
new ("flow", "reset-password"),
new ("userId", user.Key.ToString()),
new ("resetCode", tokenAttempt.Result.ToUrlBase64()),
}).ToUriComponent();
Path = BackOfficeLoginController.LoginPath,
Query = QueryString.Create(new KeyValuePair<string, string?>[]
{
new("flow", "reset-password"),
new("userId", user.Key.ToString()),
new("resetCode", tokenAttempt.Result.ToUrlBase64()),
}).ToUriComponent(),
};

return Attempt.SucceedWithStatus(UserOperationStatus.Success, uriBuilder.Uri);
}
Expand Down
31 changes: 17 additions & 14 deletions src/Umbraco.Cms.Api.Management/Security/InviteUriProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ public InviteUriProvider(
IHttpContextAccessor httpContextAccessor,
IHostingEnvironment hostingEnvironment)
{

_userManager = userManager;
_httpContextAccessor = httpContextAccessor;
_hostingEnvironment = hostingEnvironment;
Expand All @@ -43,27 +42,31 @@ public InviteUriProvider(
/// </returns>
public async Task<Attempt<Uri, UserOperationStatus>> CreateInviteUriAsync(IUser invitee)
{
Attempt<string, UserOperationStatus> tokenAttempt = await _userManager.GenerateEmailConfirmationTokenAsync(invitee);
HttpRequest? request = _httpContextAccessor.HttpContext?.Request ?? throw new NotSupportedException("Needs a HttpContext");
Comment thread
AndyButland marked this conversation as resolved.
Outdated

if (tokenAttempt.Success is false)
Uri? appUrl = _hostingEnvironment.ApplicationMainUrl;
if (appUrl is null)
{
return Attempt.FailWithStatus(tokenAttempt.Status, new Uri(string.Empty));
return Attempt.FailWithStatus<Uri, UserOperationStatus>(UserOperationStatus.ApplicationUrlNotConfigured, default!);
}

HttpRequest? request = _httpContextAccessor.HttpContext?.Request;
if (request is null)
Attempt<string, UserOperationStatus> tokenAttempt = await _userManager.GenerateEmailConfirmationTokenAsync(invitee);

if (tokenAttempt.Success is false)
{
throw new NotSupportedException("Needs a HttpContext");
return Attempt.FailWithStatus(tokenAttempt.Status, new Uri(string.Empty));
Comment thread
AndyButland marked this conversation as resolved.
Outdated
}

var uriBuilder = new UriBuilder(_hostingEnvironment.ApplicationMainUrl);
uriBuilder.Path = BackOfficeLoginController.LoginPath;
uriBuilder.Query = QueryString.Create(new KeyValuePair<string, string?>[]
var uriBuilder = new UriBuilder(appUrl)
{
new ("flow", "invite-user"),
new ("userId", invitee.Key.ToString()),
new ("inviteCode", tokenAttempt.Result.ToUrlBase64()),
}).ToUriComponent();
Path = BackOfficeLoginController.LoginPath,
Query = QueryString.Create(new KeyValuePair<string, string?>[]
{
new ("flow", "invite-user"),
new ("userId", invitee.Key.ToString()),
new ("inviteCode", tokenAttempt.Result.ToUrlBase64()),
}).ToUriComponent()
};

return Attempt.SucceedWithStatus(UserOperationStatus.Success, uriBuilder.Uri);
}
Expand Down
26 changes: 26 additions & 0 deletions src/Umbraco.Core/Configuration/Models/ApplicationUrlDetection.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
namespace Umbraco.Cms.Core.Configuration.Models;

/// <summary>
/// Specifies how the application main URL is detected from incoming HTTP requests.
/// </summary>
public enum ApplicationUrlDetection
{
/// <summary>
/// No auto-detection. The application URL must be explicitly configured
/// via <see cref="WebRoutingSettings.UmbracoApplicationUrl" />.
/// Emails will use relative links if no URL is configured.
Comment thread
AndyButland marked this conversation as resolved.
Outdated
/// </summary>
None,

/// <summary>
/// The URL is set from the first HTTP request and then locked.
/// Subsequent requests with different host headers are ignored.
/// </summary>
FirstRequest,

/// <summary>
/// The URL is updated from every new incoming HTTP request (legacy behavior).
/// This is vulnerable to host header poisoning.
/// </summary>
EveryRequest,
}
12 changes: 12 additions & 0 deletions src/Umbraco.Core/Configuration/Models/WebRoutingSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ public class WebRoutingSettings
/// </summary>
internal const bool StaticUseStrictDomainMatching = false;

/// <summary>
/// The default value for application URL detection mode.
/// </summary>
internal const ApplicationUrlDetection StaticApplicationUrlDetection = ApplicationUrlDetection.None;

/// <summary>
/// Gets or sets a value indicating whether to check if any routed endpoints match a front-end request before
/// the Umbraco dynamic router tries to map the request to an Umbraco content item.
Expand Down Expand Up @@ -123,6 +128,13 @@ public class WebRoutingSettings
/// </summary>
public string UmbracoApplicationUrl { get; set; } = null!;

/// <summary>
/// Gets or sets a value controlling how the application main URL is auto-detected
/// from incoming HTTP requests (<see cref="ApplicationUrlDetection" />).
/// </summary>
[DefaultValue(StaticApplicationUrlDetection)]
public ApplicationUrlDetection ApplicationUrlDetection { get; set; } = StaticApplicationUrlDetection;

/// <summary>
/// Gets or sets a value indicating whether strict domain matching is used when finding content to match the request.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/Umbraco.Core/EmbeddedResources/Lang/en.xml
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@
<key alias="httpsCheckExpiredCertificate">Your website's SSL certificate has expired.</key>
<key alias="httpsCheckExpiringCertificate">Your website's SSL certificate is expiring in %0% days.</key>
<key alias="healthCheckInvalidUrl">Error pinging the URL %0% - '%1%'</key>
<key alias="httpsCheckNoApplicationUrl">The application URL is not available. Configure Umbraco:CMS:WebRouting:UmbracoApplicationUrl or change ApplicationUrlDetection to enable this check.</key>
<key alias="httpsCheckIsCurrentSchemeHttps">You are currently %0% viewing the site using the HTTPS scheme.</key>
<key alias="httpsCheckConfigurationRectifyNotPossible">The appSetting 'Umbraco:CMS:Global:UseHttps' is set to 'false' in
your appSettings.json file. Once you access this site using the HTTPS scheme, that should be set to 'true'.
Expand Down
1 change: 1 addition & 0 deletions src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@
<key alias="httpsCheckExpiredCertificate">Your website's SSL certificate has expired.</key>
<key alias="httpsCheckExpiringCertificate">Your website's SSL certificate is expiring in %0% days.</key>
<key alias="healthCheckInvalidUrl">Error pinging the URL %0% - '%1%'</key>
<key alias="httpsCheckNoApplicationUrl">The application URL is not available. Configure Umbraco:CMS:WebRouting:UmbracoApplicationUrl or change ApplicationUrlDetection to enable this check.</key>
<key alias="httpsCheckIsCurrentSchemeHttps">You are currently %0% viewing the site using the HTTPS scheme.</key>
<key alias="httpsCheckConfigurationRectifyNotPossible">The appSetting 'Umbraco:CMS:Global:UseHttps' is set to 'false' in
your appSettings.json file. Once you access this site using the HTTPS scheme, that should be set to 'true'.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,16 @@ protected async Task<HealthCheckStatus> CheckForHeader()
// Access the site home page and check for the click-jack protection header or meta tag
var url = _hostingEnvironment.ApplicationMainUrl?.GetLeftPart(UriPartial.Authority);

if (url is null)
{
return new HealthCheckStatus(
LocalizedTextService.Localize("healthcheck", "httpsCheckNoApplicationUrl"))
{
ResultType = StatusResultType.Info,
ReadMoreLink = ReadMoreLink,
};
}

try
{
using HttpResponseMessage response = await HttpClient.GetAsync(url);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ private async Task<HealthCheckStatus> CheckForHeaders()
var success = false;
var url = _hostingEnvironment.ApplicationMainUrl?.GetLeftPart(UriPartial.Authority);

if (url is null)
{
return new HealthCheckStatus(
_textService.Localize("healthcheck", "httpsCheckNoApplicationUrl"))
{
ResultType = StatusResultType.Info,
ReadMoreLink = Constants.HealthChecks.DocumentationLinks.Security.ExcessiveHeadersCheck,
};
}

// Access the site home page and check for the headers
using var request = new HttpRequestMessage(HttpMethod.Head, url);
try
Expand Down
29 changes: 29 additions & 0 deletions src/Umbraco.Core/HealthChecks/Checks/Security/HttpsCheck.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,27 @@
return sslErrors == SslPolicyErrors.None;
}

private HealthCheckStatus? CheckApplicationUrlAvailable()
{
if (_hostingEnvironment.ApplicationMainUrl is not null)
{
return null;
}

return new HealthCheckStatus(
_textService.Localize("healthcheck", "httpsCheckNoApplicationUrl"))
{
ResultType = StatusResultType.Info,
};
}

private async Task<HealthCheckStatus> CheckForValidCertificate()
{
if (CheckApplicationUrlAvailable() is HealthCheckStatus unavailable)
{
return unavailable;
}

Check warning on line 97 in src/Umbraco.Core/HealthChecks/Checks/Security/HttpsCheck.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Complex Method

CheckForValidCertificate increases in cyclomatic complexity from 9 to 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
string message;
StatusResultType result;

Expand Down Expand Up @@ -154,6 +173,11 @@

private Task<HealthCheckStatus> CheckIfCurrentSchemeIsHttps()
{
if (CheckApplicationUrlAvailable() is HealthCheckStatus unavailable)
{
return Task.FromResult(unavailable);
}

Uri uri = _hostingEnvironment.ApplicationMainUrl;
var success = uri.Scheme == Uri.UriSchemeHttps;

Expand All @@ -169,6 +193,11 @@

private Task<HealthCheckStatus> CheckHttpsConfigurationSetting()
{
if (CheckApplicationUrlAvailable() is HealthCheckStatus unavailable)
{
return Task.FromResult(unavailable);
}

var httpsSettingEnabled = _globalSettings.CurrentValue.UseHttps;
Uri uri = _hostingEnvironment.ApplicationMainUrl;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,11 @@ public enum UserOperationStatus
/// The operation failed because the username is invalid.
/// </summary>
InvalidUserName,

/// <summary>
/// The operation failed because the application URL is not configured.
/// Set <c>Umbraco:CMS:WebRouting:UmbracoApplicationUrl</c> or change
/// <c>ApplicationUrlDetection</c> to <c>FirstRequest</c> or <c>EveryRequest</c>.
/// </summary>
ApplicationUrlNotConfigured,
}
Loading
Loading