Extract inline images into attachments for plain rich text messages by limonte · Pull Request #3256 · FlowCrypt/flowcrypt-browser

limonte · 2020-12-16T22:29:42Z

TODO:

@seisvelas Extract inline images into attachments for plain rich text messages #3256 (comment)

…tter to Mime

limonte · 2020-12-17T00:24:18Z

@tomholub it works now, thanks! Sometimes a good question is better than the advice :)

I'll continue working on figuring out why some tests are failing as well as writing a test for this fix.

Question: in extractInlineImagesToAttachments() I'm iterating and editing images with DOMPurify. Is that the best way to perform string -> nodes -> edit nodes -> back to string?

limonte · 2020-12-17T10:11:19Z

Another question: I noticed there was Att -> Attachment renaming recently.

There are still some /Att[A-Z']/ occurrences and I'm using Mime.createAttNode()

I guess we should also do /Att[A-Z']/ -> Attachment$1, if so I'd prefer it to be done in separate PR not to mix the fix and the refactoring in one PR.

tomholub · 2020-12-17T12:01:44Z

Well noticed! Agree gree that would be a separate pr Have to look at the other question later when on pc

…

On Thu, 17 Dec 2020, 11:11 Limon Monte, ***@***.***> wrote: Another question: I noticed there was Att -> Attachment renaming recently. There are still some /Att[A-Z]/ occurrences and I'm using Mime.createAttNode() [image: image] <https://user-images.githubusercontent.com/6059356/102473941-b739f900-4060-11eb-8ad6-e5f10c5f0ed3.png> I guess we should also do /Att[A-Z]/ -> /Attachment$1/, if so I'd prefer it to be done in separate PR not to mix the fix and the refactoring in one PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQDZEIAW4ZDMCVZMDKSDGDSVHKNPANCNFSM4U6VPZ7A> .

…mote)

tomholub

Looks pretty good :)

extension/js/common/core/mime.ts

tomholub · 2020-12-17T17:11:13Z

extension/js/common/core/mime.ts

    return node;
  }

+  private static extractInlineImagesToAttachments = (html: string, rootNode: any) => {


Looks good in high-level overview 👍 but will need to give this more scrutiny later.

@seisvelas should scrutinize it for attack vectors if any

extension/js/common/core/mime.ts

extension/js/common/platform/xss.ts

limonte · 2020-12-19T02:40:28Z

@tomholub do you maybe have any quick guess why the test is failing, there's no <img> tag in getMessageBySubject().payload.body.data. I checked the sending, the HTML is correct (with <img> tag) right before sending.

test/source/mock/google/google-data.ts

tomholub · 2020-12-19T22:40:19Z

@tomholub do you maybe have any quick guess why the test is failing, there's no <img> tag in getMessageBySubject().payload.body.data. I checked the sending, the HTML is correct (with <img> tag) right before sending.

Sorry haven't been able to have a look yet, can't think of anything without investigating

Except if the img got sanitized away somewhere using Xss class

limonte · 2020-12-19T22:41:23Z

@tomholub do you maybe have any quick guess why the test is failing, there's no <img> tag in getMessageBySubject().payload.body.data. I checked the sending, the HTML is correct (with <img> tag) right before sending.

Sorry haven't been able to have a look yet, can't think of anything without investigating

Except if the img got sanitized away somewhere using Xss class

Thanks, I'll investigate further.

limonte · 2020-12-20T00:51:09Z

@tomholub I need your help with writing a test. There's <img src="data:image/png;base64..."> instead of <img src="cid:...">. I was tracking it down to this place, mimeMsg is as expected here, with extracted inline images.

I guess something is happening mocked google endpoints, but I can't figure out what and where. Let me know if you have time for helping me with this, if not, then I'll continue debugging it by myself.

tomholub · 2020-12-23T19:01:11Z

Sorry, I only do light work this week so may take a bit of time before I get to this with meaningful help.

@rrrooommmaaa - if you have the time - could you look into this?

limonte · 2020-12-23T19:39:41Z

Sorry, I only do light work this week so may take a bit of time before I get to this with meaningful help.

@rrrooommmaaa - if you have the time - could you look into this?

Thanks, I realized that from your GitHub activity :) @rrrooommmaaa no need for help atm, I think I'll finish this by myself. I'll let you know additionally if the help will be needed here.

limonte · 2020-12-23T22:33:35Z

@tomholub I need your help with writing a test. There's <img src="data:image/png;base64..."> instead of <img src="cid:...">. I was tracking it down to this place, mimeMsg is as expected here, with extracted inline images.

Found the reason:

simpleParser is the easiest way to parse emails. You only need to provide a message source to get a parsed email structure in return. As an additional bonus all embedded images in HTML (eg. the images that point to attachments using cid: URIs) are replaced with base64 encoded data URIs, so the message can be displayed without any additional processing

https://nodemailer.com/extras/mailparser/

limonte · 2020-12-23T23:51:57Z

@tomholub the PR is ready for review 🚀

@seisvelas please evaluate the security of the extractInlineImagesToAttachments() method

Thanks!

tomholub

Looks good! It seems that FlowCrypt itself cannot render such messages yet? We may want to look into that sometime, when using Settings -> Inbox.

tomholub · 2020-12-31T16:07:04Z

test/source/tests/compose.ts

+    expect(sentMsg.payload?.body?.data).to.match(/<img src="cid:(.+)@flowcrypt">This is an automated puppeteer test: Test Sending Plain Message With Image/);
+    return;
+    // todo - this test case is a stop-gap. We need to implement rendering of such messages below,
+    //   then let test plain messages with images in them (referenced by cid) just like other types of messages below


I'll make an issue for this

tomholub · 2020-12-31T16:09:09Z

extension/chrome/elements/compose-modules/compose-send-btn-module.ts

    }
+    if (choices.richtext && !choices.encrypt && !choices.sign && msg.body['text/html']) {
+      // extract inline images of plain rich-text messages (#3256)
+      // todo - also apply to rich text signed-only messages


Also todo here for later

Extract inline images into attachments for plain rich text messages

da92e8b

This comment has been minimized.

Sign in to view

tomholub closed this Dec 16, 2020

tomholub reopened this Dec 16, 2020

This comment has been minimized.

Sign in to view

Limon Monte added 2 commits December 17, 2020 02:09

Move extractInlineImagesToAttachments() method from PlainMsgMailForma…

fb314cb

…tter to Mime

clean up

a8534c6

use MimeNode.getHeader()

2a505ec

fix displayImageSrcLinkAsImg(), do not show <img src="cid:..." as (re…

2dd1e0b

…mote)

limonte mentioned this pull request Dec 17, 2020

Refactoring: perform /Att[A-Z']/ -> Attachment$1 renaming #3257

Closed

tomholub reviewed Dec 17, 2020

View reviewed changes

limonte mentioned this pull request Dec 18, 2020

Unable to open encrypted draft: TypeError: Cannot read property 'RF' of undefined #3259

Closed

Limon Monte added 2 commits December 19, 2020 03:00

extract inline images only when sending plain rich-text messages

f68b85f

revert unnecessary addition

f896c36

limonte force-pushed the issue-2460-images-inside-plain-rich-text branch from 41a3be6 to f896c36 Compare December 19, 2020 01:08

Limon Monte added 2 commits December 19, 2020 03:26

get rid of the expensive regex

6e3bdbd

add test

247521b

limonte marked this pull request as ready for review December 19, 2020 02:03

limonte changed the title ~~[WIP] Extract inline images into attachments for plain rich text messages~~ Extract inline images into attachments for plain rich text messages Dec 19, 2020

take parsedMail.html into consideration in storeSentMessage

01790f6

tomholub reviewed Dec 19, 2020

View reviewed changes

test/source/mock/google/google-data.ts Outdated Show resolved Hide resolved

improve if

e0176f8

limonte changed the title ~~Extract inline images into attachments for plain rich text messages~~ [WIP] Extract inline images into attachments for plain rich text messages Dec 23, 2020

limonte marked this pull request as draft December 23, 2020 17:15

move extractInlineImagesToAttachments() to ComposeSendBtnModule

501193d

Limon Monte added 3 commits December 24, 2020 00:44

finalyze, keepCidLinks: true

7fcaae4

better naming: htmlWithInlineImages -> htmlWithCidImages

ba058b8

fix the 'send pwd encrypted msg & check on flowcrypt site' test

a85da67

limonte marked this pull request as ready for review December 23, 2020 23:51

limonte changed the title ~~[WIP] Extract inline images into attachments for plain rich text messages~~ Extract inline images into attachments for plain rich text messages Dec 23, 2020

Tom J added 2 commits December 31, 2020 15:52

slight style update

d491c8f

added a todo

5008408

tomholub approved these changes Dec 31, 2020

View reviewed changes

tomholub mentioned this pull request Dec 31, 2020

parse/reconstruct messages with cid image references #3280

Open

tomholub reviewed Dec 31, 2020

View reviewed changes

tomholub mentioned this pull request Dec 31, 2020

rich text signed messages - send out with cid instead of img tags? #3281

Open

tomholub merged commit 727b774 into master Dec 31, 2020

tomholub deleted the issue-2460-images-inside-plain-rich-text branch December 31, 2020 16:12

tomholub mentioned this pull request Jan 6, 2021

check security of cid replacements PR #3301

Closed

Comments

Conversation

limonte commented Dec 16, 2020 • edited by seisvelas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

limonte commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

limonte commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomholub commented Dec 17, 2020 via email

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomholub Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

tomholub Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

limonte commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tomholub commented Dec 19, 2020

Uh oh!

limonte commented Dec 19, 2020

Uh oh!

limonte commented Dec 20, 2020

Uh oh!

tomholub commented Dec 23, 2020

Uh oh!

limonte commented Dec 23, 2020

Uh oh!

limonte commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

limonte commented Dec 23, 2020

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

tomholub Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

limonte commented Dec 16, 2020 •

edited by seisvelas

Loading

limonte commented Dec 17, 2020 •

edited

Loading

limonte commented Dec 17, 2020 •

edited

Loading

limonte commented Dec 19, 2020 •

edited

Loading

limonte commented Dec 23, 2020 •

edited

Loading