One-way iframe communication #198

milesfrain · 2020-08-22T21:56:32Z

This PR "simplifies" iframe management by eliminating some JS code, and letting PS code setup the iframe directly. The previous version required some two-way message passing, along with retries and timeouts. More critiques of the previous version can be found in #192 (comment).

There are still some non-ideal aspects of this new strategy, such as adding another layer of encoding.

milesfrain · 2020-08-22T21:57:10Z

client/public/index.html

@@ -157,41 +158,6 @@
        };
      })(marker));
    }
-
-    function setupIFrame($ctr, data) {


Clearing out JS from index.html is a major goal. The final version ends up being very concise.

milesfrain · 2020-08-22T21:57:45Z

client/public/js/frame.js

@@ -1,51 +1,87 @@
-(function() {


This file is mostly the same, but with additional comments and less function nesting. Actual differences (described in more detail later) are:

Remove of event listeners

LZString decompression step

onload code to execute the parent's requested function call

milesfrain · 2020-08-22T21:58:40Z

client/public/js/frame.js

-  var parent;
-
-  document.addEventListener("DOMContentLoaded", function() {
-    window.addEventListener("message", function(event) {
-      parent = event.source;
-      parent.postMessage("trypurescript", "*");
-      var file = evalSources(event.data)("<file>");
-      if (file.main && typeof file.main === "function") {
-        file.main();
-      }
-    }, { once: true });
-  }, { once: true });
-
-  document.addEventListener("click", function(event) {
-    if (parent && event.target.nodeName === "A" && event.target.hostname === "gist.github.com") {
-      event.preventDefault();
-      parent.postMessage({
-        gistId: event.target.pathname.split("/").slice(-1)[0]
-      }, "*");
-    }
-  }, false);


Removed event listeners

milesfrain · 2020-08-22T21:59:07Z

client/public/js/frame.js

+  // Decompress values back to JS source
+  Object.keys(obj).forEach(function (key) {
+    obj[key] = LZString.decompressFromEncodedURIComponent(obj[key]);
+  });


New decompression step

milesfrain · 2020-08-22T21:59:34Z

client/public/js/frame.js

+// Call script tag contents when frame loads.
+// Expects a call to loadFrame, passing JS sources.
+window.onload = function() {
+  // https://stackoverflow.com/a/8677590
+  //grab the last script tag in the DOM
+  //this will always be the one that is currently evaluating during load
+  let tags = document.getElementsByTagName('script');
+  let tag = tags[tags.length -1];
+  //force evaluation of the contents
+  eval( tag.innerHTML );
+};


New code to execute the iframe contents

milesfrain · 2020-08-22T22:00:23Z

client/src/LzString.js

@@ -0,0 +1,10 @@
+"use strict";
+
+exports.compressToEncodedURIComponent = function (input) {


These are used for the later Code-compressed-in-URL feature too.

milesfrain · 2020-08-22T22:00:44Z

client/src/Main.purs

+    jsonStr
+      = stringify
+      $ encodeJson
+      $ map (unwrap >>> compressToEncodedURIComponent) eventData


The JS files are compressed with lz-string because I couldn't figure out how to escape the contents correctly to pass to the iframe. It would be nice to escape these a bit more efficiently, although I don't think this compression/decompression step is a major performance issue.

What do you mean by "escape the contents correctly"? What issues were you having? This step does not seem necessary to me, but I would need to know what you were doing, what outputs you were expecting, and what outputs you actually received.

The issue occurs a few lines after this comment in:

"<script src='js/frame.js'> loadFrame('" <> jsonStr <> "'); </script>"

I tried a few different schemes for escaping " and \ characters, but couldn't figure out what is needed to prevent the JSON string from breaking the HTML string.

milesfrain · 2020-08-22T22:02:46Z

client/config/dev/Try.Config.purs

-mainGist = "7ad2b2eef11ac7dcfd14aa1585dd8f69"
+mainGist = "005a86e5c843d3738c1fdd95cc278144"


The difference between these gists is that

The original creates a link to the gist:

"https://gist.github.com/" <> gist

This version creates links to TPS with the gist query param.

"https://try.purescript.org/?gist=" <> gist

This allows us to remove the gist click event listener from the old setupIFrame.

natefaubion · 2020-08-23T00:14:49Z

client/src/Main.purs

+  -- allow-top-navigation lets us click on links to other examples
+  JQuery.setAttr "sandbox" "allow-scripts allow-top-navigation" iframe
+  JQuery.setAttr "src" "frame-error.html" iframe
+  JQuery.setAttr "srcdoc" iframeSrcDoc iframe


One issue with this, and part of the reason for the old code, is just going to be browser support. This only works in the current versions of Edge that have switched to Blink. I am personally somewhat ambivalent on old Edge/IE support, but I know @hdgarrood has requested it in the past. That is, if there is a way to keep a secure and featureful experience in not-to-old browsers, we should make the effort.

It looks like there's fairly widespread support, but in case there's a compatibility issue, this error is displayed:

trypurescript/client/public/frame-error.html

Line 9 in d16d4d0

<p>Your browser is missing <a href=https://caniuse.com/#feat=iframe-srcdoc>srcdoc</a> support</p>

In general I’m less worried about compatibility for tools which are mainly meant for use by developers than I am for actual compiler output or FFI in libraries (things which will affect people who use apps written in purescript, not just purescript devs themselves.) That said, this is not a good enough reason to drop IE11 support for me. 5% is still quite a lot of people.

To be fair to @milesfrain, I'm not sure if any of us can confirm that trypurs works as-is in IE11. I've certainly never tested it.

Ah right, yes, that’s a good point. I haven’t run Windows on any of my machines since before I started contributing to PureScript.

hdgarrood · 2020-08-23T00:44:26Z

This change is still not very appealing from my perspective. I’m bothered by allow-top-navigation, as I think it makes Try PureScript very appealing as a way of concealing malicious links by having it load code which redirects to a malicious site. It doesn’t seem to simplify things that much either; less than 100 lines of code have been removed.

natefaubion · 2020-08-23T00:51:34Z

What is the reason for allow-top-navigation? Just removing the gist link listener? I agree that I would like this iframe to be locked down as much as reasonably possible.

natefaubion · 2020-08-23T00:56:26Z

Especially if the goal is to allow saving to gist support. If we allow arbitrary top-level navigation, I think it would straightforward for trypurs to be a phishing vector for github credentials by handing out official try.purescript.org links that redirect to something else which looks exactly like try-purs.

milesfrain · 2020-08-23T01:10:27Z

load code which redirects to a malicious site

That's a good catch.

I changed to allow-top-navigation-by-user-activation so the user must click on the links (which they can at least preview) before loading a new page.

Let me know if there's another type of attack to address.

hdgarrood · 2020-08-23T01:24:24Z

I’m still uncomfortable with allowing top navigation at all, even with user actions, if it can send someone to an arbitrary destination. Sorry, but I think this should be closed.

milesfrain · 2020-08-23T03:05:44Z

To clarify the current iframe policy: No links allowed, unless they point to approved code sources (gists, and github files).

hdgarrood · 2020-08-31T13:19:42Z

@milesfrain could you please fix this on try.ps.ai by removing allow-top-navigation and allow-top-navigation-on-user-action there? I see that try.ps.ai is getting a fair bit of use on FP Slack already and it's quite easy to exploit this there right now.

milesfrain · 2020-08-31T15:59:01Z

Eliminating top-navigation from the iframe will require a longer dev session, since the current architecture relies on a declarative way of setting up the iframe. I should have time for that after next Tuesday.

For now, I made a quick fix to switch to the less egregious allow-top-navigation-by-user-activation.

Another easy (partial and temporary) solution is to add a toggle (disabled by default) to enable links within the iframe.

Taking a step back, I wonder if we even need to support links within the iframe at all? We could simplify the code a lot if we either just relied on a separate page that lists examples (similar to how the cookbook lists recipes).

We could further polish this up by making the examples links more accessible, like what's done for TypeScript and ReasonML:

hdgarrood · 2020-09-05T21:43:43Z

I'm not sure I'd want to remove the ability to link from the iframe entirely, as other people might be using it elsewhere, and I don't see a pressing need to simplify this code. However, having a list of examples as part of the actual Try PureScript UI rather than relying on links we render inside the iframe does sound good to me; that way it will probably be easier to navigate around them too.

One-way iframe communication

d16d4d0

milesfrain commented Aug 22, 2020

View reviewed changes

natefaubion reviewed Aug 23, 2020

View reviewed changes

Require a user's click to redirect

182a11b

milesfrain closed this Aug 23, 2020

milesfrain mentioned this pull request Aug 23, 2020

Enable faster development loop #197

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

One-way iframe communication #198

One-way iframe communication #198

milesfrain commented Aug 22, 2020

milesfrain Aug 22, 2020

milesfrain Aug 22, 2020

milesfrain Aug 22, 2020

milesfrain Aug 22, 2020

milesfrain Aug 22, 2020

milesfrain Aug 22, 2020

milesfrain Aug 22, 2020

natefaubion Aug 23, 2020

milesfrain Aug 23, 2020

milesfrain Aug 22, 2020

natefaubion Aug 23, 2020

milesfrain Aug 23, 2020

hdgarrood Aug 23, 2020

natefaubion Aug 23, 2020

hdgarrood Aug 23, 2020

hdgarrood commented Aug 23, 2020

natefaubion commented Aug 23, 2020

natefaubion commented Aug 23, 2020 •

edited

Loading

milesfrain commented Aug 23, 2020

hdgarrood commented Aug 23, 2020

milesfrain commented Aug 23, 2020

hdgarrood commented Aug 31, 2020

milesfrain commented Aug 31, 2020

hdgarrood commented Sep 5, 2020

		@@ -0,0 +1,10 @@
		"use strict";

		exports.compressToEncodedURIComponent = function (input) {

		mainGist = "7ad2b2eef11ac7dcfd14aa1585dd8f69"
		mainGist = "005a86e5c843d3738c1fdd95cc278144"

One-way iframe communication #198

One-way iframe communication #198

Conversation

milesfrain commented Aug 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdgarrood commented Aug 23, 2020

natefaubion commented Aug 23, 2020

natefaubion commented Aug 23, 2020 • edited Loading

milesfrain commented Aug 23, 2020

hdgarrood commented Aug 23, 2020

milesfrain commented Aug 23, 2020

hdgarrood commented Aug 31, 2020

milesfrain commented Aug 31, 2020

hdgarrood commented Sep 5, 2020

natefaubion commented Aug 23, 2020 •

edited

Loading