set iframe height in javascript #747

jorsn · 2024-05-28T11:32:20Z

fixes webkit_settings_set_enable_frame_flattening() deprecation #720, fixes Expanded mail has fixed height regardless of its content #736
closes Remove enable-frame-flattening and iframe for message body #732, closes Remove enable-frame-flattening and iframe for message body #740, closes set iframe size on focus using deprecated DOM access #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--

ibuclaw · 2024-05-28T15:11:07Z

I was curious to give this a whirl. The iframe height is set too high a value.

Only after toggling between text/plain and text/html does the height correctly readjust itself.

jorsn · 2024-05-28T20:35:54Z

Strange, for me it display fine.

Do you display plain or html by default?
Do you have a sample email for which this happens?

ibuclaw · 2024-05-29T00:35:40Z

Strange, for me it display fine.

    "thread_view": {
        "open_html_part_external": "false",
        "preferred_type": "plain",
        "preferred_html_only": "true",
        "allow_remote_when_encrypted": "false",
        "open_external_link": "xdg-open",
        "default_save_directory": "~",
        "indent_messages": "false",
        "gravatar": {
            "enable": "true"
        },
        "mark_unread_delay": "0.5",
        "expand_flagged": "false"
    },

Do you display plain or html by default? Do you have a sample email for which this happens?

i.e: All mail I get from GitHub notifications.

Ah! I've just checked again, and it only affects messages that are collapsed when opening in the thread (i.e: unread tag removed). Expanding them is what triggers the above screenshot.

(I'm also running with a variant of #745 as astroid no longer compiles on my system, so it might only be a GTK4 thing).

gauteh · 2024-05-29T06:24:06Z

Hi. I don't have much capacity to test, if some of you want to help maintain I can add you.

jorsn · 2024-05-29T11:42:49Z

Alright, I can now also reproduce it. It has nothing to do with #745 nor with the astroid settings.

Sometimes, it is fixed by adding loading=lazy to the iframe. But this does not always work.

Is there any way to inspect the final html/css/js page? EDIT: I found the webinspector hidden behind cmake -DDEBUG_WEBKIT=ON.
Debugging seems next to impossible without. Somehow it seems that no event besides load fires at all, for anything that happens to the page. In principle, resize events for all elements of class email should work in principle, since as far as I read, display: none sets the dimensions to zero.

jorsn · 2024-05-29T17:58:44Z

@ibuclaw I pushed a new commit that should fix the over-sizing. Does it work for you now?

ibuclaw · 2024-05-31T18:56:36Z

@ibuclaw I pushed a new commit that should fix the over-sizing. Does it work for you now?

Looks good to me.

jorsn · 2024-05-31T19:25:22Z

Great! Are there any objections agains the enabling of javascript? It is disabled in the iframe.

Otherwise, we can merge this, can't we?

gauteh · 2024-05-31T20:33:41Z

The only one is that a sender could add javascript in the subject or in the email address. But I think that is possible to sanitize. As mentioned previously, I would be happy to give someone else access to merge.

jorsn · 2024-06-01T13:37:05Z

The only one is that a sender could add javascript in the subject or in the email address. But I think that is possible to sanitize.

Isn't this already sanitized by Glib::Markup::escape_text(...)? I checked every call to webkit_dom_element_set_inner_html, and I am pretty confident that every occurrence is sanitized in that way if it appears outside .body_iframes in the end and contains data (in contrast to user-supplied config and program-generated things like error messages).

As mentioned previously, I would be happy to give someone else access to merge.

I could take it, and help a bit with maintenance, especially when something breaks for me, but I can't tell you how much and for how long: I'm only willing to do it as long as I continue to use astroid, and I cannot give any guarantees on my time budget for this.
Further, I don't have any substantial C++, web or browser development experience, just general programming now and then.

gauteh · 2024-06-01T13:44:59Z

Thanks, i appreciate that and am happy for what you can and have time to do.

gauteh · 2024-06-01T19:47:33Z

I think sanitize should do the trick 👍

jorsn · 2024-06-02T10:29:24Z

What is 'sanitize'? The JS method? How would you invoke it safely, here?

I think, if escape_text() is insufficient, the way to go is to set innerText instead of innerHTML. For this, one has to generate the header table without the data first, and then populate with the data.

gauteh · 2024-06-02T11:15:07Z

Innertext is safer yes. I was using sanitize and escape interchangeably.

jorsn · 2024-06-03T01:38:59Z

I changed the setting of headers and preview to use innerText.
I think, this can be merged, but I don't seem to have write access to the repo, even if I am in the organization, now. At least, I cannot accept PRs.

gauteh · 2024-06-03T05:47:55Z

Looks good. I tried adding you to this repo as well, please try again.

This does not allow the email html 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-- ```

this possibly makes it harder to inject arbitrary html/scripts via email headers or tags

ibuclaw · 2024-06-04T22:18:54Z

@jorsn thanks for tackling this!

mcatanzaro · 2024-06-04T22:44:23Z

ui/thread-view.html

+  <!--
+    sandbox="allow-same-origin" is required, otherwise the scrollHeight of the
+    embedded document cannot be accessed.
+    Do not add 'allow-scripts' to the sandbox attribute. This would allow the
+    document to execute scripts itself.
+  -->
+  <iframe class="body_iframe" sandbox="allow-same-origin" onload="handleLoadIframe(this);" srcdoc=""></iframe>
 </div>

+<script>


There's also a WebKitSettings:enable-javascript-markup setting you can use to remove any doubt; that allows the application to run JavaScript but blocks the document from doing so. That would block the use of <script> here, but you could probably run it manually using webkit_web_view_evaluate_javascript().

This seems good for (faith in) security. But I did not see how I can reference this (the iframe) in cpp in a way such that I can pass it to the javascript function handleLoadIframe.

At least, I expect it to be a bit complicated.

Are your doubts strong, currently?

But I did not see how I can reference this (the iframe) in cpp in a way such that I can pass it to the javascript function handleLoadIframe.

The best solution is the reverse: don't try to access it from C++. Move work that touches the DOM into JavaScript instead. The C++ DOM APIs are all deprecated and already gone in the webkitgtk-6.0 API version, so they'll be a problem for you in the future.

Are your doubts strong, currently?

No. I think the sandbox attribute is good enough because you're careful to encode everything so it can't break out. I think you're good so long as you use Glib::Markup::escape_text on all the untrusted input, and your comment says you do that.

This guideline suggests you don't use inner HTML at all, but if all untrusted input is properly encoded then I'm not sure why not.

Thanks! Yes, in the long run, this schould be done, maybe one can just pass the emails as json to the javascript somehow, (anyway, it seems you can only pass data to js by injecting js strings, so it would be good to have dead json) and then use js to build the whole threadview. But this one is even more work.

jorsn mentioned this pull request May 28, 2024

Expanded mail has fixed height regardless of its content #736

Closed

jorsn changed the title ~~set iframe size in javascript~~ set iframe height in javascript May 28, 2024

jorsn mentioned this pull request May 31, 2024

Can't open HTML parts #737

Open

jorsn mentioned this pull request Jun 3, 2024

Use GUri instead of libsoup-2.4 and use webkit2gtk-4.1 #745

Closed

jorsn force-pushed the fix-msg-height/js branch from 86fcca4 to eef74fd Compare June 3, 2024 02:01

jorsn force-pushed the fix-msg-height/js branch 3 times, most recently from 977f0fe to eef74fd Compare June 4, 2024 22:07

jorsn and others added 3 commits June 5, 2024 00:10

thread_view: set headers and preview via innerText

88352dc

this possibly makes it harder to inject arbitrary html/scripts via email headers or tags

set email preview width in css

179587d

jorsn force-pushed the fix-msg-height/js branch from eef74fd to 179587d Compare June 4, 2024 22:11

jorsn merged commit 6af16fd into astroidmail:master Jun 4, 2024
2 of 4 checks passed

mcatanzaro reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set iframe height in javascript #747

set iframe height in javascript #747

jorsn commented May 28, 2024 •

edited

Loading

ibuclaw commented May 28, 2024

jorsn commented May 28, 2024

ibuclaw commented May 29, 2024

gauteh commented May 29, 2024

jorsn commented May 29, 2024 •

edited

Loading

jorsn commented May 29, 2024

ibuclaw commented May 31, 2024

jorsn commented May 31, 2024

gauteh commented May 31, 2024

jorsn commented Jun 1, 2024

gauteh commented Jun 1, 2024

gauteh commented Jun 1, 2024

jorsn commented Jun 2, 2024 •

edited

Loading

gauteh commented Jun 2, 2024

jorsn commented Jun 3, 2024

gauteh commented Jun 3, 2024

ibuclaw commented Jun 4, 2024

mcatanzaro Jun 4, 2024

jorsn Jun 4, 2024

mcatanzaro Jun 5, 2024

jorsn Jun 5, 2024

set iframe height in javascript #747

set iframe height in javascript #747

Conversation

jorsn commented May 28, 2024 • edited Loading

ibuclaw commented May 28, 2024

jorsn commented May 28, 2024

ibuclaw commented May 29, 2024

gauteh commented May 29, 2024

jorsn commented May 29, 2024 • edited Loading

jorsn commented May 29, 2024

ibuclaw commented May 31, 2024

jorsn commented May 31, 2024

gauteh commented May 31, 2024

jorsn commented Jun 1, 2024

gauteh commented Jun 1, 2024

gauteh commented Jun 1, 2024

jorsn commented Jun 2, 2024 • edited Loading

gauteh commented Jun 2, 2024

jorsn commented Jun 3, 2024

gauteh commented Jun 3, 2024

ibuclaw commented Jun 4, 2024

mcatanzaro Jun 4, 2024

Choose a reason for hiding this comment

jorsn Jun 4, 2024

Choose a reason for hiding this comment

mcatanzaro Jun 5, 2024

Choose a reason for hiding this comment

jorsn Jun 5, 2024

Choose a reason for hiding this comment

jorsn commented May 28, 2024 •

edited

Loading

jorsn commented May 29, 2024 •

edited

Loading

jorsn commented Jun 2, 2024 •

edited

Loading