Skip to content

Add passkey and password history to a subset of seeded credentials#7635

Open
harr1424 wants to merge 1 commit into
mainfrom
add-fido2Credentials-and-passwordHistory-to-seeder
Open

Add passkey and password history to a subset of seeded credentials#7635
harr1424 wants to merge 1 commit into
mainfrom
add-fido2Credentials-and-passwordHistory-to-seeder

Conversation

@harr1424
Copy link
Copy Markdown
Contributor

@harr1424 harr1424 commented May 14, 2026

🎟️ Tracking

Inspired by work performed as part of PM-18786.

📔 Objective

This PR adds fido2Credentials and passwordHistory objects to a subset of seeded login ciphers in order to better resemble a real-world vault. A single passkey is added to ~20% of logins and password history (1-3 iterations) is added to ~40% of logins.

@sonarqubecloud
Copy link
Copy Markdown

@harr1424 harr1424 requested review from MGibson1 and theMickster May 14, 2026 14:53
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.83%. Comparing base (91b833c) to head (f9a7c28).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7635      +/-   ##
==========================================
+ Coverage   59.82%   59.83%   +0.01%     
==========================================
  Files        2119     2119              
  Lines       93354    93354              
  Branches     8282     8282              
==========================================
+ Hits        55849    55860      +11     
+ Misses      35522    35506      -16     
- Partials     1983     1988       +5     

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

Copy link
Copy Markdown
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

Nice add! Appreciate the additional test and usage coverage as well

Copy link
Copy Markdown
Contributor

@theMickster theMickster left a comment

Choose a reason for hiding this comment

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

@harr1424 One minor thing to double-check and then I am happy to stamp for approval.

In a follow-up PR, will you take a peek with Claude Code into adding similar functionality into the preset fixtures here util/Seeder/Seeds/fixtures? I think we could make some minor adjustments to some of those deterministic ciphers using the same percentage outlined in this PR to kick it up a notch. I recommend pointing Claude to read util/Seeder/Seeds/README.md and the docs in util/Seeder/Seeds/docs + then (while in plan mode) create a plan to add those for you. 😎

var username = generator.Username.GenerateByIndex(index, totalHint: generator.CipherCount, domain: company.Domain);

// ~20% of logins get a FIDO2 passkey; ~40% get a 1-3 entry password history. Deterministic by index.
var fido2Credentials = index % 5 == 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ A local multi-agent Claude Code review brought up the following finding. I have not dug into the exact specification (might be overkill to do that), but I think Claude may be correct in it's code suggestion. Given that this is a Seeder and not real credentials I'm not sure this is a huge deal, but worth a quick double-check IMO.

FIDO2 credential created with display name as rpId instead of domain

util/Seeder/Factories/CipherComposer.cs:55
Caught by: Bug analysis agent

Details

LoginCipherSeeder.CreateFido2Credential(company.Name, username) passes the company display name (e.g., "Sterling Cooper") as the first argument. CreateFido2Credential sets both RpId = rpName and RpName = rpName, so every seeded FIDO2 credential will have an rpId like "Sterling Cooper" instead of an eTLD+1 domain like "sterling-cooper.com".

The WebAuthn/FIDO2 spec requires rpId to be a valid, registrable domain suffix. Any client-side or test code that performs a WebAuthn assertion against these seeded credentials will fail the rpId check — making every seeded passkey structurally non-functional for passkey testing.

Note the inconsistency with the new integration tests in this same PR, which correctly pass "example.com" (a domain) as the first argument to CreateFido2Credential — those tests pass precisely because they use a domain.

company.Domain is already in scope at the call site:

// Before (line 55):
? new List<Fido2CredentialViewDto> { LoginCipherSeeder.CreateFido2Credential(company.Name, username) }

// After:
? new List<Fido2CredentialViewDto> { LoginCipherSeeder.CreateFido2Credential(company.Domain, username) }

@theMickster theMickster added the ai-review Request a Claude code review label May 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

This PR adds Fido2Credentials and PasswordHistory to a deterministic subset of seeded login ciphers (~20% and ~40% respectively), wires the new PasswordHistory field through the seeder DTOs, the [EncryptProperty] path graph, and the server-side CipherLoginData mapping. Tests cover roundtrip encryption, field-path discovery, and integration with LoginCipherSeeder.Create. One substantive defect was already raised in an existing reviewer thread and remains unresolved; the rest of the change looks consistent with the seeder library's conventions.

Code Review Details
  • ⚠️ : Existing unresolved thread — LoginCipherSeeder.CreateFido2Credential(company.Name, username) passes the company display name (e.g. Salesforce) as both RpId and RpName. Per the WebAuthn spec, RpId must be a registrable domain; company.Domain is already in scope and should be used. This is already flagged by theMickster and not duplicated here.
    • util/Seeder/Factories/CipherComposer.cs:55-57

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants