Clipped decrypted large messages #4841
Conversation
martgil
changed the title
|
@rrrooommmaaa ready for review. thank you! |
| let renderableAttachments: Attachment[] = []; | ||
| let decryptedContent = decryptedBytes.toUtfStr(); | ||
| let isHtml: boolean = false; | ||
| // todo - replace with MsgBlockParser.fmtDecryptedAsSanitizedHtmlBlocks, then the extract/strip methods could be private? | ||
| if (decryptedContent.length > maxDecryptedContentLength) { | ||
| decryptedContent = decryptedContent.substring(0, maxDecryptedContentLength) + ' [clipped - message too large]'; |
There was a problem hiding this comment.
ok, if the decrypted content doesn't resemble Mime message, then this decryptedContent goes into line 234 etc.
But if it does, then it goes to line 243 and then it's not clipped (restored from the non-clipped decryptedBytes).
Won't the browser extension crash if it is too long there?
And also a question: should we clip the message before or after extracting FcAttachments, replyToken and public keys. If after (which is preferred unless it consumes a lot of processing time), then we should also think about limiting the number of attachments and public keys, as too many may also crash the frame or make it otherwise unusable. If before, the output may look a bit ugly, if a pubkey or attachment link is clipped midway.
There was a problem hiding this comment.
@rrrooommmaaa thank you sir for your helpful comments.
ok, if the decrypted content doesn't resemble Mime message, then this decryptedContent goes into line 234 etc.
But if it does, then it goes to line 243 and then it's not clipped (restored from the non-clipped decryptedBytes).
Won't the browser extension crash if it is too long there?
thank you. right it's a mistake. things should be clipped in both situation (mime/non-mime).
And also a question: should we clip the message before or after extracting FcAttachments, replyToken and public keys. If after (which is preferred unless it consumes a lot of processing time), then we should also think about limiting the number of attachments and public keys, as too many may also crash the frame or make it otherwise unusable. If before, the output may look a bit ugly, if a pubkey or attachment link is clipped midway.
Thank you for the question - this gives me more realization about the situation.
- We're dealing with inline attachment from web portal so that is one problem when it comes to limiting attachments.
- If we disregard the item no.1 (dealing with inline attachment) - I think we don't need to worry about attachments through secure message as it has 25mb? For encrypted mime messages, I think that's something we need to limit.
- I think it's reasonable to add a limit for number of public keys as we displayed their details on the page (we had check before rendering that it has to be a valid public key).
For now, do you think we could add limit text messages from decrypted data and then discuss about attachments, inline attachments, and pubkey limits later?
There was a problem hiding this comment.
@martgil were there any issues with this PR or something else should be implemented before we can finish this PR?
@sosnovsky No blocking issues. May I hear your suggestion for the following question above? I might just re-create this PR afterward. Thanks.
There was a problem hiding this comment.
I think here we should clip text in decrypted data after extracting all attachments, so it'll affect only rendered message text:
- decrypt full data
- extract all attachments
- if message text is longer than max length - clip it
- for attachments and public keys need to check how they affect page performance and then set MAX_ATTACHMENTS_NUMBER - maybe it'll work well even for 50 attachments
There was a problem hiding this comment.
I'd also recommend to implement creation of a dummy message (for both MIME and non-MIME encrypted data) of an arbitrary long size (can be later adjusted) on the fly, so that we can test better how the extension reacts to extremely big messages and where exactly it hangs or crashes. We'll have to customize our mock GoogleData.getMessage() to provide a message of the required size based on id.
You're right, we should be testing mime and non-mime messages though both will crash for extremely long messages. I'm still unsure how we could test for the right length before it crashes. I mean, let's assume that rendering 10 million chars (encrypted) will work on the better machines but that 10 million chars may be too much to handle for a dual-core PC or less (though it's acceptable as it's a comprised limitation of it). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let renderableAttachments: Attachment[] = []; | ||
| let decryptedContent: string | undefined; | ||
| let isHtml = false; | ||
| // todo - replace with MsgBlockParser.fmtDecryptedAsSanitizedHtmlBlocks, then the extract/strip methods could be private? | ||
| if (decryptedContent.length > maxDecryptedContentLength) { | ||
| decryptedContent = decryptedContent.substring(0, maxDecryptedContentLength) + ' [clipped - message too large]'; |
Check failure
Code scanning / CodeQL
Property access on null or undefined Error
| let renderableAttachments: Attachment[] = []; | ||
| let decryptedContent: string | undefined; | ||
| let isHtml = false; | ||
| // todo - replace with MsgBlockParser.fmtDecryptedAsSanitizedHtmlBlocks, then the extract/strip methods could be private? | ||
| if (decryptedContent.length > maxDecryptedContentLength) { | ||
| decryptedContent = decryptedContent.substring(0, maxDecryptedContentLength) + ' [clipped - message too large]'; |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
| let renderableAttachments: Attachment[] = []; | ||
| let decryptedContent: string | undefined; | ||
| let isHtml = false; | ||
| // todo - replace with MsgBlockParser.fmtDecryptedAsSanitizedHtmlBlocks, then the extract/strip methods could be private? | ||
| if (decryptedContent.length > maxDecryptedContentLength) { |
Check failure
Code scanning / CodeQL
Property access on null or undefined Error
|
@martgil were there any issues with this PR or something else should be implemented before we can finish this PR? |
This PR clipped the decrypted large message to prevent the app from crashing
close https://github.com/FlowCrypt/flowcrypt-security/issues/215
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):