fix: outgoing messages should not be decrypted #28
Conversation
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
Once implemented, both conversation hinting and headerEncryption should result in handleFrame/decrypt never being called on outgoing messages.
However this check is still a good idea to be safe, as well as logging programErrors.
There are a few small fixups, but think this is a good change.
Comments us the feedback ladder defined here: https://www.netlify.com/blog/2020/03/05/feedback-ladders-how-we-encode-code-reviews-at-netlify/
| if convo.doubleratchet.dhSelf.public == header.dhPublic: | ||
| info "outgoing message, no need to decrypt" | ||
| return err(ChatError(code: errDecryptOutgoing, context: "Attempted to decrypt outgoing message")) | ||
|
|
There was a problem hiding this comment.
[Pebble] With this change, handleFrame will fail with a decryption error when an outgoing message is parsed. This doesn't seem like what we want.
Decrypting outgoing messages is a state that should never occur(ProgramError), rather than an DecryptionError.
I'd suggest that errDecryptOutgoing ought to be caught in handleFrame; which then logs and returns early. At that point frame has been successfully parsed, as there is no work to do.
Alternatively the check could be performed in handleFrame prior to decryption. This way decrypt is only ever called on ciphertexts that it should be able to handle successfully.
src/naxolotl/naxolotl.nim
Outdated
|
|
||
| type Doubleratchet* = object | ||
| dhSelf: PrivateKey | ||
| dhSelf*: PrivateKey |
There was a problem hiding this comment.
[Boulder] Exposing the PrivateKey outside of the module is risky here. Only the publicKey is needed, consider making a function to access the publicKey.
Do not process outgoing messages for
handleFrame.This is happening because waku does not de-duplicate the messages which are sending out when listening a topic.
For now we return an error, but we should have a better way to resolve it when more information exposed in the header in the future.