Remove enable-frame-flattening and iframe for message body #740
Conversation
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 astroidmail#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.
|
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. |
|
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? |
|
Actually, it looks like Astroid takes a more aggressive security posture on Javascript than Geary by disabling it entirely: I don't think this is an issue here. Curious as to your thoughts, @gauteh |
|
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. |
In that case, how would you feel about adding
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. |
|
@gauteh How would you feel about the path I suggested a couple of days 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. |
|
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. |
|
Anyone with a little reading into this would know that Astroid wasn't the only application affected by this deprecation. |
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. |
|
You're right, JavaScript is disabled now (09b5c4f). 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. |
- partly fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732
- partly fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732
- fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732 or the semi-working astroidmail#743 This does not allow the document (message content) to execute javascript, as can be verified with the following email, which displays "no scripts" by default, but "scripts active" when js is active. ``` Date: Tue, 1 January 1970 00:00:00 +0000 From: Subject: MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-TUjG03Ah9OcLtWE56Ucm" --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable <div id=3D"jscontent"> no scripts </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html xmlns=3D"http://www.w3.org/1999/xhtml" lang xml:lang> <head> <meta charset=3D"utf-8" /> <meta name=3D"generator" content=3D"pandoc" /> <meta name=3D"viewport" content=3D"width=3Ddevice-width, initial-scale=3D= 1.0, user-scalable=3Dyes" /> <style> code{white-space: pre-wrap;} span.smallcaps{font-variant: small-caps;} span.underline{text-decoration: underline;} div.column{display: inline-block; vertical-align: top; width: 50%;} </style> </head> <body> <div id=3D"jscontent"> <p>no scripts</p> </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> </body> </html> --=-TUjG03Ah9OcLtWE56Ucm-- ```
|
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. |
- fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732 or the semi-working astroidmail#743 This does not allow the document (message content) to execute javascript, as can be verified with the following email, which displays "no scripts" by default, but "scripts active" when js is active. ``` Date: Tue, 1 January 1970 00:00:00 +0000 From: Subject: MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-TUjG03Ah9OcLtWE56Ucm" --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable <div id=3D"jscontent"> no scripts </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html xmlns=3D"http://www.w3.org/1999/xhtml" lang xml:lang> <head> <meta charset=3D"utf-8" /> <meta name=3D"generator" content=3D"pandoc" /> <meta name=3D"viewport" content=3D"width=3Ddevice-width, initial-scale=3D= 1.0, user-scalable=3Dyes" /> <style> code{white-space: pre-wrap;} span.smallcaps{font-variant: small-caps;} span.underline{text-decoration: underline;} div.column{display: inline-block; vertical-align: top; width: 50%;} </style> </head> <body> <div id=3D"jscontent"> <p>no scripts</p> </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> </body> </html> --=-TUjG03Ah9OcLtWE56Ucm-- ```
- fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732 or the semi-working astroidmail#743 This does not allow the document (message content) to execute javascript, as can be verified with the following email, which displays "no scripts" by default, but "scripts active" when js is active. ``` Date: Tue, 1 January 1970 00:00:00 +0000 From: Subject: MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-TUjG03Ah9OcLtWE56Ucm" --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable <div id=3D"jscontent"> no scripts </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> --=-TUjG03Ah9OcLtWE56Ucm Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html xmlns=3D"http://www.w3.org/1999/xhtml" lang xml:lang> <head> <meta charset=3D"utf-8" /> <meta name=3D"generator" content=3D"pandoc" /> <meta name=3D"viewport" content=3D"width=3Ddevice-width, initial-scale=3D= 1.0, user-scalable=3Dyes" /> <style> code{white-space: pre-wrap;} span.smallcaps{font-variant: small-caps;} span.underline{text-decoration: underline;} div.column{display: inline-block; vertical-align: top; width: 50%;} </style> </head> <body> <div id=3D"jscontent"> <p>no scripts</p> </div> <script> d =3D document.getElementById('jscontent'); d.innerHTML =3D "scripts active"; </script> </body> </html> --=-TUjG03Ah9OcLtWE56Ucm-- ```
- partly fixes astroidmail#720 - alternative to removing iframes as in astroidmail#740 or astroidmail#732
This attempts to revive #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_heightfrom the content of the iframe viawebkit_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 entireThreadView_on_load_changedpass (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-javascriptis explicitly disabled in Astroid's WebKit settings; second, potentially all of thewebextensioncode 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_imagescode 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.