From c8124667ee7423f21aaf03914e9370ae9c4a2d0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Mon, 26 Jan 2026 11:38:06 +0000 Subject: [PATCH] [PM-28842] Add validation to prevent excessive master password policy values (#6807) * Enhance MasterPasswordPolicyData with validation attributes Added data annotations for MinComplexity and MinLength properties to enforce validation rules. MinComplexity must be between 0 and 4, and MinLength must be between 12 and 128. * Implement model validation in PolicyDataValidator and enhance error handling Added a ValidateModel method to enforce validation rules for policy data. Updated error messages to provide clearer feedback on validation failures. Enhanced unit tests to cover new validation scenarios for MinLength and MinComplexity properties. * Update PoliciesControllerTests to reflect new validation rules for MinComplexity and MinLength Modified test cases to use updated values for MinComplexity (4) and MinLength (128). Added new tests to verify that excessive values for these properties return BadRequest responses. Ensured consistency across integration tests for both Admin and Public controllers. * Enhance MasterPasswordPolicyData with XML documentation for properties Added XML documentation comments for MinComplexity and MinLength properties to clarify their purpose and constraints. This improves code readability and provides better context for developers using the model. * Add unit tests for PolicyDataValidator to validate minLength and minComplexity rules Implemented new test cases to verify the behavior of the ValidateAndSerialize method in PolicyDataValidator. Tests cover scenarios for minimum and maximum values, as well as edge cases for invalid inputs, ensuring robust validation for MasterPassword policy data. --- .../Policies/MasterPasswordPolicyData.cs | 12 +- .../Utilities/PolicyDataValidator.cs | 21 ++- .../Controllers/PoliciesControllerTests.cs | 48 ++++++- .../Controllers/PoliciesControllerTests.cs | 50 ++++++- .../Utilities/PolicyDataValidatorTests.cs | 125 +++++++++++++++++- 5 files changed, 246 insertions(+), 10 deletions(-) diff --git a/src/Core/AdminConsole/Models/Data/Organizations/Policies/MasterPasswordPolicyData.cs b/src/Core/AdminConsole/Models/Data/Organizations/Policies/MasterPasswordPolicyData.cs index b66244ba5f..228d7a26f1 100644 --- a/src/Core/AdminConsole/Models/Data/Organizations/Policies/MasterPasswordPolicyData.cs +++ b/src/Core/AdminConsole/Models/Data/Organizations/Policies/MasterPasswordPolicyData.cs @@ -1,11 +1,21 @@ -using System.Text.Json.Serialization; +using System.ComponentModel.DataAnnotations; +using System.Text.Json.Serialization; namespace Bit.Core.AdminConsole.Models.Data.Organizations.Policies; public class MasterPasswordPolicyData : IPolicyDataModel { + /// + /// Minimum password complexity score (0-4). Null indicates no complexity requirement. + /// [JsonPropertyName("minComplexity")] + [Range(0, 4)] public int? MinComplexity { get; set; } + + /// + /// Minimum password length (12-128). Null indicates no minimum length requirement. + /// [JsonPropertyName("minLength")] + [Range(12, 128)] public int? MinLength { get; set; } [JsonPropertyName("requireLower")] public bool? RequireLower { get; set; } diff --git a/src/Core/AdminConsole/Utilities/PolicyDataValidator.cs b/src/Core/AdminConsole/Utilities/PolicyDataValidator.cs index 84e63f2a20..d533ca88cf 100644 --- a/src/Core/AdminConsole/Utilities/PolicyDataValidator.cs +++ b/src/Core/AdminConsole/Utilities/PolicyDataValidator.cs @@ -1,4 +1,5 @@ -using System.Text.Json; +using System.ComponentModel.DataAnnotations; +using System.Text.Json; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; @@ -30,7 +31,8 @@ public static class PolicyDataValidator switch (policyType) { case PolicyType.MasterPassword: - CoreHelpers.LoadClassFromJsonData(json); + var masterPasswordData = CoreHelpers.LoadClassFromJsonData(json); + ValidateModel(masterPasswordData, policyType); break; case PolicyType.SendOptions: CoreHelpers.LoadClassFromJsonData(json); @@ -44,11 +46,24 @@ public static class PolicyDataValidator } catch (JsonException ex) { - var fieldInfo = !string.IsNullOrEmpty(ex.Path) ? $": field '{ex.Path}' has invalid type" : ""; + var fieldName = !string.IsNullOrEmpty(ex.Path) ? ex.Path.TrimStart('$', '.') : null; + var fieldInfo = !string.IsNullOrEmpty(fieldName) ? $": {fieldName} has an invalid value" : ""; throw new BadRequestException($"Invalid data for {policyType} policy{fieldInfo}."); } } + private static void ValidateModel(object model, PolicyType policyType) + { + var validationContext = new ValidationContext(model); + var validationResults = new List(); + + if (!Validator.TryValidateObject(model, validationContext, validationResults, true)) + { + var errors = string.Join(", ", validationResults.Select(r => r.ErrorMessage)); + throw new BadRequestException($"Invalid data for {policyType} policy: {errors}"); + } + } + /// /// Validates and deserializes policy metadata based on the policy type. /// diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/PoliciesControllerTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/PoliciesControllerTests.cs index e4098ce9a9..d58538ae1c 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/PoliciesControllerTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/PoliciesControllerTests.cs @@ -150,8 +150,8 @@ public class PoliciesControllerTests : IClassFixture, IAs Enabled = true, Data = new Dictionary { - { "minComplexity", 10 }, - { "minLength", 12 }, + { "minComplexity", 4 }, + { "minLength", 128 }, { "requireUpper", true }, { "requireLower", false }, { "requireNumbers", true }, @@ -397,4 +397,48 @@ public class PoliciesControllerTests : IClassFixture, IAs // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); } + + [Fact] + public async Task Put_MasterPasswordPolicy_ExcessiveMinLength_ReturnsBadRequest() + { + // Arrange + var policyType = PolicyType.MasterPassword; + var request = new PolicyRequestModel + { + Enabled = true, + Data = new Dictionary + { + { "minLength", 129 } + } + }; + + // Act + var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}", + JsonContent.Create(request)); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task Put_MasterPasswordPolicy_ExcessiveMinComplexity_ReturnsBadRequest() + { + // Arrange + var policyType = PolicyType.MasterPassword; + var request = new PolicyRequestModel + { + Enabled = true, + Data = new Dictionary + { + { "minComplexity", 5 } + } + }; + + // Act + var response = await _client.PutAsync($"/organizations/{_organization.Id}/policies/{policyType}", + JsonContent.Create(request)); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } } diff --git a/test/Api.IntegrationTest/AdminConsole/Public/Controllers/PoliciesControllerTests.cs b/test/Api.IntegrationTest/AdminConsole/Public/Controllers/PoliciesControllerTests.cs index 6144d7eebb..a669bdd93c 100644 --- a/test/Api.IntegrationTest/AdminConsole/Public/Controllers/PoliciesControllerTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Public/Controllers/PoliciesControllerTests.cs @@ -61,7 +61,8 @@ public class PoliciesControllerTests : IClassFixture, IAs Enabled = true, Data = new Dictionary { - { "minComplexity", 15}, + { "minComplexity", 4}, + { "minLength", 128 }, { "requireLower", true} } }; @@ -78,7 +79,8 @@ public class PoliciesControllerTests : IClassFixture, IAs Assert.IsType(result.Id); Assert.NotEqual(default, result.Id); Assert.NotNull(result.Data); - Assert.Equal(15, ((JsonElement)result.Data["minComplexity"]).GetInt32()); + Assert.Equal(4, ((JsonElement)result.Data["minComplexity"]).GetInt32()); + Assert.Equal(128, ((JsonElement)result.Data["minLength"]).GetInt32()); Assert.True(((JsonElement)result.Data["requireLower"]).GetBoolean()); // Assert against the database values @@ -94,7 +96,7 @@ public class PoliciesControllerTests : IClassFixture, IAs Assert.NotNull(policy.Data); var data = policy.GetDataModel(); - var expectedData = new MasterPasswordPolicyData { MinComplexity = 15, RequireLower = true }; + var expectedData = new MasterPasswordPolicyData { MinComplexity = 4, MinLength = 128, RequireLower = true }; AssertHelper.AssertPropertyEqual(expectedData, data); } @@ -242,4 +244,46 @@ public class PoliciesControllerTests : IClassFixture, IAs // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); } + + [Fact] + public async Task Put_MasterPasswordPolicy_ExcessiveMinLength_ReturnsBadRequest() + { + // Arrange + var policyType = PolicyType.MasterPassword; + var request = new PolicyUpdateRequestModel + { + Enabled = true, + Data = new Dictionary + { + { "minLength", 129 } + } + }; + + // Act + var response = await _client.PutAsync($"/public/policies/{policyType}", JsonContent.Create(request)); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task Put_MasterPasswordPolicy_ExcessiveMinComplexity_ReturnsBadRequest() + { + // Arrange + var policyType = PolicyType.MasterPassword; + var request = new PolicyUpdateRequestModel + { + Enabled = true, + Data = new Dictionary + { + { "minComplexity", 5 } + } + }; + + // Act + var response = await _client.PutAsync($"/public/policies/{policyType}", JsonContent.Create(request)); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } } diff --git a/test/Core.Test/AdminConsole/Utilities/PolicyDataValidatorTests.cs b/test/Core.Test/AdminConsole/Utilities/PolicyDataValidatorTests.cs index 43725d23e0..dcc4ceb246 100644 --- a/test/Core.Test/AdminConsole/Utilities/PolicyDataValidatorTests.cs +++ b/test/Core.Test/AdminConsole/Utilities/PolicyDataValidatorTests.cs @@ -19,12 +19,17 @@ public class PolicyDataValidatorTests [Fact] public void ValidateAndSerialize_ValidData_ReturnsSerializedJson() { - var data = new Dictionary { { "minLength", 12 } }; + var data = new Dictionary + { + { "minLength", 12 }, + { "minComplexity", 4 } + }; var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword); Assert.NotNull(result); Assert.Contains("\"minLength\":12", result); + Assert.Contains("\"minComplexity\":4", result); } [Fact] @@ -56,4 +61,122 @@ public class PolicyDataValidatorTests Assert.IsType(result); } + + [Fact] + public void ValidateAndSerialize_ExcessiveMinLength_ThrowsBadRequestException() + { + var data = new Dictionary { { "minLength", 129 } }; + + var exception = Assert.Throws(() => + PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword)); + + Assert.Contains("Invalid data for MasterPassword policy", exception.Message); + } + + [Fact] + public void ValidateAndSerialize_ExcessiveMinComplexity_ThrowsBadRequestException() + { + var data = new Dictionary { { "minComplexity", 5 } }; + + var exception = Assert.Throws(() => + PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword)); + + Assert.Contains("Invalid data for MasterPassword policy", exception.Message); + } + + [Fact] + public void ValidateAndSerialize_MinLengthAtMinimum_Succeeds() + { + var data = new Dictionary { { "minLength", 12 } }; + + var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword); + + Assert.NotNull(result); + Assert.Contains("\"minLength\":12", result); + } + + [Fact] + public void ValidateAndSerialize_MinLengthAtMaximum_Succeeds() + { + var data = new Dictionary { { "minLength", 128 } }; + + var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword); + + Assert.NotNull(result); + Assert.Contains("\"minLength\":128", result); + } + + [Fact] + public void ValidateAndSerialize_MinLengthBelowMinimum_ThrowsBadRequestException() + { + var data = new Dictionary { { "minLength", 11 } }; + + var exception = Assert.Throws(() => + PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword)); + + Assert.Contains("Invalid data for MasterPassword policy", exception.Message); + } + + [Fact] + public void ValidateAndSerialize_MinComplexityAtMinimum_Succeeds() + { + var data = new Dictionary { { "minComplexity", 0 } }; + + var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword); + + Assert.NotNull(result); + Assert.Contains("\"minComplexity\":0", result); + } + + [Fact] + public void ValidateAndSerialize_MinComplexityAtMaximum_Succeeds() + { + var data = new Dictionary { { "minComplexity", 4 } }; + + var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword); + + Assert.NotNull(result); + Assert.Contains("\"minComplexity\":4", result); + } + + [Fact] + public void ValidateAndSerialize_MinComplexityBelowMinimum_ThrowsBadRequestException() + { + var data = new Dictionary { { "minComplexity", -1 } }; + + var exception = Assert.Throws(() => + PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword)); + + Assert.Contains("Invalid data for MasterPassword policy", exception.Message); + } + + [Fact] + public void ValidateAndSerialize_NullMinLength_Succeeds() + { + var data = new Dictionary + { + { "minComplexity", 2 } + // minLength is omitted, should be null + }; + + var result = PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword); + + Assert.NotNull(result); + Assert.Contains("\"minComplexity\":2", result); + } + + [Fact] + public void ValidateAndSerialize_MultipleInvalidFields_ThrowsBadRequestException() + { + var data = new Dictionary + { + { "minLength", 200 }, + { "minComplexity", 10 } + }; + + var exception = Assert.Throws(() => + PolicyDataValidator.ValidateAndSerialize(data, PolicyType.MasterPassword)); + + Assert.Contains("Invalid data for MasterPassword policy", exception.Message); + } }