feat(register): [PM-27084] Account Register Uses New Data Types

[Patrick-Pimentel-Bitwarden]

🎟️ Tracking

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

📔 Objective

1. Update register to use new data types
2. Added comments so that future removal of old properties is clear
3. Made accounts controller not permit nullish ambiguity
4. Added tests to demonstrate payloads result in same outcomes for the user model

📸 Screenshots

Screen.Recording.2025-12-11.at.5.21.28.PM.mov

⏰ 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

[codecov]

Codecov Report

❌ Patch coverage is 51.42857% with 85 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.85%. Comparing base (2442d2d) to head (b5dadcd).

Files with missing lines	Patch %	Lines
...Api/Request/Accounts/RegisterFinishRequestModel.cs	41.66%	68 Missing and 16 partials ⚠️
src/Identity/Controllers/AccountsController.cs	96.42%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6715      +/-   ##
==========================================
+ Coverage   54.94%   58.85%   +3.91%     
==========================================
  Files        1927     1927              
  Lines       85480    85627     +147     
  Branches     7653     7677      +24     
==========================================
+ Hits        46963    50396    +3433     
+ Misses      36729    33346    -3383     
- Partials     1788     1885      +97     

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

[github-actions]

Checkmarx One – Scan Summary & Details – f3f1c4d1-97e8-4699-95fe-24f3f947c376

New Issues (7)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CSRF	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 99	details Method at line 99 of /src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs gets a parameter from a user request from request. This... Attack Vector
2		CSRF	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 98	details Method at line 98 of /src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs gets a parameter from a user request from user. This pa... Attack Vector
3		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1170	details Method at line 1170 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector
4		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1060	details Method at line 1060 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector
5		CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 431	details Method at line 431 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow... Attack Vector
6		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1519	details Method at line 1519 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector
7		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1395	details Method at line 1395 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector

Fixed Issues (1)

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

Severity	Issue	Source File / Package
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300

[Patrick-Pimentel-Bitwarden]

Adding @eligrubb to review because he has work he is branching off of this to do!

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

⚠️ IMPORTANT: Exception thrown from validation method violates IValidatableObject contract

The GetTokenType() method (called at line 135) throws InvalidOperationException when no valid token is found (line 117). This violates the IValidatableObject.Validate() contract, which should return ValidationResult objects, not throw exceptions.

Problem:

public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
    switch (GetTokenType())  // Line 135 - can throw InvalidOperationException
    {
        // ...
    }
}

If GetTokenType() throws, the exception bypasses ASP.NET Core's validation error handling and gets caught by the exception filter instead, resulting in a generic 500 error rather than a proper 400 validation error.

Fix: Wrap the call or refactor to return validation results:

public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
    RegisterFinishTokenType tokenType;
    try
    {
        tokenType = GetTokenType();
    }
    catch (InvalidOperationException)
    {
        yield return new ValidationResult("No valid registration token provided");
        yield break;
    }

    // Check authentication/hash consistency before token validation
    if (MasterPasswordAuthentication != null && MasterPasswordHash != null)
    {
        // ... existing check
    }

    switch (tokenType)
    {
        // ... rest of validation
    }
}

[claude]

⚠️ IMPORTANT: Required field extracted after validation instead of during validation

The master password hash is extracted here with a null-coalescing throw pattern, but this check should happen during model validation, not in the controller action.

Problem:

1. model.Validate() passes (ASP.NET Core doesn't report errors)
2. Controller calls model.ToUser() (passes if fields present)
3. Controller extracts masterPasswordHash (line 150) - throws here if both are null
4. Exception caught and converted to 400 response

Why this is problematic:

• Validation timing inconsistency: Some validation in Validate(), some in controller, some in ToUser()
• The error message "MasterPasswordHash couldn't be found on either..." reveals internal API structure
• Attackers can probe which combinations of fields are accepted

Security concern - Information disclosure:
The error message reveals:

• Property names: MasterPasswordAuthenticationData, MasterPasswordHash
• API structure during migration period
• Which fields are checked and in what order

Fix: Move this validation into RegisterFinishRequestModel.Validate():

// In RegisterFinishRequestModel.Validate(), before token checks:
if (MasterPasswordAuthentication == null && MasterPasswordHash == null)
{
    yield return new ValidationResult(
        "Master password hash is required.",
        [nameof(MasterPasswordAuthentication), nameof(MasterPasswordHash)]);
}

Then in controller, this becomes a simple assertion:

string masterPasswordHash = model.MasterPasswordAuthentication?.MasterPasswordAuthenticationHash 
                         ?? model.MasterPasswordHash!;  // Safe: validated above

[claude]

🎨 SUGGESTED: Force unwraps rely on validation that can be bypassed

The null-forgiving operators (!) on lines 159, 167, 174, 181, and 190 assume that validation has ensured these values are non-null. However, validation can be incomplete or bypassed in several scenarios:

Current risk:

• If validation has gaps (see other comments about incomplete validation)
• If [ApiController] attribute is not present (automatic validation disabled)
• If validation is explicitly bypassed in tests or other contexts

Evidence of risk:
The validation for tokens only checks if they're null/empty in the switch statement (lines 137-192 in RegisterFinishRequestModel), but these checks only run if GetTokenType() succeeds. If token detection fails, validation may not run.

Suggestion: Use explicit null checks with appropriate error handling:

case RegisterFinishTokenType.EmailVerification:
    if (string.IsNullOrEmpty(model.EmailVerificationToken))
    {
        throw new BadRequestException("Email verification token is required");
    }
    identityResult = await _registerUserCommand.RegisterUserViaEmailVerificationToken(
        user,
        masterPasswordHash,
        model.EmailVerificationToken);
    return ProcessRegistrationResult(identityResult, user);

This provides defense in depth and makes the code's assumptions explicit, reducing risk of NullReferenceException if validation is bypassed.

[claude]

❌ CRITICAL: Validate() violates IValidatableObject contract by allowing GetTokenType() to throw exceptions

Details and fix

Problem:
GetTokenType() can throw InvalidOperationException (line 117) when no valid token is present. The ASP.NET validation framework does not catch exceptions from IValidatableObject.Validate() - they propagate as unhandled exceptions, bypassing validation entirely and returning 500 errors instead of 400 validation errors.

Execution path:

1. Client submits request with invalid token combination
2. Model binding triggers Validate()
3. Line 135 calls GetTokenType()
4. No valid token type matches → line 117 throws InvalidOperationException
5. Exception bypasses validation framework → 500 Internal Server Error

Fix:
Wrap the GetTokenType() call in try-catch and convert exceptions to ValidationResults:

public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
    // Validate token type without throwing
    RegisterFinishTokenType tokenType;
    try
    {
        tokenType = GetTokenType();
    }
    catch (InvalidOperationException)
    {
        yield return new ValidationResult("Invalid registration request: no valid token provided");
        yield break;
    }

    // Rest of validation using tokenType...

Impact: Without this fix, invalid registration requests crash with 500 errors instead of returning proper 400 validation errors, breaking client error handling and making debugging difficult.

Reference: IValidatableObject documentation

[quexten]

Please rename to "masterPasswordAuthenticationHash", or just use the MasterPasswordAuthentication struct all the way through. Even if it is not on the request model, you can still construct it from the email and kdf that are on the request model.

[mzieniukbw]

⚠️ This doesn't sound correct, to override all of the test's user key, just because the tests themselves have invalid encrypted string.
This requires fixing the tests that use this function.