FEATURE: Docker log-based password recovery with KEK-DEK preservation #303
Reference in New Issue
Block a user
Delete Branch "main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Key Features
Technical Implementation
recovery_dek,backup_encrypted_dek,zero_trust_modefields/recovery/*endpoints for complete recovery flowZeroTrustMigrationclass for future security upgradesSecurity Model
Breaking Changes
Bug Fixes
Test Plan
🤖 Generated with Claude Code
Summary of Changes
Hello @ZacharyZcR, I'm Gemini Code Assist1 ! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the application's password recovery capabilities and security posture. It introduces a user-friendly recovery mechanism that leverages Docker logs for physical access verification, while critically ensuring that user-encrypted data remains intact during password resets. Furthermore, it lays the groundwork for a more robust security model by offering a migration path from a 'compromise mode' (with recovery) to a 'zero-trust mode' (requiring a user-managed recovery seed), giving users control over their security trade-offs.
Highlights
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either
/gemini <command>or@gemini-code-assist <command>. Below is a summary of the supported commands on the current page./gemini review/gemini summary/gemini helpCustomization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a
.gemini/folder in the base of the repository. Detailed instructions can be found here.Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩︎
Code Review
This pull request introduces a significant new feature: a Docker log-based password recovery system. This is a well-intentioned effort to improve user experience and fix a critical data loss bug during password reset. The implementation correctly preserves user data during a reset and provides a clear migration path to a more secure zero-trust model. However, the current "compromise mode" introduces several critical security vulnerabilities by storing decryption keys in plaintext within the database, both permanently in the
userstable and temporarily in thesettingstable. My review focuses on these critical security issues, along with other high-severity concerns related to cryptographic best practices (improper salt usage), unsafe key construction, and code maintainability (bypassing type safety). These must be addressed before this feature can be safely merged.@@ -1262,0 +1297,4 @@// Clean sensitive dataoriginalDEK.fill(0);newKEK.fill(0);newRecoveryDEK.fill(0);The code frequently uses
(userCrypto as any)to access methods likedecryptDEK,generateKEKSalt,deriveKEK,encryptDEK,storeKEKSalt, andstoreEncryptedDEK. This bypasses TypeScript's type safety, breaks the encapsulation of theUserCryptoclass, and makes the code brittle and hard to refactor. These methods are likely private for a reason. A proper public API should be exposed on theUserCryptoclass to handle the password reset and re-encryption flow in a controlled and type-safe manner.@@ -1935,0 +2139,4 @@const originalDEK = (userCrypto as any).decryptDEK(backupEncryptedDEK, recoveryDEK);// Create user session with decrypted DEKconst tempToken = crypto.randomBytes(32).toString('hex');Math.random()is not a cryptographically secure random number generator. For security-sensitive operations like generating a recovery code, it is best practice to use a secure source of randomness. Node.js'scryptomodule provides a safer alternative.@@ -1935,0 +2143,4 @@const expiresAt = Date.now() + 5 * 60 * 1000; // 5 minutes// Store temporary sessionconst tempKey = `temp_recovery_session_${username}`;Constructing the database key for the recovery code using a raw username (
recovery_code_${username}) is unsafe. If a username contains special characters, it could lead to unexpected behavior or potential injection vulnerabilities, depending on the underlying database driver. It is much safer to use a stable, sanitized identifier, such as the user's ID, to construct this key. This same issue applies to the temporary session key on line 2146.@@ -1935,0 +2256,4 @@username,userId: sessionData.userId,});Storing the original Data Encryption Key (
dekHex) in plaintext in thesettingstable, even as part of a temporary session, is a critical security vulnerability. An attacker with database access during this 5-minute window can capture the DEK and decrypt all of that user's data. The DEK should never be stored in plaintext anywhere on the server. Instead, consider creating a temporary, server-side encrypted session that doesn't expose the raw DEK, or pass the DEK to the client to hold in memory for the next step of the flow.@@ -0,0 +86,4 @@100000,32,'sha256');The
crypto.pbkdf2Syncfunction is using a hardcoded string'zero_trust_recovery'as the salt. When the salt is the same for all users, it is not a salt but a "pepper". This is less secure than using a unique, randomly generated salt for each user, as it makes multi-target attacks (like rainbow tables) more feasible. A unique salt should be generated for each user and stored in the database alongside thebackup_encrypted_dek.@@ -251,0 +340,4 @@}function resetRecoveryState() {setRecoveryStep("request");This
console.logstatement appears to be for debugging purposes. It should be removed from the production codebase to avoid leaking information about token handling into the browser console.The legacy password reset flow is conditionally rendered based on
false && .... This is an unconventional way to disable a feature in the UI and leaves dead code in the component. If this flow is deprecated, the code should be removed entirely to improve maintainability. If it needs to be kept for reference, it should be properly commented out.