[PM-26429] Add validation to policy data and metadata #6460
Conversation
…policy data and metadata.
…lid data types in PolicyRequestModel and SavePolicyRequest.
…AndSerializePolicyData nullable
test/Api.IntegrationTest/AdminConsole/Controllers/PoliciesControllerTests.cs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6460 +/- ##
==========================================
+ Coverage 52.15% 52.19% +0.03%
==========================================
Files 1908 1909 +1
Lines 84413 84431 +18
Branches 7537 7537
==========================================
+ Hits 44026 44065 +39
+ Misses 38675 38652 -23
- Partials 1712 1714 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (3)Checkmarx found the following issues in this Pull Request
|
…ing of invalid data types in policy updates.
…ata and metadata based on policy type.
…stModel to utilize PolicyDataValidator for data validation and serialization, removing redundant methods and improving code clarity.
…d Metadata properties with empty dictionaries.
…validation methods
…empty data and metadata, and remove null assignments in test cases for improved clarity.
…ic details in BadRequestException messages.
…st responses by checking for specific field names in the response content.
|
@jrmccannon, I have updated the error message to explicitly say which field had the wrong type. |
…able dictionaries for Data and Metadata properties; enhance validation methods in PolicyDataValidator to handle null cases.
…oliciesController
|
Claude finished @r-tome's task —— View job Comprehensive PR ReviewI've completed a thorough review of this PR which adds server-side validation for policy data and metadata. The implementation centralizes validation logic in a new Summary of Changes Since Last ReviewThis PR has evolved significantly since the initial commits, incorporating feedback from previous reviews:
Critical Issues1. Incomplete Validation CoverageSeverity: The
Details & ImpactLocation: Current Code: switch (policyType)
{
case PolicyType.MasterPassword:
CoreHelpers.LoadClassFromJsonData<MasterPasswordPolicyData>(json);
break;
case PolicyType.SendOptions:
CoreHelpers.LoadClassFromJsonData<SendOptionsPolicyData>(json);
break;
case PolicyType.ResetPassword:
CoreHelpers.LoadClassFromJsonData<ResetPasswordDataModel>(json);
break;
}Problem: The other 16 policy types receive NO validation. Invalid data types will be accepted and stored, potentially causing runtime errors when the data is later deserialized for use. Example Attack Vector: // This would be accepted for PasswordGenerator policy without validation:
{
"minLength": "not a number", // Wrong type
"useNumbers": "yes" // Wrong type
}Recommendation:
2. Silent Metadata Validation FailuresSeverity: The Details & ImpactLocation: Current Code: try
{
var json = JsonSerializer.Serialize(metadata);
return policyType switch
{
PolicyType.OrganizationDataOwnership =>
CoreHelpers.LoadClassFromJsonData<OrganizationModelOwnershipPolicyModel>(json),
_ => new EmptyMetadataModel()
};
}
catch (JsonException)
{
return new EmptyMetadataModel(); // Silent failure!
}Problem: If a client sends invalid metadata (wrong types, bad format), the request succeeds with HTTP 200, but the metadata is silently ignored. This violates the principle of least surprise and makes debugging difficult. Scenario: POST /organizations/{id}/policies/OrganizationDataOwnership/vnext
{
"policy": { "type": 5, "enabled": true },
"metadata": { "defaultUserCollectionName": 12345 } // Wrong type!
}
// Returns 200 OK, but metadata is silently droppedRecommendation:
Inconsistency Note: The 3. Missing JSON Deserialization OptionsSeverity: The validator uses default Details & ImpactLocation: Current Code: var json = JsonSerializer.Serialize(data); // Uses DEFAULT options
// Then later...
CoreHelpers.LoadClassFromJsonData<T>(json); // Uses CUSTOM _jsonSerializerOptionsProblem: The initial serialization might produce JSON that doesn't match what Recommendation:
Significant Issues & Improvements4. Incomplete Error ContextSeverity: 🎨 Medium Error messages could be more helpful by including the actual received value type. Example ImprovementCurrent: Better: Implementation Suggestion: catch (JsonException ex)
{
var fieldInfo = !string.IsNullOrEmpty(ex.Path)
? $": field '{ex.Path}' - {ex.Message}"
: $": {ex.Message}";
throw new BadRequestException($"Invalid data for {policyType} policy{fieldInfo}.");
}5. Missing Input ValidationSeverity: 🎨 Low-Medium The methods don't validate their DetailsLocation: Both validation methods Recommendation: Add enum validation at the start: if (!Enum.IsDefined(typeof(PolicyType), policyType))
{
throw new ArgumentException($"Invalid policy type: {policyType}", nameof(policyType));
}6. Test Coverage GapsSeverity: 🎨 Low-Medium Based on the codecov report showing 96.67% coverage with 2 lines missing: Missing Test Cases
Note: The 2 missing lines identified by codecov are likely in the 7. Documentation GapSeverity: 📝 Low The validator class lacks XML documentation explaining its purpose, when to use it, and the validation strategy for different policy types. Suggested Addition/// <summary>
/// Centralized validator for policy data and metadata across all policy endpoints.
/// This class ensures type safety and data integrity before policy data is persisted.
/// </summary>
/// <remarks>
/// Only policy types with structured data models are validated. Policy types without
/// validation switches will accept any valid JSON structure.
/// </remarks>
public static class PolicyDataValidatorSecurity Assessment✅ No critical security issues identified The changes align with Bitwarden's security principles:
Minor Security Note: Consider rate limiting on validation failures to prevent potential DoS through expensive validation operations, though this is likely handled at the API gateway level. Good Practices Observed
Action Items for AuthorHigh Priority:
Medium Priority: Low Priority: Architecture & DesignThe overall approach is sound: ✅ Strengths:
🤔 Considerations for Future:
ConclusionThis is a well-structured PR that successfully centralizes validation logic and adds comprehensive test coverage. The main concerns are:
Once these issues are addressed, the PR will significantly improve the robustness of policy data handling. The existing reviewers have already approved, but I recommend addressing at least the two high-priority items before merging. Overall Assessment: The implementation quality is good, but the incomplete validation coverage is a significant gap that should be addressed to fully meet the PR objectives. |
…e policy data and metadata
…wnership metadata
r-tome
deleted the
ac/pm-26429/add-validation-to-policy-data-and-metadata
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26429
📔 Objective
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes