Fixes 10730 - Route hijacking with public access

Author: Shazwazza

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<IPublicAccessRequestHandler>();
+            publicAccessRequestHandler.Setup(x => x.RewriteForPublishedContentAccessAsync(It.IsAny<HttpContext>(), It.IsAny<UmbracoRouteValues>()))
+                .Returns((HttpContext ctx, UmbracoRouteValues routeVals) => Task.FromResult(routeVals));
+
             var transformer = new UmbracoRouteValueTransformer(
                 new NullLogger<UmbracoRouteValueTransformer>(),
                 ctx,
@@ -59,7 +63,8 @@ private UmbracoRouteValueTransformer GetTransformer(
                 filter ?? Mock.Of<IRoutableDocumentFilter>(x => x.IsDocumentRequest(It.IsAny<string>()) == true),
                 Mock.Of<IDataProtectionProvider>(),
                 Mock.Of<IControllerActionSearcher>(),
-                Mock.Of<IEventAggregator>());
+                Mock.Of<IEventAggregator>(),
+                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<IPublishedRequest>();
@@ -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<UmbracoRouteValues>();
+            Assert.IsNull(routeVals);
+        }
+
+        [Test]
+        public async Task Assigns_UmbracoRouteValues_To_HttpContext_Feature()
+        {
+            IUmbracoContext umbracoContext = GetUmbracoContext(true);
+            IPublishedRequest request = Mock.Of<IPublishedRequest>(x => x.PublishedContent == Mock.Of<IPublishedContent>());
+
+            UmbracoRouteValueTransformer transformer = GetTransformerWithRunState(
+                Mock.Of<IUmbracoContextAccessor>(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<UmbracoRouteValues>();
             Assert.IsNotNull(routeVals);
             Assert.AreEqual(routeVals.PublishedRequest, umbracoContext.PublishedRequest);

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<MemberModelBuilderFactory>();
 
-            builder.Services.AddSingleton<PublicAccessMiddleware>();
+            builder.Services.AddSingleton<IPublicAccessRequestHandler, PublicAccessRequestHandler>();
             builder.Services.AddSingleton<BasicAuthenticationMiddleware>();
 
             builder

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
         /// <returns></returns>
         public static IUmbracoApplicationBuilderContext UseWebsite(this IUmbracoApplicationBuilderContext builder)
         {
-            builder.AppBuilder.UseMiddleware<PublicAccessMiddleware>();
             builder.AppBuilder.UseMiddleware<BasicAuthenticationMiddleware>();
             return builder;
         }

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
+    {
+        /// <summary>
+        /// Ensures that access to current node is permitted.
+        /// </summary>
+        /// <param name="httpContext"></param>
+        /// <param name="routeValues">The current route values</param>
+        /// <returns>Updated route values if public access changes the rendered content, else the original route values.</returns>
+        /// <remarks>Redirecting to a different site root and/or culture will not pick the new site root nor the new culture.</remarks>
+        Task<UmbracoRouteValues> RewriteForPublishedContentAccessAsync(HttpContext httpContext, UmbracoRouteValues routeValues);
+    }
+}

src/Umbraco.Web.Website/Middleware/PublicAccessMiddleware.cs renamed to 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<PublicAccessMiddleware> _logger;
+        private readonly ILogger<PublicAccessRequestHandler> _logger;
         private readonly IPublicAccessService _publicAccessService;
         private readonly IPublicAccessChecker _publicAccessChecker;
         private readonly IUmbracoContextAccessor _umbracoContextAccessor;
         private readonly IUmbracoRouteValuesFactory _umbracoRouteValuesFactory;
         private readonly IPublishedRouter _publishedRouter;
 
-        public PublicAccessMiddleware(
-            ILogger<PublicAccessMiddleware> logger,
+        public PublicAccessRequestHandler(
+            ILogger<PublicAccessRequestHandler> logger,
             IPublicAccessService publicAccessService,
             IPublicAccessChecker publicAccessChecker,
             IUmbracoContextAccessor umbracoContextAccessor,
@@ -40,37 +39,22 @@ public PublicAccessMiddleware(
             _publishedRouter = publishedRouter;
         }
 
-        public async Task InvokeAsync(HttpContext context, RequestDelegate next)
-        {
-            UmbracoRouteValues umbracoRouteValues = context.Features.Get<UmbracoRouteValues>();
-
-            if (umbracoRouteValues != null)
-            {
-                await EnsurePublishedContentAccess(context, umbracoRouteValues);
-            }
-
-            await next(context);
-        }
-
-        /// <summary>
-        /// Ensures that access to current node is permitted.
-        /// </summary>
-        /// <remarks>Redirecting to a different site root and/or culture will not pick the new site root nor the new culture.</remarks>
-        private async Task EnsurePublishedContentAccess(HttpContext httpContext, UmbracoRouteValues routeValues)
+        /// <inheritdoc />
+        public async Task<UmbracoRouteValues> RewriteForPublishedContentAccessAsync(HttpContext httpContext, UmbracoRouteValues routeValues)
         {
             // because these might loop, we have to have some sort of infinite loop detection
             int i = 0;
             const int maxLoop = 8;
             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<UmbracoRouteValues> 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<UmbracoRouteValues>(null);
-                return null;
+                throw new InvalidOperationException("Public Access rule has a redirect node set to itself, nothing can be routed.");
             }
         }
     }

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;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="UmbracoRouteValueTransformer"/> 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;
         }
 
         /// <inheritdoc/>
@@ -125,20 +128,10 @@ public override async ValueTask<RouteValueDictionary> 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<RouteValueDictionary> 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<RouteValueDictionary> TransformAsync(HttpContext
             return newValues;
         }
 
-        private async Task<IPublishedRequest> RouteRequestAsync(HttpContext httpContext, IUmbracoContext umbracoContext)
+        private async Task<IPublishedRequest> RouteRequestAsync(IUmbracoContext umbracoContext)
         {
             // ok, process