feat(auth): use medium keys for Yappr document operations - #393
feat(auth): use medium keys for Yappr document operations#393PastaPastaPasta wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying yappr-v2 with
|
| Latest commit: |
ca4822b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://321cf32a.yappr-v2.pages.dev |
| Branch Preview URL: | https://feat-medium-auth-keys.yappr-v2.pages.dev |
Deploying yappr with
|
| Latest commit: |
ca4822b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://41820502.yappr.pages.dev |
| Branch Preview URL: | https://feat-medium-auth-keys.yappr.pages.dev |
|
⛔ Final review complete — 2 blocking finding(s) (commit ca4822b) · triage: critical · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified both supplied findings against head ca4822b. The MEDIUM registration default breaks in-app username registration for affected wallet logins, and the DM cache prevents newly published sender keys from overriding identity fallbacks. Both issues affect workflows introduced or explicitly supported by this PR.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This changes authentication key acceptance, cryptographic signer selection, fee-bearing document writes, wallet registration, and authorization requirements across 95 contract document types, where errors could compromise security or block transactions. - Phase 1 reviewers: not run (skipped for throughput: 24 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer
🔴 2 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `lib/services/identity-update-builder.ts`:
- [BLOCKING] lib/services/identity-update-builder.ts:207-211: Provide DPNS step-up authorization for MEDIUM wallet logins
New wallet key registrations now create a MEDIUM authentication key, but the username-registration flow cannot use it. `PlatformAuthController.loginWithLoginKey()` derives and stores that authentication key and calls login with `skipUsernameCheck: false`. For users without a username, `DpnsRegistrationWizard.handleRegister()` reads the stored key and accepts only CRITICAL or HIGH authentication keys (`components/dpns/registration-wizard.tsx:97–137`), so registration rejects the newly registered key on every attempt. The wizard provides no temporary higher-level key input or wallet-signing path. Retaining DPNS's authorization requirement is correct, but changing the default from HIGH needs a step-up signing flow or an actionable alternative so these wallet users can still register a username.
In `lib/services/direct-message-service.ts`:
- [BLOCKING] lib/services/direct-message-service.ts:85-94: Let new MEDIUM invites override cached identity fallback keys
Publishing the actual sender key does not override an identity fallback already cached by the recipient. If Alice sends to Bob before Bob has published an invite, `findDMPublicKey()` prefers Bob's HIGH key and `getPublicKeyForUser()` caches it. When Bob replies using a newly supported MEDIUM login, this code publishes his MEDIUM public key, but Alice's subsequent decryption still returns the cached HIGH key at lines 527–528 without checking the invite. The reply therefore cannot be decrypted, and subsequent outgoing messages continue using the wrong key. The cache has no refresh or invalidation path, and fetching conversation invites does not update it. Distinguish provisional identity fallbacks from invite-derived keys and refresh the fallback when the peer's invite becomes available, so the advertised sender key can actually take precedence.
| const authKey = new wasm.IdentityPublicKeyInCreation({ | ||
| keyId: authKeyId, | ||
| purpose: 'authentication', | ||
| securityLevel: 'high', | ||
| securityLevel: 'medium', | ||
| keyType: authKeyType, |
There was a problem hiding this comment.
🔴 Blocking: Provide DPNS step-up authorization for MEDIUM wallet logins
New wallet key registrations now create a MEDIUM authentication key, but the username-registration flow cannot use it. PlatformAuthController.loginWithLoginKey() derives and stores that authentication key and calls login with skipUsernameCheck: false. For users without a username, DpnsRegistrationWizard.handleRegister() reads the stored key and accepts only CRITICAL or HIGH authentication keys (components/dpns/registration-wizard.tsx:97–137), so registration rejects the newly registered key on every attempt. The wizard provides no temporary higher-level key input or wallet-signing path. Retaining DPNS's authorization requirement is correct, but changing the default from HIGH needs a step-up signing flow or an actionable alternative so these wallet users can still register a username.
source: ['claude']
| // Publish the key actually used for ECDH, including for MEDIUM keys. | ||
| // Another HIGH key on the identity may belong to a different login. | ||
| const inviteResult = await stateTransitionService.createDocument( | ||
| this.contractId, | ||
| 'conversationInvite', | ||
| senderId, | ||
| { | ||
| recipientId: identifierStringToDocumentBytes(recipientId), | ||
| conversationId: conversationIdDocumentBytes, | ||
| ...(senderPubKeyDocumentBytes ? { senderPubKey: senderPubKeyDocumentBytes } : {}) | ||
| conversationId: conversationIdBytes, | ||
| senderPubKey |
There was a problem hiding this comment.
🔴 Blocking: Let new MEDIUM invites override cached identity fallback keys
Publishing the actual sender key does not override an identity fallback already cached by the recipient. If Alice sends to Bob before Bob has published an invite, findDMPublicKey() prefers Bob's HIGH key and getPublicKeyForUser() caches it. When Bob replies using a newly supported MEDIUM login, this code publishes his MEDIUM public key, but Alice's subsequent decryption still returns the cached HIGH key at lines 527–528 without checking the invite. The reply therefore cannot be decrypted, and subsequent outgoing messages continue using the wrong key. The cache has no refresh or invalidation path, and fetching conversation invites does not update it. Distinguish provisional identity fallbacks from invite-derived keys and refresh the fallback when the peer's invite becomes available, so the advertised sender key can actually take precedence.
source: ['claude']
2 participants
Yappr rejected MEDIUM authentication keys and requested HIGH keys during wallet registration. Allow MEDIUM, HIGH, or CRITICAL authentication for login and all application document writes, including writes with YAPP fees; new wallet registration transactions request MEDIUM authentication keys.
Set
signatureSecurityLevelRequirement: 3on all 95 document types in the contract templates and on schemas cloned by the fresh-registration scripts. This accompanies the planned fresh contract deployment; deployed IDs are unchanged. DPNS registration, explicit token operations, credit transfers, and identity updates retain their Platform-required authorization.DM public-key lookup now supports MEDIUM authentication keys, and new conversation invites carry the actual sender key so another HIGH key on the identity cannot mask it. Add a MEDIUM key to fresh test identities and an
E2E_AUTH_KEY_LEVEL=MEDIUMoption without changing existing key derivation paths.Validation: production build (including lint/type checks), 143 unit tests, knip, v5/v6 contract generator self-tests, and a semantic comparison confirming contract changes only add the security requirement. New offline tests use real WASM signing with mocked network calls to check login, fee-bearing creates, replace/delete/index-only delete signer selection, disabled-key rejection, wallet registration payloads, and CRITICAL escalation for purchases. Live writes against the planned fresh deployment have not been run.
This pull request was created by Codex.