Allow iframe sandbox options to be configured in connectToChild

[n8agrin]

Iframes support a sandbox attribute which is widely supported by modern browsers and can provided an added layer of security.

This patch simply allows passing in a list of sandbox attributes, and assumes they will be applied to the internally constructed sandbox frame.

[Aaronius]

Hi @n8agrin. Here's how we're adding sandbox attributes in our projects currently:

const connection = Penpal.connectToChild({
  ...
});

connection.iframe.setAttribute(
  'sandbox',
  'allow-popups allow-popups-to-escape-sandbox allow-same-origin allow-scripts'
);

I've been reluctant to add sandbox options to connectToChild because it could be a slippery slope. Consumers may start asking for an iframeId option and then an iframeClassNames option, etc. These are all things that would get applied to the iframe, but could also just be added directly to the iframe by the consumer as shown above.

Does that make sense? Thoughts?

[n8agrin]

@Aaronius thanks for the quick response.

Your suggestion is similar to the first attempt I made using Penpal, but I believe that to be secure you must assign the sandbox attribute values before the child frame is appended to the parent document. I'm not even sure what happens to frames that have been appended to a document, then have a src (or srcdoc) attribute set, and then apply sandbox attributes. Possibly no sandboxing is applied?

In any case, I believe this line makes adding the sandbox attributes before the frame is appended to the document body impossible, so my patch seemed like the easiest and most secure way to proceed.

I may be missing something, so please let me know if you see any holes in that logic.

[n8agrin]

@Aaronius also, as for slippery slope, I totally respect that and agree. It's tough to be permissive on some options and not others. Security seemed like a reasonable compromise to me, but I'm open to entirely other solutions.

[Aaronius]

I'm glad you've thought through this. I initially had the same concern when we added the sandbox attribute after the iframe was added to the dom. After I tested it out, though, I felt quite confident that it was secure. First, the iframe's content won't be loaded by the time the sandbox attribute gets added. I believe this is guaranteed by how JS engines handle threading. Second, even though the sandbox attribute is applied after the iframe is added to the DOM, it still gets respected from then on. I believe in our project I tested this in Chrome, FF, Safari, and IE 11 and up, but I'm not 100% sure I did.

With this in mind, I'm curious to know if you would still prefer the approach you've taken in this PR. Even if the way we did it in our project proves to be 100% secure, it may still give peace of mind to consumers to allow what you've proposed here.

[Aaronius]

Any additional thoughts on this @n8agrin? Maybe @jrencz?

[n8agrin]

@Aaronius I ended up going a different route and building my own library to handle my needs, since it was such a small use case. I'm no longer really eager to shepherd this code through.

That said, my advice would be to be cautious about adding the sandbox attributes after you've added the frame to the dom. All one would need to do to violate the security assumptions around JS being single threaded would be to wrap any step of the construction of a Penpal instance, or the setting of the sandbox attribute, in a setTimeout. It's asking developers to understand the entire execution context of the library they use, their code and the security implications, which is very easy to mitigate when put behind a proper interface. In other words, ensure you set the sandbox attributes in the same call stack, which is trivial to do before you append the frame to the dom.

I don't have a horse in this race any longer. I really appreciate how attentive you have been about this PR and I have the impression you guys really like external contributions which is a welcome change from the normal firewall I experience opening PRs on other projects. I'm super happy to help if I can around this particular issue, so if you find it compelling let me know and I'd chip in. For now I'm going to close this, to keep your project clean.