From c4fdf808d362d2face254e9982de36eec0de117a Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 21 Sep 2021 10:49:45 -0600 Subject: [PATCH] Fixes 10730 - Route hijacking with public access --- .../UmbracoRouteValueTransformerTests.cs | 27 ++++++++++- .../UmbracoBuilderExtensions.cs | 4 +- .../UmbracoApplicationBuilder.Website.cs | 2 - .../Routing/IPublicAccessRequestHandler.cs | 18 ++++++++ .../PublicAccessRequestHandler.cs} | 46 ++++++------------- .../Routing/UmbracoRouteValueTransformer.cs | 32 +++++++------ 6 files changed, 76 insertions(+), 53 deletions(-) create mode 100644 src/Umbraco.Web.Website/Routing/IPublicAccessRequestHandler.cs rename src/Umbraco.Web.Website/{Middleware/PublicAccessMiddleware.cs => Routing/PublicAccessRequestHandler.cs} (77%) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs index d00f85fd0a48..11a24230516a 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs @@ -48,6 +48,10 @@ private UmbracoRouteValueTransformer GetTransformer( IPublishedRouter router = null, IUmbracoRouteValuesFactory routeValuesFactory = null) { + var publicAccessRequestHandler = new Mock(); + publicAccessRequestHandler.Setup(x => x.RewriteForPublishedContentAccessAsync(It.IsAny(), It.IsAny())) + .Returns((HttpContext ctx, UmbracoRouteValues routeVals) => Task.FromResult(routeVals)); + var transformer = new UmbracoRouteValueTransformer( new NullLogger(), ctx, @@ -59,7 +63,8 @@ private UmbracoRouteValueTransformer GetTransformer( filter ?? Mock.Of(x => x.IsDocumentRequest(It.IsAny()) == true), Mock.Of(), Mock.Of(), - Mock.Of()); + Mock.Of(), + publicAccessRequestHandler.Object); return transformer; } @@ -155,7 +160,7 @@ public async Task Assigns_PublishedRequest_To_UmbracoContext() } [Test] - public async Task Assigns_UmbracoRouteValues_To_HttpContext_Feature() + public async Task Null_When_No_Content_On_PublishedRequest() { IUmbracoContext umbracoContext = GetUmbracoContext(true); IPublishedRequest request = Mock.Of(); @@ -168,6 +173,24 @@ public async Task Assigns_UmbracoRouteValues_To_HttpContext_Feature() var httpContext = new DefaultHttpContext(); RouteValueDictionary result = await transformer.TransformAsync(httpContext, new RouteValueDictionary()); + UmbracoRouteValues routeVals = httpContext.Features.Get(); + Assert.IsNull(routeVals); + } + + [Test] + public async Task Assigns_UmbracoRouteValues_To_HttpContext_Feature() + { + IUmbracoContext umbracoContext = GetUmbracoContext(true); + IPublishedRequest request = Mock.Of(x => x.PublishedContent == Mock.Of()); + + UmbracoRouteValueTransformer transformer = GetTransformerWithRunState( + Mock.Of(x => x.TryGetUmbracoContext(out umbracoContext)), + router: GetRouter(request), + routeValuesFactory: GetRouteValuesFactory(request)); + + var httpContext = new DefaultHttpContext(); + RouteValueDictionary result = await transformer.TransformAsync(httpContext, new RouteValueDictionary()); + UmbracoRouteValues routeVals = httpContext.Features.Get(); Assert.IsNotNull(routeVals); Assert.AreEqual(routeVals.PublishedRequest, umbracoContext.PublishedRequest); diff --git a/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs index 72d8ec58ce6e..797a4b2202ae 100644 --- a/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs @@ -8,8 +8,6 @@ using Umbraco.Cms.Web.Common.Middleware; using Umbraco.Cms.Web.Common.Routing; using Umbraco.Cms.Web.Website.Collections; -using Umbraco.Cms.Web.Website.Controllers; -using Umbraco.Cms.Web.Website.Middleware; using Umbraco.Cms.Web.Website.Models; using Umbraco.Cms.Web.Website.Routing; using Umbraco.Cms.Web.Website.ViewEngines; @@ -51,7 +49,7 @@ public static IUmbracoBuilder AddWebsite(this IUmbracoBuilder builder) builder.Services.AddSingleton(); - builder.Services.AddSingleton(); + builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder diff --git a/src/Umbraco.Web.Website/Extensions/UmbracoApplicationBuilder.Website.cs b/src/Umbraco.Web.Website/Extensions/UmbracoApplicationBuilder.Website.cs index c549609397f5..33d42e07c933 100644 --- a/src/Umbraco.Web.Website/Extensions/UmbracoApplicationBuilder.Website.cs +++ b/src/Umbraco.Web.Website/Extensions/UmbracoApplicationBuilder.Website.cs @@ -3,7 +3,6 @@ using Microsoft.Extensions.DependencyInjection; using Umbraco.Cms.Web.Common.ApplicationBuilder; using Umbraco.Cms.Web.Common.Middleware; -using Umbraco.Cms.Web.Website.Middleware; using Umbraco.Cms.Web.Website.Routing; namespace Umbraco.Extensions @@ -20,7 +19,6 @@ public static partial class UmbracoApplicationBuilderExtensions /// public static IUmbracoApplicationBuilderContext UseWebsite(this IUmbracoApplicationBuilderContext builder) { - builder.AppBuilder.UseMiddleware(); builder.AppBuilder.UseMiddleware(); return builder; } diff --git a/src/Umbraco.Web.Website/Routing/IPublicAccessRequestHandler.cs b/src/Umbraco.Web.Website/Routing/IPublicAccessRequestHandler.cs new file mode 100644 index 000000000000..0ce3ddad9289 --- /dev/null +++ b/src/Umbraco.Web.Website/Routing/IPublicAccessRequestHandler.cs @@ -0,0 +1,18 @@ +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Umbraco.Cms.Web.Common.Routing; + +namespace Umbraco.Cms.Web.Website.Routing +{ + public interface IPublicAccessRequestHandler + { + /// + /// Ensures that access to current node is permitted. + /// + /// + /// The current route values + /// Updated route values if public access changes the rendered content, else the original route values. + /// Redirecting to a different site root and/or culture will not pick the new site root nor the new culture. + Task RewriteForPublishedContentAccessAsync(HttpContext httpContext, UmbracoRouteValues routeValues); + } +} diff --git a/src/Umbraco.Web.Website/Middleware/PublicAccessMiddleware.cs b/src/Umbraco.Web.Website/Routing/PublicAccessRequestHandler.cs similarity index 77% rename from src/Umbraco.Web.Website/Middleware/PublicAccessMiddleware.cs rename to src/Umbraco.Web.Website/Routing/PublicAccessRequestHandler.cs index 8bbb4e13e41b..88bb6622bd61 100644 --- a/src/Umbraco.Web.Website/Middleware/PublicAccessMiddleware.cs +++ b/src/Umbraco.Web.Website/Routing/PublicAccessRequestHandler.cs @@ -10,22 +10,21 @@ using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; using Umbraco.Cms.Web.Common.Routing; -using Umbraco.Cms.Web.Website.Routing; using Umbraco.Extensions; -namespace Umbraco.Cms.Web.Website.Middleware +namespace Umbraco.Cms.Web.Website.Routing { - public class PublicAccessMiddleware : IMiddleware + public class PublicAccessRequestHandler : IPublicAccessRequestHandler { - private readonly ILogger _logger; + private readonly ILogger _logger; private readonly IPublicAccessService _publicAccessService; private readonly IPublicAccessChecker _publicAccessChecker; private readonly IUmbracoContextAccessor _umbracoContextAccessor; private readonly IUmbracoRouteValuesFactory _umbracoRouteValuesFactory; private readonly IPublishedRouter _publishedRouter; - public PublicAccessMiddleware( - ILogger logger, + public PublicAccessRequestHandler( + ILogger logger, IPublicAccessService publicAccessService, IPublicAccessChecker publicAccessChecker, IUmbracoContextAccessor umbracoContextAccessor, @@ -40,23 +39,8 @@ public PublicAccessMiddleware( _publishedRouter = publishedRouter; } - public async Task InvokeAsync(HttpContext context, RequestDelegate next) - { - UmbracoRouteValues umbracoRouteValues = context.Features.Get(); - - if (umbracoRouteValues != null) - { - await EnsurePublishedContentAccess(context, umbracoRouteValues); - } - - await next(context); - } - - /// - /// Ensures that access to current node is permitted. - /// - /// Redirecting to a different site root and/or culture will not pick the new site root nor the new culture. - private async Task EnsurePublishedContentAccess(HttpContext httpContext, UmbracoRouteValues routeValues) + /// + public async Task RewriteForPublishedContentAccessAsync(HttpContext httpContext, UmbracoRouteValues routeValues) { // because these might loop, we have to have some sort of infinite loop detection int i = 0; @@ -64,13 +48,13 @@ private async Task EnsurePublishedContentAccess(HttpContext httpContext, Umbraco PublicAccessStatus publicAccessStatus = PublicAccessStatus.AccessAccepted; do { - _logger.LogDebug(nameof(EnsurePublishedContentAccess) + ": Loop {LoopCounter}", i); + _logger.LogDebug(nameof(RewriteForPublishedContentAccessAsync) + ": Loop {LoopCounter}", i); IPublishedContent publishedContent = routeValues.PublishedRequest?.PublishedContent; if (publishedContent == null) { - return; + return routeValues; } var path = publishedContent.Path; @@ -117,8 +101,10 @@ private async Task EnsurePublishedContentAccess(HttpContext httpContext, Umbraco if (i == maxLoop) { - _logger.LogDebug(nameof(EnsurePublishedContentAccess) + ": Looks like we are running into an infinite loop, abort"); + _logger.LogDebug(nameof(RewriteForPublishedContentAccessAsync) + ": Looks like we are running into an infinite loop, abort"); } + + return routeValues; } @@ -139,17 +125,11 @@ private async Task SetPublishedContentAsOtherPageAsync(HttpC // we need to change the content item that is getting rendered so we have to re-create UmbracoRouteValues. UmbracoRouteValues updatedRouteValues = await _umbracoRouteValuesFactory.CreateAsync(httpContext, reRouted); - // Update the feature - httpContext.Features.Set(updatedRouteValues); - return updatedRouteValues; } else { - _logger.LogWarning("Public Access rule has a redirect node set to itself, nothing can be routed."); - // Update the feature to nothing - cannot continue - httpContext.Features.Set(null); - return null; + throw new InvalidOperationException("Public Access rule has a redirect node set to itself, nothing can be routed."); } } } diff --git a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs index fc54350097d8..0bca8d7215b4 100644 --- a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs +++ b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs @@ -52,6 +52,7 @@ public class UmbracoRouteValueTransformer : DynamicRouteValueTransformer private readonly IDataProtectionProvider _dataProtectionProvider; private readonly IControllerActionSearcher _controllerActionSearcher; private readonly IEventAggregator _eventAggregator; + private readonly IPublicAccessRequestHandler _publicAccessRequestHandler; /// /// Initializes a new instance of the class. @@ -67,7 +68,8 @@ public UmbracoRouteValueTransformer( IRoutableDocumentFilter routableDocumentFilter, IDataProtectionProvider dataProtectionProvider, IControllerActionSearcher controllerActionSearcher, - IEventAggregator eventAggregator) + IEventAggregator eventAggregator, + IPublicAccessRequestHandler publicAccessRequestHandler) { if (globalSettings is null) { @@ -85,6 +87,7 @@ public UmbracoRouteValueTransformer( _dataProtectionProvider = dataProtectionProvider; _controllerActionSearcher = controllerActionSearcher; _eventAggregator = eventAggregator; + _publicAccessRequestHandler = publicAccessRequestHandler; } /// @@ -125,20 +128,10 @@ public override async ValueTask TransformAsync(HttpContext }; } - IPublishedRequest publishedRequest = await RouteRequestAsync(httpContext, umbracoContext); + IPublishedRequest publishedRequest = await RouteRequestAsync(umbracoContext); umbracoRouteValues = await _routeValuesFactory.CreateAsync(httpContext, publishedRequest); - // Store the route values as a httpcontext feature - httpContext.Features.Set(umbracoRouteValues); - - // Need to check if there is form data being posted back to an Umbraco URL - PostedDataProxyInfo postedInfo = GetFormInfo(httpContext, values); - if (postedInfo != null) - { - return HandlePostedValues(postedInfo, httpContext); - } - if (!umbracoRouteValues?.PublishedRequest?.HasPublishedContent() ?? false) { // No content was found, not by any registered 404 handlers and @@ -149,6 +142,19 @@ public override async ValueTask TransformAsync(HttpContext return null; } + // now we need to do some public access checks + umbracoRouteValues = await _publicAccessRequestHandler.RewriteForPublishedContentAccessAsync(httpContext, umbracoRouteValues); + + // Store the route values as a httpcontext feature + httpContext.Features.Set(umbracoRouteValues); + + // Need to check if there is form data being posted back to an Umbraco URL + PostedDataProxyInfo postedInfo = GetFormInfo(httpContext, values); + if (postedInfo != null) + { + return HandlePostedValues(postedInfo, httpContext); + } + // See https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.dynamicroutevaluetransformer.transformasync?view=aspnetcore-5.0#Microsoft_AspNetCore_Mvc_Routing_DynamicRouteValueTransformer_TransformAsync_Microsoft_AspNetCore_Http_HttpContext_Microsoft_AspNetCore_Routing_RouteValueDictionary_ // We should apparenlty not be modified these values. // So we create new ones. @@ -164,7 +170,7 @@ public override async ValueTask TransformAsync(HttpContext return newValues; } - private async Task RouteRequestAsync(HttpContext httpContext, IUmbracoContext umbracoContext) + private async Task RouteRequestAsync(IUmbracoContext umbracoContext) { // ok, process