From 75e1307a5d94e8ea9632e28910b436982b73d009 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Leh=C3=B3czky?= Date: Wed, 6 Dec 2023 16:06:15 +0100 Subject: [PATCH 1/5] Fixing FormatException when the login screen is posted with values other than true/false for RememberMe --- .../OrchardCore.Users/Controllers/AccountController.cs | 1 - .../OrchardCore.Users/Views/Account/Login.cshtml | 10 ++++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs b/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs index 6db514be774..a264bde7205 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs @@ -13,7 +13,6 @@ using Microsoft.Extensions.Localization; using Microsoft.Extensions.Logging; using OrchardCore.DisplayManagement.Notify; -using OrchardCore.Entities; using OrchardCore.Modules; using OrchardCore.Mvc.Core.Utilities; using OrchardCore.Settings; diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml b/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml index 6f5f4b55666..89deb506c65 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml +++ b/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml @@ -1,4 +1,5 @@ @using Microsoft.AspNetCore.Identity +@using Microsoft.AspNetCore.Mvc.ModelBinding @using OrchardCore.Entities @using OrchardCore.Settings @using OrchardCore.Users @@ -14,6 +15,15 @@ var allowResetPassword = (await SiteService.GetSiteSettingsAsync()).As().AllowResetPassword; var loginProviders = (await SignInManager.GetExternalAuthenticationSchemesAsync()).ToList(); var disableLocalLogin = (await SiteService.GetSiteSettingsAsync()).As().DisableLocalLogin; + + // When the value of the RememberMe field is set to anything else than "true" or "false" by the client, which is + // the case with automated cracking attempts, then otherwise we'd get a FormatException from the model binder. + if (!ViewContext.ModelState.IsValid && + ViewContext.ModelState.TryGetValue(nameof(LoginViewModel.RememberMe), out var rememberMeValue) && + rememberMeValue.ValidationState == ModelValidationState.Invalid) + { + rememberMeValue.RawValue = "false"; + } } From 8cbd8f01f277060ce68a74cbed1a916eafabd951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Leh=C3=B3czky?= Date: Wed, 13 Dec 2023 22:09:56 +0100 Subject: [PATCH 2/5] Changing anti-cracking attempt model binding to use a custom model binder instead --- .../Services/SafeBoolModelBinder.cs | 40 +++++++++++++++++++ .../Services/SafeBoolModelBinderProvider.cs | 19 +++++++++ .../OrchardCore.Users/Startup.cs | 10 ++++- .../Views/Account/Login.cshtml | 10 ----- 4 files changed, 68 insertions(+), 11 deletions(-) create mode 100644 src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs create mode 100644 src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs b/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs new file mode 100644 index 00000000000..a03aea89e48 --- /dev/null +++ b/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs @@ -0,0 +1,40 @@ +using System; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Localization; +using OrchardCore.Users.ViewModels; + +namespace OrchardCore.Users.Services; + +/// +/// Model binder to produce a validation error when a Boolean field contains a value that's not a valid bool. The +/// default model binder would throw a . That's an issue for that can get invalid values from automated cracking attempts. +/// +internal class SafeBoolModelBinder : IModelBinder +{ + public Task BindModelAsync(ModelBindingContext bindingContext) + { + ArgumentNullException.ThrowIfNull(nameof(bindingContext)); + + var valueProviderResult = bindingContext.ValueProvider.GetValue(bindingContext.ModelName); + + if (valueProviderResult == ValueProviderResult.None) + { + return Task.CompletedTask; + } + + if (bool.TryParse(valueProviderResult.FirstValue, out bool result)) + { + bindingContext.Result = ModelBindingResult.Success(result); + return Task.CompletedTask; + } + + var localizer = bindingContext.HttpContext.RequestServices.GetService>(); + + bindingContext.ModelState.TryAddModelError(bindingContext.ModelName, localizer["Invalid Boolean value."]); + + return Task.CompletedTask; + } +} diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs b/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs new file mode 100644 index 00000000000..f6014645340 --- /dev/null +++ b/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs @@ -0,0 +1,19 @@ +using System; +using Microsoft.AspNetCore.Mvc.ModelBinding; + +namespace OrchardCore.Users.Services; + +internal class SafeBoolModelBinderProvider : IModelBinderProvider +{ + public IModelBinder GetBinder(ModelBinderProviderContext context) + { + ArgumentNullException.ThrowIfNull(context, nameof(context)); + + if (context.Metadata.ModelType == typeof(bool)) + { + return new SafeBoolModelBinder(); + } + + return null; + } +} diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs b/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs index f538777369c..273042340ca 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs @@ -17,8 +17,8 @@ using OrchardCore.Admin.Models; using OrchardCore.Data; using OrchardCore.Data.Migration; -using OrchardCore.DisplayManagement.Descriptors; using OrchardCore.Deployment; +using OrchardCore.DisplayManagement.Descriptors; using OrchardCore.DisplayManagement.Handlers; using OrchardCore.DisplayManagement.Theming; using OrchardCore.Environment.Commands; @@ -258,6 +258,14 @@ public override void ConfigureServices(IServiceCollection services) services.AddScoped(); services.AddRecipeExecutionStep(); + + // When the value of LoginViewModel.RememberMe is set to anything else than "true" or "false" by the + // client, which is the case with automated cracking attempts, then we'd get a FormatException from the + // default model binder. + services.AddMvc(options => + { + options.ModelBinderProviders.Insert(0, new SafeBoolModelBinderProvider()); + }); } } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml b/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml index 89deb506c65..6f5f4b55666 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml +++ b/src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml @@ -1,5 +1,4 @@ @using Microsoft.AspNetCore.Identity -@using Microsoft.AspNetCore.Mvc.ModelBinding @using OrchardCore.Entities @using OrchardCore.Settings @using OrchardCore.Users @@ -15,15 +14,6 @@ var allowResetPassword = (await SiteService.GetSiteSettingsAsync()).As().AllowResetPassword; var loginProviders = (await SignInManager.GetExternalAuthenticationSchemesAsync()).ToList(); var disableLocalLogin = (await SiteService.GetSiteSettingsAsync()).As().DisableLocalLogin; - - // When the value of the RememberMe field is set to anything else than "true" or "false" by the client, which is - // the case with automated cracking attempts, then otherwise we'd get a FormatException from the model binder. - if (!ViewContext.ModelState.IsValid && - ViewContext.ModelState.TryGetValue(nameof(LoginViewModel.RememberMe), out var rememberMeValue) && - rememberMeValue.ValidationState == ModelValidationState.Invalid) - { - rememberMeValue.RawValue = "false"; - } } From 90f4035d2651ef9c9b06c8bc221ea3de635518b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Leh=C3=B3czky?= Date: Wed, 13 Dec 2023 23:21:32 +0100 Subject: [PATCH 3/5] Moving SafeBoolModelBinder to OrchardCore.Mvc.Core --- src/OrchardCore.Modules/OrchardCore.Users/Startup.cs | 8 -------- .../ModelBinding}/SafeBoolModelBinder.cs | 7 +++---- .../ModelBinding}/SafeBoolModelBinderProvider.cs | 2 +- src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs | 2 ++ 4 files changed, 6 insertions(+), 13 deletions(-) rename src/{OrchardCore.Modules/OrchardCore.Users/Services => OrchardCore/OrchardCore.Mvc.Core/ModelBinding}/SafeBoolModelBinder.cs (86%) rename src/{OrchardCore.Modules/OrchardCore.Users/Services => OrchardCore/OrchardCore.Mvc.Core/ModelBinding}/SafeBoolModelBinderProvider.cs (91%) diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs b/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs index 273042340ca..21ec8b3c407 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/Startup.cs @@ -258,14 +258,6 @@ public override void ConfigureServices(IServiceCollection services) services.AddScoped(); services.AddRecipeExecutionStep(); - - // When the value of LoginViewModel.RememberMe is set to anything else than "true" or "false" by the - // client, which is the case with automated cracking attempts, then we'd get a FormatException from the - // default model binder. - services.AddMvc(options => - { - options.ModelBinderProviders.Insert(0, new SafeBoolModelBinderProvider()); - }); } } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs similarity index 86% rename from src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs rename to src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs index a03aea89e48..0d4e8a066a9 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinder.cs +++ b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs @@ -3,14 +3,13 @@ using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Localization; -using OrchardCore.Users.ViewModels; -namespace OrchardCore.Users.Services; +namespace OrchardCore.Mvc.ModelBinding; /// /// Model binder to produce a validation error when a Boolean field contains a value that's not a valid bool. The -/// default model binder would throw a . That's an issue for that can get invalid values from automated cracking attempts. +/// default model binder would throw a . That's an issue for e.g. the Users module, see +/// . /// internal class SafeBoolModelBinder : IModelBinder { diff --git a/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs similarity index 91% rename from src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs rename to src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs index f6014645340..1da355f80fa 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/Services/SafeBoolModelBinderProvider.cs +++ b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs @@ -1,7 +1,7 @@ using System; using Microsoft.AspNetCore.Mvc.ModelBinding; -namespace OrchardCore.Users.Services; +namespace OrchardCore.Mvc.ModelBinding; internal class SafeBoolModelBinderProvider : IModelBinderProvider { diff --git a/src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs b/src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs index 9d11747fa13..8b19b315ffb 100644 --- a/src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs +++ b/src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs @@ -82,6 +82,8 @@ public override void ConfigureServices(IServiceCollection services) // Custom model binder to testing purpose options.ModelBinderProviders.Insert(0, new CheckMarkModelBinderProvider()); + + options.ModelBinderProviders.Insert(0, new SafeBoolModelBinderProvider()); }); // Add a route endpoint selector policy. From b80852464865444eb6e9415ee06b18221e866615 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Leh=C3=B3czky?= Date: Thu, 14 Dec 2023 00:06:50 +0100 Subject: [PATCH 4/5] Sealing internal classes --- .../OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs | 2 +- .../ModelBinding/SafeBoolModelBinderProvider.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs index 0d4e8a066a9..97adef3dd58 100644 --- a/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs +++ b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs @@ -11,7 +11,7 @@ namespace OrchardCore.Mvc.ModelBinding; /// default model binder would throw a . That's an issue for e.g. the Users module, see /// . /// -internal class SafeBoolModelBinder : IModelBinder +internal sealed class SafeBoolModelBinder : IModelBinder { public Task BindModelAsync(ModelBindingContext bindingContext) { diff --git a/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs index 1da355f80fa..46aecd0447e 100644 --- a/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs +++ b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinderProvider.cs @@ -3,7 +3,7 @@ namespace OrchardCore.Mvc.ModelBinding; -internal class SafeBoolModelBinderProvider : IModelBinderProvider +internal sealed class SafeBoolModelBinderProvider : IModelBinderProvider { public IModelBinder GetBinder(ModelBinderProviderContext context) { From 70c50d1ae29663d8a24e7fc77d686be0e8d340c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Leh=C3=B3czky?= Date: Thu, 14 Dec 2023 15:02:29 +0100 Subject: [PATCH 5/5] Formatting Co-authored-by: Hisham Bin Ateya --- .../OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs index 97adef3dd58..e5a54dfdb24 100644 --- a/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs +++ b/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding/SafeBoolModelBinder.cs @@ -27,6 +27,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext) if (bool.TryParse(valueProviderResult.FirstValue, out bool result)) { bindingContext.Result = ModelBindingResult.Success(result); + return Task.CompletedTask; }