[PM-26177] Allow mobile clients to create passkeys

[iinuwa]

🎟️ Tracking

PM-26177
PM-27177
PM-27265
PM-27264

📔 Objective

As part of the initiative to have unphishable onboarding from mobile clients to the web vault or browser extension, we need to allow mobile clients to be able to create new passkeys.

This PR allows mobile clients to create and update passkeys. At this time, we are not expecting mobile clients to authenticate directly with passkeys, so the assertion endpoint was left restricted to the web vault.

⏰ 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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Checkmarx One – Scan Summary & Details – aeb81031-0df2-4828-ad33-c1d98a6c0c3b

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

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.63%. Comparing base (a565fd9) to head (1589f3e).

Files with missing lines	Patch %	Lines
src/Api/Vault/Models/CipherFido2CredentialModel.cs	33.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6383       +/-   ##
===========================================
+ Coverage   13.02%   50.63%   +37.61%     
===========================================
  Files        1112     1864      +752     
  Lines       49293    82552    +33259     
  Branches     3820     7290     +3470     
===========================================
+ Hits         6420    41800    +35380     
+ Misses      42754    39156     -3598     
- Partials      119     1596     +1477     

☔ 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 @iinuwa's task —— View job

---

Code Review Complete

Summary of Changes

This PR modifies authorization policies on the WebAuthnController to allow mobile clients (via the Policies.Application policy) to create and update passkeys, while keeping assertion (authentication) restricted to the web vault. Specifically:

• WebAuthnController.cs: Moved [Authorize] from class-level to method-level, allowing different policies per endpoint
• CipherFido2CredentialModel.cs & CipherLoginFido2CredentialData.cs: Added HmacSecret field to support PRF (Pseudo-Random Function) storage

---

❌ Critical Issues

1. CSRF Vulnerability in Newly Opened Endpoints (src/Api/Auth/Controllers/WebAuthnController.cs)

The endpoints AttestationOptions (L74-90), Post (L109-127), and UpdateCredential (L155-177) were changed from Policies.Web to Policies.Application, making them accessible to mobile clients. However:

Issue: These state-changing POST/PUT endpoints now accept requests from non-web clients without traditional CSRF protection. While the API likely uses Bearer token authentication (which has some CSRF resistance), the Checkmarx scanner flagged a CSRF concern at line 259.

Impact:

• An attacker controlling a mobile client or compromising a mobile token could potentially create/update passkeys without additional verification
• The SecretVerificationRequestModel at AttestationOptions requires the user's master password/secret, which provides some protection, but Post and UpdateCredential rely only on token validation

Recommendation:

// Consider adding additional verification for state-changing operations:
// 1. For AttestationOptions - ✅ Already has SecretVerificationRequestModel
// 2. For Post (line 109) - Consider requiring re-authentication or rate limiting
// 3. For UpdateCredential (line 155) - ✅ Uses assertion challenge which acts as CSRF protection

The UpdateCredential endpoint appears safe due to the challenge-response nature of WebAuthn assertions. However, Post should be reviewed for additional protections.

Reference: src/Api/Auth/Controllers/WebAuthnController.cs:109-127

---

2. Missing Unit Tests for Authorization Changes

Issue: The PR changes authorization policies from Web to Application for critical endpoints, but no new tests verify this authorization change works correctly for mobile clients.

Current Test Gap:

• Existing tests in WebAuthnControllerTests.cs don't verify that mobile clients (with Policies.Application) can now access these endpoints
• No tests confirm web-only endpoints still reject mobile clients

Recommendation: Add authorization tests:

[Theory, BitAutoData]
public async Task AttestationOptions_WithApplicationPolicy_Succeeds(...)
{
    // Verify mobile clients can access AttestationOptions
}

[Theory, BitAutoData] 
public async Task AssertionOptions_WithApplicationPolicy_Fails(...)
{
    // Verify mobile clients CANNOT access AssertionOptions (should stay Web-only)
}

---

⚠️ Security Concerns

3. Sensitive Data Encryption Verification

The new HmacSecret field stores PRF secrets which are cryptographically sensitive.

Good: The field is marked with [EncryptedString] and [EncryptedStringLength(1000)] attributes (CipherFido2CredentialModel.cs:68-70)

Concern: Verify that:

• The encryption happens before storage and persists through the entire data flow
• The secret is never logged or exposed in error messages
• The encryption key management follows zero-knowledge principles

Recommendation: Manual code inspection of the encryption pipeline to ensure HmacSecret remains encrypted at rest, in transit, and in use.

---

4. Null Safety Issues in Data Models

Both modified model files have #nullable disable at the top:

• src/Api/Vault/Models/CipherFido2CredentialModel.cs:2
• src/Core/Vault/Models/Data/CipherLoginFido2CredentialData.cs:2

Issue: This suppresses nullable reference type checking, which can lead to null reference exceptions at runtime.

Recommendation: Address the FIXME comments and enable nullable reference types as per ADR-0024. For the new HmacSecret property, explicitly mark it nullable or required:

public string? HmacSecret { get; set; } // If optional
// OR
[Required]
public string HmacSecret { get; set; } = string.Empty; // If required

---

🎨 Suggestions for Improvement

5. Inconsistent Policy Documentation

The change moves authorization policies but doesn't document why certain endpoints require Web vs Application policies.

Recommendation: Add XML comments:

/// <summary>
/// Gets attestation options for creating a new WebAuthn credential.
/// Available to all authenticated clients (web, mobile, etc).
/// </summary>
[Authorize(Policies.Application)]
[HttpPost("attestation-options")]
public async Task<WebAuthnCredentialCreateOptionsResponseModel> AttestationOptions(...)

/// <summary>
/// Gets assertion options for WebAuthn authentication.
/// Restricted to web vault only - mobile clients cannot authenticate via passkeys yet.
/// </summary>
[Authorize(Policies.Web)]
[HttpPost("assertion-options")]
public async Task<WebAuthnLoginAssertionOptionsResponseModel> AssertionOptions(...)

---

6. Policy Requirement Validation

The ValidateIfUserCanUsePasskeyLogin method is called at AttestationOptions and Post, but not at UpdateCredential.

Question: Should UpdateCredential also validate that the user can still use passkey login, or is the assertion validation sufficient?

Recommendation: Clarify in code comments or consider adding the same validation for consistency.

---

7. Missing Database Migration Impact Assessment

Adding HmacSecret to CipherLoginFido2CredentialData may require database schema changes.

Question:

• Does this field already exist in the database?
• If this is a new column, where is the migration script?
• How are existing passkeys without HmacSecret handled?

Recommendation: Verify database migration strategy and backward compatibility.

---

✅ Good Practices Observed

• Token-based validation for credential creation and updates
• Delayed response on verification failures to prevent timing attacks (VerifyUserAsync:208)
• Proper exception handling with appropriate status codes
• Encrypted string attributes on sensitive data fields

---

📋 Action Items

1. High Priority:

   • Address CSRF concerns for the Post endpoint or document why Bearer auth is sufficient
   • Add unit tests for authorization policy changes
   • Verify HmacSecret encryption pipeline and zero-knowledge compliance
   • Confirm database migration strategy for HmacSecret field

2. Medium Priority:

   • Enable nullable reference types in modified files (per ADR-0024)
   • Add XML documentation for policy authorization rationale
   • Clarify whether UpdateCredential needs policy validation

3. Low Priority:

   • Consider rate limiting for credential creation endpoints

---

📊 Review Statistics

• Files Changed: 3
• Lines Added: 12
• Lines Removed: 1
• New Tests: 0
• Critical Issues: 2
• Security Concerns: 2
• Suggestions: 3

---

Overall Assessment

The core functionality appears sound - moving authorization from class-level to method-level allows fine-grained control for mobile passkey creation. However, the security-sensitive nature of this feature requires:

1. Additional test coverage for authorization changes
2. CSRF risk assessment and documentation
3. Database migration verification
4. Null safety improvements

Recommendation: Address critical issues before merging, particularly the missing tests and CSRF documentation.

---