Implement end-to-end encryption for private chat messages#194
Implement end-to-end encryption for private chat messages#194vandan09 wants to merge 2 commits intoVault-Web:mainfrom
Conversation
…tion-for-chat Implement end-to-end encryption for private chat messages
There was a problem hiding this comment.
Pull request overview
This pull request implements end-to-end encryption (E2EE) for private chat messages in Vault Web. The implementation uses client-side AES-GCM encryption where messages are encrypted in the browser before being sent to the server, which stores and forwards only the encrypted payloads without decrypting them. Group messages continue to use the existing server-side encryption approach.
Changes:
- Added browser-side AES-GCM crypto service using Web Crypto API for encrypting/decrypting private chat messages
- Modified backend to accept and persist pre-encrypted payloads for private chats without server-side decryption
- Updated private chat history endpoint to return encrypted payloads instead of decrypted content
- Added test coverage for encrypted message saving and validation of encrypted payloads
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/environments/environment.ts | Added hardcoded base64-encoded encryption key for client-side encryption |
| frontend/src/app/services/chat-crypto.service.ts | New service implementing AES-GCM encryption/decryption with base64 encoding utilities |
| frontend/src/app/pages/private-chat-dialog/private-chat-dialog.component.ts | Updated to encrypt messages before sending and decrypt on receive/history load |
| frontend/src/app/models/dtos/ChatMessageDto.ts | Made content optional and added cipherText, iv, and senderId fields to support encrypted payloads |
| backend/src/main/java/vaultWeb/dtos/ChatMessageDto.java | Removed @NotBlank validation on content, added cipherText and iv fields for encrypted payloads |
| backend/src/main/java/vaultWeb/services/ChatService.java | Added contentAlreadyEncrypted parameter to saveMessage method to bypass server-side encryption for pre-encrypted messages |
| backend/src/main/java/vaultWeb/controllers/PrivateChatController.java | Modified to return encrypted payloads instead of decrypting messages server-side |
| backend/src/main/java/vaultWeb/controllers/ChatController.java | Updated to accept encrypted payloads for private messages and forward without decryption |
| backend/src/test/java/vaultWeb/services/ChatServiceTest.java | Added tests for saving encrypted private messages and validating missing encrypted payloads |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public ChatMessage saveMessage(ChatMessageDto dto) { | ||
| return saveMessage(dto, false); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the single-parameter saveMessage method overload. While the overload at line 137 simply delegates to the two-parameter version with false, it would be good practice to have at least one test that verifies this delegation works correctly. This ensures the default behavior (server-side encryption) is explicitly tested and documented.
| this.privateMessageSub = this.wsService | ||
| .subscribeToPrivateMessages() | ||
| .subscribe((msg) => { | ||
| .subscribe(async (msg) => { | ||
| if (msg.privateChatId === this.privateChatId) { | ||
| this.messages.push(msg); | ||
| this.messages.push(await this.decryptMessage(msg)); | ||
| this.shouldScroll = true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The subscribe callback is async but doesn't handle errors properly. If the decryptMessage promise rejects, the error will be unhandled and could break the subscription. Additionally, there's a potential race condition: if messages arrive rapidly, they might be decrypted and pushed to the array out of order. Consider using RxJS operators like concatMap or handling errors within the async callback to ensure reliable message ordering and error handling.
| .subscribe(async (msg) => { | ||
| if (msg.privateChatId === this.privateChatId) { | ||
| this.messages.push(msg); | ||
| this.messages.push(await this.decryptMessage(msg)); |
There was a problem hiding this comment.
Missing error handling for decryption failures when receiving real-time messages. If a newly received message fails to decrypt, the async operation will throw an error that is not caught, potentially breaking the WebSocket subscription. Consider wrapping the decryptMessage call in a try-catch block to handle individual message decryption failures gracefully.
| EncryptionUtil.EncryptResult encrypted; | ||
| try { | ||
| encrypted = encryptionUtil.encrypt(dto.getContent()); |
There was a problem hiding this comment.
Missing null and blank validation for content field when contentAlreadyEncrypted is false. If dto.getContent() returns null or blank, the encryptionUtil.encrypt() call may fail or produce unexpected results. Add validation to check if content is null or blank before attempting encryption, similar to the validation done for encrypted payloads on lines 88-90.
| EncryptionUtil.EncryptResult encrypted; | |
| try { | |
| encrypted = encryptionUtil.encrypt(dto.getContent()); | |
| String content = dto.getContent(); | |
| if (content == null || content.isBlank()) { | |
| throw new EncryptionFailedException("Missing content to encrypt"); | |
| } | |
| EncryptionUtil.EncryptResult encrypted; | |
| try { | |
| encrypted = encryptionUtil.encrypt(content); |
| if (!message.cipherText || !message.iv) { | ||
| return message; | ||
| } | ||
|
|
||
| return { | ||
| ...message, | ||
| content: await this.chatCryptoService.decrypt(message.cipherText, message.iv), | ||
| }; |
There was a problem hiding this comment.
No backward compatibility handling for existing unencrypted private messages. If there are any private chat messages in the database from before this change that were encrypted with the server-side key, they will not be decryptable by clients using the new client-side key. The decryptMessage method silently returns the original message if cipherText or iv is missing, but if old messages have cipherText/iv encrypted with a different key, decryption will fail without any indication to users. Consider adding a migration strategy or version indicator to distinguish between old and new encryption schemes.
| if (!message.cipherText || !message.iv) { | |
| return message; | |
| } | |
| return { | |
| ...message, | |
| content: await this.chatCryptoService.decrypt(message.cipherText, message.iv), | |
| }; | |
| // Backward compatibility: | |
| // If there is no cipherText/iv, assume this is an old or unencrypted message | |
| // and return it as-is (content may already be present). | |
| if (!message.cipherText || !message.iv) { | |
| return message; | |
| } | |
| try { | |
| const decryptedContent = await this.chatCryptoService.decrypt( | |
| message.cipherText, | |
| message.iv, | |
| ); | |
| return { | |
| ...message, | |
| content: decryptedContent, | |
| }; | |
| } catch (err) { | |
| // If decryption fails (e.g. message encrypted with an old scheme or key), | |
| // log the error and return a message with an explicit indication instead | |
| // of failing silently. | |
| console.warn('Failed to decrypt private chat message:', err); | |
| return { | |
| ...message, | |
| content: '[Unable to decrypt this message]', | |
| }; | |
| } |
| private uint8ToBase64(bytes: Uint8Array): string { | ||
| let binary = ''; | ||
| bytes.forEach((b) => (binary += String.fromCharCode(b))); | ||
| return btoa(binary); | ||
| } | ||
|
|
||
| private base64ToUint8(base64: string): Uint8Array { | ||
| const binary = atob(base64); | ||
| return Uint8Array.from(binary, (char) => char.charCodeAt(0)); | ||
| } |
There was a problem hiding this comment.
The base64 encoding implementation using String.fromCharCode and btoa/atob may not handle all Unicode characters correctly. While this works for binary data like encrypted ciphertext and IVs, be aware that btoa/atob are designed for ASCII strings. For the current use case (encoding binary crypto data), this is acceptable, but if the implementation ever needs to handle arbitrary Unicode text, consider using TextEncoder/TextDecoder or a more robust base64 library. The current implementation is safe for the encryption payloads.
| private readonly encoder = new TextEncoder(); | ||
| private readonly decoder = new TextDecoder(); | ||
| private readonly keyPromise = this.importKey(); | ||
|
|
There was a problem hiding this comment.
The IV generation uses crypto.getRandomValues with 12 bytes (96 bits), which is the recommended size for AES-GCM. However, there's a security consideration: with a shared key (as currently implemented), after approximately 2^32 encryptions there's a risk of IV collision. For typical usage this is unlikely to be a problem, but if this system scales to millions of messages, consider implementing IV collision detection or switching to per-user or per-conversation keys. For the current implementation with a shared key, this is acceptable but should be documented as a scalability limitation.
| /** | |
| * Encrypts the given content using a single, shared AES-GCM key. | |
| * | |
| * IV generation uses 12 random bytes (96 bits), which is the recommended size for AES-GCM. | |
| * With a shared, long-lived key, there is a non-negligible risk of IV collision after | |
| * approximately 2^32 encryptions. For typical usage this is acceptable, but if this service | |
| * is scaled to very large numbers of messages, consider: | |
| * - rotating the key more frequently, or | |
| * - using per-user or per-conversation keys, or | |
| * - adding IV management on the backend. | |
| * | |
| * This limitation should be taken into account when assessing the system's scalability and | |
| * security properties. | |
| */ |
|
|
||
| String decryptedContent = | ||
| chatService.decrypt(savedMessage.getCipherText(), savedMessage.getIv()); | ||
| public void sendPrivateMessage(@Payload ChatMessageDto messageDto) { |
There was a problem hiding this comment.
The removal of the @Valid annotation from the @Payload parameter means Spring will no longer validate ChatMessageDto fields. Since the @NotBlank constraint was removed from the content field in ChatMessageDto, this change is intentional for encrypted messages. However, consider whether any other validation constraints exist on ChatMessageDto that should still be enforced, or if custom validation logic is needed in the controller to verify required fields based on the message type.
| /** | ||
| * Saves a chat message to a group or private chat. | ||
| * | ||
| * <p>The message content is encrypted before being persisted. The sender is identified either by | ||
| * ID or username. If a timestamp is not provided, the current time is used. The message must | ||
| * belong to either a group or a private chat. | ||
| * | ||
| * @param dto DTO containing the message content, sender information, timestamp, and either a | ||
| * groupId or privateChatId. | ||
| * @return The persisted ChatMessage entity with encrypted content. | ||
| * @throws UserNotFoundException if the sender cannot be found by ID or username. | ||
| * @throws GroupNotFoundException if neither groupId nor privateChatId is provided, or if the | ||
| * specified group/private chat does not exist. | ||
| * @throws EncryptionFailedException if encryption fails. | ||
| */ |
There was a problem hiding this comment.
The Javadoc comment still references "message content" which is outdated. Since the new parameter is contentAlreadyEncrypted and the method now supports both encrypted and plaintext content depending on the boolean flag, update the documentation to clarify this dual behavior. Specifically, explain when content vs cipherText/iv should be provided, and how the contentAlreadyEncrypted parameter affects the processing.
| export class ChatCryptoService { | ||
| private readonly encoder = new TextEncoder(); | ||
| private readonly decoder = new TextDecoder(); | ||
| private readonly keyPromise = this.importKey(); |
There was a problem hiding this comment.
The keyPromise field is initialized once and reused across all encryption/decryption operations. If the key import fails (e.g., due to invalid base64 or incorrect key length), the promise will be rejected. All subsequent encrypt/decrypt operations will then fail with the same error. Consider adding error handling during initialization or providing a more informative error message when the key import fails, so users understand the root cause rather than seeing generic crypto errors on every operation.
|
@vandan09 I am currently busy with university and other commitments. However, I have already started working on E2EE in #164. Since this involves security-critical functionality, I want to proceed carefully and take additional time for both the implementation and the review process. Due to the current workload and stress, it may take up to a month before you hear from me again regarding E2EE. Meanwhile you can checkout my pr and search for differences and improvements in both prs. |
Summary
Private-chat WebSocket handling now persists and forwards encrypted payloads (cipherText + iv) without decrypting on the server.
Private-chat history endpoint now returns encrypted payload fields so clients decrypt locally.
Added browser-side AES-GCM crypto service and wired private chat UI to encrypt before send and decrypt on receive/history load.
Added backend tests for encrypted-save path and missing encrypted payload validation.
Linked issue
Closes #98
How to test
Frontend build:
cd frontend && npm run build
Backend unit tests:
cd backend && ./mvnw -Dtest=ChatServiceTest test (if Maven download is available in your environment)
Notes / Risk
Encryption key is currently configured via frontend environment (chatEncryptionKeyBase64), which is functional for local/dev but should be managed securely per environment for production.
This change shifts trust/decryption to clients; server no longer inspects private-chat plaintext during send/history retrieval.
Commands used to prepare this: git show --name-only --oneline HEAD, plus nl -ba ... | sed -n ... on the modified files.