Skip to content

feat(kdf-settings-validator): Enforce salt cannot be empty string.#7628

Open
enmande wants to merge 3 commits into
auth/pm-35393/master-password-service-auth-integrationfrom
auth/master-password-service-disallow-empty-string-salt
Open

feat(kdf-settings-validator): Enforce salt cannot be empty string.#7628
enmande wants to merge 3 commits into
auth/pm-35393/master-password-service-auth-integrationfrom
auth/master-password-service-disallow-empty-string-salt

Conversation

@enmande
Copy link
Copy Markdown
Contributor

@enmande enmande commented May 13, 2026

🎟️ Tracking

PM-35393

📔 Objective

[Required] attribution on salt-related fields enforces by default that salt cannot be null, empty, or whitespace. However, this is a configurable opinion. While it should probably not be configured to allow whitespace, because this is a critical path, reinforcement should be present to guard against configuration drift, which would otherwise be invisible to this logic.
string.IsNullOrWhitespace properly captures all of these.

Tests have been added to support.

@enmande enmande added the ai-review Request a Claude code review label May 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 13, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR layers defense-in-depth against null/empty/whitespace master-password salts. KdfSettingsValidator.ValidateAuthenticationAndUnlockData and ValidateKdfAndSaltAgreement now short-circuit with a ValidationResult when either salt fails string.IsNullOrWhiteSpace, and the request models MasterPasswordAuthenticationDataRequestModel.Salt and MasterPasswordUnlockDataRequestModel.Salt have been tightened to [Required(AllowEmptyStrings = false)]. The yield break short-circuit mirrors the existing KDF-mismatch handling, and the new [Theory] tests cover empty string, space, tab, and newline inputs for both salt fields across both validator entry points. The validator guard is the load-bearing layer since the validator consumes data-layer models reachable from non-request-model call paths.

Code Review Details

No findings.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.88%. Comparing base (eeb3dd5) to head (e105c20).

Additional details and impacted files
@@                                   Coverage Diff                                   @@
##           auth/pm-35393/master-password-service-auth-integration    #7628   +/-   ##
=======================================================================================
  Coverage                                                   59.87%   59.88%           
=======================================================================================
  Files                                                        2124     2124           
  Lines                                                       93469    93492   +23     
  Branches                                                     8307     8311    +4     
=======================================================================================
+ Hits                                                        55965    55988   +23     
  Misses                                                      35527    35527           
  Partials                                                     1977     1977           

☔ 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.

@enmande enmande marked this pull request as ready for review May 14, 2026 13:32
@enmande enmande requested a review from ike-kottlowski May 14, 2026 13:32
Copy link
Copy Markdown
Contributor

@ike-kottlowski ike-kottlowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 I'm thinking we can tighten up the data annotation on the request model themselves as well. This would give us the benefit of failing closer to the controller and the KdfSettingsValidator changes act as a safety net that should be unreachable in practice.

// MasterPasswordAuthenticationDataRequestModel.cs
[Required(AllowEmptyStrings = false)]
[StringLength(256)]
public required string Salt { get; init; }

// MasterPasswordUnlockDataRequestModel.cs
[Required(AllowEmptyStrings = false)]
[StringLength(256)]
public required string Salt { get; init; }

@enmande enmande requested a review from a team as a code owner May 18, 2026 15:29
@enmande enmande requested review from quexten and removed request for a team May 18, 2026 15:29
@sonarqubecloud
Copy link
Copy Markdown

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.

3 participants