search

astroidmail / astroid

A graphical threads-with-tags style, lightweight and fast, e-mail client for Notmuch
http://astroidmail.github.io
Other
613 stars 65 forks source link

Remove enable-frame-flattening and iframe for message body #740

Closed rothn closed 4 months ago

rothn commented 1 year ago

This attempts to revive https://github.com/astroidmail/astroid/pull/732 which contains an extremely important usability fix. Original message from @ibuclaw below:

As of WebKitGTK 2.40, enable-frame-flattening is no longer supported, the property does nothing, resulting in all messages in the thread viewer being cut off at around 150px, making all long messages unreadable.

I initially had a look into seeing whether the iframe height could be set programmatically with webkit_dom_html_iframe_element_set_height from the content of the iframe via webkit_dom_element_get_scroll_height. However nothing worked at page load time of an email thread, the content scroll height remained at 150 throughout the entire ThreadView_on_load_changed pass (because iframes are rendered lazily after the main window thread has finished? Can't think of any other reason why this was observed). The scroll height was only found to update after the thread had finished loading, and triggering a hide/show toggle on each message.

Another option would be to use the JavaScriptCore API, and have all the logic to manipulate the height of the iframe in JS. Two possible problems with this: Firstly enable-javascript is explicitly disabled in Astroid's WebKit settings; second, potentially all of the webextension code may need to be rewritten in JS - given the huge amount of deprecation warnings when building Astroid, this likely will have to be done in anger at some point in the future anyway.

I gave up going down the path of manipulating the iframe after setting srcdoc, and replaced the iframe with a div instead. CSS has been fixed up and theme version bumped as it would be a breaking change for all existing users that override the theme of the UI.

I can't see any concernable difference between before and after when viewing both text/html and text/plain messages, though it is difficult to gauge as my starting point is broken messages.

The AstroidExtension::reload_images code path is the least tested of all the changes. The minimal check I did to see whether the logic is still fine was to send myself an email with a CID attachment, and observe it render correctly after pressing C-i in thread view.

As far as this change is concerned, this is one way to fix #720 that suits my use of Astroid. I'm not convinced that it is the correct way to go about it though, as I have no reason to dismiss the rationale for using iframes in the first place - see comments delete by this patch that make reference to style issues, deadlocks and security concerns by not having the content contained to its own iframe.

gauteh commented 1 year ago

I think that going away from the iframe the content needs to be sanitized, to avoid harmful HTML or possibly JavaScript. It could in theory re-write the entire UI and make it appear as if the email is from someone else etc. Sanitizing is difficult to get right, and critical to get right. Luckily, it is a problem that is pretty common so there should exist good tools and libraries. That is, we should not roll our own solution. If there is no C++ library it might be possible to filter it through some command.

rothn commented 1 year ago

Hmm, yes, Geary had these issues:

A similar approach could approach for us, since Geary also did not go down the iframe path. I'm not sure what you mean by "harmful HTML" though-- could you clarify?

rothn commented 1 year ago

Actually, it looks like Astroid takes a more aggressive security posture on Javascript than Geary by disabling it entirely: https://github.com/astroidmail/astroid/blob/8f2a8aeefd634c3f861ba51f7ca5fe256c7350b1/src/modes/thread_view/thread_view.cc#L88

I don't think this is an issue here. Curious as to your thoughts, @gauteh

gauteh commented 1 year ago

It is possible to enable javascript in astroid. But that's a conscious choice. If the body text or HTML in the email body breaks the thread-view HTML it can change an entire thread. Or it can just make an enormous blank box, moving everything else out of the way so that it appears to be emails from someone else. I'm not sure if that's exactly how it works, but that's why we usually show the text part and don't render the HTML.

rothn commented 1 year ago

It is possible to enable javascript in astroid. But that's a conscious choice.

In that case, how would you feel about adding "enable-javascript-markup", FALSE to the config? If I understand correctly, this would protect the people who choose to manually enable javascript unless they explicitly allow javascript markup by editing the code.

If the body text or HTML in the email body breaks the thread-view HTML it can change an entire thread. Or it can just make an enormous blank box, moving everything else out of the way so that it appears to be emails from someone else.

I can't easily disprove that this might be possible. However, compared to the alternative of most email being broken, and since I have not had this issue, I would suggest dealing with this if it ever comes up.

EDIT: One thing we can do about this is roll back on the first sign that this is breaking peoples' workflows. At that point, we would also have more information on what specifically breaks, how it's broken, and we'd have a test-case as well.

rothn commented 1 year ago

@gauteh How would you feel about the path I suggested a couple of days ago?

gauteh commented 1 year ago

I understand the argument, and usually I don't think "deal with it when it becomes a problem" is a deal-breaker. However, I don't think it is a good idea to ignore a known security risk, that will expose our users. There is no way back from that, and it is not ethical. We should deal with the issue in a good enough matter now, that solution probably had to be sanitize html and JavaScript. That is what makes this issue so hard to fix.

ibuclaw commented 1 year ago

FWIW I concur with @gauteh. I still run with this patch though as for me, my email account only receives messages from text-only mailing lists, so would never hit HTML issues as pointed out both in the issue, and the code comment that gets removed by this patch.

ibuclaw commented 1 year ago

Anyone with a little reading into this would know that Astroid wasn't the only application affected by this deprecation.

jorsn commented 1 year ago

It is possible to enable javascript in astroid. But that's a conscious choice. -- @gauteh

How is this possible? Only by modifying the source code? Then, I wouldn't call it configuration and hold everyone who does it responsible on their own.

gauteh commented 1 year ago

You're right, JavaScript is disabled now (https://github.com/astroidmail/astroid/commit/09b5c4f33387941fe2965207320fb21143a14c03). Used to be possible outside the message content I think. At least we had mathjax support previously. It is still possible to mess up the view in a harmful way with just html and css.

jorsn commented 4 months ago

Btw, a problem with this here is that I cannot scroll using the mouse any more, only with keyboard. Especially, touchpad scrolling is helpful at the laptop.