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

#5458 Improve parsing of pgp/mime message with attachment #5518

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Dec 8, 2023

This PR fixes parsing of large PGP/MIME encrypted messages with attachment, which previously were not detected as encrypted ones.

close #5458

before (original message isn't detected as encrypted and rendered as plain text):

Screenshot 2023-12-14 at 09 54 36

after (encrypted message is parsed and correctly renders decryption error):

Screenshot 2023-12-14 at 09 54 49

Tests (delete all except exactly one):

  • Difficult to test (explain why) - it happens just for some messages, wasn't able to recreate similar pgp/mime message

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

@sosnovsky sosnovsky changed the title #5498 Improve decryption of pgp/mime message with attachment #5498 Improve parsing of pgp/mime message with attachment Dec 14, 2023
@sosnovsky sosnovsky marked this pull request as ready for review December 14, 2023 08:38
@sosnovsky sosnovsky requested a review from martgil December 14, 2023 08:38
@sosnovsky sosnovsky enabled auto-merge (squash) December 14, 2023 08:38
@sosnovsky
Copy link
Collaborator Author

@martgil this one ready for review, thanks!

@martgil
Copy link
Collaborator

martgil commented Dec 14, 2023

@sosnovsky does the above email from the screenshot available to our compatibility email account? Perhaps I can review it and potentially generate a test email for this one?

@sosnovsky
Copy link
Collaborator Author

@sosnovsky does the above email from the screenshot available to our compatibility email account? Perhaps I can review it and potentially generate a test email for this one?

Yes, it's in flowcrypt.compatibility account - thread Re: FES message for #5458 from October 16

@martgil
Copy link
Collaborator

martgil commented Dec 14, 2023

Got it - thank you, Roma.

@sosnovsky
Copy link
Collaborator Author

By the way, original message was created using FES 2023-11, the same message from FES 2023-12 doesn't have such issue. Also it happens only in Gmail inbox, while in FlowCrypt Encrypted Inbox message is rendered correctly even without current fix.

@martgil
Copy link
Collaborator

martgil commented Dec 14, 2023

Okay, I did try to create a test email, but I think it is difficult to do so. The issue is, it must be long enough to trigger Gmail's email body message clipping. This means that the non-encrypted data would include large characters, which the browser extension cannot handle, making it difficult to create a test email. That would be a different story since the above screenshot results in a decryption error (probably because it was an encrypted email for another key).

I have tested it and it should work on other instances wherein the decrypted content is not very long but the ASCII armored is long enough to trigger Gmail's email body message clipping feature.

@sosnovsky sosnovsky merged commit 749c102 into master Dec 14, 2023
14 checks passed
@sosnovsky sosnovsky deleted the 5458-fix-fes-reply-decryption branch December 14, 2023 09:26
@sosnovsky sosnovsky changed the title #5498 Improve parsing of pgp/mime message with attachment #5458 Improve parsing of pgp/mime message with attachment Dec 14, 2023
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.

FES reply with attachment is not decrypted
2 participants