search

Aaronius / penpal

A promise-based library for securely communicating with iframes via postMessage.
MIT License
381 stars 56 forks source link

Iframe Removal Monitoring not Working with Custom Elements #62

Closed lpellegr closed 3 years ago

lpellegr commented 3 years ago

Let's say you have a single-page Web app that makes use of web components (for instance a lit-element Web app). The page is rendered through a custom element that has an iframe in its Shadow DOM. This custom element interacts with this iframe using Penpal.

In this scenario, the connection between the parent and child is automatically destroyed after 60 seconds.

After investigation, I identified the problem is caused by a condition that assumes the iframe is not hosted within a custom element (see line 17 below):

https://github.com/Aaronius/penpal/blob/da8df793bc08b2cdfc80fdbec61e46d988981143/src/parent/monitorIframeRemoval.ts#L14-L26

On line 17, document always references the window document. However, with the scenario described the iframe is hosted in the Shadow DOM of the custom element and not the window document. As a consequence, as soon as the monitoring code triggers, the condition !document.contains(iframe) evaluates to true and the connection is destroyed.

A simple solution that I tested with success locally is to replace !document.contains(iframe) by !iframe.isConnected. The property isConnected considers the right context object depending on whether the iframe is in normal DOM or shadow DOM. It seems to also fit the browsers supported by the library (i.e. only IE11 has no support for this property).

Would you accept a PR to fix the issue with the proposed solution?

More generally, would you accept another PR that allows disabling iframe removal monitoring?

Aaronius commented 3 years ago

Thanks for finding and reporting this @lpellegr! I saw your two pull requests and I'll dig deeper into them ASAP. Regarding being able to configure iframe removal monitoring, could you explain the use case for that? Thanks again.

lpellegr commented 3 years ago

Thanks for your quick reply.

I have 2 use cases: the first is about disabling iframe removal monitoring completely since I destroy connections properly and the window that loads the app does not aim to remain open a long time. The second is about using Penpal in a third-party widget. In that case, I prefer to keep the feature enabled but a check every hour is more than enough.

Aaronius commented 3 years ago

@lpellegr Is the current monitoring causing issues, are you worried about performance, or something else? It's a very inexpensive check, so I don't see performance being a concern.

Aaronius commented 3 years ago

Hey, I haven't tested this yet and I'm having a bit of difficulty finding understanding this from documentation, but you may know the answer. If the iframe is added to the web component but the web component is not added to the window's document, will iframe.isConnected return true or false? If it returns true, I don't think that's a good thing, because that means Penpal won't close the connection automatically and the iframe will be stuck in memory.

lpellegr commented 3 years ago

@Aaronius The answer to your question is false. If the iframe is added to the Web component but this last is not attached to the window DOM tree, then the iframe is not connected. The same is true if the Web component is attached and then detached. Here is a simple example to illustrate both cases (uncomment the 2 lines to test the second case):

https://jsfiddle.net/lpellegr/qxupLb0y/87/

Regarding the iframe removal monitoring option it's more about having control over what happens. I agree with you the performance hit for such a check made every minute is not critical but when embedded inside third-party pages by other developers, it becomes very hard to argue and you may lose trust when it is noticed you have code running periodically that is not required or could run much less often. Besides, in the case of an issue with Web components like pointed or whatever else involving iframe removal monitoring, you can simply disable the feature as a temporary workaround.

The PR does not change the default behavior and the changes are minimal. Although I think that could benefit other users of the library, I understand if that's something you prefer not to add. In that case, I will use a fork.

Aaronius commented 3 years ago

Thanks for setting up that test and explaining your reasoning @lpellegr. Thanks for the PRs as well. I'm going decline the monitoring configuration one until I see a more compelling reason to make it configurable. I'll do my best to get the other two merged and released tonight. Thanks!

Aaronius commented 3 years ago

The iframe.isConnected fix was published in v5.3.0. Thanks @lpellegr!