[PM-29208] Remove individual cryptographic-key states & migrate key service #18164
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18164 +/- ##
==========================================
- Coverage 42.28% 42.25% -0.03%
==========================================
Files 3599 3599
Lines 104552 104527 -25
Branches 15782 15774 -8
==========================================
- Hits 44208 44170 -38
- Misses 58469 58485 +16
+ Partials 1875 1872 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ); | ||
| } | ||
|
|
||
| if (tokenResponse.hasMasterKeyEncryptedUserKey()) { |
There was a problem hiding this comment.
Here, the migration is to just set the account cryptographic state conditionally. Private key creation is happening anymore, so there is no need for this special branching logic.
quexten
force-pushed
the
km/remove-individual-key-states
branch
from
f206a76 to
b3921d5
Compare
quexten
changed the title
quexten
changed the title
quexten
force-pushed
the
km/remove-individual-key-states
branch
from
4425d87 to
dc75f19
Compare
|
Claude finished @quexten's task in 4m 10s —— View job Overall Assessment: REQUEST CHANGES This PR removes four individual cryptographic state keys (privateKey, userSigningKey, userSignedPublicKey, accountSecurityState) and consolidates them into a single accountCryptographicState subscription pattern. The migration is correctly implemented, and the refactoring follows the intended architecture. However, there are critical null safety issues in login strategy implementations. Code Review Details
Migration Quality: The migration correctly removes all four legacy state keys and is properly registered. Test coverage validates the removal logic. Service Injection: The dependency ordering changes are correct - Architecture: The refactoring successfully eliminates redundant state subscriptions and prepares the codebase for SDK service direct subscription to accountCryptographicState. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await this.keyService.setPrivateKey( | ||
| response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), | ||
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❌ CRITICAL: Null reference exception when accountKeysResponseModel is null
Details and fix
This code unconditionally calls .toWrappedAccountCryptographicState() on response.accountKeysResponseModel, but this field can be null according to the type definition in IdentityTokenResponse (line 30: accountKeysResponseModel: PrivateKeysResponseModel | null = null).
When this breaks:
- JIT-provisioned SSO users who don't yet have asymmetric key pairs
- Any scenario where the server doesn't return accountKeys (noted in sync service comment about PM-21768)
Execution path:
- User logs in with password
- Server returns token response with
accountKeysResponseModel = null - Line 156 executes:
response.accountKeysResponseModel.toWrappedAccountCryptographicState() - Throws:
Cannot read property 'toWrappedAccountCryptographicState' of null
Fix required:
Add null check like SSO login strategy does:
protected override async setAccountCryptographicState(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
if (response.accountKeysResponseModel) {
await this.accountCryptographicStateService.setAccountCryptographicState(
response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
userId,
);
}
}Why SSO strategy is correct: See sso-login.strategy.ts lines 338-348 and test at line 199 which explicitly tests for accountKeysResponseModel = null.
Why this matters: The PR description states this removes the "old path for account private key regen", but the replacement assumes accountKeysResponseModel is always present. Per the sync service comment (line 243), the server doesn't always return this field yet (PM-21768 tracking).
There was a problem hiding this comment.
This is not correct. Password login strategy never has null account keys, only SSO login does.
| await this.keyService.setPrivateKey( | ||
| response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), | ||
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❌ CRITICAL: Null reference exception when accountKeysResponseModel is null
Details and fix
Same null safety issue as password login strategy. This unconditionally accesses response.accountKeysResponseModel.toWrappedAccountCryptographicState() without checking if accountKeysResponseModel is null.
Fix required:
protected override async setAccountCryptographicState(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
if (response.accountKeysResponseModel) {
await this.accountCryptographicStateService.setAccountCryptographicState(
response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
userId,
);
}
}Reference the SSO login strategy (lines 338-348) for the correct pattern.
There was a problem hiding this comment.
This is not correct. Password login strategy never has null account keys, only SSO login does.
| await this.keyService.setPrivateKey( | ||
| response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), | ||
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❌ CRITICAL: Null reference exception when accountKeysResponseModel is null
Details and fix
Same null safety issue. Add null check:
protected override async setAccountCryptographicState(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
if (response.accountKeysResponseModel) {
await this.accountCryptographicStateService.setAccountCryptographicState(
response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
userId,
);
}
}There was a problem hiding this comment.
This is not correct. Password login strategy never has null account keys, only SSO login does.
libs/state/src/state-migrations/migrations/75-remove-user-encrypted-private-key.spec.ts
Show resolved
Hide resolved
libs/state/src/state-migrations/migrations/75-remove-user-encrypted-private-key.ts
Outdated
Show resolved
Hide resolved
|
Disabling claude since it's just duplicating. |
|
BIT tests are most likely failing due to the outdated server version that BIT is using. I'll check in with autofill about this before merging, but locally everything works. |
|
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29208
📔 Objective
This PR removes the following individual cryptographic states:
and replaces all subscriptions to these states, with subscriptions to the account cryptography state.
These temporary mappings can be removed once the SDK service subscribes directly to the account cryptographic state, and their services can be removed.
Please note, this also drops the old path for account private key regen, if an account does not have a private key. This can instead be done in the Data Recovery tool.
📸 Screenshots
⏰ Reminders before review
🦮 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