fix(prevent-null): [PM-27510] AccountController Prevent Null Values

[Patrick-Pimentel-Bitwarden]

🎟️ Tracking

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

📔 Objective

Does not allow for null values in the AccountController.

📸 Screenshots

⏰ 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

[Patrick-Pimentel-Bitwarden]

Jared reviewed the previous pr #6348 so I'm giving him a break and would love your eyes on it Ike!

[claude]

Claude finished @Patrick-Pimentel-Bitwarden's task —— View job

---

Code Review Complete

Summary of Changes

This PR addresses null safety in AccountController.cs by:

• Removing #nullable disable pragma to enable nullable reference types
• Adding explicit null checks and proper null handling throughout the controller
• Adding 5 new localized error messages for various null scenarios
• Adding comprehensive unit tests for null handling cases
• Removing redundant #nullable enable directive from IUserRepository.cs

Critical Issues

1. Unsafe Null-Forgiving Operator Usage (High Priority)

Location: AccountController.cs:785

if (email == null) throw new Exception(_i18nService.T("NoEmailFoundWhenFallingBackToEmailForOrgUserLookup"));

This null check occurs after the method has already attempted to use email in line 780. The flow is:

// Line 780 - orgUser may have been found via user.Id
orgUser = orgUsersByUserId.SingleOrDefault(u => u.OrganizationId == organizationId);

// Line 785 - null check for email
if (email == null) throw new Exception(...);

// Line 787 - uses email
orgUser ??= await _organizationUserRepository.GetByOrganizationEmailAsync(organizationId, email);

The null check should occur before attempting any email-based lookup, not after. Additionally, if user is not null but email is null, we have an inconsistent state that should be caught earlier.

Recommendation: Move the null check immediately after checking if user is null:

// Try to find OrgUser via existing User Id.
if (user != null)
{
    var orgUsersByUserId = await _organizationUserRepository.GetManyByUserAsync(user.Id);
    orgUser = orgUsersByUserId.SingleOrDefault(u => u.OrganizationId == organizationId);
}

// If no org user found and we need to search by email, ensure email is present
if (orgUser == null)
{
    if (email == null)
    {
        throw new Exception(_i18nService.T("NoEmailFoundWhenFallingBackToEmailForOrgUserLookup"));
    }
    orgUser = await _organizationRepository.GetByOrganizationEmailAsync(organizationId, email);
}

2. Null-Forgiving Operators Without Justification

Locations: Multiple places in AccountController.cs

The code uses several null-forgiving operators (!) without clear justification:

• Line 388: if (User.Identity!.IsAuthenticated) - What guarantees User.Identity is non-null here?
• Line 439: var acrClaim = externalUser!.FindFirst(...) - Already checked but uses ! instead of proper flow control
• Line 458: var userIdClaim = externalUser!.FindFirst(...) - Same issue
• Line 903: var sid = externalResult.Principal!.Claims.FirstOrDefault(...) - No null check for Principal
• Line 910: var idToken = externalResult.Properties!.GetTokenValue("id_token") - No null check for Properties
• Line 926: if (User.Identity!.IsAuthenticated) - Duplicate concern from line 388

Recommendation: Either:

1. Add proper null checks before these usages, OR
2. Add code comments explaining why these are guaranteed to be non-null in these contexts

3. Inconsistent Null Handling in FindUserFromExternalProviderAsync

Location: AccountController.cs:421-480

The method signature now returns User? (nullable), but the internal flow has inconsistencies:

private async Task<(User? user, string provider, string providerUserId, List<Claim> claims, SsoConfigurationData ssoConfigData)> FindUserFromExternalProviderAsync(AuthenticateResult result)
{
    if (result.Properties?.Items == null || ...) // Good null check
    {
        throw new Exception(...);
    }
    
    var externalUser = result.Principal; // No null check
    
    // Later uses null-forgiving operator
    var acrClaim = externalUser!.FindFirst(...); 

result.Principal is never checked for null before using !.

Recommendation: Add explicit null check:

var externalUser = result.Principal;
if (externalUser == null)
{
    throw new Exception(_i18nService.T("ExternalAuthenticationError"));
}

4. Exception Message Inconsistency

Location: AccountController.cs:598

var emailDomain = CoreHelpers.GetEmailDomain(email ?? throw new Exception(_i18nService.T("NoEmailFoundWhenMarkingDomainAsVerified")));

The comment above this line states:

"We cannot reach this code with email currently being null so there is no way to provoke this exception."

Issues:

1. If this is truly unreachable, the null-coalescing operator is defensive but unnecessary
2. The comment suggests this is dead code for the exception case
3. If it's unreachable, consider using email! with a comment explaining why it's safe, or better yet, add an assertion

Recommendation: Either:

1. Use Debug.Assert(email != null, "Email should not be null at this point") + email!, OR
2. Keep the exception but update the comment to explain under what theoretical conditions it could occur, OR
3. If truly impossible, simplify to var emailDomain = CoreHelpers.GetEmailDomain(email!); with a clear comment

Suggested Improvements

5. Redundant Null Checks in AutoProvisionUserAsync

Location: AccountController.cs:500-560

The method signature shows (User user, Organization foundOrganization, OrganizationUser foundOrgUser) as the return type (all non-nullable), but there are code paths where these could potentially be returned as null before the refactoring properly validated all paths.

Looking at line 560: return (existingUser, organization, orgUser);

At this point:

• existingUser is confirmed non-null (checked on line 536)
• organization is confirmed non-null (from TryGetOrganizationByProviderAsync which throws on null)
• orgUser is confirmed non-null (checked on line 546 and ensured on line 552)

This is actually good - the return type correctly reflects that these are never null. However, the XML documentation on line 498 states:

"Guaranteed to return the user to sign in as well as the found organization and org user."

Recommendation: The code and documentation align well here. No changes needed, but verify that all code paths indeed guarantee non-null returns.

6. Overly Broad Exception Messages

Location: Multiple error messages in SharedResources.en.resx

Several new error messages are generic and don't provide enough context for debugging:

• NoEmailFoundWhenFallingBackToEmailForOrgUserLookup - Doesn't indicate what user or organization
• NoProviderOnAuthenticateResult - Doesn't indicate which authentication result

Recommendation: Consider including contextual information in the exceptions:

throw new Exception(_i18nService.T("NoEmailFoundWhenFallingBackToEmailForOrgUserLookup", organizationId));

And update the resource string to include placeholders:

<value>Tried to use email as a fallback when searching for org user in organization '{0}', but there was no email found in claims.</value>

7. Test Coverage Gap

Location: AccountControllerTest.cs

The tests added are excellent for covering null scenarios, but there's a gap:

• Tests for LoginAsync with null context and null domain_hint ✓
• Tests for ExternalCallback with missing scheme ✓
• Tests for TryGetOrganizationUserByUserAndOrgOrEmail with null email ✓
• Missing: Test for FindUserFromExternalProviderAsync with null result.Principal

Recommendation: Add a test case:

[Theory, BitAutoData]
public async Task FindUserFromExternalProviderAsync_WhenPrincipalIsNull_ThrowsExternalAuthenticationError(
    SutProvider<AccountController> sutProvider)
{
    // Arrange
    var properties = new AuthenticationProperties
    {
        Items = { ["scheme"] = Guid.NewGuid().ToString() }
    };
    var ticket = new AuthenticationTicket(null, properties, "scheme"); // null principal
    var authResult = AuthenticateResult.Success(ticket);
    
    // Act + Assert
    var ex = await Assert.ThrowsAsync<Exception>(() => 
        sutProvider.Sut.ExternalCallback());
    Assert.Equal("ExternalAuthenticationError", ex.Message);
}

8. Code Clarity: Variable Naming

Location: AccountController.cs:609

The variable newUser is introduced to disambiguate from existingUser, but the naming could be clearer:

var newUser = new User { ... };
await _registerUserCommand.RegisterUser(newUser);

Later code creates orgUser which can be confusing alongside newUser.

Recommendation: Consider renaming to jitProvisionedUser or registeredUser to make the intent clearer, especially since this is in the JIT provisioning section.

9. Missing Null Check for Properties in ExternalCallback

Location: AccountController.cs:357-363

var returnUrl = "~/";
if (result.Properties?.Items != null
    && result.Properties.Items.TryGetValue("return_url", out var storedReturnUrl)
    && !string.IsNullOrWhiteSpace(storedReturnUrl))
{
    returnUrl = storedReturnUrl;
}

This is good defensive programming, but earlier in the same method (line 277), we access result.Properties?.Items without null checks:

string? userIdentifier =
    result.Properties?.Items != null && result.Properties.Items.ContainsKey("user_identifier")
        ? result.Properties.Items["user_identifier"]
        : null;

Recommendation: The pattern is consistent and good. No changes needed, but consider extracting this to a helper method:

private string? TryGetAuthenticationProperty(AuthenticateResult result, string key)
{
    return result.Properties?.Items != null && result.Properties.Items.TryGetValue(key, out var value)
        ? value
        : null;
}

10. Potential Performance Issue: Multiple Repository Calls

Location: TryGetOrganizationUserByUserAndOrgOrEmail method usage

The measurement tests show that when feature flag is ON and user exists but orgUser doesn't, the method makes:

• 1 call to GetManyByUserAsync (which returns a collection)
• 1 call to GetByOrganizationEmailAsync (fallback)

This is working as intended, but the GetManyByUserAsync returns all org users for a user, then filters client-side with SingleOrDefault. For users with many organizations, this could be inefficient.

Recommendation: Consider adding a repository method GetByOrganizationAsync(Guid organizationId, Guid userId) that does the filtering server-side, or verify that the existing implementation is performant enough for expected user organization counts.

Security Implications

Positive Security Changes

1. Removes #nullable disable: This is a significant security improvement as it enables compile-time null reference checking, preventing potential null reference exceptions that could lead to denial of service
2. Explicit null validation: All null checks now throw meaningful exceptions rather than allowing null references to propagate
3. No PII in error messages: The new error messages appropriately avoid including sensitive user data

Potential Security Concerns

1. Exception information disclosure: The error messages are somewhat descriptive (e.g., "No provider found in the scheme property"). Consider whether these should be logged but return more generic messages to users to avoid information disclosure about system internals.

2. No new authentication/authorization bypasses identified: The null handling changes don't introduce any obvious security bypasses.

Good Practices Observed

• Comprehensive unit test coverage for null scenarios with clear naming
• Consistent use of null-conditional operators (?.) throughout
• Proper i18n for all error messages
• XML documentation updated to reflect nullable types
• Return type changes properly reflect nullability contracts

Action Items for Author

1. Critical: Fix null check ordering in TryGetOrganizationUserByUserAndOrgOrEmail (move check before usage)
2. Critical: Add null check for result.Principal in FindUserFromExternalProviderAsync
3. High: Remove or justify all null-forgiving operators (!) with comments
4. High: Add missing test case for null Principal
5. Medium: Consider adding context to error messages (user ID, org ID where appropriate)
6. Medium: Clarify the unreachable exception comment at line 598
7. Low: Consider variable renaming for clarity (newUser → registeredUser)
8. Low: Consider extracting authentication property helper method

Test Coverage Analysis

Looking at the Codecov report showing 66.67% patch coverage with 14 lines missing coverage:

The following scenarios appear to be covered:

• Null authorization context ✓
• Missing/null domain hint ✓
• Missing scheme in auth result ✓
• Null email in org user lookup ✓
• Various org user status validations ✓

The 14 lines with missing coverage likely include:

• Some exception handling paths
• Edge cases in claim processing
• Certain conditional branches in the refactored null-safe code

Recommendation: Review the Codecov report details to identify which specific lines lack coverage and add targeted tests for those paths.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 102d1c06-d6d1-4245-8f84-1d32045fe5e8

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.84%. Comparing base (02be341) to head (90db2fc).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...n_license/src/Sso/Controllers/AccountController.cs	66.66%	10 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6508      +/-   ##
==========================================
+ Coverage   51.49%   51.84%   +0.35%     
==========================================
  Files        1900     1902       +2     
  Lines       84121    84213      +92     
  Branches     7517     7561      +44     
==========================================
+ Hits        43318    43663     +345     
+ Misses      39115    38838     -277     
- Partials     1688     1712      +24     

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