set iframe height in javascript

jorsn commented 4 months ago

fixes #720, fixes #736
closes #732, closes #740, closes #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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

jorsn commented 4 months ago

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 commented 4 months ago

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

ibuclaw commented 4 months ago

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

Looks good to me.

jorsn commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

gauteh commented 4 months ago

I think sanitize should do the trick 👍

jorsn commented 4 months ago

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 commented 4 months ago

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

jorsn commented 4 months ago

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 commented 4 months ago

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

ibuclaw commented 4 months ago

@jorsn thanks for tackling this!

astroidmail / astroid

set iframe height in javascript #747