PM-27108 - Add OrgId checks in SSO Process

[rr-bw]

🎟️ Tracking

PM-27108

📔 Objective

Throw an error in /Accounts/ExternalChallenge if the scheme (organization ID) passed in does not match the organization ID from the ssoToken.

📸 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

[github-actions]

Checkmarx One – Scan Summary & Details – 59028af4-a4e8-404f-b869-64eea0192015

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

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 53.66%. Comparing base (acc2529) to head (8259e32).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...n_license/src/Sso/Controllers/AccountController.cs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6710      +/-   ##
==========================================
+ Coverage   53.63%   53.66%   +0.02%     
==========================================
  Files        1926     1926              
  Lines       85717    85724       +7     
  Branches     7686     7687       +1     
==========================================
+ Hits        45978    46001      +23     
+ Misses      37966    37948      -18     
- Partials     1773     1775       +2     

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

[claude]

Claude finished @rr-bw's task —— View job

---

Code Review Complete

Todo List

• Read changed files to understand implementation
• Run comprehensive code review using Bitwarden reviewer agent
• Post review findings

---

Code Review: Post-SSO Lock Screen Bypass Fix (PM-27108)

Overall Assessment: ✅ APPROVE

This security fix properly addresses the lock screen bypass vulnerability by validating that the organization ID in the scheme parameter matches the organization ID from the ssoToken.

---

🔒 Security Analysis

✅ Finding 1: Validation Logic is Sound

Details and analysis

The new ValidateSchemeAgainstSsoToken method (lines 247-264 in AccountController.cs) correctly:

1. Unpacks the protected ssoToken to extract the SsoTokenable
2. Validates the scheme parameter is a valid GUID
3. Compares the parsed scheme GUID against tokenable.OrganizationId
4. Throws appropriate exceptions for invalid tokens or mismatches

This prevents an attacker from:

• Substituting a different organization's ID in the scheme parameter while reusing a valid ssoToken
• Bypassing the lock screen by redirecting authentication to a different organization's SSO configuration

Security Impact: This fix closes a critical authentication bypass vulnerability where an attacker could manipulate the scheme parameter to redirect the authentication challenge to a different organization while keeping a valid token for their original organization.

✅ Finding 2: Validation Placement is Correct

Details and analysis

The validation is called at the beginning of ExternalChallenge (line 211), before any authentication challenge is issued. This is the correct placement because:

• It's the entry point where the scheme and ssoToken parameters come together
• It occurs before the Challenge() call that would initiate the external SSO flow
• It prevents any downstream security implications of mismatched parameters

The flow is:

1. LoginAsync validates the token and redirects to ExternalChallenge with ssoToken parameter
2. ExternalChallenge now validates the scheme matches the organization from the token
3. Only then does it proceed with the authentication challenge

✅ Finding 3: Error Handling Approach

Details and analysis

The error handling uses generic Exception throws with i18n message keys:

• InvalidSsoToken - for malformed/corrupted tokens
• SsoOrganizationIdMismatch - for scheme/token org ID mismatches

Considerations:

• Using generic Exception is consistent with the existing codebase pattern (see lines 172, 222, etc.)
• The broad catch block (line 255) appropriately treats all token unprotection failures as invalid tokens
• Error messages don't leak sensitive information about the validation failure

Note: The i18n message keys (InvalidSsoToken, SsoOrganizationIdMismatch) are referenced in code but should be verified to exist in the localization resource files for proper error messaging.

---

🧪 Test Coverage Analysis

✅ Finding 4: Comprehensive Test Coverage

Details and analysis

The test suite adds 4 new tests for ExternalChallenge validation (lines 1143-1267):

1. ExternalChallenge_WithMatchingOrgId_Succeeds - Happy path test verifying normal flow works when org IDs match
2. ExternalChallenge_WithMismatchedOrgId_ThrowsSsoOrganizationIdMismatch - Tests rejection when scheme uses a different org ID than the token
3. ExternalChallenge_WithInvalidSchemeFormat_ThrowsSsoOrganizationIdMismatch - Tests rejection when scheme is not a valid GUID
4. ExternalChallenge_WithInvalidSsoToken_ThrowsInvalidSsoToken - Tests rejection when token cannot be unprotected

Coverage Assessment:

• ✅ All three code paths in ValidateSchemeAgainstSsoToken are tested
• ✅ Both error conditions are validated
• ✅ Happy path confirmation ensures no regression
• ✅ Test isolation is good with proper mocking of dependencies

---

📋 Code Quality Assessment

✅ Finding 5: Code Quality and Standards

Details and analysis

Strengths:

• Clear XML documentation for the new method
• Consistent with existing code patterns in the controller
• Proper use of dependency injection (_dataProtector, _i18nService)
• Follows single responsibility principle
• Method naming is descriptive

Minor Observations:

• The method signature change to ExternalChallenge adds the ssoToken parameter, which is a breaking change at the API level but appropriate for security
• The validation method is private, which is correct - it's an internal implementation detail
• The broad catch block on line 254 is appropriate for security (fail closed)

✅ Finding 6: Compliance with Project Guidelines

Details and analysis

Reviewing against .claude/CLAUDE.md requirements:

• ✅ Security: Follows secure defaults and proper input validation
• ✅ Zero-knowledge principles: No vault data involved, authentication layer only
• ✅ Error handling: Appropriate exceptions with internationalized messages
• ✅ Testing: Comprehensive xUnit tests added with mocking
• ✅ No code regions: Not used
• ✅ Logging: Existing error handling doesn't log sensitive data

The implementation follows architectural decision records and security best practices outlined in the contributing guidelines.

---

💡 Additional Observations

✅ Finding 7: Integration with Existing Security Measures

Details and analysis

This fix integrates well with existing security measures:

1. Token validation in LoginAsync (lines 191-196) - Already validates token expiry and organization match via tokenable.TokenIsValid(organization)
2. New validation in ExternalChallenge - Adds defense-in-depth by re-validating the organization ID match at the challenge point
3. Data protection - Uses IDataProtectorTokenFactory<SsoTokenable> for cryptographic token protection

This creates a robust security chain where the token's organizational context is validated at multiple points in the authentication flow.

📝 Finding 8: Localization Keys

Details and recommendation

The error message keys InvalidSsoToken and SsoOrganizationIdMismatch are used but should be verified.

Recommendation: Before merging, confirm that these localization keys exist in the i18n resource files (typically in a locales/ directory). If they don't exist, they need to be added to provide proper user-facing error messages.

This is not blocking for the security fix but should be confirmed before release.

---

Summary

Category	Status	Notes
Security	✅ APPROVED	Properly addresses SSO lock screen bypass vulnerability
Correctness	✅ APPROVED	Validation logic is sound, properly placed, handles all edge cases
Test Coverage	✅ APPROVED	Comprehensive test suite covering happy path and error conditions
Code Quality	✅ APPROVED	Follows Bitwarden coding standards consistently

Pre-Merge Action Item: Verify localization keys InvalidSsoToken and SsoOrganizationIdMismatch exist in i18n resource files.

---

Files changed:

• bitwarden_license/src/Sso/Controllers/AccountController.cs:211 - Added validation call
• bitwarden_license/src/Sso/Controllers/AccountController.cs:247-264 - New validation method
• bitwarden_license/test/SSO.Test/Controllers/AccountControllerTest.cs:1143-1267 - Added 4 comprehensive test cases

[JaredSnider-Bitwarden]

Nice work! Please make sure this is tested on the feature.