-
Notifications
You must be signed in to change notification settings - Fork 5.4k
fix: encryption for character secrets in correct order #6217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rect order - Added functionality to encrypt both settings.secrets and root-level secrets when agents are added or updated. - Updated tests to verify encryption and decryption of secrets, including handling of empty and mixed-type secret values. - Ensured that non-string secret values are preserved during the encryption process.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a critical security bug where environment variables merged into character secrets were not being encrypted. The fix ensures that secrets are encrypted in the correct order: first merge environment variables, then encrypt all secrets.
Key Changes
- Encryption order corrected: Secrets are now encrypted AFTER environment variables are merged, ensuring all sensitive data (including env vars) is properly encrypted
- Support for root-level secrets: Added encryption handling for
character.secretsin addition tocharacter.settings.secrets - Removed premature encryption: Removed the
encryptedCharacter()wrapper call inserver/index.tsthat was encrypting secrets before env var merging
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/server/src/index.ts |
Removed premature encryptedCharacter() call and import, allowing encryption to happen at the correct time in elizaos.ts |
packages/server/src/api/agents/crud.ts |
Added encryption for both character.secrets and character.settings.secrets in POST and PATCH handlers; moved salt declaration outside conditional blocks for reuse |
packages/core/src/elizaos.ts |
Added encryption logic after environment variable merging in addAgents() to ensure all secrets including env vars are encrypted |
packages/core/src/__tests__/elizaos.test.ts |
Added comprehensive test suite covering encryption of settings.secrets, root-level secrets, edge cases, and encryption format validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @standujar's task —— View job PR Review: Encryption for Character SecretsTodo List:
Code Review SummaryThis PR implements encryption for character secrets at multiple levels (root and settings.secrets) with comprehensive test coverage and proper TypeScript handling. ✅ PASSED: Test coverage is excellent with 209 new tests covering all encryption scenarios 💡 SUGGESTION: TypeScript strictness → Consider: Remove type assertions in server/index.ts:273-290 for better type safety The implementation is solid and follows project conventions. The encryption logic properly handles both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
No description provided.