Skip to content

Conversation

@r-tome
Copy link
Contributor

@r-tome r-tome commented Jan 7, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28842

📔 Objective

Fixes an issue where organization owners could set unreasonably large values for the master password policy's minimum length (e.g., 1000).

Clients PR

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

r-tome added 3 commits January 7, 2026 16:22
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.
…andling

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.
…nComplexity 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.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

Logo
Checkmarx One – Scan Summary & Detailsac761254-412f-4562-a3f6-b63995ff3ee6

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM Use_Of_Hardcoded_Password /src/Core/Constants.cs: 169

@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.07%. Comparing base (f2aa742) to head (fcc38f4).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...Core/AdminConsole/Utilities/PolicyDataValidator.cs 83.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6807      +/-   ##
==========================================
+ Coverage   55.03%   55.07%   +0.03%     
==========================================
  Files        1934     1934              
  Lines       85581    85787     +206     
  Branches     7656     7677      +21     
==========================================
+ Hits        47098    47244     +146     
- Misses      36695    36751      +56     
- Partials     1788     1792       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

r-tome added 2 commits January 8, 2026 13:59
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.
…omplexity 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.
@r-tome r-tome marked this pull request as ready for review January 8, 2026 16:30
@r-tome r-tome requested a review from a team as a code owner January 8, 2026 16:30
@r-tome r-tome requested a review from BTreston January 8, 2026 16:30
@claude
Copy link
Contributor

claude bot commented Jan 8, 2026

Claude finished @r-tome's task in 3m 2s —— View job


Overall Assessment: APPROVE

This PR adds validation to prevent organization owners from setting unreasonably large values for master password policy's minimum length and complexity. The implementation is well-designed with comprehensive test coverage across unit and integration test layers.

Code Review Details

Security

  • ✅ Validation limits are appropriate (MinLength: 12-128, MinComplexity: 0-4)
  • ✅ Prevents denial-of-service through impossible password requirements
  • ✅ Zero-knowledge principles maintained - no exposure of sensitive data

Correctness

  • ✅ Validation logic correctly implemented using DataAnnotations
  • ✅ Error messages are clear and informative
  • ✅ Validation occurs at request model conversion, preventing invalid new policies
  • ✅ Existing policies with values outside ranges are grandfathered (not a breaking change)

Test Coverage

  • ✅ Comprehensive unit tests for boundary conditions (min, max, below min, above max)
  • ✅ Integration tests for both Admin and Public API endpoints
  • ✅ Edge cases covered (null values, multiple invalid fields, type mismatches)

Code Quality

  • ✅ XML documentation added for clarity
  • ✅ Follows established patterns in the codebase
  • ✅ Consistent with other policy validation approaches

Breaking Changes

  • ✅ None - Only validates NEW policy updates
  • ✅ Existing policies continue to function
  • ✅ Database migration not required

No issues found. The implementation is production-ready.


🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.5 [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants