Skip to content
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

issue #5247 add restriction when rendering large message #5248

Merged
merged 9 commits into from Jun 30, 2023

Conversation

martgil
Copy link
Collaborator

@martgil martgil commented Jun 24, 2023

This PR This PR adds restriction when rendering large message that goes beyond 50k characters.

close #5247


Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@martgil martgil requested a review from sosnovsky as a code owner June 24, 2023 06:50
@martgil martgil marked this pull request as draft June 24, 2023 06:50
@martgil
Copy link
Collaborator Author

martgil commented Jun 25, 2023

From Roma from #4841

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

@martgil
Copy link
Collaborator Author

martgil commented Jun 25, 2023

@sosnovsky may I hear your suggestion about the following:

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

Is it acceptable to repeat the same file 50 times during testing for inline attachments? Also, since RSA keys tend to have larger ASCII armor compared to ECC curve keys, can we generate an ECC key and include 50 copies of the same key within the encrypted message for testing purposes?

@sosnovsky
Copy link
Collaborator

Is it acceptable to repeat the same file 50 times during testing for inline attachments? Also, since RSA keys tend to have larger ASCII armor compared to ECC curve keys, can we generate an ECC key and include 50 copies of the same key within the encrypted message for testing purposes?

Yes, I think it's acceptable to use the same file and same key for testing purposes, as here our main goal is to test extension performance with big number of attachments.

@martgil
Copy link
Collaborator Author

martgil commented Jun 27, 2023

Thanks @sosnovsky

I have completed the test and this is what I observed.

The FlowCrypt browser extension was able to render 50 inline public keys and 50 inline attachments (via ECP page) without any issues. However, it crashes when trying to render a regular public key attachment with approximately ~800 bytes (ecc curve key). Although occasionally it can still be rendered successfully, there is a greater chance of failure. After conducting several tests, I found that the extension can manage up to 40 regular public key attachments, but this might vary depending on the user's computer device specifications.

50 inline public key: https://mail.google.com/mail/u/flowcrypt.compatibility@gmail.com/#inbox/FMfcgzGsnLNMchfzwnbrkjKdsbLcgWxB
50 inline attachment: https://mail.google.com/mail/u/flowcrypt.compatibility@gmail.com/#inbox/FMfcgzGsnLNMchfPDfnflSgdqgCgWgzk
50 public key attachment: https://mail.google.com/mail/u/flowcrypt.compatibility@gmail.com/#inbox/FMfcgzGsnLNMcjmLnRljKLrkXvRmtmph
40 public key attachment: https://mail.google.com/mail/u/flowcrypt.compatibility@gmail.com/#inbox/FMfcgzGsnLNMcjnPqxFTrCFztnXsRTdC

@martgil
Copy link
Collaborator Author

martgil commented Jun 27, 2023

It seems that the attachment in an encrypted MIME message is not recognized by the FlowCrypt browser extension.

Sample encrypted mime message: https://mail.google.com/mail/u/flowcrypt.compatibility@gmail.com/#inbox/FMfcgzGsnLNMcnLqxmcDmbBfNbZbnJfr

@martgil
Copy link
Collaborator Author

martgil commented Jun 27, 2023

Do you think this PR is good for implementing the large text restriction? In my opinion, we should settle the following on another PR?

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

@sosnovsky
Copy link
Collaborator

However, it crashes when trying to render a regular public key attachment with approximately ~800 bytes (ecc curve key).

Does it crash browser or just some error is shown? Is there any message in console?

Do you think this PR is good for implementing the large text restriction? In my opinion, we should settle the following on another PR?

Yes, let's implement only max text length restriction in current PR. For attachments limit we should better think about possible implementation, as it can work well for 100 small attachments, but also can fail for 50 public keys attachments.

@martgil
Copy link
Collaborator Author

martgil commented Jun 28, 2023

The extension crashes to the extent that it automatically disables itself, but not the browser. In some cases, the browser slows down, especially when running simultaneous applications. However, for me, it does not lead to my browser crashing.

image

@martgil martgil marked this pull request as ready for review June 28, 2023 03:40
Copy link
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation works well for text-only message, but if message include some inline images then it's trimmed incorrectly. I tried to send email with such content (using Rich text (PGP/MIME) in Firefox):

Please check this image:
[100kb inline image]

When recipient receives this message then shown content is only Please check this image:, as image exceeds 50000 size limit. decryptedContent for this message looks like this:

<div dir="ltr">Please check this image:<br></div><div><br></div><div><img src="data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAASABIAAD....

I also checked how Gmail trims long messages - it has size limit 102kb, but this includes only message html code, without inline attachments. So let's make our trimming similar to Gmail:

  • increase limit from 50000 to 100000
  • don't include inline attachments data in this limit

@martgil martgil marked this pull request as draft June 30, 2023 05:43
@martgil martgil force-pushed the issue-5247-clip-large-encrypted-message branch from 60b7475 to 9fae03d Compare June 30, 2023 10:26
@martgil
Copy link
Collaborator Author

martgil commented Jun 30, 2023

Thank you very much for sharing it. I have updated the code and the test. I have to remove the test for signed mime messages for large messages because it is now too large for Gmail to handle (100k + mime messages). Hence, I just replaced it with another that ensures that data belonging to an inline message would not be part of the decrypt content length evaluation.

@martgil martgil marked this pull request as ready for review June 30, 2023 11:16
@martgil martgil requested a review from sosnovsky June 30, 2023 11:17
@martgil martgil requested a review from sosnovsky June 30, 2023 13:06
Copy link
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well 👍

@sosnovsky sosnovsky merged commit 8d941da into master Jun 30, 2023
14 checks passed
@sosnovsky sosnovsky deleted the issue-5247-clip-large-encrypted-message branch June 30, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement message rendering limit for encrypted email and encrypted MIME message
2 participants