#2414 Show links to remote images

[ioanmo226]

This PR implemented feature to show links to remote images

close #2414 // if this PR closes an issue

---

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

[ioanmo226]

@sosnovsky Could you check current implementation if it's implemented correctly?
After I'll start to work on adding UI test.

[sosnovsky]

@sosnovsky Could you check current implementation if it's implemented correctly? After I'll start to work on adding UI test.

Implementation looks correct, but I noticed a small bug - some characters are rendered in wrong encoding:

it's message from github about your comment, in original message there is —

in original message there is a space:

And another issue is that previous thread replies are not hidden with 3-dots button, it causes check thread rendering and archived message actions ui test fail.

[sosnovsky]

I found another issue:

• send plain unencrypted message from Gmail web
• try to reply to it from FlowCrypt iOS app
• compose screen will show html code of original message, not plain text

Also in unencrypted threads disappeared > symbols for previous replies (I'll send comparison screenshots to your email)

[ioanmo226]

Thank you. Nice catch 👍 .
let me fix it

[ioanmo226]

For this one do we have to display text content or html content which is translated into NSAttributedString?

I found another issue:

[sosnovsky]

For this one do we have to display text content or html content which is translated into NSAttributedString?

let's show text content for reply compose, without remote images

[ioanmo226]

OK

[ioanmo226]

@sosnovsky, wrestling with HTML because we need to pluck out img tags from an HTML string and turn them into links. I’m hitting a snag with parsing quotes, though.

Here are a couple of solutions I'm considering:

• Whip up a regex to sniff out the gmail_quote class and split the quoted content from the main.
• Leverage a third-party library like SwiftSoup to navigate the HTML and isolate gmail_quote, separating the quoted and main content afterward.

What do you think? Got any slicker ideas?

flowcrypt-ios/FlowCrypt/Functionality/Mail Provider/Message Provider/ProcessedMessage.swift

Line 116 in c8f7464

body = try await Core.shared.sanitizeHtml(html: html)

flowcrypt-ios/FlowCrypt/Functionality/Mail Provider/Message Provider/ProcessedMessage.swift

Line 128 in c8f7464

private static func parseQuote(text: String) -> (String, String?) {

[sosnovsky]

I checked how it's implemented on Android - it just uses plain text from initial message, without image links:

Probably it'll be easier to implement than trying to parse plain text from html?

[ioanmo226]

I've checked and found that in the Android version, the app sanitizes the HTML and directly displays it in the EmailWebView, rather than using plain text, which contrasts with how things are handled in the iOS app.

In iOS app, we're trying to parse out the main content and quotes from the HTML to differentiate primary content from quoted messages, with the latter being accessible via a '3 dots' button. (As you know parsing main content and quote from plain text is already done in iOS. but as we want to display img links, we need to use html content.)

As of now, this parsing feature isn't present in the Android app. (FYI this happens with unencrypted messages which is regular Gmail message)

Do correct me if I've gotten anything wrong.

cc: @DenBond7

[sosnovsky]

What if we'll additionally sanitize processedMessage.fullText and remove all html tags before sending it to compose screen here -

flowcrypt-ios/FlowCrypt/Controllers/Threads/ThreadDetailsViewController+TapActions.swift

Line 212 in dae6c8e

text: processedMessage.fullText,

[ioanmo226]

Sorry @sosnovsky .
I'm talking about thread detail screen(Not compose screen)
In thread detail page, we have to split main content and quoted content like below image. But current problem is we don't have way to parse quote & main content from html content.

[sosnovsky]

Sorry @sosnovsky . I'm talking about thread detail screen(Not compose screen) In thread detail page, we have to split main content and quoted content like below image. But current problem is we don't have way to parse quote & main content from html content.

What if we'll just use regex to extract quote content from .gmail_quote div?

[ioanmo226]

Yep. It is same as my 1st suggested solution.
But one thing I worry is that processing html with regex might produce another issues (like not displaying contents correctly).

Whip up a regex to sniff out the gmail_quote class and split the quoted content from the main.
Leverage a third-party library like SwiftSoup to navigate the HTML and isolate gmail_quote, separating the quoted and main content afterward.

[sosnovsky]

Yep. It is same as my 1st suggested solution.

Oops, missed it 👍

But one thing I worry is that processing html with regex might produce another issues (like not displaying contents correctly).

I think it should be ok, as we won't make any complex transformations - just parse div.gmail_quote content, should work well. Also it'll be applied only to non-encrypted messages, as we don't have html in encrypted ones.

[ioanmo226]

@sosnovsky Fixed above issues. Please check again.
Code looks a bit messy though :-(

[sosnovsky]

It works better now, but still there are issues with rendering quote content in some threads, as parsing html and removing tags sometimes leads to broken rendering (your assumption about issues with incorrect contents display was correct)

Some possible solutions:

• use plain text message body for quote content and html message body for message content
• add separate action show html version in 3-dots message menu
• try to render html content like it's currently implemented on Android - I haven't noticed issues with rendering quotes content there

What do you think about mentioned solutions?

[ioanmo226]

What issues do you have while rendering quote content?

Also here are my thoughts for your solutions

1. So you mean you only have issues while rendering quote content? you do not notice issues while rendering main content?
2. Even though we implement show html version, we will have same issues when user clicks show html version button?
3. This way, we will lose 3 dots toggle button which would toggle quote content view.

[sosnovsky]

So you mean you only have issues while rendering quote content? you do not notice issues while rendering main content?

Initially I noticed only quoted content issues, for example when using text body different quote levels are differentiated by different number of > in the beginning of the line, while in html body they indented with tabs which are removed when html is sanitized, so all previous quotes are on the same level.

But then I found that it affects also main content of messages too, as html body usually includes additional markup and just removing tags leads to broken message structure. Like this message from Google:

current PR

App Store version

Android

Even though we implement show html version, we will have same issues when user clicks show html version button?

Original issue reported by user was that he wasn't able to approve security requests because image with link wasn't loaded in FlowCrypt app. Separate show html version button will allow to load full html version, which will include all images and links. And broken rendering of quoted content shouldn't be too critical there.

This way, we will lose 3 dots toggle button which would toggle quote content view.

Yeah, that's why it's difficult to choose which solution will be the best one, all of them have pros and cons :)
I prefer this one, it should make rendered content look better, as even current implementation with text body has some flaws like unnecessary newlines (they're presented in the screenshot from App Store version).

So if we'll be able to separate quoted content from html body - it should give us the best result.

[ioanmo226]

Sorry but which one do you mean?

I prefer this one, it should make rendered content look better

[sosnovsky]

Sorry but which one do you mean?

I prefer this one, it should make rendered content look better

This one - try to render html content like it's currently implemented on Android

[ioanmo226]

You mean just display html content in WebView after sanitizing html? (Without show quoted content button`).
Also only for regular plain emails?

[sosnovsky]

I'm not expert at this security stuff but I think we might consider about CSS Injection via style tag?

https://www.getastra.com/blog/knowledge-base/guide-on-css-injection-prevention/

Seems like it applies only to opening such HTML in browser, where injection can fetch some data. While WKWebView is isolated web view, which just renders provided HTML so it shouldn't leak any user data.

It's also possible to completely disable JS in WKWebView - https://developer.apple.com/documentation/webkit/wkwebpagepreferences/3552422-allowscontentjavascript, so no malicious code will be executed.

[ioanmo226]

Sounds good. Thank you

[ioanmo226]

@sosnovsky As you can see, Core unit test fails because of background property not removed from style attribute.
Like below leak html (which background css is in background).
Are we fine to remove those checks?

flowcrypt-ios/Core/source/assets/html/leak.html

Line 399 in 56eabbc

<div style="background: url() url() url() url() url(https://leaking.via/inline-css-multiple-backgrounds);"></div>

flowcrypt-ios/Core/source/test.ts

Line 1086 in b907686

expect(clean).to.not.contain('background');

flowcrypt-ios/Core/source/test.ts

Line 1088 in b907686

expect(clean).to.not.contain('style');

[sosnovsky]

@sosnovsky As you can see, Core unit test fails because of background property not removed from style attribute. Like below leak html (which background css is in background). Are we fine to remove those checks?

No, let's leave these checks - it seems html-sanitize allows filtering of css styles (https://github.com/apostrophecms/sanitize-html?tab=readme-ov-file#allowed-css-styles), let's try to remove background properties which have url in value, so no remote content will be loaded

[ioanmo226]

@sosnovsky Ready for review. Please check. Thank you

[sosnovsky]

Great job 👏 I like how messages look with these updates, but some improvements still needed:

• change font to iOS system font, it can be done by adding <style> tag after <meta name="viewport" ... line:

<meta name="viewport" ... >
<style>* { font-family: -apple-system, "Helvetica Neue", sans-serif; }</style>

• support for dark mode (as currently messages always have white background) can be added in <style> tag too:

:root { color-scheme: light dark; supported-color-schemes: light dark; }

And then we can set background in dark mode to be the same as app background using

@media (prefers-color-scheme: dark) {
  :root {
    background-color: OUR_IOS_DARK_BACKGROUND;
    color: white;
  }
}

• web view has additional side paddings (around 8px, I think), let's remove it so web view text will be the same width as text in encrypted message. Maybe it's can be done through CSS too (like html, body { padding: 0 !important; margin: 0 !important; }) or by increasing width of web view.

[ioanmo226]

Sounds good 👍

change font to iOS system font, it can be done by adding <style> tag after <meta name="viewport" ... line:

Oops. Missed it. Thank you for your notice

support for dark mode (as currently messages always have white background) can be added in <style> tag too:

OKay

web view has additional side paddings (around 8px, I think)

[ioanmo226]

@sosnovsky It looks great with your suggestions. Thanks.
Please check.

[sosnovsky]

Well done, just a couple small fixes left and then let's work on hiding/showing quote text in another PR

[sosnovsky]

Oops, just noticed that issue from earlier reviews is still present (#2422 (review)):

• send non-encrypted message from Gmail web interface
• open it in iOS app
• tap reply button
• on compose screen there will be html code

Let's use plain text body on compose screen, as we don't support creating of html messages yet.

[ioanmo226]

Oops!!
Sorry for that and thank you for your notice.

[sosnovsky]

Looks good now, well done 👍