Skip to content

[PM-32401] fix: Prevent TOTP autofill for non-premium accounts#2467

Merged
matt-livefront merged 3 commits intomainfrom
matt/PM-32401-autofill-totp-premium
Mar 24, 2026
Merged

[PM-32401] fix: Prevent TOTP autofill for non-premium accounts#2467
matt-livefront merged 3 commits intomainfrom
matt/PM-32401-autofill-totp-premium

Conversation

@matt-livefront
Copy link
Copy Markdown
Collaborator

@matt-livefront matt-livefront commented Mar 19, 2026

🎟️ Tracking

PM-32401

📔 Objective

This fixes an issue where non-premium accounts were able to use TOTP autofill.

  • Adds an authorization check prior to autofilling OTP codes.
  • Only creates ASOneTimeCodeCredentialIdentity for the credential identity store if authorized.

@matt-livefront matt-livefront requested a review from a team as a code owner March 19, 2026 16:27
@matt-livefront matt-livefront added ai-review Request a Claude code review t:bug Change Type - Bug labels Mar 19, 2026
@github-actions github-actions bot added the app:password-manager Bitwarden Password Manager app context label Mar 19, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 19, 2026

Claude finished @matt-livefront's task in 1m 35s —— View job


Code Review: PM-32401 Prevent TOTP autofill for non-premium accounts

  • Gather context and understand changes
  • Review authorization logic across all layers
  • Review test coverage
  • Post review findings

Summary

Overall Assessment: ✅ APPROVE

This PR adds defense-in-depth authorization checks to prevent TOTP autofill for non-premium accounts that lack organizational TOTP access. Changes span three layers: a new centralized isTotpAuthorized(for:) method in TOTPService, filtering of OTP credential identities in CredentialIdentityFactory, and a guard in the credential provision path in AutofillCredentialService. Test coverage addresses all authorization permutations (premium, org-affiliated, unauthorized) across all modified components.

No findings. The authorization logic is consistent across both the identity registration and credential provision layers, premium status is fetched once per batch operation to avoid redundant calls, and the test updates correctly adopt the guard #available pattern for XCTest compatibility.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 19, 2026

Logo
Checkmarx One – Scan Summary & Details7948b683-d70c-457f-a1d6-4b611f44aa68

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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 92.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.86%. Comparing base (aa0a9b0) to head (ee9af5d).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...fill/Services/AutofillCredentialServiceTests.swift 82.19% 13 Missing ⚠️
...fill/Services/CredentialIdentityFactoryTests.swift 95.77% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
- Coverage   86.88%   86.86%   -0.02%     
==========================================
  Files        1841     1846       +5     
  Lines      162382   163149     +767     
==========================================
+ Hits       141078   141725     +647     
- Misses      21304    21424     +120     

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

Copy link
Copy Markdown
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

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

Looks good, just one ⛏️

Comment on lines +924 to +925
@available(iOS 18.0, *)
func test_provideOTPCredential_totpAuthorized() async throws {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎨 On XCTest the @available is not respected for the test runs (it does on new Swift Testing Framework), so if one runs this on iOS 17 simulator it will try to run the test and it will fail. I'd add a guard at the beginning as with other similar tests like in ASPasskeyCredentialRequestExtensionsTests.

Suggested change
@available(iOS 18.0, *)
func test_provideOTPCredential_totpAuthorized() async throws {
func test_provideOTPCredential_totpAuthorized() async throws {
guard #available(iOS 18.0, *) else {
throw XCTSkip("Skipped on iOS < 18.0")
}

Same applies to other tests here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, updated!

Comment on lines +80 to +83
func isTotpAuthorized(for cipher: CipherView) async -> Bool {
let accountHasPremium = await stateService.doesActiveAccountHavePremium()
return cipher.organizationUseTotp || accountHasPremium
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

♻️ Perhaps we could create a tech debt task to update other places where this logic is used like DefaultVaultRepository or DefaultVaultListPreparedDataBuilder. What do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep, great idea, I added a ticket: https://bitwarden.atlassian.net/browse/PM-34016.

@matt-livefront matt-livefront merged commit e8ba73f into main Mar 24, 2026
30 checks passed
@matt-livefront matt-livefront deleted the matt/PM-32401-autofill-totp-premium branch March 24, 2026 14:14
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 app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants