fix: remove base64 encoding #403
Conversation
WalkthroughThis pull request updates the way decrypted and zip file data are processed. In Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/api/upload.js(1 hunks)client/helpers/zip.js(2 hunks)client/util/constants.js(0 hunks)shared/helpers/crypto.js(2 hunks)
💤 Files with no reviewable changes (1)
- client/util/constants.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Scan the docker image
🔇 Additional comments (6)
client/helpers/zip.js (2)
1-1: Import from buffer moduleGood job adding the buffer import to replace the removed tweetnacl-util dependency.
36-36:❓ Verification inconclusive
Format change from base64 to hex
The function now returns data in hexadecimal format instead of base64. This change is consistent with the PR's goal to remove base64 encoding.
🏁 Script executed:
#!/bin/bash # Verify all callers of zipFiles are updated to handle hex format rg "zipFiles\\(" --type jsLength of output: 192
Hex Encoding Verified – Confirm Downstream Handling
The helper change inclient/helpers/zip.jsto return hexadecimal data is correct and aligns with the PR’s goal of replacing base64 encoding. The grep results show that the only call site besides the export is inclient/stores/secretStore.js. Please double-check that this caller (and any downstream logic) correctly processes the hex-encoded string, as opposed to the previous base64 format.shared/helpers/crypto.js (4)
19-19: Switch from Buffer to tweetnacl-util for key processingChanged from using Buffer.from to decodeUTF8 for key processing. This creates a more consistent approach by using tweetnacl-util's methods throughout the encryption process.
36-36: Switch from Buffer to tweetnacl-util for key processingSame change as in the encrypt function - using decodeUTF8 instead of Buffer.from for consistent key processing.
42-42: Bug fix: Using correct length propertyFixed a bug by using
messageWithNonceAsUint8Array.lengthinstead ofmessageWithNonce.length. This ensures the correct array is used for determining the slice length.
52-53:❓ Verification inconclusive
Verify return value format consistency
The decrypt function returns a UTF-8 encoded string with
encodeUTF8(decrypted), but in client/api/upload.js, this return value is being treated as a hex string withBuffer.from(decrypt(...), 'hex'). Please verify this is intentional and that the formats match.
🏁 Script executed:
#!/bin/bash # Looking at what decrypt returns and how it's used rg -A 2 "const base64DecryptedMessage" shared/helpers/crypto.js # Find places where decrypt output is consumed rg "decrypt\\(.*\\)" --type js --glob !shared/helpers/crypto.js -A 1Length of output: 832
Verify return value format for decrypt function usage
It appears that the
decryptfunction returns a UTF-8 encoded string usingencodeUTF8(decrypted)(see lines 52–53 inshared/helpers/crypto.js), but inclient/api/upload.jsthe output is interpreted as a hex string viaBuffer.from(decrypt(...), 'hex'). Please verify that this discrepancy is intentional. In particular:
- Confirm whether the decryption result should indeed be a UTF-8 string, in which case the client code in
client/api/upload.jsshould be updated to handle it appropriately.- Alternatively, if a hex string is expected, update the decryption logic in
shared/helpers/crypto.jsto return the appropriately formatted hex string.
|
Previously for larger files, the base64 encoding threw an error. This PR will fix that.
However, this will change the entire secret approach, meaning old secrets will no longer work.
Also, I am not sure about this approach. It tries to encrypt everything into memory in the browser, which is not ideal. 200mb files crashed on my MacBook Pro m2.
Streaming is the best solution, but this is a temp solution for most files.
Summary by CodeRabbit
Refactor
Chores