Conversation
🦋 Changeset detectedLatest commit: 426369d The changes in this PR will be included in the next version bump. This PR includes changesets to release 52 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for staging-learncardapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for learncarddocs canceled.
|
|
👋 Hey there! It looks like you modified code, but didn't update the documentation in If this PR introduces new features, changes APIs, or modifies behavior that users or developers need to know about, please consider updating the docs. 🏄 Windsurf TipYou can ask Windsurf to help:
Windsurf will review your changes and suggest appropriate documentation updates based on what was modified. 📚 Documentation Guide
This is an automated reminder. If no docs are needed, feel free to ignore this message. |
Custard7
temporarily deployed
to
learn-cloud-network-api-staging
GitHub Actions
Inactive
Custard7
temporarily deployed
to
learn-cloud-storage-api-staging
GitHub Actions
Inactive
![gitstream-cm[bot]](https://avatars.githubusercontent.com/in/230441?s=60&v=4){kind=small}
There was a problem hiding this comment.
✨ PR Review
The PR implements a comprehensive migration from Web3Auth to self-hosted SSS key management with a well-structured AuthCoordinator pattern. The architecture appears sound with proper separation of concerns. However, there is a critical security issue with how cryptographic key shares are stored during the QR login flow that needs immediate attention.
1 issues detected:
🔒 Security - Cryptographic key share stored in sessionStorage is never explicitly cleared, creating an unnecessary window for potential XSS-based exfiltration
Details: The QR login flow stores a cryptographic device share in sessionStorage without cleanup. The 'qr_login_device_share' and 'qr_login_share_version' items persist in sessionStorage until the tab closes, even after they're used by the coordinator. This creates a window where XSS attacks on the same origin could exfiltrate the share. While one share alone isn't sufficient to reconstruct the private key, storing cryptographic key material in browser storage without immediate cleanup violates security best practices and expands the attack surface. The code should clear these sessionStorage items immediately after the coordinator consumes them, or use a more secure temporary storage mechanism.
File:apps/learn-card-app/src/pages/login/LoginPage.tsx
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
Custard7
temporarily deployed
to
learn-card-app-api-staging
GitHub Actions
Inactive
There was a problem hiding this comment.
✨ PR Review
The PR introduces a comprehensive SSS key management system with good architectural separation. However, there are several critical bugs in error handling and async operation management that need to be addressed before merge.
3 issues detected:
🐞 Bug - Async logout operation is not awaited, causing subsequent cleanup to run prematurely 🛠️
Details: The coordinatorLogout() call is not awaited, causing queryClient.resetQueries() and clearDB() to execute before logout completes. This creates a race condition where cleanup operations may run against inconsistent state, potentially leaving the app in a partially logged-out state.
File:
apps/learn-card-app/src/pages/consentFlow/ExternalConsentFlowDoor.tsx (200-200)
🛠️ A suggested code correction is included in the review comments.🐞 Bug - Session refresh failure is not detected, causing success state to display even when refresh fails 🛠️
Details: After re-authentication, refreshAuthSession() is called but its return value is not checked. If the refresh fails (returns false), the UI still displays "Session Restored" and calls onSuccess, misleading the user that authentication succeeded when it actually failed. This occurs in both Google (line 140) and Apple (line 212) re-auth handlers.
File:
apps/learn-card-app/src/components/auth/ReAuthOverlay.tsx (140-140)
🛠️ A suggested code correction is included in the review comments.🐞 Bug - Loading state is only cleared on error, not on success, causing permanent loading UI
Details: The recovery handlers for device, phrase, backup, and email methods set loading to true at the start but only set it to false in the catch block. When recovery succeeds without throwing, the loading state remains true permanently, leaving the UI in a perpetual loading state that prevents further user interaction. This affects onApproved callback at line 351, handlePhraseRecovery at line 108, handleBackupRecovery at line 128, and handleEmailRecovery at line 153.
File:apps/learn-card-app/src/components/recovery/RecoveryFlowModal.tsx
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
There was a problem hiding this comment.
✨ PR Review
The PR successfully introduces a self-hosted SSS key management system with comprehensive recovery flows and coordinator-based architecture. The code is well-structured with proper error handling in most areas. However, there are a couple of race conditions and resource management issues that need attention.
2 issues detected:
🐞 Bug - Async logout operation runs concurrently with cleanup instead of sequentially 🛠️
Details: The coordinatorLogout() call is not awaited before queryClient.resetQueries() and clearDB() execute. This creates the same race condition as issue_3 (now resolved in useLogout.tsx), where cleanup operations may run against inconsistent state before logout completes, potentially leaving the app in a partially logged-out state.
File:
apps/learn-card-app/src/pages/consentFlow/ExternalConsentFlowDoor.tsx (200-200)
🛠️ A suggested code correction is included in the review comments.🐞 Bug - BarcodeScanner listener not cleaned up on error or component unmount
Details: The BarcodeScanner listener may not be cleaned up if startScan() throws an error or if the promise is abandoned (e.g., component unmounts before scan completes). The async Promise executor pattern is also an anti-pattern. If BarcodeScanner.startScan() throws after the listener is added but before the scan begins, the listener remains attached indefinitely.
File:apps/learn-card-app/src/components/device-link/DeviceLinkModal.tsx
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
There was a problem hiding this comment.
✨ PR Review
The SSS key management migration introduces significant architectural improvements. However, there are a few race conditions and resource management issues that need attention, particularly around logout sequencing and barcode scanner cleanup.
3 issues detected:
🐞 Bug - Async coordinatorLogout() is not awaited, causing subsequent cleanup operations to race with the logout process. 🛠️
Details: The coordinatorLogout() call is not awaited before queryClient.resetQueries() and clearDB() execute. This creates a race condition where cleanup operations may run against inconsistent state before logout completes, potentially leaving the app in a partially logged-out state or corrupting user data.
File:
apps/learn-card-app/src/pages/consentFlow/ExternalConsentFlowDoor.tsx (200-200)
🛠️ A suggested code correction is included in the review comments.🐞 Bug - Async Promise executor is an anti-pattern and the listener cleanup is not guaranteed if the component unmounts or if certain errors occur, potentially leaving orphaned event listeners.
Details: The BarcodeScanner listener uses an async Promise executor (anti-pattern) and may not be cleaned up if the component unmounts before scan completes or if startScan() throws after the listener is added. The error handler attempts cleanup but doesn't guarantee listener removal in all failure paths.
File:apps/learn-card-app/src/components/device-link/DeviceLinkModal.tsx🧹 Maintainability - Button trigger references a popover that doesn't exist on mobile, creating an inconsistent component state.
Details: The IonPopover is conditionally rendered based on !isMobile, but the trigger button with id="trigger-button" is always rendered. On mobile devices, the button will reference a non-existent popover via the trigger attribute, which may cause runtime warnings or silent failures. This pattern appears in multiple files (LaunchPadPopOverButton, LearnerInsightsPopOverButton, SkillsHubPopOverButton).
File:apps/learn-card-app/src/components/ai-sessions/AiSessionsSearch/AiSessionsPopOverButton.tsx
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
There was a problem hiding this comment.
✨ PR Review
This is a substantial architectural change replacing Web3Auth with a self-hosted SSS system. The AuthCoordinator state machine and recovery flows are well-structured. However, one critical race condition was identified in the logout flow that should be addressed before deployment.
1 issues detected:
🐞 Bug - coordinatorLogout() is called without await, causing queryClient.resetQueries() and clearDB() to execute before logout completes 🛠️
Details: The coordinatorLogout() call on line 200 is not awaited before queryClient.resetQueries() and clearDB() execute. This creates a race condition where cleanup operations may run before logout completes, potentially leaving the application in an inconsistent state or corrupting user data. The same pattern was correctly fixed in useLogout.tsx (line 85) by adding await, but this instance was missed.
File:
apps/learn-card-app/src/pages/consentFlow/ExternalConsentFlowDoor.tsx (200-200)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
TaylorBeeston
left a comment
TaylorBeeston
left a comment
There was a problem hiding this comment.
Phenomenal job!!!
Huge, sweeping changes to auth that were sorely needed, and I really like all the new features! =) did a lot of testing and the only times things were broken always ended up being issues with my own environment!
Tested:
- Migrating an existing account
- Making a new account
- Linking a new device
- Setting up email recovery
- Bip39 Seed Phrase Recovery
- Encrypted Backup file
- Passkey
All of them worked with no issue! Everything also looks really slick too =)
2 participants
Replace Web3Auth with Self-Hosted Shamir Secret Sharing (SSS) Key Management
https://www.loom.com/share/c2807fdcb7c64aa1bbe4e261e2fc23a7
Summary
This PR replaces the third-party Web3Auth key management system with a fully self-hosted Shamir Secret Sharing (SSS) architecture, introduces a unified AuthCoordinator state machine, adds modular auth provider support, and builds comprehensive recovery and cross-device login flows — all backed by new server routes in
lca-api.The core goal: users' private keys are split into shares using a 2-of-3 threshold scheme. Any two shares can reconstruct the key, and no single party (device, server, or recovery method) ever holds enough information to derive it alone.
Architecture Overview
New Packages
packages/sss-key-manager(new)Self-contained SSS key management library. Zero UI dependencies.
sss.tssss-strategy.tsKeyDerivationStrategyimplementation — orchestrates setup, recovery, share rotation, server communicationstorage.tscrypto.tspasskey.tsrecovery-phrase.tsqr-crypto.tsqr-login.tsatomic-operations.tsapi-client.ts/keys/*and/qr-login/*routestypes.ts@learncard/auth-types13 test files, ~249 unit tests covering every module.
packages/auth-types(new)Zero-dependency package containing provider-agnostic interfaces (
AuthProvider,AuthUser,KeyDerivationStrategy,RecoveryMethodInfo, etc.). Shared by bothsss-key-managerandlearn-card-baseto avoid circular dependencies.Key Changes by Layer
packages/learn-card-base— AuthCoordinator & ProvidersAuthCoordinator (
src/auth-coordinator/)idle → authenticating → authenticated → checking_key_status → needs_setup | needs_migration | needs_recovery → deriving_key → readyKeyDerivationStrategylegacyAccountThresholdMsAuthCoordinatorProvider,useAuthCoordinatorhook,useAuthCoordinatorAutoSetupEmailLinkOverlay,ErrorOverlay,StalledMigrationOverlayAuth Providers (
src/auth-providers/)createFirebaseAuthProvider— wraps Firebase Auth SDK into theAuthProviderinterfaceConfig (
src/config/)authConfig.ts— runtime configuration for auth provider + key derivation strategyproviderRegistry.ts— registry pattern for managing multiple auth provider implementationsservices/learn-card-network/lca-api— Server RoutesKey Management Routes (
/keys/*)/keys/auth-shareshareVersionfor version negotiation)/keys/auth-share/keys/recovery/keys/recovery/keys/migrate/keys/recovery-email/add/keys/recovery-email/verify/keys/recovery-email/keys/email-backup/keys/upgrade-contact-method/keys/deleteQR Login Relay (
/qr-login/*)/qr-login/session/qr-login/session/{lookup}/qr-login/session/{sessionId}/approve/qr-login/notifySupporting infrastructure:
UserKeyMongoDB model with contact-method-keyed documents, share versioning,previousAuthShareshistory, orphaned recovery method pruningauth.helpers.ts— provider-agnostic token verification (Firebase, Supertokens, Keycloak, OIDC) with E2E test bypassshareEncryption.helpers.ts— server-side AES-256-GCM encryption using HKDF-derived keys from server SEEDdelivery/— pluggable email delivery service (Postmark adapter for production, log adapter for development/E2E)maskEmail.ts— email masking for privacy-safe displaypruneOrphanedRecoveryMethods.ts— automatic cleanup of recovery methods whose share version is no longer in historylca-apiapps/learn-card-app— UI IntegrationAuthCoordinatorProvider.tsx— app-level coordinator wiring with Firebase auth provider + SSS strategyRecoveryFlowModal.tsx— guided recovery flow (passkey, phrase, backup file, email backup)RecoverySetupModal.tsx— post-login recovery method setup promptsRecoveryBanner.tsx— persistent banner prompting users to set up recoveryReAuthOverlay.tsx— re-authentication modal when session expiresAuthHandoff.tsx— auth handoff page for cross-device flowsEmailLinkOverlay.tsx— phone→email upgrade flow with OTP verificationAuthKeyDebugWidget.tsx— developer debug overlay for inspecting auth/key stateuseFirebase.ts/useLogout.tsx/useBoostRecoveryCheck.tsx— updated hookspackages/plugins/lca-api-pluginUpdated to expose SSS key management methods (
storeAuthShare,getAuthShare,addRecoveryMethod,getRecoveryShare,markMigrated,deleteUserKey) via the LearnCard plugin interface for E2E testing and programmatic access.Share Architecture
Security levels:
Migration Path (Web3Auth → SSS)
Existing Web3Auth users are auto-detected via
legacyAccountThresholdMs. When a user with an existing account but no SSS record logs in:needs_migrationstateThe
StalledMigrationOverlayhandles edge cases where migration is interrupted.✨ PR Description
Purpose: Add SSS-