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();