From 2252db0d55ae0c7f4ddba19bde6f408dbfb9eb93 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 12 Jul 2019 13:40:21 +1000 Subject: [PATCH 1/7] Fixes 5822 by reinstating optional method overloads of loadBaseType --- src/Umbraco.Core/Services/EntityService.cs | 28 +++++++++++++++++++ src/Umbraco.Core/Services/IEntityService.cs | 26 ++++++++++++++++- src/Umbraco.Core/Services/IRelationService.cs | 27 ++++++++++++++++++ src/Umbraco.Core/Services/RelationService.cs | 27 ++++++++++++++++++ 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Services/EntityService.cs b/src/Umbraco.Core/Services/EntityService.cs index f4b1b71732..84f5e2d9fd 100644 --- a/src/Umbraco.Core/Services/EntityService.cs +++ b/src/Umbraco.Core/Services/EntityService.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using System.Linq; using System.Linq.Expressions; using Umbraco.Core.CodeAnnotations; @@ -717,5 +718,32 @@ namespace Umbraco.Core.Services } return node.NodeId; } + + #region Obsolete - only here for compat + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity GetByKey(Guid key, bool loadBaseType = true) => GetByKey(key); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity Get(int id, bool loadBaseType = true) => Get(id); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity GetByKey(Guid key, UmbracoObjectTypes umbracoObjectType, bool loadBaseType = true) => GetByKey(key, umbracoObjectType); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity GetByKey(Guid key, bool loadBaseType = true) where T : IUmbracoEntity => GetByKey(key); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity Get(int id, UmbracoObjectTypes umbracoObjectType, bool loadBaseType = true) => Get(id, umbracoObjectType); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity Get(int id, bool loadBaseType = true) where T : IUmbracoEntity => Get(id); + #endregion } } diff --git a/src/Umbraco.Core/Services/IEntityService.cs b/src/Umbraco.Core/Services/IEntityService.cs index a6a2254138..a0c7363c60 100644 --- a/src/Umbraco.Core/Services/IEntityService.cs +++ b/src/Umbraco.Core/Services/IEntityService.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using Umbraco.Core.Models; using Umbraco.Core.Models.EntityBase; using Umbraco.Core.Persistence.DatabaseModelDefinitions; @@ -55,6 +56,10 @@ namespace Umbraco.Core.Services /// An IUmbracoEntity GetByKey(Guid key); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity GetByKey(Guid key, bool loadBaseType = true); + /// /// Gets an UmbracoEntity by its Id, and optionally loads the complete object graph. /// @@ -62,9 +67,13 @@ namespace Umbraco.Core.Services /// By default this will load the base type with a minimum set of properties. /// /// Id of the object to retrieve - /// An + /// An IUmbracoEntity Get(int id); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity Get(int id, bool loadBaseType = true); + /// /// Gets an UmbracoEntity by its Id and UmbracoObjectType, and optionally loads the complete object graph. /// @@ -76,6 +85,14 @@ namespace Umbraco.Core.Services /// An IUmbracoEntity GetByKey(Guid key, UmbracoObjectTypes umbracoObjectType); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity GetByKey(Guid key, UmbracoObjectTypes umbracoObjectType, bool loadBaseType = true); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity GetByKey(Guid key, bool loadBaseType = true) where T : IUmbracoEntity; + /// /// Gets an UmbracoEntity by its Id and UmbracoObjectType, and optionally loads the complete object graph. /// @@ -87,6 +104,9 @@ namespace Umbraco.Core.Services /// An IUmbracoEntity Get(int id, UmbracoObjectTypes umbracoObjectType); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity Get(int id, UmbracoObjectTypes umbracoObjectType, bool loadBaseType = true); /// /// Gets an UmbracoEntity by its Id and specified Type. Optionally loads the complete object graph. @@ -99,6 +119,10 @@ namespace Umbraco.Core.Services /// An IUmbracoEntity Get(int id) where T : IUmbracoEntity; + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity Get(int id, bool loadBaseType = true) where T : IUmbracoEntity; + /// /// Gets the parent of entity by its id /// diff --git a/src/Umbraco.Core/Services/IRelationService.cs b/src/Umbraco.Core/Services/IRelationService.cs index b5ca9ee019..819952b697 100644 --- a/src/Umbraco.Core/Services/IRelationService.cs +++ b/src/Umbraco.Core/Services/IRelationService.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using Umbraco.Core.Models; using Umbraco.Core.Models.EntityBase; @@ -145,6 +146,10 @@ namespace Umbraco.Core.Services /// An IUmbracoEntity GetChildEntityFromRelation(IRelation relation); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity GetChildEntityFromRelation(IRelation relation, bool loadBaseType = false); + /// /// Gets the Parent object from a Relation as an /// @@ -152,6 +157,10 @@ namespace Umbraco.Core.Services /// An IUmbracoEntity GetParentEntityFromRelation(IRelation relation); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IUmbracoEntity GetParentEntityFromRelation(IRelation relation, bool loadBaseType = false); + /// /// Gets the Parent and Child objects from a Relation as a "/> with . /// @@ -159,6 +168,10 @@ namespace Umbraco.Core.Services /// Returns a Tuple with Parent (item1) and Child (item2) Tuple GetEntitiesFromRelation(IRelation relation); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + Tuple GetEntitiesFromRelation(IRelation relation, bool loadBaseType = false); + /// /// Gets the Child objects from a list of Relations as a list of objects. /// @@ -166,6 +179,10 @@ namespace Umbraco.Core.Services /// An enumerable list of IEnumerable GetChildEntitiesFromRelations(IEnumerable relations); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IEnumerable GetChildEntitiesFromRelations(IEnumerable relations, bool loadBaseType = false); + /// /// Gets the Parent objects from a list of Relations as a list of objects. /// @@ -173,6 +190,10 @@ namespace Umbraco.Core.Services /// An enumerable list of IEnumerable GetParentEntitiesFromRelations(IEnumerable relations); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IEnumerable GetParentEntitiesFromRelations(IEnumerable relations, bool loadBaseType = false); + /// /// Gets the Parent and Child objects from a list of Relations as a list of objects. /// @@ -181,6 +202,12 @@ namespace Umbraco.Core.Services IEnumerable> GetEntitiesFromRelations( IEnumerable relations); + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + IEnumerable> GetEntitiesFromRelations( + IEnumerable relations, + bool loadBaseType = false); + /// /// Relates two objects by their entity Ids. /// diff --git a/src/Umbraco.Core/Services/RelationService.cs b/src/Umbraco.Core/Services/RelationService.cs index a940096ded..7ddd2ecf12 100644 --- a/src/Umbraco.Core/Services/RelationService.cs +++ b/src/Umbraco.Core/Services/RelationService.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using System.Linq; using Umbraco.Core.Events; using Umbraco.Core.Logging; @@ -736,5 +737,31 @@ namespace Umbraco.Core.Services /// public static event TypedEventHandler> SavedRelationType; #endregion + + #region Obsolete - only here for compat + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity GetChildEntityFromRelation(IRelation relation, bool loadBaseType = false) => GetChildEntityFromRelation(relation); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IUmbracoEntity GetParentEntityFromRelation(IRelation relation, bool loadBaseType = false) => GetParentEntityFromRelation(relation); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public Tuple GetEntitiesFromRelation(IRelation relation, bool loadBaseType = false) => GetEntitiesFromRelation(relation); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IEnumerable GetChildEntitiesFromRelations(IEnumerable relations, bool loadBaseType = false) => GetChildEntitiesFromRelations(relations); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IEnumerable GetParentEntitiesFromRelations(IEnumerable relations, bool loadBaseType = false) => GetParentEntitiesFromRelations(relations); + + [Obsolete("Use the overload that doesn't specify loadBaseType instead, loadBaseType will not affect any results")] + [EditorBrowsable(EditorBrowsableState.Never)] + public IEnumerable> GetEntitiesFromRelations(IEnumerable relations, bool loadBaseType = false) => GetEntitiesFromRelations(relations); + #endregion } } From 2aaca865e76ba45720b5b9954d81a83af6233dbb Mon Sep 17 00:00:00 2001 From: skttl Date: Fri, 12 Jul 2019 07:33:17 +0200 Subject: [PATCH 2/7] changes innerjoin to left join, to allow folders in sql query --- src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs index 7b10b19e1e..c8fdd8da57 100644 --- a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs @@ -370,7 +370,7 @@ namespace Umbraco.Core.Persistence.Repositories if (isMedia) { - entitySql.InnerJoin("cmsMedia media").On("media.nodeId = umbracoNode.id"); + entitySql.LeftJoin("cmsMedia media").On("media.nodeId = umbracoNode.id"); } entitySql.LeftJoin("cmsContentType contenttype").On("contenttype.nodeId = content.contentType"); From 0cbe0b47967f7cc70f8d6291e983a1b706e37405 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 16 Jul 2019 22:34:34 +1000 Subject: [PATCH 3/7] Cherry picks ValidateUmbracoFormRouteStringAttribute implementation --- .../Controllers/UmbLoginController.cs | 1 + .../Controllers/UmbLoginStatusController.cs | 1 + .../Controllers/UmbProfileController.cs | 1 + .../Controllers/UmbRegisterController.cs | 1 + .../HttpUmbracoFormRouteStringException.cs | 23 ++++++++ ...ValidateUmbracoFormRouteStringAttribute.cs | 52 +++++++++++++++++++ src/Umbraco.Web/Umbraco.Web.csproj | 2 + 7 files changed, 81 insertions(+) create mode 100644 src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs create mode 100644 src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs diff --git a/src/Umbraco.Web/Controllers/UmbLoginController.cs b/src/Umbraco.Web/Controllers/UmbLoginController.cs index 2980b4a4c0..2ff80e2668 100644 --- a/src/Umbraco.Web/Controllers/UmbLoginController.cs +++ b/src/Umbraco.Web/Controllers/UmbLoginController.cs @@ -22,6 +22,7 @@ namespace Umbraco.Web.Controllers [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleLogin([Bind(Prefix = "loginModel")]LoginModel model) { if (ModelState.IsValid == false) diff --git a/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs b/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs index fdc2de8c5f..8f572404fc 100644 --- a/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs +++ b/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs @@ -24,6 +24,7 @@ namespace Umbraco.Web.Controllers [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectModel model) { if (ModelState.IsValid == false) diff --git a/src/Umbraco.Web/Controllers/UmbProfileController.cs b/src/Umbraco.Web/Controllers/UmbProfileController.cs index b14652ad2e..d9333e8e65 100644 --- a/src/Umbraco.Web/Controllers/UmbProfileController.cs +++ b/src/Umbraco.Web/Controllers/UmbProfileController.cs @@ -23,6 +23,7 @@ namespace Umbraco.Web.Controllers [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleUpdateProfile([Bind(Prefix = "profileModel")] ProfileModel model) { var provider = Core.Security.MembershipProviderExtensions.GetMembersMembershipProvider(); diff --git a/src/Umbraco.Web/Controllers/UmbRegisterController.cs b/src/Umbraco.Web/Controllers/UmbRegisterController.cs index b0187d6127..4f4173a67d 100644 --- a/src/Umbraco.Web/Controllers/UmbRegisterController.cs +++ b/src/Umbraco.Web/Controllers/UmbRegisterController.cs @@ -24,6 +24,7 @@ namespace Umbraco.Web.Controllers [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]RegisterModel model) { if (ModelState.IsValid == false) diff --git a/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs b/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs new file mode 100644 index 0000000000..d4734e5d24 --- /dev/null +++ b/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs @@ -0,0 +1,23 @@ +using System; +using System.Net; +using System.Web; + +namespace Umbraco.Web.Mvc +{ + /// + /// Exception that occurs when an Umbraco form route string is invalid + /// + /// + [Serializable] + public sealed class HttpUmbracoFormRouteStringException : HttpException + { + /// + /// Initializes a new instance of the class. + /// + /// The error message displayed to the client when the exception is thrown. + public HttpUmbracoFormRouteStringException(string message) + : base(message) + { } + + } +} diff --git a/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs new file mode 100644 index 0000000000..a57ce7e7f7 --- /dev/null +++ b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs @@ -0,0 +1,52 @@ +using System; +using System.Net; +using System.Web.Mvc; +using Umbraco.Core; + +namespace Umbraco.Web.Mvc +{ + /// + /// Represents an attribute that is used to prevent an invalid Umbraco form request route string on a request. + /// + /// + /// + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] + public sealed class ValidateUmbracoFormRouteStringAttribute : FilterAttribute, IAuthorizationFilter + { + /// + /// Called when authorization is required. + /// + /// The filter context. + /// filterContext + /// The required request field \"ufprt\" is not present. + /// or + /// The Umbraco form request route string could not be decrypted. + /// or + /// The provided Umbraco form request route string was meant for a different controller and action. + public void OnAuthorization(AuthorizationContext filterContext) + { + if (filterContext == null) + { + throw new ArgumentNullException(nameof(filterContext)); + } + + var ufprt = filterContext.HttpContext.Request["ufprt"]; + if (ufprt.IsNullOrWhiteSpace()) + { + throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + } + + if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(ufprt, out var additionalDataParts)) + { + throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + } + + if (additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] != filterContext.ActionDescriptor.ControllerDescriptor.ControllerName || + additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action] != filterContext.ActionDescriptor.ActionName || + additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].NullOrWhiteSpaceAsNull() != filterContext.RouteData.DataTokens["area"]?.ToString().NullOrWhiteSpaceAsNull()) + { + throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + } + } + } +} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 105a40b4a7..39a05ddbc4 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -219,8 +219,10 @@ + + From d52420183e85e8a94b70b2cb0c68ab46e6f4a681 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 16 Jul 2019 23:03:26 +1000 Subject: [PATCH 4/7] Cherry picks ValidateUmbracoFormRouteStringAttribute implementation and fixes up some logic --- ...oAntiForgeryAdditionalDataProviderTests.cs | 157 ------------------ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 - .../Controllers/UmbLoginController.cs | 1 + .../Controllers/UmbLoginStatusController.cs | 1 + .../Controllers/UmbProfileController.cs | 1 + .../Controllers/UmbRegisterController.cs | 1 + src/Umbraco.Web/HtmlHelperRenderExtensions.cs | 7 - .../HttpUmbracoFormRouteStringException.cs | 23 +++ ...ValidateUmbracoFormRouteStringAttribute.cs | 52 ++++++ ...mbracoAntiForgeryAdditionalDataProvider.cs | 91 ---------- src/Umbraco.Web/Umbraco.Web.csproj | 3 +- src/Umbraco.Web/WebBootManager.cs | 2 - 12 files changed, 81 insertions(+), 259 deletions(-) delete mode 100644 src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs create mode 100644 src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs create mode 100644 src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs delete mode 100644 src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs diff --git a/src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs b/src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs deleted file mode 100644 index c81c108e0d..0000000000 --- a/src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs +++ /dev/null @@ -1,157 +0,0 @@ -using System.Collections.Specialized; -using System.Web; -using System.Web.Helpers; -using Moq; -using Newtonsoft.Json; -using NUnit.Framework; -using Umbraco.Core; -using Umbraco.Tests.TestHelpers; -using Umbraco.Web.Mvc; -using Umbraco.Web.Security; - -namespace Umbraco.Tests.Security -{ - [TestFixture] - public class UmbracoAntiForgeryAdditionalDataProviderTests - { - [Test] - public void Test_Wrapped_Non_BeginUmbracoForm() - { - var wrapped = Mock.Of(x => x.GetAdditionalData(It.IsAny()) == "custom"); - var provider = new UmbracoAntiForgeryAdditionalDataProvider(wrapped); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var data = provider.GetAdditionalData(httpContextFactory.HttpContext); - - Assert.IsTrue(data.DetectIsJson()); - var json = JsonConvert.DeserializeObject(data); - Assert.AreEqual(null, json.Ufprt); - Assert.IsTrue(json.Stamp != default); - Assert.AreEqual("custom", json.WrappedValue); - } - - [Test] - public void Null_Wrapped_Non_BeginUmbracoForm() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var data = provider.GetAdditionalData(httpContextFactory.HttpContext); - - Assert.IsTrue(data.DetectIsJson()); - var json = JsonConvert.DeserializeObject(data); - Assert.AreEqual(null, json.Ufprt); - Assert.IsTrue(json.Stamp != default); - Assert.AreEqual("default", json.WrappedValue); - } - - [Test] - public void Validate_Non_Json() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "hello"); - - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_Invalid_Json() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '0'}"); - Assert.IsFalse(isValid); - - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': ''}"); - Assert.IsFalse(isValid); - - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'hello': 'world'}"); - Assert.IsFalse(isValid); - - } - - [Test] - public void Validate_No_Request_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - //there is a ufprt in the additional data, but not in the request - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': 'ASBVDFDFDFDF'}"); - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_No_AdditionalData_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var requestMock = Mock.Get(httpContextFactory.HttpContext.Request); - requestMock.SetupGet(x => x["ufprt"]).Returns("ABCDEFG"); - - //there is a ufprt in the additional data, but not in the request - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': ''}"); - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_No_AdditionalData_Or_Request_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - - //there is a ufprt in the additional data, but not in the request - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': ''}"); - Assert.IsTrue(isValid); - } - - [Test] - public void Validate_Request_And_AdditionalData_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var routeParams1 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - var routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var requestMock = Mock.Get(httpContextFactory.HttpContext.Request); - requestMock.SetupGet(x => x["ufprt"]).Returns(routeParams1.EncryptWithMachineKey()); - - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsTrue(isValid); - - routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Invalid")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_Wrapped_Request_And_AdditionalData_Ufprt() - { - var wrapped = Mock.Of(x => x.ValidateAdditionalData(It.IsAny(), "custom") == true); - var provider = new UmbracoAntiForgeryAdditionalDataProvider(wrapped); - - var routeParams1 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - var routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var requestMock = Mock.Get(httpContextFactory.HttpContext.Request); - requestMock.SetupGet(x => x["ufprt"]).Returns(routeParams1.EncryptWithMachineKey()); - - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsFalse(isValid); - - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'custom', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsTrue(isValid); - - routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Invalid")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsFalse(isValid); - } - } -} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 7e6fdf4823..72939c12d9 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -197,7 +197,6 @@ - diff --git a/src/Umbraco.Web/Controllers/UmbLoginController.cs b/src/Umbraco.Web/Controllers/UmbLoginController.cs index ba46d0a17e..a952a944bc 100644 --- a/src/Umbraco.Web/Controllers/UmbLoginController.cs +++ b/src/Umbraco.Web/Controllers/UmbLoginController.cs @@ -12,6 +12,7 @@ namespace Umbraco.Web.Controllers { [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleLogin([Bind(Prefix = "loginModel")]LoginModel model) { if (ModelState.IsValid == false) diff --git a/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs b/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs index 8e063bf2a3..e1c334bf0b 100644 --- a/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs +++ b/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs @@ -13,6 +13,7 @@ namespace Umbraco.Web.Controllers { [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectModel model) { if (ModelState.IsValid == false) diff --git a/src/Umbraco.Web/Controllers/UmbProfileController.cs b/src/Umbraco.Web/Controllers/UmbProfileController.cs index 7def7af826..19d2905da1 100644 --- a/src/Umbraco.Web/Controllers/UmbProfileController.cs +++ b/src/Umbraco.Web/Controllers/UmbProfileController.cs @@ -16,6 +16,7 @@ namespace Umbraco.Web.Controllers { [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleUpdateProfile([Bind(Prefix = "profileModel")] ProfileModel model) { var provider = global::Umbraco.Core.Security.MembershipProviderExtensions.GetMembersMembershipProvider(); diff --git a/src/Umbraco.Web/Controllers/UmbRegisterController.cs b/src/Umbraco.Web/Controllers/UmbRegisterController.cs index 7931565c47..3e20277537 100644 --- a/src/Umbraco.Web/Controllers/UmbRegisterController.cs +++ b/src/Umbraco.Web/Controllers/UmbRegisterController.cs @@ -11,6 +11,7 @@ namespace Umbraco.Web.Controllers { [HttpPost] [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] public ActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]RegisterModel model) { if (ModelState.IsValid == false) diff --git a/src/Umbraco.Web/HtmlHelperRenderExtensions.cs b/src/Umbraco.Web/HtmlHelperRenderExtensions.cs index 0e2a2a6450..180cf195aa 100644 --- a/src/Umbraco.Web/HtmlHelperRenderExtensions.cs +++ b/src/Umbraco.Web/HtmlHelperRenderExtensions.cs @@ -293,13 +293,6 @@ namespace Umbraco.Web _controllerName = controllerName; _encryptedString = UmbracoHelper.CreateEncryptedRouteString(controllerName, controllerAction, area, additionalRouteVals); - //For UmbracoForm's we want to add our routing string to the httpcontext items in the case where anti-forgery tokens are used. - //In which case our custom UmbracoAntiForgeryAdditionalDataProvider will kick in and validate the values in the request against - //the values that will be appended to the token. This essentially means that when anti-forgery tokens are used with UmbracoForm's forms, - //that each token is unique to the controller/action/area instead of the default ASP.Net implementation which is that the token is unique - //per user. - _viewContext.HttpContext.Items["ufprt"] = _encryptedString; - } diff --git a/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs b/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs new file mode 100644 index 0000000000..d4734e5d24 --- /dev/null +++ b/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs @@ -0,0 +1,23 @@ +using System; +using System.Net; +using System.Web; + +namespace Umbraco.Web.Mvc +{ + /// + /// Exception that occurs when an Umbraco form route string is invalid + /// + /// + [Serializable] + public sealed class HttpUmbracoFormRouteStringException : HttpException + { + /// + /// Initializes a new instance of the class. + /// + /// The error message displayed to the client when the exception is thrown. + public HttpUmbracoFormRouteStringException(string message) + : base(message) + { } + + } +} diff --git a/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs new file mode 100644 index 0000000000..f3d0cc0e27 --- /dev/null +++ b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs @@ -0,0 +1,52 @@ +using System; +using System.Net; +using System.Web.Mvc; +using Umbraco.Core; + +namespace Umbraco.Web.Mvc +{ + /// + /// Represents an attribute that is used to prevent an invalid Umbraco form request route string on a request. + /// + /// + /// + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] + public sealed class ValidateUmbracoFormRouteStringAttribute : FilterAttribute, IAuthorizationFilter + { + /// + /// Called when authorization is required. + /// + /// The filter context. + /// filterContext + /// The required request field \"ufprt\" is not present. + /// or + /// The Umbraco form request route string could not be decrypted. + /// or + /// The provided Umbraco form request route string was meant for a different controller and action. + public void OnAuthorization(AuthorizationContext filterContext) + { + if (filterContext == null) + { + throw new ArgumentNullException(nameof(filterContext)); + } + + var ufprt = filterContext.HttpContext.Request["ufprt"]; + if (ufprt.IsNullOrWhiteSpace()) + { + throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + } + + if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(ufprt, out var additionalDataParts)) + { + throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + } + + if (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller].InvariantEquals(filterContext.ActionDescriptor.ControllerDescriptor.ControllerName) || + !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action].InvariantEquals(filterContext.ActionDescriptor.ActionName) || + (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].IsNullOrWhiteSpace() && !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].InvariantEquals(filterContext.RouteData.DataTokens["area"]?.ToString()))) + { + throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + } + } + } +} diff --git a/src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs b/src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs deleted file mode 100644 index 660f7e58fc..0000000000 --- a/src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs +++ /dev/null @@ -1,91 +0,0 @@ -using System; -using Umbraco.Web.Mvc; -using Umbraco.Core; -using System.Web.Helpers; -using System.Web; -using Newtonsoft.Json; - -namespace Umbraco.Web.Security -{ - /// - /// A custom to create a unique antiforgery token/validator per form created with BeginUmbracoForm - /// - public class UmbracoAntiForgeryAdditionalDataProvider : IAntiForgeryAdditionalDataProvider - { - private readonly IAntiForgeryAdditionalDataProvider _defaultProvider; - - /// - /// Constructor, allows wrapping a default provider - /// - /// - public UmbracoAntiForgeryAdditionalDataProvider(IAntiForgeryAdditionalDataProvider defaultProvider) - { - _defaultProvider = defaultProvider; - } - - public string GetAdditionalData(HttpContextBase context) - { - return JsonConvert.SerializeObject(new AdditionalData - { - Stamp = DateTime.UtcNow.Ticks, - //this value will be here if this is a BeginUmbracoForms form - Ufprt = context.Items["ufprt"]?.ToString(), - //if there was a wrapped provider, add it's value to the json, else just a static value - WrappedValue = _defaultProvider?.GetAdditionalData(context) ?? "default" - }); - } - - public bool ValidateAdditionalData(HttpContextBase context, string additionalData) - { - if (!additionalData.DetectIsJson()) - return false; //must be json - - AdditionalData json; - try - { - json = JsonConvert.DeserializeObject(additionalData); - } - catch - { - return false; //couldn't parse - } - - if (json.Stamp == default) return false; - - //if there was a wrapped provider, validate it, else validate the static value - var validateWrapped = _defaultProvider?.ValidateAdditionalData(context, json.WrappedValue) ?? json.WrappedValue == "default"; - if (!validateWrapped) - return false; - - var ufprtRequest = context.Request["ufprt"]?.ToString(); - - //if the custom BeginUmbracoForms route value is not there, then it's nothing more to validate - if (ufprtRequest.IsNullOrWhiteSpace() && json.Ufprt.IsNullOrWhiteSpace()) - return true; - - //if one or the other is null then something is wrong - if (!ufprtRequest.IsNullOrWhiteSpace() && json.Ufprt.IsNullOrWhiteSpace()) return false; - if (ufprtRequest.IsNullOrWhiteSpace() && !json.Ufprt.IsNullOrWhiteSpace()) return false; - - if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(json.Ufprt, out var additionalDataParts)) - return false; - - if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(ufprtRequest, out var requestParts)) - return false; - - //ensure they all match - return additionalDataParts.Count == requestParts.Count - && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] - && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Action] - && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Area]; - } - - internal class AdditionalData - { - public string Ufprt { get; set; } - public long Stamp { get; set; } - public string WrappedValue { get; set; } - } - - } -} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 167c99b9b9..2fb54a2c80 100644 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -307,7 +307,8 @@ - + + diff --git a/src/Umbraco.Web/WebBootManager.cs b/src/Umbraco.Web/WebBootManager.cs index ff6fcff164..46b67a60c7 100644 --- a/src/Umbraco.Web/WebBootManager.cs +++ b/src/Umbraco.Web/WebBootManager.cs @@ -190,8 +190,6 @@ namespace Umbraco.Web base.Complete(afterComplete); - AntiForgeryConfig.AdditionalDataProvider = new UmbracoAntiForgeryAdditionalDataProvider(AntiForgeryConfig.AdditionalDataProvider); - //Now, startup all of our legacy startup handler ApplicationEventsResolver.Current.InstantiateLegacyStartupHandlers(); From b675f6252c1bb471e5a76c01329844905c379e4a Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 16 Jul 2019 23:07:31 +1000 Subject: [PATCH 5/7] removes files (should have been part of cherry pick?), fixes logic --- ...oAntiForgeryAdditionalDataProviderTests.cs | 157 ------------------ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 - ...ValidateUmbracoFormRouteStringAttribute.cs | 6 +- src/Umbraco.Web/Runtime/WebFinalComponent.cs | 2 - ...mbracoAntiForgeryAdditionalDataProvider.cs | 92 ---------- src/Umbraco.Web/Umbraco.Web.csproj | 1 - 6 files changed, 3 insertions(+), 256 deletions(-) delete mode 100644 src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs delete mode 100644 src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs diff --git a/src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs b/src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs deleted file mode 100644 index c81c108e0d..0000000000 --- a/src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs +++ /dev/null @@ -1,157 +0,0 @@ -using System.Collections.Specialized; -using System.Web; -using System.Web.Helpers; -using Moq; -using Newtonsoft.Json; -using NUnit.Framework; -using Umbraco.Core; -using Umbraco.Tests.TestHelpers; -using Umbraco.Web.Mvc; -using Umbraco.Web.Security; - -namespace Umbraco.Tests.Security -{ - [TestFixture] - public class UmbracoAntiForgeryAdditionalDataProviderTests - { - [Test] - public void Test_Wrapped_Non_BeginUmbracoForm() - { - var wrapped = Mock.Of(x => x.GetAdditionalData(It.IsAny()) == "custom"); - var provider = new UmbracoAntiForgeryAdditionalDataProvider(wrapped); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var data = provider.GetAdditionalData(httpContextFactory.HttpContext); - - Assert.IsTrue(data.DetectIsJson()); - var json = JsonConvert.DeserializeObject(data); - Assert.AreEqual(null, json.Ufprt); - Assert.IsTrue(json.Stamp != default); - Assert.AreEqual("custom", json.WrappedValue); - } - - [Test] - public void Null_Wrapped_Non_BeginUmbracoForm() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var data = provider.GetAdditionalData(httpContextFactory.HttpContext); - - Assert.IsTrue(data.DetectIsJson()); - var json = JsonConvert.DeserializeObject(data); - Assert.AreEqual(null, json.Ufprt); - Assert.IsTrue(json.Stamp != default); - Assert.AreEqual("default", json.WrappedValue); - } - - [Test] - public void Validate_Non_Json() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "hello"); - - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_Invalid_Json() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '0'}"); - Assert.IsFalse(isValid); - - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': ''}"); - Assert.IsFalse(isValid); - - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'hello': 'world'}"); - Assert.IsFalse(isValid); - - } - - [Test] - public void Validate_No_Request_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - //there is a ufprt in the additional data, but not in the request - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': 'ASBVDFDFDFDF'}"); - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_No_AdditionalData_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var requestMock = Mock.Get(httpContextFactory.HttpContext.Request); - requestMock.SetupGet(x => x["ufprt"]).Returns("ABCDEFG"); - - //there is a ufprt in the additional data, but not in the request - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': ''}"); - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_No_AdditionalData_Or_Request_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - - //there is a ufprt in the additional data, but not in the request - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': ''}"); - Assert.IsTrue(isValid); - } - - [Test] - public void Validate_Request_And_AdditionalData_Ufprt() - { - var provider = new UmbracoAntiForgeryAdditionalDataProvider(null); - - var routeParams1 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - var routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var requestMock = Mock.Get(httpContextFactory.HttpContext.Request); - requestMock.SetupGet(x => x["ufprt"]).Returns(routeParams1.EncryptWithMachineKey()); - - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsTrue(isValid); - - routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Invalid")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsFalse(isValid); - } - - [Test] - public void Validate_Wrapped_Request_And_AdditionalData_Ufprt() - { - var wrapped = Mock.Of(x => x.ValidateAdditionalData(It.IsAny(), "custom") == true); - var provider = new UmbracoAntiForgeryAdditionalDataProvider(wrapped); - - var routeParams1 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - var routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - - var httpContextFactory = new FakeHttpContextFactory("/hello/world"); - var requestMock = Mock.Get(httpContextFactory.HttpContext.Request); - requestMock.SetupGet(x => x["ufprt"]).Returns(routeParams1.EncryptWithMachineKey()); - - var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsFalse(isValid); - - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'custom', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsTrue(isValid); - - routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Invalid")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco"; - isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}"); - Assert.IsFalse(isValid); - } - } -} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index cc569d6989..4f4a83dc26 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -147,7 +147,6 @@ - diff --git a/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs index a57ce7e7f7..f3d0cc0e27 100644 --- a/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs +++ b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs @@ -41,9 +41,9 @@ namespace Umbraco.Web.Mvc throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); } - if (additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] != filterContext.ActionDescriptor.ControllerDescriptor.ControllerName || - additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action] != filterContext.ActionDescriptor.ActionName || - additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].NullOrWhiteSpaceAsNull() != filterContext.RouteData.DataTokens["area"]?.ToString().NullOrWhiteSpaceAsNull()) + if (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller].InvariantEquals(filterContext.ActionDescriptor.ControllerDescriptor.ControllerName) || + !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action].InvariantEquals(filterContext.ActionDescriptor.ActionName) || + (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].IsNullOrWhiteSpace() && !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].InvariantEquals(filterContext.RouteData.DataTokens["area"]?.ToString()))) { throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); } diff --git a/src/Umbraco.Web/Runtime/WebFinalComponent.cs b/src/Umbraco.Web/Runtime/WebFinalComponent.cs index ba606e8d5e..6177d9b868 100644 --- a/src/Umbraco.Web/Runtime/WebFinalComponent.cs +++ b/src/Umbraco.Web/Runtime/WebFinalComponent.cs @@ -36,8 +36,6 @@ namespace Umbraco.Web.Runtime // ensure WebAPI is initialized, after everything GlobalConfiguration.Configuration.EnsureInitialized(); - - AntiForgeryConfig.AdditionalDataProvider = new UmbracoAntiForgeryAdditionalDataProvider(AntiForgeryConfig.AdditionalDataProvider); } public void Terminate() diff --git a/src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs b/src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs deleted file mode 100644 index c6ad4c6901..0000000000 --- a/src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs +++ /dev/null @@ -1,92 +0,0 @@ -using System; -using Umbraco.Web.Mvc; -using Umbraco.Core; -using System.Web.Helpers; -using System.Web; -using Newtonsoft.Json; -using Umbraco.Web.Composing; - -namespace Umbraco.Web.Security -{ - /// - /// A custom to create a unique antiforgery token/validator per form created with BeginUmbracoForm - /// - public class UmbracoAntiForgeryAdditionalDataProvider : IAntiForgeryAdditionalDataProvider - { - private readonly IAntiForgeryAdditionalDataProvider _defaultProvider; - - /// - /// Constructor, allows wrapping a default provider - /// - /// - public UmbracoAntiForgeryAdditionalDataProvider(IAntiForgeryAdditionalDataProvider defaultProvider) - { - _defaultProvider = defaultProvider; - } - - public string GetAdditionalData(HttpContextBase context) - { - return JsonConvert.SerializeObject(new AdditionalData - { - Stamp = DateTime.UtcNow.Ticks, - //this value will be here if this is a BeginUmbracoForms form - Ufprt = context.Items["ufprt"]?.ToString(), - //if there was a wrapped provider, add it's value to the json, else just a static value - WrappedValue = _defaultProvider?.GetAdditionalData(context) ?? "default" - }); - } - - public bool ValidateAdditionalData(HttpContextBase context, string additionalData) - { - if (!additionalData.DetectIsJson()) - return false; //must be json - - AdditionalData json; - try - { - json = JsonConvert.DeserializeObject(additionalData); - } - catch - { - return false; //couldn't parse - } - - if (json.Stamp == default) return false; - - //if there was a wrapped provider, validate it, else validate the static value - var validateWrapped = _defaultProvider?.ValidateAdditionalData(context, json.WrappedValue) ?? json.WrappedValue == "default"; - if (!validateWrapped) - return false; - - var ufprtRequest = context.Request["ufprt"]?.ToString(); - - //if the custom BeginUmbracoForms route value is not there, then it's nothing more to validate - if (ufprtRequest.IsNullOrWhiteSpace() && json.Ufprt.IsNullOrWhiteSpace()) - return true; - - //if one or the other is null then something is wrong - if (!ufprtRequest.IsNullOrWhiteSpace() && json.Ufprt.IsNullOrWhiteSpace()) return false; - if (ufprtRequest.IsNullOrWhiteSpace() && !json.Ufprt.IsNullOrWhiteSpace()) return false; - - if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(json.Ufprt, out var additionalDataParts)) - return false; - - if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(ufprtRequest, out var requestParts)) - return false; - - //ensure they all match - return additionalDataParts.Count == requestParts.Count - && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] - && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Action] - && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Area]; - } - - internal class AdditionalData - { - public string Ufprt { get; set; } - public long Stamp { get; set; } - public string WrappedValue { get; set; } - } - - } -} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 39a05ddbc4..41a3393fa4 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -232,7 +232,6 @@ - From bfb69a34ef2353229d390b9129103e61f6b4c704 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 17 Jul 2019 21:15:18 +1000 Subject: [PATCH 6/7] re-adds back in the serialization overloads for the custom exception, re-adds detailed error messages, adds more documentation. Adds unit tests. --- src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + .../Mvc/HtmlHelperExtensionMethodsTests.cs | 3 +- ...ateUmbracoFormRouteStringAttributeTests.cs | 37 +++++++++++++++++++ .../HttpUmbracoFormRouteStringException.cs | 18 +++++++++ ...ValidateUmbracoFormRouteStringAttribute.cs | 28 +++++++++----- src/Umbraco.Web/UmbracoHelper.cs | 2 +- 6 files changed, 78 insertions(+), 11 deletions(-) create mode 100644 src/Umbraco.Tests/Web/Mvc/ValidateUmbracoFormRouteStringAttributeTests.cs diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 72939c12d9..d45c556d37 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -398,6 +398,7 @@ + diff --git a/src/Umbraco.Tests/Web/Mvc/HtmlHelperExtensionMethodsTests.cs b/src/Umbraco.Tests/Web/Mvc/HtmlHelperExtensionMethodsTests.cs index e7e4f363c1..7be0717c7e 100644 --- a/src/Umbraco.Tests/Web/Mvc/HtmlHelperExtensionMethodsTests.cs +++ b/src/Umbraco.Tests/Web/Mvc/HtmlHelperExtensionMethodsTests.cs @@ -4,7 +4,8 @@ using Umbraco.Web; namespace Umbraco.Tests.Web.Mvc { - [TestFixture] + + [TestFixture] public class HtmlHelperExtensionMethodsTests { [SetUp] diff --git a/src/Umbraco.Tests/Web/Mvc/ValidateUmbracoFormRouteStringAttributeTests.cs b/src/Umbraco.Tests/Web/Mvc/ValidateUmbracoFormRouteStringAttributeTests.cs new file mode 100644 index 0000000000..a942f846a0 --- /dev/null +++ b/src/Umbraco.Tests/Web/Mvc/ValidateUmbracoFormRouteStringAttributeTests.cs @@ -0,0 +1,37 @@ +using NUnit.Framework; +using Umbraco.Web; +using Umbraco.Web.Mvc; + +namespace Umbraco.Tests.Web.Mvc +{ + [TestFixture] + public class ValidateUmbracoFormRouteStringAttributeTests + { + [Test] + public void Validate_Route_String() + { + var attribute = new ValidateUmbracoFormRouteStringAttribute(); + + Assert.Throws(() => attribute.ValidateRouteString(null, null, null, null)); + + const string ControllerName = "Test"; + const string ControllerAction = "Index"; + const string Area = "MyArea"; + var validUfprt = UmbracoHelper.CreateEncryptedRouteString(ControllerName, ControllerAction, Area); + + var invalidUfprt = validUfprt + "z"; + Assert.Throws(() => attribute.ValidateRouteString(invalidUfprt, null, null, null)); + + Assert.Throws(() => attribute.ValidateRouteString(validUfprt, ControllerName, ControllerAction, "doesntMatch")); + Assert.Throws(() => attribute.ValidateRouteString(validUfprt, ControllerName, ControllerAction, null)); + Assert.Throws(() => attribute.ValidateRouteString(validUfprt, ControllerName, "doesntMatch", Area)); + Assert.Throws(() => attribute.ValidateRouteString(validUfprt, ControllerName, null, Area)); + Assert.Throws(() => attribute.ValidateRouteString(validUfprt, "doesntMatch", ControllerAction, Area)); + Assert.Throws(() => attribute.ValidateRouteString(validUfprt, null, ControllerAction, Area)); + + Assert.DoesNotThrow(() => attribute.ValidateRouteString(validUfprt, ControllerName, ControllerAction, Area)); + Assert.DoesNotThrow(() => attribute.ValidateRouteString(validUfprt, ControllerName.ToLowerInvariant(), ControllerAction.ToLowerInvariant(), Area.ToLowerInvariant())); + } + + } +} diff --git a/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs b/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs index d4734e5d24..b08fde081a 100644 --- a/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs +++ b/src/Umbraco.Web/Mvc/HttpUmbracoFormRouteStringException.cs @@ -1,5 +1,6 @@ using System; using System.Net; +using System.Runtime.Serialization; using System.Web; namespace Umbraco.Web.Mvc @@ -11,6 +12,14 @@ namespace Umbraco.Web.Mvc [Serializable] public sealed class HttpUmbracoFormRouteStringException : HttpException { + /// Initializes a new instance of the class. + /// + /// The that holds the serialized object data about the exception being thrown. + /// The that holds the contextual information about the source or destination. + private HttpUmbracoFormRouteStringException(SerializationInfo info, StreamingContext context) + : base(info, context) + { } + /// /// Initializes a new instance of the class. /// @@ -19,5 +28,14 @@ namespace Umbraco.Web.Mvc : base(message) { } + /// + /// Initializes a new instance of the class. + /// + /// The error message displayed to the client when the exception is thrown. + /// The , if any, that threw the current exception. + public HttpUmbracoFormRouteStringException(string message, Exception innerException) + : base(message, innerException) + { } + } } diff --git a/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs index f3d0cc0e27..8d929197e1 100644 --- a/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs +++ b/src/Umbraco.Web/Mvc/ValidateUmbracoFormRouteStringAttribute.cs @@ -1,15 +1,21 @@ using System; using System.Net; +using System.Net.Http; using System.Web.Mvc; using Umbraco.Core; namespace Umbraco.Web.Mvc { /// - /// Represents an attribute that is used to prevent an invalid Umbraco form request route string on a request. + /// Attribute used to check that the request contains a valid Umbraco form request string. /// /// /// + /// + /// Applying this attribute/filter to a or SurfaceController Action will ensure that the Action can only be executed + /// when it is routed to from within Umbraco, typically when rendering a form with BegingUmbracoForm. It will mean that the natural MVC route for this Action + /// will fail with a . + /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] public sealed class ValidateUmbracoFormRouteStringAttribute : FilterAttribute, IAuthorizationFilter { @@ -26,27 +32,31 @@ namespace Umbraco.Web.Mvc public void OnAuthorization(AuthorizationContext filterContext) { if (filterContext == null) - { throw new ArgumentNullException(nameof(filterContext)); - } var ufprt = filterContext.HttpContext.Request["ufprt"]; + ValidateRouteString(ufprt, filterContext.ActionDescriptor?.ControllerDescriptor.ControllerName, filterContext.ActionDescriptor?.ActionName, filterContext.RouteData?.DataTokens["area"]?.ToString()); + } + + public void ValidateRouteString(string ufprt, string currentController, string currentAction, string currentArea) + { if (ufprt.IsNullOrWhiteSpace()) { - throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + throw new HttpUmbracoFormRouteStringException("The required request field \"ufprt\" is not present."); } if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(ufprt, out var additionalDataParts)) { - throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + throw new HttpUmbracoFormRouteStringException("The Umbraco form request route string could not be decrypted."); } - if (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller].InvariantEquals(filterContext.ActionDescriptor.ControllerDescriptor.ControllerName) || - !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action].InvariantEquals(filterContext.ActionDescriptor.ActionName) || - (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].IsNullOrWhiteSpace() && !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].InvariantEquals(filterContext.RouteData.DataTokens["area"]?.ToString()))) + if (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller].InvariantEquals(currentController) || + !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action].InvariantEquals(currentAction) || + (!additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].IsNullOrWhiteSpace() && !additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area].InvariantEquals(currentArea))) { - throw new HttpUmbracoFormRouteStringException("The required Umbraco request data is invalid."); + throw new HttpUmbracoFormRouteStringException("The provided Umbraco form request route string was meant for a different controller and action."); } + } } } diff --git a/src/Umbraco.Web/UmbracoHelper.cs b/src/Umbraco.Web/UmbracoHelper.cs index c00d37f216..5320b6085a 100644 --- a/src/Umbraco.Web/UmbracoHelper.cs +++ b/src/Umbraco.Web/UmbracoHelper.cs @@ -1656,7 +1656,7 @@ namespace Umbraco.Web { decryptedString = ufprt.DecryptWithMachineKey(); } - catch (FormatException) + catch (Exception ex) when (ex is FormatException || ex is ArgumentException) { LogHelper.Warn("A value was detected in the ufprt parameter but Umbraco could not decrypt the string"); parts = null; From 26d4d056be662a3e91e33740c9813b1604c5be96 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 17 Jul 2019 22:28:06 +1000 Subject: [PATCH 7/7] bumps version --- src/SolutionInfo.cs | 4 ++-- src/Umbraco.Web.UI/Umbraco.Web.UI.csproj | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/SolutionInfo.cs b/src/SolutionInfo.cs index 9ed398d52f..841e054aee 100644 --- a/src/SolutionInfo.cs +++ b/src/SolutionInfo.cs @@ -18,5 +18,5 @@ using System.Resources; [assembly: AssemblyVersion("8.0.0")] // these are FYI and changed automatically -[assembly: AssemblyFileVersion("8.1.0")] -[assembly: AssemblyInformationalVersion("8.1.0")] +[assembly: AssemblyFileVersion("8.1.1")] +[assembly: AssemblyInformationalVersion("8.1.1")] diff --git a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj index b27f6aa335..e58f44e1ae 100644 --- a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj +++ b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj @@ -345,9 +345,9 @@ False True - 8100 + 8110 / - http://localhost:8100 + http://localhost:8110 False False