Skip to content

Conversation

martgil
Copy link
Collaborator

@martgil martgil commented Jul 24, 2024

This PR added message decryption and its necessary UI improvements on Thunderbird email client.

close #5667


Tests (delete all except exactly one):

  • Difficult to test (cannot find a reliable way to perform tests in a Thunderbird mail environment)

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 July 24, 2024 09:23
@martgil martgil marked this pull request as draft July 24, 2024 09:24
Copy link

gitguardian bot commented Aug 21, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
2422507 Triggered Generic Password f901bc5 extension/chrome/elements/compose.ts View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@martgil
Copy link
Collaborator Author

martgil commented Aug 21, 2024

Update: Embedding iframe within the messageDisplay of the Thunderbird Mail client is impossible. So as a reliable option for us is to just embed the CSS and images within the document.body and format it to as close as what we have in PGP_block page.

image

Another problem I have is that, opgp object from openpgpjs-custom.js reference is undefined but when I include it, there is error in Object from which error details is conceal from Thunderbird console with no way to obtain relevant information, but I think its because the openpgp.js file is simply not injected so I'll check if I can find a workaround to it otherwise, message decryption is definitely possible.

@sosnovsky, just sharing this with you. There's no need to check it for now, but I'll let you know when help is needed.

@sosnovsky
Copy link
Collaborator

@martgil thanks for update! In the case if it's not possible to directly decrypt message inside Thunderbird UI, we can add decrypt message button above message content which will open and decrypt this message inside FlowCrypt extension.


public static thunderbirdContentScriptRegistration = async () => {
await messenger.messageDisplayScripts.register({
js: [{ file: './js/content_scripts/thunderbird-content-script.js' }],
Copy link
Collaborator

Choose a reason for hiding this comment

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

after build changes all extension versions (not only thunderbird), include additional thunderbird-content-script.js which is almost 5mb:

Screenshot 2024-08-26 at 20 16 37

for browsers we use injectFcIntoWebmail method which adds all needed files to each gmail tab.

do you think it'll be possible to adapt this method for thunderbird?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

after build changes all extension versions (not only thunderbird), include additional thunderbird-content-script.js which is almost 5mb:

Ah, yes - sorry about that. I'll find a way a viable alternative solution to it from which thunderbird-content-script.js should only be created for the Thunderbird build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's start with trying to adapt injectFcIntoWebmail method to inject all needed scripts into thunderbird, so it'll work the same in browsers and thunderbird.
and if it doesn't work - then use thunderbird-content-script.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @sosnovsky - I tried to adapt using injectFcIntoWebmail and there are incompatibility that prevents me from somehow make it work as close as how it works on browser. For example, in Thunderbird, registration for content script is done within messageDisplayScript.register, from which it only accept one of the two parameter (file or code). And then several files doesnt seem to work when defined in .register() - https://webextension-api.thunderbird.net/en/91/messageDisplayScripts.html.

However, I have done my best to achieve the best result while still complying with the requirements, which I believe I have accomplished in this PR. Please review and test it out. From my perspective, the results are extremely positive, and I'm excited to hear your feedback. :)

Thanks for your help.

@sosnovsky
Copy link
Collaborator

Decryption works great, very smoothly, looks like native Thunderbird functionality 👏

By the way, is it possible to remove this warning about secret key when message was successfully decrypted by FlowCrypt?
Screenshot 2024-08-26 at 20 26 09

@sosnovsky
Copy link
Collaborator

Detect needs passphrase decryption error and if it occurs, then show passphrase through the FlowCrypt browser extension page, unlocks the keys there and redirect user back to the Thunderbird mail extension.

This will be a better solution, as storing unlocked private key will make it more vulnerable for possible attacks. Let's go with this one

@martgil
Copy link
Collaborator Author

martgil commented Aug 27, 2024

By the way, is it possible to remove this warning about secret key when message was successfully decrypted by FlowCrypt?

@sosnovsky Unfortunately, I've spend times working on that and that part of the Thunderbird isn't really changable. I mean, it is changable in a way user would be importing custom css file in their machine to get rid of it. Some says, that, it can be removable through custom CSS, like editing and overriding the userChrome.css and import it in Thunderbird, so there is that but not programmatically changable in some extent.

This will be a better solution, as storing unlocked private key will make it more vulnerable for possible attacks. Let's go with this one

Thanks for confirming that. I'll take a look and let you know.

@martgil martgil requested a review from sosnovsky August 28, 2024 06:52
@martgil martgil marked this pull request as ready for review August 28, 2024 06:59
@martgil
Copy link
Collaborator Author

martgil commented Aug 28, 2024

Hello @sosnovsky - this one is ready for review. Thank you!

@sosnovsky
Copy link
Collaborator

Hi @martgil, sorry for the long review - I was just checking how script injection works in Thunderbird, added some additional logging and noticed that injectContentScripts isn't performed there.

After some investigation I've found that our script didn't get correct list of tabs in Thunderbird, because we expect tab URL to start with https://mail.google.com, while in Thunderbird URLs are different.

I'll try to make injection work for Thunderbird, to make it's logic similar to browser extension, so it'll work the same for different platforms.

Or maybe you've already tried to change expected tab URLs for Thunderbird and it didn't work?

@martgil
Copy link
Collaborator Author

martgil commented Aug 31, 2024

Hello @sosnovsky - Thank you for thoroughly reviewing this.

I was just checking how script injection works in Thunderbird, added some additional logging and noticed that injectContentScripts isn't performed there.

Ah yes, I didn’t change this within injectContentScripts because, as far as I know, content scripts are slightly different in Thunderbird compared to what I understand from its development. In Thunderbird, we use messageDisplayScript and register it. I’m not entirely sure how exactly content script injection works in Thunderbird, especially since it appears to be applied per tab. I reviewed this during the last review and found that it might not be usable for Thunderbird, given the restrictions I understand.

However, I might have been confused and not fully understood the entire concept of content script injection, including which specific scripts need to be injected.

After some investigation I've found that our script didn't get correct list of tabs in Thunderbird, because we expect tab URL to start with https://mail.google.com, while in Thunderbird URLs are different.

Yes, to be exact, it is in the imap:// protocol of some sort, if I’m not mistaken from my last check. Origin can be checked via Thunderbird's dev tools if I understood it correctly.

I'll try to make injection work for Thunderbird, to make it's logic similar to browser extension, so it'll work the same for different platforms.

I understand, Roma. I apologize if I haven’t been reliable on this matter. But I'm here to help if you need me check some other things.

Or maybe you've already tried to change expected tab URLs for Thunderbird and it didn't work?

I actually do use CSP, but the imap:// protocol doesn’t seem to work with it. For example, window.open() is blocked by the imap:// origin. Unfortunately, it’s been hard to find resources on this. To be clear, the changes and tests I conducted are not related to injectContentScripts().

@sosnovsky
Copy link
Collaborator

I understand, Roma. I apologize if I haven’t been reliable on this matter. But I'm here to help if you need me check some other things.

You've done a great job, initially I didn't expect that it'll be possible to show decrypted messages directly in the Thunderbird message UI, but you did and it works well. Current solution is good, I just want to check Thunderbird injection logic to find possibility make it work like on browsers, so we won't need to support 2 different injection approaches in the future.

@sosnovsky
Copy link
Collaborator

Hi @martgil, it seems messenger.messageDisplayScripts.register can register multiple files, so there is no need to compile all js files into thunderbird-content-script.js.

I updated thunderbirdContentScriptRegistration this way and it injects all needed files:

public static thunderbirdContentScriptRegistration = async () => {
    const contentScriptGroups = chrome.runtime.getManifest().content_scripts ?? []; // we know it's in the manifest
    const files = contentScriptGroups[0].js?.map(url => url.replace(/moz-extension:\/\/[^/]+\//, './')) ?? [];
    await messenger.messageDisplayScripts.register({
      js: files.map(file => ({ file })),
      css: [{ file: './css/cryptup.css' }],
    });
  };

But looks like sweetalert2.js has some incompabilities with Thunderbird, as I was receiving uncaught exception: Object error in console until I removed sweetalert2.js from manifest.json. Will check it further.

@martgil
Copy link
Collaborator Author

martgil commented Sep 4, 2024

But looks like sweetalert2.js has some incompabilities with Thunderbird, as I was receiving uncaught exception: Object error in console until I removed sweetalert2.js from manifest.json. Will check it further.

Hi @sosnovsky - thank you for the information you've shared with me and for your help. In that case, I think we can just leave sweetalert2.js if that's the only js library that prevents it from working since we can use iframe within the Thunderbird UI and so we won't be able to display some sweetalert pop ups there. What do you think?

@sosnovsky
Copy link
Collaborator

I'll try to find possible fix for using sweetalert lib in Thunderbird, so we won't need to remake our popups functionality.
For now, I've pushed my changes based on this PR to 5667-thunderbird-fix branch

@martgil
Copy link
Collaborator Author

martgil commented Sep 5, 2024

I'll try to find possible fix for using sweetalert lib in Thunderbird, so we won't need to remake our popups functionality. For now, I've pushed my changes based on this PR to 5667-thunderbird-fix branch

I understand - thanks! I'll help you with this after I've finished all the GH issue assigned to me in the enterprise-server repo.

@sosnovsky
Copy link
Collaborator

I'll try to find possible fix for using sweetalert lib in Thunderbird, so we won't need to remake our popups functionality. For now, I've pushed my changes based on this PR to 5667-thunderbird-fix branch

I understand - thanks! I'll help you with this after I've finished all the GH issue assigned to me in the enterprise-server repo.

Thanks, will keep you updated!

@sosnovsky
Copy link
Collaborator

finished in #5824

@sosnovsky sosnovsky closed this Sep 11, 2024
@martgil
Copy link
Collaborator Author

martgil commented Sep 11, 2024

superseded by #5824

@sosnovsky sosnovsky deleted the issue-5667-added-message-decryption-on-thunderbird branch September 11, 2024 11:26
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.

[thunderbird] Recognize encrypted emails and decrypt them

3 participants