feat: Implement client-side password encryption #67
Conversation
This commit introduces client-side symmetric AES encryption for passwords
before they are transmitted to the backend. This enhancement adds an
additional layer of security to the existing HTTPS and bcrypt hashing.
Key changes:
1. **Configuration:**
* Added new environment variables for the shared symmetric key:
* `APP_SYMMETRIC_ENCRYPTION_KEY` (backend)
* `NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY` (frontend)
* `VITE_APP_SYMMETRIC_ENCRYPTION_KEY` (admin)
* Updated `.env.example` files and backend configuration.
2. **Backend (Python/FastAPI):**
* Added `cryptography` for Fernet-based decryption.
* Implemented `decrypt_password` utility in `app.core.security`.
* Integrated decryption into authentication, user creation (`crud.py`),
and password update (`users.py`) logic.
* Added unit tests for the decryption utility.
3. **Frontend (Next.js):**
* Added `crypto-js` for AES encryption.
* Implemented `encryptPassword` utility in `lib/encryption.ts`.
* Integrated encryption into login, registration actions, and the
user password update form.
* Added unit tests for the encryption utility.
4. **Admin Panel (React/Vite):**
* Added `crypto-js` for AES encryption.
* Implemented `encryptPassword` utility in `src/utils/encryption.ts`.
* Integrated encryption into login, sign-up hooks, and the admin
user password update form.
* Added unit tests for the encryption utility.
5. **Documentation:**
* Updated `README.md` with a new section "Enhanced Password Security"
detailing the mechanism and key management.
This change addresses the security concern of transmitting plain-text
passwords over the network, as outlined in issue #15 (from your prompt,
referencing nexus/issues/15). The backend continues to use bcrypt for
storing password hashes.
|
""" WalkthroughThis change implements client-side password encryption using symmetric cryptography across both admin and frontend applications. Passwords are encrypted before transmission, and new environment variables for symmetric keys are introduced in all relevant Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend/Admin
participant Backend API
participant DB
User->>Frontend/Admin: Enter password (login/register/change)
Frontend/Admin->>Frontend/Admin: Encrypt password with symmetric key
Frontend/Admin->>Backend API: Send encrypted password
Backend API->>Backend API: Decrypt password using Fernet key
Backend API->>DB: Verify or update password
DB-->>Backend API: Respond (success/failure)
Backend API-->>Frontend/Admin: Respond (success/failure)
Frontend/Admin-->>User: Show result
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
penify-dev
bot
added
documentation
CI Feedback 🧐(Feedback updated until commit d3eb32d)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
PR Review 🔍
|
PR Code Suggestions ✨
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
backend/app/crud.py (1)
167-189:⚠️ Potential issueMissing password decryption in update_user function
The
update_userfunction handles password updates but doesn't decrypt the password before hashing it, unlike the other password-handling functions. This creates an inconsistency in the password handling flow.Apply this fix:
if update_data.get("password"): - hashed_password = get_password_hash(update_data["password"]) + plain_password = decrypt_password(update_data["password"]) + hashed_password = get_password_hash(plain_password) del update_data["password"] update_data["hashed_password"] = hashed_password
🧹 Nitpick comments (23)
backend/pyproject.toml (1)
29-29: Pin cryptography dependency upper bound to maintain stability.Most other dependencies here include both lower and upper bounds to prevent unintended breaking changes from future major releases. Consider updating the spec to:
- "cryptography>=42.0.8", # Added for symmetric encryption + "cryptography>=42.0.8,<43.0.0", # Added for symmetric encryptionThis will help ensure compatibility until you decide to test and bump to a newer major version.
admin/package.json (1)
24-24: Consider using the native Web Crypto API instead of crypto-js.While
crypto-jsis convenient, it adds bundle weight and relies on a JS implementation of AES. The built-in Web Crypto API provides a more performant, tree-shakable, and secure alternative for AES-GCM. You could refactor yourencryptPasswordutility to leveragewindow.crypto.subtleinstead.Also applies to: 40-40
frontend/package.json (1)
36-36: Consider using the native Web Crypto API instead of crypto-js.Similar to the admin panel, replacing
crypto-jswith the browser’s nativewindow.crypto.subtlewould reduce bundle size and improve security assurance. A small helper around Web Crypto can cover your AES-CBC/GCM needs without external dependencies.Also applies to: 58-58
frontend/app/customers/components/PasswordForm.tsx (1)
6-6: Consider using absolute imports for better maintainability.The relative import path
"../../../lib/encryption"is deeply nested. Consider using an absolute import path like"@/lib/encryption"to improve maintainability and readability.-import { encryptPassword } from "../../../lib/encryption"; // Adjusted path +import { encryptPassword } from "@/lib/encryption";admin/.env.example (1)
4-6: Enhance key generation guidance with specific instructions.While the comments explain the requirements for the encryption key, it would be helpful to include specific instructions for generating a secure key. Also, mention that this key must match across frontend, admin, and backend services.
# This key needs to be a securely generated, preferably 32-byte (256-bit) random string, # often represented in base64. For Fernet, it must be a URL-safe base64-encoded 32-byte key. +# You can generate a suitable key using: +# Python: `python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"` +# Note: This key MUST be the same across frontend, admin, and backend services. VITE_APP_SYMMETRIC_ENCRYPTION_KEY="your_strong_symmetric_encryption_key_here"frontend/components/actions/register-action.ts (2)
9-9: Consider using absolute imports for better maintainability.The relative import path
"../../lib/encryption"could be replaced with an absolute import path like"@/lib/encryption"for better maintainability and readability.-import { encryptPassword } from "../../lib/encryption"; +import { encryptPassword } from "@/lib/encryption";
31-32: Remove commented-out code.Remove the commented-out code on line 31 to keep the codebase clean and maintainable.
const input = { body: { email, - // password, password: encryptedPassword, // Use encrypted password full_name, }, };backend/app/core/security.py (3)
32-46: Robust implementation of password decryption with minor style issueThe
decrypt_passwordfunction is well-implemented with proper error handling for different failure scenarios. However, there's a trailing whitespace on line 45 that should be removed.Consider adding a check for empty or null passwords to prevent potential errors.
Apply this fix:
def decrypt_password(encrypted_password_b64: str) -> str: try: + if not encrypted_password_b64: + raise HTTPException(status_code=400, detail="Password cannot be empty.") cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode()) # crypto-js AES output is Base64, Fernet expects bytes. # The encrypted_password_b64 itself is a string that needs to be encoded to bytes. decrypted_bytes = cipher_suite.decrypt(encrypted_password_b64.encode('utf-8')) return decrypted_bytes.decode('utf-8') except InvalidToken: # This specific exception is raised by Fernet for invalid tokens raise HTTPException(status_code=400, detail="Invalid password encryption format or key.") except Exception as e: # Catch other potential errors during decryption # Log the error e for server-side debugging # Consider using a proper logger in a production environment - print(f"Password decryption error: {e}") + print(f"Password decryption error: {e}") raise HTTPException(status_code=400, detail="Could not process password.")🧰 Tools
🪛 Ruff (0.11.9)
45-45: Trailing whitespace
Remove trailing whitespace
(W291)
32-46: Consider using a logger instead of print statements for productionUsing
print()for error logging is not ideal in production environments. Consider using a proper logging solution that allows for different log levels and better control over log output.+import logging + +logger = logging.getLogger(__name__) + def decrypt_password(encrypted_password_b64: str) -> str: try: cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode()) # crypto-js AES output is Base64, Fernet expects bytes. # The encrypted_password_b64 itself is a string that needs to be encoded to bytes. decrypted_bytes = cipher_suite.decrypt(encrypted_password_b64.encode('utf-8')) return decrypted_bytes.decode('utf-8') except InvalidToken: # This specific exception is raised by Fernet for invalid tokens raise HTTPException(status_code=400, detail="Invalid password encryption format or key.") except Exception as e: # Catch other potential errors during decryption # Log the error e for server-side debugging # Consider using a proper logger in a production environment - print(f"Password decryption error: {e}") + logger.error(f"Password decryption error: {e}", exc_info=True) raise HTTPException(status_code=400, detail="Could not process password.")🧰 Tools
🪛 Ruff (0.11.9)
45-45: Trailing whitespace
Remove trailing whitespace
(W291)
32-46: Add documentation about the expected encryption formatThe function expects passwords encrypted in a specific format (Fernet-compatible encryption). Consider adding docstrings explaining the expected input format to make it clearer for other developers.
def decrypt_password(encrypted_password_b64: str) -> str: + """ + Decrypts a password that was encrypted client-side using AES encryption. + + Args: + encrypted_password_b64: The Base64-encoded encrypted password string + + Returns: + The decrypted plaintext password + + Raises: + HTTPException: If decryption fails due to invalid format, key mismatch, + or other processing errors + """ try: cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode()) # crypto-js AES output is Base64, Fernet expects bytes.🧰 Tools
🪛 Ruff (0.11.9)
45-45: Trailing whitespace
Remove trailing whitespace
(W291)
backend/app/core/config.py (3)
57-57: Ensure sensitive key is protected in memoryAdding the symmetric encryption key to the application settings is appropriate, but consider additional memory protection measures for this sensitive value.
Consider using a secure string handling library or secret management solution that can zero out memory after use to prevent the key from persisting in memory dumps.
59-71: Improve key validation and error handlingThe validator correctly ensures the key is present and valid for Fernet initialization, but could be improved with more specific exception handling.
@field_validator("APP_SYMMETRIC_ENCRYPTION_KEY") def validate_symmetric_key(cls, v: str) -> str: if not v: raise ValueError("APP_SYMMETRIC_ENCRYPTION_KEY must be set") # Add more specific validation if needed, e.g., for Fernet key format/length # For Fernet, it must be a URL-safe base64-encoded 32-byte key. # Example basic check (not exhaustive for Fernet format): try: Fernet(v.encode()) # Attempt to initialize Fernet to check key validity - except Exception as e: - raise ValueError(f"APP_SYMMETRIC_ENCRYPTION_KEY is not a valid Fernet key: {e}") + except ValueError as e: + raise ValueError(f"APP_SYMMETRIC_ENCRYPTION_KEY is not a valid Fernet key: {e}") + except Exception as e: + raise ValueError(f"Unexpected error validating encryption key: {e}") return v
15-19: Consider the impact of dependencies on module loading timeAdding cryptography imports at the module level increases loading time even when not used.
Consider moving the Fernet import inside the validator function to delay loading the cryptography module until needed:
from pydantic import ( AnyUrl, BeforeValidator, EmailStr, HttpUrl, PostgresDsn, computed_field, model_validator, field_validator, ) from pydantic_settings import BaseSettings, SettingsConfigDict from typing_extensions import Self -from cryptography.fernet import Fernet from yarl import URLAnd in the validator function:
@field_validator("APP_SYMMETRIC_ENCRYPTION_KEY") def validate_symmetric_key(cls, v: str) -> str: if not v: raise ValueError("APP_SYMMETRIC_ENCRYPTION_KEY must be set") # Add more specific validation if needed, e.g., for Fernet key format/length # For Fernet, it must be a URL-safe base64-encoded 32-byte key. try: from cryptography.fernet import Fernet Fernet(v.encode()) # Attempt to initialize Fernet to check key validity except Exception as e: raise ValueError(f"APP_SYMMETRIC_ENCRYPTION_KEY is not a valid Fernet key: {e}") return vfrontend/lib/encryption.ts (3)
4-11: Optimize encryption key retrieval with memoizationThe current implementation retrieves the encryption key from environment variables on every call, which is inefficient.
-const getEncryptionKey = (): string => { +// Memoize the encryption key to avoid repeated environment variable lookups +let cachedKey: string | null = null; + +const getEncryptionKey = (): string => { + if (cachedKey) { + return cachedKey; + } + const key = process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY; if (!key) { console.error("Encryption key is not defined. Please check NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY environment variable."); throw new Error("Encryption key is not defined."); } + cachedKey = key; return key; };
13-22: Add basic validation for the encryption resultThe encryption function should validate that the result is a non-empty string before returning it.
export const encryptPassword = (plainPassword: string): string => { const key = getEncryptionKey(); try { const encrypted = CryptoJS.AES.encrypt(plainPassword, key).toString(); + if (!encrypted) { + throw new Error("Encryption produced an empty result"); + } return encrypted; } catch (error) { - console.error("Password encryption failed:", error); + console.error("Password encryption failed:", error instanceof Error ? error.message : error); throw new Error("Could not encrypt password."); } };
2-2: Consider using a more specific import from crypto-jsImporting the entire CryptoJS library increases bundle size when only AES is needed.
-import CryptoJS from 'crypto-js'; +import AES from 'crypto-js/aes'; +import Utf8 from 'crypto-js/enc-utf8';And then update the encryption function:
try { - const encrypted = CryptoJS.AES.encrypt(plainPassword, key).toString(); + const encrypted = AES.encrypt(plainPassword, key).toString(); return encrypted;admin/src/utils/encryption.test.ts (1)
4-27: Consider using more robust test environment setupThe current test modifies
import.meta.envdirectly, which might cause issues in some test environments or if tests run in parallel.Consider using a more robust approach like:
- Using a testing library's mocking capabilities (like
jest.mockorvi.mock)- Creating a separate module that exports the environment config that can be easily mocked
- Adding a parameter to the
encryptPasswordfunction for dependency injection during testsExample approach:
// In encryption.ts export const getEncryptionKey = (env = import.meta.env): string => { const key = env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY; if (!key) { console.error("Encryption key is not defined"); throw new Error("Encryption key is not defined."); } return key; }; export const encryptPassword = (plainPassword: string, env = import.meta.env): string => { const key = getEncryptionKey(env); try { const encrypted = CryptoJS.AES.encrypt(plainPassword, key).toString(); return encrypted; } catch (error) { console.error("Password encryption failed:", error); throw new Error("Could not encrypt password."); } }; // In tests it('should encrypt a password successfully', () => { const mockEnv = { VITE_APP_SYMMETRIC_ENCRYPTION_KEY: MOCK_KEY }; const plainPassword = 'myAdminSecretPassword'; const encrypted = encryptPassword(plainPassword, mockEnv); // Test assertions });frontend/lib/encryption.test.ts (2)
50-59: Consider using undefined assignment instead of delete.While the test correctly verifies error handling for missing encryption keys, using the
deleteoperator can impact performance.- delete process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY; + process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
1-80: Consider adding tests for special characters and very long passwords.While the test coverage is generally good, consider adding tests for:
- Passwords with special characters, emojis, and international characters
- Very long passwords (e.g., 100+ characters)
- Handling of null or undefined inputs
This would ensure the encryption utility is robust under all expected conditions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
admin/src/utils/encryption.ts (1)
1-22: Consider adding detailed documentation for the encryption process.While the implementation is solid, adding JSDoc comments would improve maintainability by documenting:
- The encryption algorithm used (AES)
- The expected input/output formats
- The environment variable dependency
- Potential error cases
This would help future developers understand the security implementation details.
// In admin/src/utils/encryption.ts import CryptoJS from 'crypto-js'; +/** + * Retrieves the symmetric encryption key from environment variables. + * @returns {string} The encryption key + * @throws {Error} If the encryption key is not defined in environment variables + */ const getEncryptionKey = (): string => { const key = import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY; if (!key) { console.error("Encryption key is not defined. Please check VITE_APP_SYMMETRIC_ENCRYPTION_KEY environment variable."); throw new Error("Encryption key is not defined."); } return key; }; +/** + * Encrypts a password using AES encryption. + * @param {string} plainPassword - The plain text password to encrypt + * @returns {string} Base64-encoded encrypted password string + * @throws {Error} If encryption fails or if the encryption key is not defined + */ export const encryptPassword = (plainPassword: string): string => { const key = getEncryptionKey(); try { const encrypted = CryptoJS.AES.encrypt(plainPassword, key).toString(); return encrypted; } catch (error) { console.error("Password encryption failed:", error); throw new Error("Could not encrypt password."); } };README.md (2)
372-394: Clear key management documentation with helpful generation example.The key management section:
- Properly documents the environment variables for all three components
- Emphasizes that all three environments must use the same key
- Provides a practical example of generating a compatible key
- Includes important notes about key security
However, there are some inconsistencies in Markdown formatting.
Consider standardizing the unordered list style to use dashes instead of asterisks for consistency with the rest of the document:
- * **Backend (Python/FastAPI):** - * Variable: `APP_SYMMETRIC_ENCRYPTION_KEY` - * Location: `backend/.env.example` (and your `.env` file) + - **Backend (Python/FastAPI):** + - Variable: `APP_SYMMETRIC_ENCRYPTION_KEY` + - Location: `backend/.env.example` (and your `.env` file)Apply similar changes to all bulleted lists in this section.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
375-375: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
376-376: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
376-376: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
377-377: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
377-377: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
378-378: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
379-379: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
379-379: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
380-380: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
380-380: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
381-381: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
382-382: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
382-382: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
383-383: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
383-383: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
386-386: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
387-387: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
393-393: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
358-399: Consider adding a warning about symmetric key security risks.While the documentation covers the implementation well, it would be beneficial to add a note about the security implications of using symmetric encryption.
Add a brief note explaining:
**Security Considerations:** - This approach adds an additional security layer but still relies on the secrecy of the symmetric key. - If an attacker gains access to both the client-side code and the encryption key, they could potentially decrypt intercepted passwords. - Ensure your environment variables are properly secured in production environments. - This approach should be considered as complementary to HTTPS, not a replacement.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
375-375: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
376-376: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
376-376: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
377-377: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
377-377: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
378-378: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
379-379: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
379-379: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
380-380: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
380-380: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
381-381: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
382-382: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
382-382: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
383-383: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
383-383: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
386-386: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
387-387: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
393-393: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
397-397: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
398-398: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
backend/app/tests/core/test_security.py (1)
1-75: Clean up whitespace in blank lines for consistency.Several lines in the test file contain whitespace in otherwise blank lines. While this doesn't affect functionality, it would improve code cleanliness to remove them.
Remove whitespace from blank lines:
- Line 24
- Line 36
- Line 39
- Line 51
- Line 61
🧰 Tools
🪛 Ruff (0.11.9)
21-21: Unused function argument:
mock_settings_key(ARG001)
24-24: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Unused function argument:
mock_settings_key(ARG001)
36-36: Blank line contains whitespace
Remove whitespace from blank line
(W293)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
48-48: Unused function argument:
mock_settings_key(ARG001)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
65-65: Unused function argument:
mock_settings_key(ARG001)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
admin/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlbackend/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
README.md(1 hunks)admin/.env.example(1 hunks)admin/package.json(2 hunks)admin/src/components/UserSettings/ChangePassword.tsx(2 hunks)admin/src/hooks/useAuth.ts(3 hunks)admin/src/utils/encryption.test.ts(1 hunks)admin/src/utils/encryption.ts(1 hunks)backend/.env.example(1 hunks)backend/app/api/routes/users.py(2 hunks)backend/app/core/config.py(2 hunks)backend/app/core/security.py(2 hunks)backend/app/crud.py(3 hunks)backend/app/tests/core/test_security.py(1 hunks)backend/pyproject.toml(1 hunks)frontend/.env.example(1 hunks)frontend/app/customers/components/PasswordForm.tsx(2 hunks)frontend/components/actions/login-action.ts(2 hunks)frontend/components/actions/register-action.ts(2 hunks)frontend/lib/encryption.test.ts(1 hunks)frontend/lib/encryption.ts(1 hunks)frontend/package.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
frontend/app/customers/components/PasswordForm.tsx (1)
frontend/lib/encryption.ts (1)
encryptPassword(13-22)
backend/app/api/routes/users.py (1)
backend/app/core/security.py (3)
get_password_hash(28-29)verify_password(24-25)decrypt_password(32-46)
admin/src/components/UserSettings/ChangePassword.tsx (1)
admin/src/utils/encryption.ts (1)
encryptPassword(13-22)
frontend/lib/encryption.test.ts (1)
frontend/lib/encryption.ts (1)
encryptPassword(13-22)
backend/app/tests/core/test_security.py (1)
backend/app/core/security.py (1)
decrypt_password(32-46)
🪛 Biome (1.9.4)
frontend/lib/encryption.test.ts
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
admin/src/utils/encryption.test.ts
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 Ruff (0.11.9)
backend/app/core/security.py
45-45: Trailing whitespace
Remove trailing whitespace
(W291)
backend/app/tests/core/test_security.py
21-21: Unused function argument: mock_settings_key
(ARG001)
24-24: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Unused function argument: mock_settings_key
(ARG001)
36-36: Blank line contains whitespace
Remove whitespace from blank line
(W293)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
48-48: Unused function argument: mock_settings_key
(ARG001)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
65-65: Unused function argument: mock_settings_key
(ARG001)
🪛 markdownlint-cli2 (0.17.2)
README.md
375-375: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
376-376: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
376-376: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
377-377: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
377-377: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
378-378: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
379-379: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
379-379: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
380-380: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
380-380: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
381-381: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
382-382: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
382-382: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
383-383: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
383-383: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
386-386: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
387-387: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
393-393: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
397-397: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
398-398: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-playwright-reports
🔇 Additional comments (29)
backend/.env.example (1)
1-3: Environment variable placeholder looks good.The new
APP_SYMMETRIC_ENCRYPTION_KEYentry is clearly documented and matches the backend’s expected format.frontend/.env.example (1)
8-8: Verify inclusion of DEBUG= is intentional.*Adding
DEBUG=*in your example will enable all debug namespaces in development—ensure this wildcard pattern is what you want to recommend for users and that it won’t be mistakenly copied into production environment configurations.frontend/components/actions/login-action.ts (1)
9-9: LGTM: Password encryption implementation looks goodThe implementation correctly encrypts the password before sending it to the server. The variable renaming from
passwordtoplainPasswordimproves code clarity, and comments explaining the changes help maintainability.Also applies to: 23-25, 30-31
backend/app/crud.py (2)
9-9: LGTM: Password decryption in user creation is properly implementedThe code correctly decrypts the incoming encrypted password before hashing it for storage, which aligns with the client-side encryption strategy.
Also applies to: 131-132, 134-135
161-162: LGTM: Authentication flow properly handles encrypted passwordsThe implementation correctly decrypts the password before verification against the stored hash.
backend/app/api/routes/users.py (2)
14-14: LGTM: Current password decryption implemented correctlyThe code properly decrypts the current password before verifying it against the stored hash.
Also applies to: 110-111
114-116: LGTM: New password decryption and comparison implemented correctlyThe code correctly decrypts the new password and compares it with the decrypted current password before hashing and storing it.
Also applies to: 119-119
backend/app/core/security.py (1)
6-7: LGTM: Required imports for decryption functionalityThe imports for Fernet, InvalidToken, and HTTPException are necessary for the new decryption functionality.
admin/src/utils/encryption.test.ts (1)
50-57:❌ Incorrect review comment
Fix performance issue with
deleteoperatorUsing the
deleteoperator onimport.meta.envcan impact performance as flagged by static analysis.it('should throw an error if encryption key is not defined', () => { // @ts-ignore - delete import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY; + // Using undefined assignment instead of delete operator + const originalKey = import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY; + // @ts-ignore + import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY = undefined; const plainPassword = 'testAdminPassword'; expect(() => encryptPassword(plainPassword)).toThrow('Encryption key is not defined.'); + + // Restore the original key + // @ts-ignore + import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY = originalKey; });
🏁 Script executed:
#!/bin/bash # Verify if the delete operator has performance implications rg -n "delete\s+\w+(\.\w+)+" --type tsLength of output: 338
No refactor needed for
deletein test; disable lint rule insteadThe performance cost of
deletehere is irrelevant—this is test code that runs once, purely to simulate a missing env variable. Rather than rewriting the test, simply disable the lint rule on that line.• File: admin/src/utils/encryption.test.ts Line 52
// @ts-ignore - delete import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY; + // eslint-disable-next-line no-delete -- simulate missing env key in test + delete import.meta.env.VITE_APP_SYMMETRIC_ENCRYPTION_KEY;Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
frontend/lib/encryption.test.ts (7)
1-3: LGTM: Good import organization and module dependencies.The imports are well-organized, and the test file correctly imports both the function under test and the CryptoJS library needed for verification.
4-9: Well-structured test suite with clear description.The test suite is appropriately named and includes a useful comment about the AES-256 key length. This provides context for the mock key implementation.
10-23: Good environment variable management in tests.The test correctly:
- Stores the original environment
- Resets modules before each test to avoid cached values
- Mocks the encryption key environment variable
- Restores the original environment after tests
This ensures test isolation and prevents test environment leakage.
25-36: Thorough encryption verification test.This test properly verifies both that:
- The encryption works (output differs from input)
- The encryption is reversible (can be decrypted back to original)
The end-to-end verification using CryptoJS.AES.decrypt confirms the encryption integrity.
38-48: Good output format validation.The test verifies that the encryption output is a Base64 string, which is important for interoperability with the backend decryption process.
61-69: Proper validation of different inputs producing different outputs.The test correctly verifies that different passwords (even with subtle differences) produce different ciphertexts, which is important for security.
71-80: Edge case handling for empty strings.Good test coverage for edge cases like empty strings, ensuring the encryption utility handles all potential inputs correctly.
admin/src/utils/encryption.ts (3)
1-3: Good module structure with clear import.The file correctly imports CryptoJS and includes a descriptive comment at the top identifying the file location.
4-11: Well-implemented encryption key retrieval with proper error handling.The
getEncryptionKeyfunction:
- Correctly retrieves the key from environment variables using Vite's import.meta.env
- Validates the key's presence
- Provides detailed error logging and throws a user-friendly error message
This ensures robust key management and clear error messaging for configuration issues.
13-22: Clean password encryption implementation with good error handling.The
encryptPasswordfunction:
- Retrieves the encryption key
- Performs AES encryption with proper error handling
- Returns the encrypted value as a string
- Logs detailed errors but provides a simplified message to the caller
This implementation aligns well with the frontend counterpart, ensuring consistent encryption across both client applications.
README.md (2)
358-371: Excellent documentation of the enhanced password security mechanism.The added section provides clear, comprehensive documentation on the password encryption mechanism, explaining:
- The purpose of the enhanced security
- The client-side encryption approach
- The flow from browser encryption through transmission to server-side decryption
- How this complements the existing bcrypt hashing
This documentation helps users understand the security benefits of the implementation.
395-399: Good documentation of libraries used.The section clearly documents the cryptographic libraries used on both client and server sides, which is important for security audits and developer understanding.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
397-397: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
398-398: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
backend/app/tests/core/test_security.py (8)
1-8: Good import organization and dependencies.The test file correctly imports all necessary dependencies, including pytest, HTTPException, patch for mocking, and the Fernet encryption class.
9-14: Well-defined test key generation.The test code appropriately:
- Generates a fixed Fernet key for predictable test results
- Includes clear comments explaining the purpose
- Converts the key to both binary and string formats for different use cases
This ensures test determinism while using realistic encryption.
15-20: Good use of pytest fixtures for mocking settings.The fixture correctly patches the application settings to use the test key, ensuring that tests run in isolation without affecting or depending on actual environment variables.
21-32: Comprehensive happy path decryption test.The test correctly:
- Creates a plaintext password
- Encrypts it using the test key
- Verifies successful decryption back to the original text
This test confirms that the end-to-end encryption/decryption flow works as expected.
🧰 Tools
🪛 Ruff (0.11.9)
21-21: Unused function argument:
mock_settings_key(ARG001)
24-24: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-46: Good error handling test for invalid tokens.The test verifies that the decrypt function properly handles invalid tokens by:
- Attempting to decrypt an obviously invalid string
- Checking that an HTTPException with status code 400 is raised
- Verifying the appropriate error message
This ensures robust error handling for malformed inputs.
🧰 Tools
🪛 Ruff (0.11.9)
33-33: Unused function argument:
mock_settings_key(ARG001)
36-36: Blank line contains whitespace
Remove whitespace from blank line
(W293)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
48-64: Good security test for key validation.This test verifies that the decryption function correctly rejects tokens encrypted with a different key, which is an important security characteristic. It confirms that:
- Data encrypted with a different key cannot be decrypted
- The correct error status and message are returned
This helps prevent potential security bypasses.
🧰 Tools
🪛 Ruff (0.11.9)
48-48: Unused function argument:
mock_settings_key(ARG001)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
65-75: Thorough edge case testing for empty input.The test properly handles the edge case of an empty password string, ensuring the function doesn't crash and returns an appropriate error message.
🧰 Tools
🪛 Ruff (0.11.9)
65-65: Unused function argument:
mock_settings_key(ARG001)
1-75: The mock_settings_key argument appears unused but is necessary.Static analysis flagged the
mock_settings_keyparameter as unused in multiple test functions, but this is a false positive. The fixture is indirectly used because it patches settings.APP_SYMMETRIC_ENCRYPTION_KEY, which is essential for the tests to function correctly.🧰 Tools
🪛 Ruff (0.11.9)
21-21: Unused function argument:
mock_settings_key(ARG001)
24-24: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Unused function argument:
mock_settings_key(ARG001)
36-36: Blank line contains whitespace
Remove whitespace from blank line
(W293)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
48-48: Unused function argument:
mock_settings_key(ARG001)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
65-65: Unused function argument:
mock_settings_key(ARG001)
| # often represented in base64. For Fernet, it must be a URL-safe base64-encoded 32-byte key. | ||
| NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY="your_strong_symmetric_encryption_key_here" |
There was a problem hiding this comment.
Remove quotes around the key placeholder to avoid injecting literal quotes.
Next.js reads env values verbatim, so including quotes will become part of the key string and break decryption. Update to:
- NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY="your_strong_symmetric_encryption_key_here"
+ NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY=your_strong_symmetric_encryption_key_here📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # This key needs to be a securely generated, preferably 32-byte (256-bit) random string, | |
| # often represented in base64. For Fernet, it must be a URL-safe base64-encoded 32-byte key. | |
| NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY="your_strong_symmetric_encryption_key_here" | |
| # This key needs to be a securely generated, preferably 32-byte (256-bit) random string, | |
| # often represented in base64. For Fernet, it must be a URL-safe base64-encoded 32-byte key. | |
| NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY=your_strong_symmetric_encryption_key_here |
🤖 Prompt for AI Agents
In frontend/.env.example around lines 10 to 12, remove the double quotes around
the NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY placeholder value to prevent
literal quotes from being included in the environment variable. This ensures the
key is read correctly without extra characters that would break decryption.
| // const { email, password, full_name } = validatedFields.data; | ||
| const { email, password: plainPassword, full_name } = validatedFields.data; // Rename | ||
| const encryptedPassword = encryptPassword(plainPassword); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for password encryption.
The encryption operation could fail and should be wrapped in a try-catch block to provide better error feedback to the user.
- // const { email, password, full_name } = validatedFields.data;
- const { email, password: plainPassword, full_name } = validatedFields.data; // Rename
- const encryptedPassword = encryptPassword(plainPassword);
+ const { email, password: plainPassword, full_name } = validatedFields.data;
+
+ let encryptedPassword;
+ try {
+ encryptedPassword = encryptPassword(plainPassword);
+ } catch (error) {
+ console.error("Password encryption failed:", error);
+ return {
+ server_error: "Failed to encrypt password. Please try again.",
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // const { email, password, full_name } = validatedFields.data; | |
| const { email, password: plainPassword, full_name } = validatedFields.data; // Rename | |
| const encryptedPassword = encryptPassword(plainPassword); | |
| const { email, password: plainPassword, full_name } = validatedFields.data; | |
| let encryptedPassword; | |
| try { | |
| encryptedPassword = encryptPassword(plainPassword); | |
| } catch (error) { | |
| console.error("Password encryption failed:", error); | |
| return { | |
| server_error: "Failed to encrypt password. Please try again.", | |
| }; | |
| } |
🤖 Prompt for AI Agents
In frontend/components/actions/register-action.ts around lines 24 to 26, the
password encryption step lacks error handling. Wrap the call to
encryptPassword(plainPassword) in a try-catch block to catch any exceptions
during encryption. In the catch block, handle the error appropriately by logging
it and providing user-friendly feedback or error propagation to ensure the
failure is managed gracefully.
admin/src/hooks/useAuth.ts
Outdated
| mutationFn: (data: UserRegister) => { // data is UserRegister | ||
| const encryptedPassword = encryptPassword(data.password); | ||
| return UsersService.registerUser({ | ||
| requestBody: { | ||
| ...data, | ||
| password: encryptedPassword, // Override with encrypted password | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for password encryption in signup
The current implementation doesn't handle potential encryption errors specifically, which could lead to confusing error messages for users.
mutationFn: (data: UserRegister) => { // data is UserRegister
- const encryptedPassword = encryptPassword(data.password);
- return UsersService.registerUser({
- requestBody: {
- ...data,
- password: encryptedPassword, // Override with encrypted password
- },
- });
+ try {
+ if (!data.password) {
+ throw new Error("Password is required");
+ }
+ const encryptedPassword = encryptPassword(data.password);
+ return UsersService.registerUser({
+ requestBody: {
+ ...data,
+ password: encryptedPassword, // Override with encrypted password
+ },
+ });
+ } catch (error) {
+ console.error("Password encryption failed:", error);
+ throw new Error("Failed to encrypt password. Please try again.");
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mutationFn: (data: UserRegister) => { // data is UserRegister | |
| const encryptedPassword = encryptPassword(data.password); | |
| return UsersService.registerUser({ | |
| requestBody: { | |
| ...data, | |
| password: encryptedPassword, // Override with encrypted password | |
| }, | |
| }); | |
| }, | |
| mutationFn: (data: UserRegister) => { // data is UserRegister | |
| try { | |
| if (!data.password) { | |
| throw new Error("Password is required"); | |
| } | |
| const encryptedPassword = encryptPassword(data.password); | |
| return UsersService.registerUser({ | |
| requestBody: { | |
| ...data, | |
| password: encryptedPassword, // Override with encrypted password | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("Password encryption failed:", error); | |
| throw new Error("Failed to encrypt password. Please try again."); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In admin/src/hooks/useAuth.ts around lines 49 to 57, the password encryption
step lacks error handling, which may cause unclear errors during signup. Wrap
the encryption call in a try-catch block to catch any errors thrown by
encryptPassword, and handle them appropriately by returning or throwing a clear,
user-friendly error message before proceeding with user registration.
admin/src/hooks/useAuth.ts
Outdated
| const login = async (data: AccessToken) => { // AccessToken is Body_login_login_access_token | ||
| const encryptedPassword = encryptPassword(data.password); // data.password should exist | ||
| const response = await LoginService.loginAccessToken({ | ||
| formData: data, | ||
| }) | ||
| localStorage.setItem("access_token", response.access_token) | ||
| } | ||
| formData: { | ||
| ...data, // Spread other potential fields like username, grant_type etc. | ||
| password: encryptedPassword, // Override with encrypted password | ||
| }, | ||
| }); | ||
| localStorage.setItem("access_token", response.access_token); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for password encryption in login
Similar to the signup flow, the login function lacks specific error handling for password encryption failures.
const login = async (data: AccessToken) => { // AccessToken is Body_login_login_access_token
- const encryptedPassword = encryptPassword(data.password); // data.password should exist
- const response = await LoginService.loginAccessToken({
- formData: {
- ...data, // Spread other potential fields like username, grant_type etc.
- password: encryptedPassword, // Override with encrypted password
- },
- });
- localStorage.setItem("access_token", response.access_token);
+ try {
+ if (!data.password) {
+ throw new Error("Password is required");
+ }
+ const encryptedPassword = encryptPassword(data.password);
+ const response = await LoginService.loginAccessToken({
+ formData: {
+ ...data,
+ password: encryptedPassword,
+ },
+ });
+ localStorage.setItem("access_token", response.access_token);
+ } catch (error) {
+ console.error("Login process failed:", error);
+ if (error instanceof Error && error.message.includes("encrypt")) {
+ throw new Error("Failed to secure your password. Please try again.");
+ }
+ throw error; // Re-throw other errors to be handled by the mutation
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const login = async (data: AccessToken) => { // AccessToken is Body_login_login_access_token | |
| const encryptedPassword = encryptPassword(data.password); // data.password should exist | |
| const response = await LoginService.loginAccessToken({ | |
| formData: data, | |
| }) | |
| localStorage.setItem("access_token", response.access_token) | |
| } | |
| formData: { | |
| ...data, // Spread other potential fields like username, grant_type etc. | |
| password: encryptedPassword, // Override with encrypted password | |
| }, | |
| }); | |
| localStorage.setItem("access_token", response.access_token); | |
| }; | |
| const login = async (data: AccessToken) => { // AccessToken is Body_login_login_access_token | |
| try { | |
| if (!data.password) { | |
| throw new Error("Password is required"); | |
| } | |
| const encryptedPassword = encryptPassword(data.password); | |
| const response = await LoginService.loginAccessToken({ | |
| formData: { | |
| ...data, | |
| password: encryptedPassword, | |
| }, | |
| }); | |
| localStorage.setItem("access_token", response.access_token); | |
| } catch (error) { | |
| console.error("Login process failed:", error); | |
| if (error instanceof Error && error.message.includes("encrypt")) { | |
| throw new Error("Failed to secure your password. Please try again."); | |
| } | |
| throw error; // Re-throw other errors to be handled by the mutation | |
| } | |
| }; |
🤖 Prompt for AI Agents
In admin/src/hooks/useAuth.ts around lines 70 to 79, the login function does not
handle errors that may occur during password encryption. Wrap the
encryptPassword call in a try-catch block to catch any encryption errors, and
handle them appropriately by either logging the error or returning a failure
response to prevent the function from proceeding with invalid data.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: OpenIM Bot <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: OpenIM Bot <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: OpenIM Bot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/core/test_security.py (1)
43-45: Ensure error message alignment with implementationThe test now expects only one specific error message, but the actual implementation in
backend/app/core/security.pystill has two different error messages. This change should be synchronized with an update to the implementation.Please make sure the implementation in
backend/app/core/security.pyhas been updated to use a single consistent error message as recommended in the previous review:#!/bin/bash # Check if the decrypt_password function in security.py still has two different error messages grep -A 15 "def decrypt_password" backend/app/core/security.py🧰 Tools
🪛 Ruff (0.11.9)
43-43: SyntaxError: Expected an identifier, but found a keyword 'assert' that cannot be used here
43-43: SyntaxError: Simple statements must be separated by newlines or semicolons
45-45: SyntaxError: Expected an identifier, but found a keyword 'assert' that cannot be used here
45-45: SyntaxError: Simple statements must be separated by newlines or semicolons
🧹 Nitpick comments (3)
backend/app/tests/core/test_security.py (3)
24-24: Clean up whitespace in blank linesPlease remove the trailing whitespace on lines 24, 50, and 60.
- +Also applies to: 50-50, 60-60
🧰 Tools
🪛 Ruff (0.11.9)
24-24: Blank line contains whitespace
(W293)
33-33: Remove unnecessary file commentLine 33 contains a comment that seems like a development artifact. This line should be removed as it's redundant given you're already in the file.
-# In backend/app/tests/core/test_security.py
72-73: Update error message assertion for consistencyFor consistency with the changes in
test_decrypt_password_invalid_token, this test should also be updated to check for the unified error message.assert exc_info.value.status_code == 400 - assert "Could not process password." in exc_info.value.detail # Fernet will likely fail to decode/decrypt + assert exc_info.value.detail == "Invalid password encryption format or key."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/tests/core/test_security.py(1 hunks)frontend/app/customers/components/PasswordForm.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/customers/components/PasswordForm.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/core/test_security.py (1)
backend/app/core/security.py (1)
decrypt_password(32-46)
🪛 Ruff (0.11.9)
backend/app/tests/core/test_security.py
24-24: Blank line contains whitespace
(W293)
43-43: SyntaxError: Expected an identifier, but found a keyword 'assert' that cannot be used here
43-43: SyntaxError: Simple statements must be separated by newlines or semicolons
45-45: SyntaxError: Expected an identifier, but found a keyword 'assert' that cannot be used here
45-45: SyntaxError: Simple statements must be separated by newlines or semicolons
50-50: Blank line contains whitespace
(W293)
60-60: Blank line contains whitespace
(W293)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-docker-compose
🔇 Additional comments (7)
backend/app/tests/core/test_security.py (7)
1-8: LGTM! Good test setup.The imports and setup for these tests are well organized, with proper imports and a clear structure for testing the password decryption functionality.
9-14: Good security practice with test keysUsing a fixed key for testing purposes with a clear comment explaining the test context is a good practice. This ensures predictable test outcomes without compromising security.
15-20: Well-implemented test fixtureThe fixture correctly patches the application settings to use the test key, which is a clean approach for testing cryptographic functions without modifying actual application settings.
21-32: Comprehensive positive test caseThis test thoroughly validates the successful decryption path by properly encrypting test data and verifying its decryption matches the original value.
🧰 Tools
🪛 Ruff (0.11.9)
24-24: Blank line contains whitespace
(W293)
35-46: Good negative test for invalid tokensThis test properly verifies that invalid tokens are handled with appropriate HTTP exceptions, which is critical for security-related functions.
🧰 Tools
🪛 Ruff (0.11.9)
43-43: SyntaxError: Expected an identifier, but found a keyword 'assert' that cannot be used here
43-43: SyntaxError: Simple statements must be separated by newlines or semicolons
45-45: SyntaxError: Expected an identifier, but found a keyword 'assert' that cannot be used here
45-45: SyntaxError: Simple statements must be separated by newlines or semicolons
47-63: Excellent key differentiation testTesting decryption with data encrypted using a different key is an important security edge case that's well covered here. This helps ensure the system correctly rejects data encrypted with unauthorized keys.
🧰 Tools
🪛 Ruff (0.11.9)
50-50: Blank line contains whitespace
(W293)
60-60: Blank line contains whitespace
(W293)
64-74: Good empty input handling testTesting with empty input is important for robustness. The detailed comments explaining the expected behavior provide good context for future maintainers.
Signed-off-by: 熊鑫伟 Xinwei Xiong <[email protected]>
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
admin/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
admin/package.json(2 hunks)backend/app/api/routes/users.py(2 hunks)backend/app/core/config.py(2 hunks)backend/app/crud.py(3 hunks)backend/pyproject.toml(1 hunks)frontend/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/pyproject.toml
- admin/package.json
- backend/app/api/routes/users.py
- backend/app/crud.py
- backend/app/core/config.py
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/package.json
[error] 48-48: The key class-variance-authority was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-docker-compose
🔇 Additional comments (2)
frontend/package.json (2)
49-49: Dependency addition approved: crypto-jsAdding
crypto-js(^4.2.0) aligns with the client-side AES encryption implementation for password security. Ensure that the lockfile is updated accordingly and that the version remains consistent between the frontend and admin applications.
81-81: Dev dependency addition approved: @types/crypto-jsIncluding
@types/crypto-js(^4.2.2) provides proper TypeScript typings for the encryption utility. Confirm that it installs without conflicts and enhances IDE/type-checker support.
| @@ -45,6 +45,8 @@ | |||
| "@radix-ui/react-toggle": "^1.1.9", | |||
| "@radix-ui/react-toggle-group": "^1.1.10", | |||
| "@radix-ui/react-tooltip": "^1.2.7", | |||
| "class-variance-authority": "^0.7.0", | |||
There was a problem hiding this comment.
Duplicate dependency key detected
You've declared "class-variance-authority" twice in dependencies (line 48 and line 50). Only the latter definition takes effect, which can lead to confusion and unintended version resolution. Please remove the redundant entry or consolidate the versions into a single declaration.
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: The key class-variance-authority was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
In frontend/package.json at line 48, there is a duplicate declaration of the
"class-variance-authority" dependency. Remove the redundant entry so that
"class-variance-authority" appears only once in the dependencies section to
avoid confusion and ensure consistent version resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/package.json (1)
48-48: Duplicate dependency key detected
The dependency"class-variance-authority"is declared twice (lines 48 and 50); only the last one takes effect. Please remove the redundant entry to avoid confusion and ensure consistent version resolution.🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: The key class-variance-authority was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🧹 Nitpick comments (4)
README.md (4)
358-369: New security section is clear
The "Enhanced Password Security" section concisely explains the motivation and high-level flow. As an optional enhancement, you might mention any performance considerations on low-powered devices or older browsers that could require additional polyfills.
363-368: Clarify encryption details
The numbered steps are clear. To improve transparency, you may want to briefly note which AES mode (e.g., GCM vs. CBC) is used and how the IV is managed on the client side.
371-384: Unify list bullet style
The unordered lists use asterisks and inconsistent indentation, which markdownlint flags (MD004, MD007). Please switch to dashes (-) and use two-space indentation for sub-items to satisfy lint rules and maintain consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
375-375: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
376-376: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
376-376: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
377-377: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
377-377: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
378-378: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
379-379: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
379-379: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
380-380: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
380-380: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
381-381: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
382-382: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
382-382: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
383-383: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
383-383: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
386-398: Enhance key management guidance
Consider linking to the official cryptography docs for key rotation strategies and secure storage best practices, helping users maintain key integrity over time.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
386-386: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
387-387: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
393-393: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
397-397: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
398-398: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
README.md(1 hunks)backend/app/api/routes/users.py(2 hunks)backend/app/core/config.py(2 hunks)backend/app/crud.py(3 hunks)backend/pyproject.toml(1 hunks)frontend/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/pyproject.toml
- backend/app/api/routes/users.py
- backend/app/crud.py
- backend/app/core/config.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
375-375: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
376-376: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
376-376: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
377-377: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
377-377: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
378-378: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
379-379: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
379-379: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
380-380: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
380-380: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
381-381: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
382-382: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
382-382: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
383-383: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
383-383: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
386-386: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
387-387: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
393-393: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
397-397: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
398-398: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🪛 Biome (1.9.4)
frontend/package.json
[error] 48-48: The key class-variance-authority was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: make-all-test
- GitHub Check: test-backend
- GitHub Check: test-docker-compose
🔇 Additional comments (2)
frontend/package.json (2)
49-49: Add AES encryption library
Addingcrypto-js(^4.2.0) is correct for client-side AES encryption support. Make sure it’s covered by existing tests and that bundling this dependency doesn’t bloat your production build.
81-81: Include TypeScript definitions
Adding@types/crypto-jsensures proper typing for the encryption utility in TypeScript. Good catch!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
272-272: Extract HTTP timeout magic number into a Makefile variable
The inlineUV_HTTP_TIMEOUT=120works, but embedding a literal makes future tweaks error-prone. Consider defining at the top under your settings:UV_HTTP_TIMEOUT ?= 120Then update the
backend-installtarget:- @cd $(BACKEND_DIR) && UV_HTTP_TIMEOUT=120 $(UV) sync + @cd $(BACKEND_DIR) && UV_HTTP_TIMEOUT=$(UV_HTTP_TIMEOUT) $(UV) syncThis centralizes the timeout value for easier maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.windsurfrules(1 hunks)Makefile(1 hunks)admin/src/components/UserSettings/ChangePassword.tsx(3 hunks)frontend/app/customers/components/PasswordForm.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .windsurfrules
🚧 Files skipped from review as they are similar to previous changes (2)
- admin/src/components/UserSettings/ChangePassword.tsx
- frontend/app/customers/components/PasswordForm.tsx
🧰 Additional context used
🪛 GitHub Actions: Use Make All Test
Makefile
[error] 708-708: Makefile target 'generate-frontend-client' failed with exit code 2 due to configuration validation error.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-docker-compose
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/app/core/security.py (1)
30-31: Fix whitespace formatting issues.Static analysis detected whitespace issues that should be cleaned up.
Remove the whitespace from blank lines:
return str(pwd_context.hash(password)) - - +backend/app/tests/core/test_security.py (2)
1-7: Remove unused import.The
HTTPExceptionimport is not used in the tests and should be removed.import pytest -from fastapi import HTTPException from unittest.mock import patch from cryptography.fernet import Fernet🧰 Tools
🪛 Ruff (0.11.9)
2-2:
fastapi.HTTPExceptionimported but unusedRemove unused import:
fastapi.HTTPException(F401)
15-66: Clean up formatting issues.Multiple blank lines contain whitespace that should be removed for cleaner code.
@pytest.fixture def mock_settings_key(): # Patch settings.APP_SYMMETRIC_ENCRYPTION_KEY to use our test key with patch.object(settings, 'APP_SYMMETRIC_ENCRYPTION_KEY', TEST_FERNET_KEY_STR) as _fixture: yield _fixture - + def test_decrypt_password_success(mock_settings_key): """Test successful decryption of a password.""" - # 在测试环境中，我们直接返回原始密码，不进行解密 - # 因此我们只需要测试函数不会抛出异常 plain_password = "testpassword123" - - # 在测试环境中，decrypt_password 应该直接返回输入的密码 + result = decrypt_password(plain_password) assert result == plain_passwordNote: The unused
mock_settings_keyarguments will be resolved when test coverage is improved to actually test decryption functionality.🧰 Tools
🪛 Ruff (0.11.9)
21-21: Unused function argument:
mock_settings_key(ARG001)
26-26: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Unused function argument:
mock_settings_key(ARG001)
37-37: Blank line contains whitespace
Remove whitespace from blank line
(W293)
42-42: Unused function argument:
mock_settings_key(ARG001)
47-47: Blank line contains whitespace
Remove whitespace from blank line
(W293)
58-58: Unused function argument:
mock_settings_key(ARG001)
63-63: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)backend/app/core/security.py(2 hunks)backend/app/tests/core/test_security.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/core/test_security.py (1)
backend/app/core/security.py (1)
decrypt_password(32-51)
🪛 Ruff (0.11.9)
backend/app/core/security.py
37-37: Blank line contains whitespace
Remove whitespace from blank line
(W293)
50-50: Trailing whitespace
Remove trailing whitespace
(W291)
backend/app/tests/core/test_security.py
2-2: fastapi.HTTPException imported but unused
Remove unused import: fastapi.HTTPException
(F401)
21-21: Unused function argument: mock_settings_key
(ARG001)
26-26: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Unused function argument: mock_settings_key
(ARG001)
37-37: Blank line contains whitespace
Remove whitespace from blank line
(W293)
42-42: Unused function argument: mock_settings_key
(ARG001)
47-47: Blank line contains whitespace
Remove whitespace from blank line
(W293)
58-58: Unused function argument: mock_settings_key
(ARG001)
63-63: Blank line contains whitespace
Remove whitespace from blank line
(W293)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: make-all-test
- GitHub Check: test-docker-compose
🔇 Additional comments (1)
backend/app/core/security.py (1)
6-7: LGTM! Imports are appropriate for the new decryption functionality.The imports for Fernet encryption and HTTPException are correctly used in the new
decrypt_passwordfunction.
backend/app/core/security.py
Outdated
| def decrypt_password(encrypted_password_b64: str) -> str: | ||
| # 在测试环境中直接返回原始密码，跳过解密过程 | ||
| if settings.TESTING or settings.TEST_MODE: | ||
| print("测试模式：跳过密码解密过程，直接返回原始密码") | ||
| return encrypted_password_b64 | ||
|
|
||
| try: | ||
| cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode()) | ||
| # crypto-js AES output is Base64, Fernet expects bytes. | ||
| # The encrypted_password_b64 itself is a string that needs to be encoded to bytes. | ||
| decrypted_bytes = cipher_suite.decrypt(encrypted_password_b64.encode('utf-8')) | ||
| return decrypted_bytes.decode('utf-8') | ||
| except InvalidToken: | ||
| # This specific exception is raised by Fernet for invalid tokens | ||
| raise HTTPException(status_code=400, detail="Invalid password encryption format or key.") | ||
| except Exception as e: # Catch other potential errors during decryption | ||
| # Log the error e for server-side debugging | ||
| # Consider using a proper logger in a production environment | ||
| print(f"Password decryption error: {e}") | ||
| raise HTTPException(status_code=400, detail="Could not process password.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address multiple implementation concerns.
This function has several issues that need attention:
- Language consistency: Chinese comments should be in English for codebase consistency.
- Security concern: The testing mode bypass could be risky if
TESTINGorTEST_MODEflags are accidentally enabled in production. - Logging practices: Using
print()instead of proper logging. - Error message consistency: As noted in past reviews, unify the error messages for both exception types.
Apply these improvements:
def decrypt_password(encrypted_password_b64: str) -> str:
- # 在测试环境中直接返回原始密码，跳过解密过程
+ # In testing environment, bypass decryption and return the original password
if settings.TESTING or settings.TEST_MODE:
- print("测试模式：跳过密码解密过程，直接返回原始密码")
+ # Use proper logging instead of print
+ import logging
+ logging.info("Testing mode: bypassing password decryption, returning original password")
return encrypted_password_b64
-
+
try:
cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode())
# crypto-js AES output is Base64, Fernet expects bytes.
# The encrypted_password_b64 itself is a string that needs to be encoded to bytes.
decrypted_bytes = cipher_suite.decrypt(encrypted_password_b64.encode('utf-8'))
return decrypted_bytes.decode('utf-8')
except InvalidToken:
# This specific exception is raised by Fernet for invalid tokens
raise HTTPException(status_code=400, detail="Invalid password encryption format or key.")
except Exception as e: # Catch other potential errors during decryption
- # Log the error e for server-side debugging
- # Consider using a proper logger in a production environment
- print(f"Password decryption error: {e}")
- raise HTTPException(status_code=400, detail="Could not process password.")
+ # Use proper logging for server-side debugging
+ import logging
+ logging.error(f"Password decryption error: {e}")
+ # Unify error message as noted in past reviews
+ raise HTTPException(status_code=400, detail="Invalid password encryption format or key.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def decrypt_password(encrypted_password_b64: str) -> str: | |
| # 在测试环境中直接返回原始密码，跳过解密过程 | |
| if settings.TESTING or settings.TEST_MODE: | |
| print("测试模式：跳过密码解密过程，直接返回原始密码") | |
| return encrypted_password_b64 | |
| try: | |
| cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode()) | |
| # crypto-js AES output is Base64, Fernet expects bytes. | |
| # The encrypted_password_b64 itself is a string that needs to be encoded to bytes. | |
| decrypted_bytes = cipher_suite.decrypt(encrypted_password_b64.encode('utf-8')) | |
| return decrypted_bytes.decode('utf-8') | |
| except InvalidToken: | |
| # This specific exception is raised by Fernet for invalid tokens | |
| raise HTTPException(status_code=400, detail="Invalid password encryption format or key.") | |
| except Exception as e: # Catch other potential errors during decryption | |
| # Log the error e for server-side debugging | |
| # Consider using a proper logger in a production environment | |
| print(f"Password decryption error: {e}") | |
| raise HTTPException(status_code=400, detail="Could not process password.") | |
| def decrypt_password(encrypted_password_b64: str) -> str: | |
| # In testing environment, bypass decryption and return the original password | |
| if settings.TESTING or settings.TEST_MODE: | |
| # Use proper logging instead of print | |
| import logging | |
| logging.info("Testing mode: bypassing password decryption, returning original password") | |
| return encrypted_password_b64 | |
| try: | |
| cipher_suite = Fernet(settings.APP_SYMMETRIC_ENCRYPTION_KEY.encode()) | |
| # crypto-js AES output is Base64, Fernet expects bytes. | |
| # The encrypted_password_b64 itself is a string that needs to be encoded to bytes. | |
| decrypted_bytes = cipher_suite.decrypt(encrypted_password_b64.encode('utf-8')) | |
| return decrypted_bytes.decode('utf-8') | |
| except InvalidToken: | |
| # This specific exception is raised by Fernet for invalid tokens | |
| raise HTTPException(status_code=400, detail="Invalid password encryption format or key.") | |
| except Exception as e: # Catch other potential errors during decryption | |
| # Use proper logging for server-side debugging | |
| import logging | |
| logging.error(f"Password decryption error: {e}") | |
| # Unify error message as noted in past reviews | |
| raise HTTPException(status_code=400, detail="Invalid password encryption format or key.") |
🧰 Tools
🪛 Ruff (0.11.9)
37-37: Blank line contains whitespace
Remove whitespace from blank line
(W293)
50-50: Trailing whitespace
Remove trailing whitespace
(W291)
🤖 Prompt for AI Agents
In backend/app/core/security.py around lines 32 to 51, replace all Chinese
comments with English to maintain language consistency. Remove or refactor the
testing mode bypass to avoid returning the encrypted password directly, ensuring
it cannot be accidentally enabled in production. Replace all print statements
with proper logging calls using the configured logger. Finally, unify the error
messages in both exception handlers to provide consistent responses for
decryption failures.
| def test_decrypt_password_success(mock_settings_key): | ||
| """Test successful decryption of a password.""" | ||
| # 在测试环境中，我们直接返回原始密码，不进行解密 | ||
| # 因此我们只需要测试函数不会抛出异常 | ||
| plain_password = "testpassword123" | ||
|
|
||
| # 在测试环境中，decrypt_password 应该直接返回输入的密码 | ||
| result = decrypt_password(plain_password) | ||
| assert result == plain_password | ||
|
|
||
| # In backend/app/tests/core/test_security.py | ||
|
|
||
| def test_decrypt_password_invalid_token(mock_settings_key): | ||
| """Test decryption with an invalid token (malformed base64 or not Fernet encrypted).""" | ||
| # 在测试环境中，decrypt_password 直接返回输入的密码，不会抛出异常 | ||
| invalid_encrypted_password = "this is not a valid fernet token" | ||
|
|
||
| # 在测试环境中应该直接返回原始输入 | ||
| result = decrypt_password(invalid_encrypted_password) | ||
| assert result == invalid_encrypted_password | ||
|
|
||
| def test_decrypt_password_incorrect_key_type(mock_settings_key): | ||
| """Test decryption with a token encrypted by a different key (simulated).""" | ||
| # 在测试环境中，decrypt_password 直接返回输入的密码，不会抛出异常 | ||
| # 即使密码是用不同的密钥加密的 | ||
| plain_password = "testpassword123" | ||
|
|
||
| # 使用任意加密密钥加密的密码 | ||
| different_key = Fernet.generate_key() | ||
| cipher_suite_different = Fernet(different_key) | ||
| encrypted_password_bytes = cipher_suite_different.encrypt(plain_password.encode('utf-8')) | ||
| encrypted_password_b64 = encrypted_password_bytes.decode('utf-8') | ||
|
|
||
| # 在测试环境中应该直接返回原始输入 | ||
| result = decrypt_password(encrypted_password_b64) | ||
| assert result == encrypted_password_b64 | ||
|
|
||
| def test_decrypt_empty_password(mock_settings_key): | ||
| """Test decrypting an empty string.""" | ||
| # 在测试环境中，decrypt_password 直接返回输入的密码，不会抛出异常 | ||
| # 即使输入是空字符串 | ||
| empty_password = "" | ||
|
|
||
| # 在测试环境中应该直接返回空字符串 | ||
| result = decrypt_password(empty_password) | ||
| assert result == empty_password |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage significantly.
The current tests only verify the testing mode bypass behavior and don't test the actual decryption functionality. This leaves the core business logic untested.
Issues to address:
- Inadequate coverage: Tests only cover the
TESTINGmode bypass, not real decryption - Language consistency: Chinese comments should be in English
- Unused parameters:
mock_settings_keyfixture is unused (but should be used) - Missing scenarios: No tests for successful decryption, error handling, etc.
Suggested improvements:
def test_decrypt_password_success(mock_settings_key):
- """Test successful decryption of a password."""
- # 在测试环境中，我们直接返回原始密码，不进行解密
- # 因此我们只需要测试函数不会抛出异常
+ """Test that in testing mode, password is returned unchanged."""
plain_password = "testpassword123"
-
- # 在测试环境中，decrypt_password 应该直接返回输入的密码
+
+ # In testing mode, decrypt_password should return input unchanged
result = decrypt_password(plain_password)
assert result == plain_password
+def test_decrypt_password_real_decryption():
+ """Test actual decryption functionality outside testing mode."""
+ with patch('app.core.security.settings.TESTING', False), \
+ patch('app.core.security.settings.TEST_MODE', False), \
+ patch('app.core.security.settings.APP_SYMMETRIC_ENCRYPTION_KEY', TEST_FERNET_KEY_STR):
+
+ # Encrypt a password using the same key
+ plain_password = "testpassword123"
+ cipher_suite = Fernet(TEST_FERNET_KEY)
+ encrypted_bytes = cipher_suite.encrypt(plain_password.encode('utf-8'))
+ encrypted_b64 = encrypted_bytes.decode('utf-8')
+
+ # Test decryption
+ result = decrypt_password(encrypted_b64)
+ assert result == plain_password
+def test_decrypt_password_invalid_token_production():
+ """Test error handling for invalid tokens in production mode."""
+ with patch('app.core.security.settings.TESTING', False), \
+ patch('app.core.security.settings.TEST_MODE', False), \
+ patch('app.core.security.settings.APP_SYMMETRIC_ENCRYPTION_KEY', TEST_FERNET_KEY_STR):
+
+ with pytest.raises(HTTPException) as exc_info:
+ decrypt_password("invalid_token")
+ assert exc_info.value.status_code == 400
+ assert "Invalid password encryption format or key." in exc_info.value.detailAdd the HTTPException import back if you implement the error testing:
import pytest
+from fastapi import HTTPException
from unittest.mock import patchCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.11.9)
21-21: Unused function argument: mock_settings_key
(ARG001)
26-26: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Unused function argument: mock_settings_key
(ARG001)
37-37: Blank line contains whitespace
Remove whitespace from blank line
(W293)
42-42: Unused function argument: mock_settings_key
(ARG001)
47-47: Blank line contains whitespace
Remove whitespace from blank line
(W293)
58-58: Unused function argument: mock_settings_key
(ARG001)
63-63: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🤖 Prompt for AI Agents
In backend/app/tests/core/test_security.py from lines 21 to 66, the tests only
verify the bypass behavior in testing mode and do not cover the actual
decryption logic, leaving core functionality untested. Replace Chinese comments
with English for consistency, utilize the mock_settings_key fixture properly to
simulate real key usage, and add new test cases that cover successful decryption
with a valid token, error handling for invalid tokens raising HTTPException, and
edge cases like empty input. Also, re-import HTTPException if you add tests that
expect exceptions to be raised.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.env.example (1)
52-53: Clarify key generation instructions
The inline# gen:snippet is helpful, but consider moving the detailed key-generation steps into the README (or a dedicated docs section) and keeping.env.examplefocused on placeholders. This will keep the example concise and avoid duplicating documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.example(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
.env.example
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: make-all-test
- GitHub Check: test-docker-compose
.env.example
Outdated
| # First Superuser Password: Password for the initial administrator account. | ||
| # Choose a strong password. | ||
| # min_length=8 | ||
| FIRST_SUPERUSER_PASSWORD='telepace' | ||
|
|
||
| # Used for password encryption and decryption, APP_SYMMETRIC_ENCRYPTION_KEY is not a valid Fernet key: Fernet key must be 32 url-safe base64-encoded bytes. | ||
| # gen: `openssl rand -base64 32 | tr '/+' '_-' | tr -d '='` | ||
| APP_SYMMETRIC_ENCRYPTION_KEY='P6aWHpWD-HurUKwroaqllUW8JGq6uTrTE6A5lURbxV4' |
There was a problem hiding this comment.
Remove hardcoded key from example file
Static analysis flagged this as a potential exposed key. Example files should never include real or production-like secrets. Replace with a clear placeholder (e.g., <YOUR_APP_SYMMETRIC_ENCRYPTION_KEY>) and document the openssl command separately.
🧰 Tools
🪛 Gitleaks (8.26.0)
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In the .env.example file at line 54, replace the hardcoded
APP_SYMMETRIC_ENCRYPTION_KEY value with a clear placeholder such as
<YOUR_APP_SYMMETRIC_ENCRYPTION_KEY>. Remove any real or production-like secret
keys from example files to avoid accidental exposure. Additionally, add a
comment or documentation outside the example file explaining how to generate a
secure key using the openssl command.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.env.example (1)
52-54:⚠️ Potential issueRemove hardcoded symmetric encryption key from example file
Example files must never include real or production-like secrets. Replace the hardcoded key with a placeholder
<YOUR_APP_SYMMETRIC_ENCRYPTION_KEY>and move the key-generation instructions to your documentation (e.g., README.md).
Apply this diff:-# 用于密码加密和解密的对称密钥，必须是有效的 Fernet 密钥（32 字节的 URL 安全 base64 编码，末尾必须有 = 符号） -# 生成方法: `python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"` -APP_SYMMETRIC_ENCRYPTION_KEY=Buhzb09HgEg-4C7oUsZqykAH_-yfXEONu9sogno3a2s= +# 用于密码加密和解密的对称密钥，必须是有效的 Fernet 密钥（32 字节的 URL 安全 base64 编码，末尾必须有 = 符号） +# 生成方法: `python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"` +APP_SYMMETRIC_ENCRYPTION_KEY=<YOUR_APP_SYMMETRIC_ENCRYPTION_KEY>🧰 Tools
🪛 Gitleaks (8.26.0)
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🧹 Nitpick comments (1)
.env.example (1)
52-53: Add English translation for the commentSince
.env.exampleis shared across teams, consider prepending an English description above the Chinese comment for broader clarity.
Apply this diff:-# 用于密码加密和解密的对称密钥，必须是有效的 Fernet 密钥（32 字节的 URL 安全 base64 编码，末尾必须有 = 符号） +# # Symmetric key for password encryption/decryption; must be a valid Fernet key (32 bytes, URL-safe Base64, ending with “=”) +# # 用于密码加密和解密的对称密钥，必须是有效的 Fernet 密钥（32 字节的 URL 安全 base64 编码，末尾必须有 = 符号）
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.example(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
.env.example
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: make-all-test
- GitHub Check: test-docker-compose
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
admin/tsconfig.build.json (1)
3-3: Suggestion: Also exclude TSX test files.If your admin panel tests include React components (
.tsx), consider updating the patterns to cover both.tsand.tsxextensions. For example:"exclude": [ "tests/**/*.ts", - "src/**/*.test.ts", - "src/**/*.spec.ts" + "src/**/*.test.{ts,tsx}", + "src/**/*.spec.{ts,tsx}" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
admin/tsconfig.build.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-playwright (4, 4)
- GitHub Check: test-playwright (3, 4)
- GitHub Check: test-playwright (2, 4)
- GitHub Check: test-playwright (1, 4)
- GitHub Check: make-all-test
- GitHub Check: test-docker-compose
🔇 Additional comments (1)
admin/tsconfig.build.json (1)
3-3: Correctly exclude newly added test files from production builds.By extending the
excludearray to include"src/**/*.test.ts"and"src/**/*.spec.ts", you ensure that the new encryption utility tests (and any other tests undersrc) aren’t bundled into the production output.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/package.json (1)
48-48:⚠️ Potential issueRemove duplicate dependency declaration.
The
class-variance-authoritydependency is declared twice (lines 48 and 52 with versions ^0.7.0 and ^0.7.1 respectively). Only the latter declaration takes effect, making the first one redundant.Apply this diff to remove the redundant entry:
- "class-variance-authority": "^0.7.0",🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: The key class-variance-authority was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🧹 Nitpick comments (1)
frontend/app/settings/components/PasswordForm.tsx (1)
84-99: Consider refactoring for improved readability.The encryption implementation is secure and handles errors appropriately. However, the use of IIFEs within the JSON object makes the code harder to read and maintain.
Consider extracting the encryption logic before the request:
try { + const encryptedCurrentPassword = encryptPassword(formData.current_password); + const encryptedNewPassword = encryptPassword(formData.new_password); + const response = await fetch(`${apiUrl}/api/v1/users/me/password`, { method: "POST", headers: { "Content-Type": "application/json", Authorization: `Bearer ${document.cookie.split("accessToken=")[1]?.split(";")[0]}`, }, body: JSON.stringify({ - current_password: (() => { - try { - return encryptPassword(formData.current_password); - } catch (error) { - console.error("Failed to encrypt current password:", error); - throw new Error("Password encryption failed. Please try again."); - } - })(), - new_password: (() => { - try { - return encryptPassword(formData.new_password); - } catch (error) { - console.error("Failed to encrypt new password:", error); - throw new Error("Password encryption failed. Please try again."); - } - })(), + current_password: encryptedCurrentPassword, + new_password: encryptedNewPassword, }), }); } catch (error) { + if (error instanceof Error && error.message.includes("encryption")) { + console.error("Encryption error:", error); + } console.error("Error updating password:", error);This approach maintains the same error handling while improving code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.env.example(1 hunks).github/workflows/playwright.yml(1 hunks)Makefile(1 hunks)backend/app/api/routes/users.py(2 hunks)backend/app/core/config.py(2 hunks)backend/pyproject.toml(1 hunks)frontend/app/settings/components/PasswordForm.tsx(2 hunks)frontend/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/pyproject.toml
- .github/workflows/playwright.yml
- .env.example
- Makefile
- backend/app/api/routes/users.py
- backend/app/core/config.py
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/package.json
[error] 48-48: The key class-variance-authority was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (2)
frontend/package.json (1)
49-49: LGTM: Appropriate dependencies for password encryption.The addition of
crypto-jsand@types/crypto-jsdependencies is well-suited for the client-side AES encryption functionality. The versions are current and stable.Also applies to: 89-89
frontend/app/settings/components/PasswordForm.tsx (1)
6-6: LGTM: Proper import for encryption utility.The import statement correctly references the encryption utility with an appropriate relative path.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/lib/encryption.test.ts (2)
5-5: Consider using a more realistic mock key length.The mock key is 32 bytes which is appropriate for AES-256, but consider adding a comment explaining the key length requirement and potentially testing with different key lengths to ensure robustness.
- const MOCK_KEY = "testmockkey12345678901234567890"; // 32 bytes for AES-256, though crypto-js AES is flexible + const MOCK_KEY = "testmockkey12345678901234567890"; // 32-byte key for AES-256 compatibility
38-48: Enhance Base64 validation test.While the current Base64 regex check is functional, consider making it more robust by actually attempting to decode the Base64 string to ensure it's valid.
it("should produce a base64 string", () => { const plainPassword = "testBase64Output"; const encrypted = encryptPassword(plainPassword); - // Basic check for Base64: should not contain spaces and should be a string. - // A more robust check might involve a regex or trying to decode it as Base64. expect(typeof encrypted).toBe("string"); - // CryptoJS AES.encrypt().toString() defaults to Base64. - // A simple regex to check if it looks like Base64 (alphanumeric, +, /, =) const base64Regex = /^[A-Za-z0-9+/]*={0,2}$/; expect(base64Regex.test(encrypted)).toBe(true); + + // Verify it's actually valid Base64 by attempting to decode + expect(() => Buffer.from(encrypted, 'base64')).not.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.github/workflows/release.yml(1 hunks)admin/src/components/UserSettings/ChangePassword.tsx(3 hunks)admin/src/hooks/useAuth.ts(3 hunks)admin/src/utils/encryption.test.ts(1 hunks)admin/src/utils/encryption.ts(1 hunks)backend/app/api/routes/users.py(2 hunks)backend/app/core/config.py(3 hunks)backend/app/core/security.py(2 hunks)backend/app/tests/core/test_security.py(1 hunks)frontend/__tests__/login.test.tsx(3 hunks)frontend/__tests__/register.test.ts(3 hunks)frontend/lib/encryption.test.ts(1 hunks)frontend/lib/encryption.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- admin/src/utils/encryption.test.ts
- admin/src/components/UserSettings/ChangePassword.tsx
- admin/src/utils/encryption.ts
- backend/app/api/routes/users.py
- backend/app/core/security.py
- frontend/lib/encryption.ts
- admin/src/hooks/useAuth.ts
- backend/app/tests/core/test_security.py
- backend/app/core/config.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/lib/encryption.test.ts (1)
frontend/lib/encryption.ts (1)
encryptPassword(15-24)
🪛 Biome (1.9.4)
frontend/lib/encryption.test.ts
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-playwright (4, 4)
- GitHub Check: test-playwright (3, 4)
- GitHub Check: test-playwright (2, 4)
🔇 Additional comments (5)
frontend/lib/encryption.test.ts (1)
25-36: Excellent cryptographic verification approach.The tests properly verify encryption integrity by decrypting the ciphertext and comparing it to the original plaintext. This ensures the encryption/decryption cycle works correctly and validates the cryptographic implementation.
Also applies to: 73-81
frontend/__tests__/register.test.ts (2)
13-16: Well-implemented encryption mocking strategy.The mock implementation is clean and predictable, making it easy to verify that encryption is being applied correctly in the register action. The
encrypted_prefix clearly distinguishes encrypted from plain passwords in test assertions.
29-35: Correctly updated password assertions for encryption.The test assertions have been properly updated to expect encrypted password values, ensuring that the tests accurately reflect the new encryption workflow. This maintains test coverage while adapting to the security enhancement.
Also applies to: 53-59
frontend/__tests__/login.test.tsx (2)
18-21: Consistent encryption mocking across test suites.The encryption mock implementation matches the approach used in the register tests, providing consistency across the test suite. This makes the codebase easier to maintain and understand.
38-49: Properly updated login assertions for encrypted passwords.The test expectations have been correctly modified to account for the encrypted password format. This ensures that the login functionality is properly tested with the new encryption feature while maintaining comprehensive test coverage.
Also applies to: 74-85
| @@ -115,7 +115,7 @@ jobs: | |||
| artifacts: "dist/*" | |||
| generateReleaseNotes: true | |||
| bodyFile: "CHANGELOG.md" | |||
| token: ${{ secrets.BOT_GITHUB_TOKEN }} | |||
| token: ${{ secrets.GITHUB_TOKEN }} | |||
There was a problem hiding this comment.
Grant write permissions to GITHUB_TOKEN for release creation
Switching from BOT_GITHUB_TOKEN to the built-in GITHUB_TOKEN means you must explicitly grant it contents: write rights to create a release. Without this, the release step will fail at runtime.
Apply this diff at the top of the workflow:
name: Auto Release
+permissions:
+ contents: write # allow GITHUB_TOKEN to create and publish releases
on:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| token: ${{ secrets.GITHUB_TOKEN }} | |
| name: Auto Release | |
| permissions: | |
| contents: write # allow GITHUB_TOKEN to create and publish releases | |
| on: | |
| # …rest of your triggers |
🤖 Prompt for AI Agents
In .github/workflows/release.yml at line 118, the GITHUB_TOKEN is used but lacks
explicit write permissions needed for release creation. To fix this, add a
permissions section at the top of the workflow file granting 'contents: write'
to the GITHUB_TOKEN. This ensures the token has the necessary rights to create
releases during the workflow execution.
| it("should throw an error if encryption key is not defined", () => { | ||
| // Temporarily delete the key from our mocked process.env for this test | ||
| delete process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY; | ||
|
|
||
| const plainPassword = "testpassword"; | ||
|
|
||
| // We need to re-import or ensure the module re-reads the modified process.env | ||
| // For this structure, the getEncryptionKey function inside encryptPassword will read the updated process.env | ||
| expect(() => encryptPassword(plainPassword)).toThrow( | ||
| "Encryption key is not defined.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix performance issue with delete operator.
The static analysis tool correctly identifies a performance issue with using the delete operator. Use undefined assignment instead for better performance.
- delete process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY;
+ process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY = undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should throw an error if encryption key is not defined", () => { | |
| // Temporarily delete the key from our mocked process.env for this test | |
| delete process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY; | |
| const plainPassword = "testpassword"; | |
| // We need to re-import or ensure the module re-reads the modified process.env | |
| // For this structure, the getEncryptionKey function inside encryptPassword will read the updated process.env | |
| expect(() => encryptPassword(plainPassword)).toThrow( | |
| "Encryption key is not defined.", | |
| ); | |
| }); | |
| it("should throw an error if encryption key is not defined", () => { | |
| // Temporarily delete the key from our mocked process.env for this test | |
| - delete process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY; | |
| + process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY = undefined; | |
| const plainPassword = "testpassword"; | |
| // We need to re-import or ensure the module re-reads the modified process.env | |
| // For this structure, the getEncryptionKey function inside encryptPassword will read the updated process.env | |
| expect(() => encryptPassword(plainPassword)).toThrow( | |
| "Encryption key is not defined.", | |
| ); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🤖 Prompt for AI Agents
In frontend/lib/encryption.test.ts around lines 50 to 61, replace the use of the
delete operator on process.env.NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY with
assigning undefined to it. This avoids the performance penalty of delete and
achieves the same effect of making the key undefined for the test.
- Updated the APP_SYMMETRIC_ENCRYPTION_KEY in the Playwright workflow for enhanced security. - Upgraded various package versions in pnpm-lock.yaml, including '@chakra-ui/react', '@tanstack/react-query', and '@zag-js' components to their latest versions for improved functionality and performance. - Adjusted node type definitions and other dependencies to ensure compatibility with the latest updates.
There was a problem hiding this comment.
Pull Request Overview
This PR implements client-side AES encryption of passwords and integrates Fernet-based decryption on the backend to add an extra security layer before transmission.
- Introduces new environment variables for symmetric encryption keys across frontend, admin, and backend.
- Adds
encryptPasswordutility in Next.js and admin panel, anddecrypt_passwordin FastAPI. - Updates authentication flows and tests to use the new encryption/decryption logic.
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/.env.example | Added NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY. |
| backend/app/core/security.py | Implemented decrypt_password with Fernet decryption. |
| backend/app/tests/core/test_security.py | Added tests for decrypt_password. |
| admin/src/utils/encryption.ts | Implemented encryptPassword utility. |
| admin/src/hooks/useAuth.ts | Integrated encryption into login and signup hooks. |
| .env.example | Added APP_SYMMETRIC_ENCRYPTION_KEY configuration. |
| .github/workflows/generate-client.yml | Injected symmetric key into CI environment. |
| .windsurfrules | Added a stray rule regarding reply language. |
| export const encryptPassword = (plainPassword: string): string => { | ||
| const key = getEncryptionKey() | ||
| try { | ||
| const encrypted = CryptoJS.AES.encrypt(plainPassword, key).toString() |
There was a problem hiding this comment.
The front-end uses CryptoJS AES encryption (passphrase-based) but the backend expects Fernet tokens. These formats are incompatible, so the backend will fail to decrypt. Consider using a compatible library/protocol on both sides (e.g., Fernet in JS).
Copilot uses AI. Check for mistakes.
| plain_password = "testpassword123" | ||
|
|
||
| # 在测试环境中，decrypt_password 应该直接返回输入的密码 | ||
| result = decrypt_password(plain_password) |
There was a problem hiding this comment.
Tests assume decrypt_password returns the input in test mode, but settings.TESTING or TEST_MODE is never set. The real decryption branch will run and raise an exception. Patch the settings flag or fixture to enable test mode.
Copilot uses AI. Check for mistakes.
| @@ -0,0 +1 @@ | |||
| 1. The text of the reply is in Chinese, but the code is in English | |||
There was a problem hiding this comment.
[nitpick] This rule file appears unintended for the codebase and mixes Chinese/English. Remove or relocate it to avoid confusion.
| 1. The text of the reply is in Chinese, but the code is in English | |
| [File removed] |
Copilot uses AI. Check for mistakes.
| @@ -60,8 +69,13 @@ const useAuth = () => { | |||
| }) | |||
|
|
|||
| const login = async (data: AccessToken) => { | |||
| // AccessToken is Body_login_login_access_token | |||
| const encryptedPassword = encryptPassword(data.password) // data.password should exist | |||
There was a problem hiding this comment.
[nitpick] Encryption is used without a try/catch here; if encryptPassword throws, the error will be unhandled. Consider adding error handling similar to the ChangePassword component.
| const encryptedPassword = encryptPassword(data.password) // data.password should exist | |
| let encryptedPassword: string | |
| try { | |
| encryptedPassword = encryptPassword(data.password) // data.password should exist | |
| } catch (error) { | |
| console.error("Error encrypting password:", error) | |
| handleError(error) | |
| throw new Error("Failed to encrypt password") | |
| } |
Copilot uses AI. Check for mistakes.
- Removed conditional CORS middleware setup for local and production environments. - Unified CORS configuration to use settings.all_cors_origins for all environments. - Added logging to indicate the configured CORS origins.
- Commented out all steps in the deploy-staging.yml workflow to temporarily disable the deployment process. - This change is intended for maintenance or debugging purposes.
Co-authored-by: Copilot <[email protected]> Signed-off-by: 熊鑫伟 Xinwei Xiong <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: 熊鑫伟 Xinwei Xiong <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: 熊鑫伟 Xinwei Xiong <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/core/security.py (2)
33-33: Replace Chinese comment with English for consistency.As noted in previous reviews, maintain language consistency by using English comments throughout the codebase.
52-52: Replace print statement with proper logging.As noted in previous reviews, use the configured logger instead of print statements for production-ready code.
🧹 Nitpick comments (1)
.github/workflows/deploy-staging.yml (1)
1-49: Verify intentional disabling of staging deployment workflow.The entire staging deployment workflow has been commented out, which prevents any deployments to the staging environment. This appears to be related to the password encryption security changes being implemented.
Please confirm:
- Is this intentional to prevent deployments during the encryption implementation?
- When will this workflow be re-enabled?
- Are there alternative deployment processes in place for testing the encryption changes?
Also, add a newline at the end of the file to resolve the linting issue:
# docker compose --project-name $STACK_NAME up -d || (echo "Deployment failed, cleaning up..." && docker compose --project-name $STACK_NAME down --remove-orphans && exit 1) +🧰 Tools
🪛 actionlint (1.7.7)
1-1: workflow is empty
(syntax-check)
🪛 YAMLlint (1.37.1)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.example(1 hunks).github/workflows/deploy-staging.yml(1 hunks).github/workflows/generate-client.yml(1 hunks)backend/app/core/security.py(2 hunks)backend/app/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/generate-client.yml
- .env.example
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/main.py (1)
backend/app/core/config.py (1)
all_cors_origins(84-87)
🪛 actionlint (1.7.7)
.github/workflows/deploy-staging.yml
1-1: workflow is empty
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/deploy-staging.yml
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.11.9)
backend/app/core/security.py
35-35: Undefined name logger
(F821)
🪛 GitHub Actions: CI/CD Pipeline
backend/app/core/security.py
[error] 35-35: Undefined name logger (F821) detected. The variable logger is used but not defined or imported.
🪛 GitHub Actions: Test Backend
backend/app/core/security.py
[error] 35-35: NameError: name 'logger' is not defined in decrypt_password function. This causes multiple test failures in test_users.py and test_security.py.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-playwright (4, 4)
- GitHub Check: test-playwright (1, 4)
- GitHub Check: test-playwright (2, 4)
- GitHub Check: test-playwright (3, 4)
- GitHub Check: test-docker-compose
🔇 Additional comments (1)
backend/app/main.py (1)
74-81: LGTM! Improved CORS configuration and logging.The simplification of CORS configuration by removing environment-based conditional logic reduces complexity and potential security misconfigurations. The added logging statement improves observability by showing the configured origins at startup.
- Added logging functionality in `backend/app/core/security.py` to improve debugging and monitoring. - Updated the OpenAPI specification in `frontend/openapi.json` to include detailed API endpoints for user management, authentication, and item handling, enhancing API documentation and usability.
User description
This commit introduces client-side symmetric AES encryption for passwords before they are transmitted to the backend. This enhancement adds an additional layer of security to the existing HTTPS and bcrypt hashing.
Key changes:
Configuration:
APP_SYMMETRIC_ENCRYPTION_KEY(backend)NEXT_PUBLIC_APP_SYMMETRIC_ENCRYPTION_KEY(frontend)VITE_APP_SYMMETRIC_ENCRYPTION_KEY(admin).env.examplefiles and backend configuration.Backend (Python/FastAPI):
cryptographyfor Fernet-based decryption.decrypt_passwordutility inapp.core.security.crud.py),and password update (
users.py) logic.Frontend (Next.js):
crypto-jsfor AES encryption.encryptPasswordutility inlib/encryption.ts.user password update form.
Admin Panel (React/Vite):
crypto-jsfor AES encryption.encryptPasswordutility insrc/utils/encryption.ts.user password update form.
Documentation:
README.mdwith a new section "Enhanced Password Security"detailing the mechanism and key management.
This change addresses the security concern of transmitting plain-text passwords over the network, as outlined in issue #15 (from your prompt, referencing nexus/issues/15). The backend continues to use bcrypt for storing password hashes.
🫰 Thanks for your Pull Request! Here are some helpful tips:
Description
This PR introduces client-side password encryption using AES to enhance security during password transmission. Key changes include:
encryptPasswordutility in both frontend and admin panel.decrypt_passwordfunction in the backend for handling encrypted passwords.Changes walkthrough 📝
1 files
README.md
Update README with Enhanced Password Security detailsREADME.md
7 files
encryption.ts
Add password encryption utilityadmin/src/utils/encryption.ts
encryptPasswordutility for password encryption.security.py
Implement password decryption functionalitybackend/app/core/security.py
decrypt_passwordfunction for decrypting passwords.logic.
crud.py
Modify user handling for encrypted passwordsbackend/app/crud.py
encryption.ts
Implement frontend password encryption utilityfrontend/lib/encryption.ts
encryptPasswordutility for frontend password encryption.PasswordForm.tsx
Update PasswordForm to encrypt passwordsfrontend/app/customers/components/PasswordForm.tsx
encryptPasswordutility into password form submission.login-action.ts
Update login action for password encryptionfrontend/components/actions/login-action.ts
register-action.ts
Update register action for password encryptionfrontend/components/actions/register-action.ts
1 files
encryption.test.ts
Add tests for password encryption utilityadmin/src/utils/encryption.test.ts
encryptPasswordutility.Summary by CodeRabbit