Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate embedding Partytown via a cross-origin iframe #5

Closed
lawnsea opened this issue Sep 23, 2021 · 8 comments
Closed

Investigate embedding Partytown via a cross-origin iframe #5

lawnsea opened this issue Sep 23, 2021 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed stale

Comments

@lawnsea
Copy link
Contributor

lawnsea commented Sep 23, 2021

I believe that Partytown's isolation could be improved by embedding it via a cross-origin iframe rather than directly as a worker.

Problem

Because the worker inherits the embedding page's origin, it can access data from that origin via APIs like self.indexedDB that are available in the WorkerGlobalScope.

Proposed solution

Instead of integrating Partytown directly into the embedding document as described in the documentation, users could instead host the service worker, the web worker, and a minimal HTML document on a separate origin:

  • document.com
    • /index.html - the embedding document
  • sandbox.com
    • /sandbox.html - the sandbox iframe
    • /web-worker.js - the web worker script
    • /service-worker.js - the service worker script

The sandbox iframe would be responsible for

  1. registering the service worker
  2. starting the web worker
  3. exposing Partytown's API to the embedding document via postMessage

This will impose a small latency cost for the extra postMessage hop between the iframe and the embedding document, but would ensure that the web worker cannot access any resources from the embedding document that are subject to the same origin policy.

@adamdbradley
Copy link
Contributor

I see what you mean, in that you purposely do not want the scripts to even run on the same domain to specifically not allow access to indexedDB? You're right, in that right now between the sandbox iframe and the main window there isn't a postMessage, so doing so would add one more communication hop.

I think before we dig too far into that I'd be curious to investigate Atomics further. Right now I don't know enough what the implementation will look like, and don't want to go too far down this path just to find out we scrap it later. I'll leave this issue open and will revisit, but would love to keep discussing ideas here. Thanks

@adamdbradley
Copy link
Contributor

Put some more thought into this and certainly makes a lot of sense, especially if we want to sandbox and isolate even further. So step one, like you said, would be to the iframe sandbox to only communicate via postMessage.

Next I think the iframe hosted under a different origin, which might be harder for a default setup. So maybe the default continues to work under /~partytown/, but if it was moved to a different subdomain, it'd still just work, but more of an opt-in to ensure it's fully sandboxed. Thoughts?

@steve8708
Copy link
Contributor

Wonder if using sandboxed iframes would accomplish the same goals without having to have another domain to host the iframe on. May be able to easily create a sandboxed iframe inline using srcdoc

@steve8708
Copy link
Contributor

Ah tho I see why we would need Atomics to use inline sandboxes iframes as I’m not sure how it would work with the service worker approach. Ignore me 😄

@lawnsea
Copy link
Contributor Author

lawnsea commented Sep 27, 2021

Thanks for the interest in this suggestion, @adamdbradley!

Next I think the iframe hosted under a different origin, which might be harder for a default setup. So maybe the default continues to work under /~partytown/, but if it was moved to a different subdomain, it'd still just work,

I like this idea. It would simplify the documentation and make it easy for folks to opt into stronger isolation.

It's a bummer to impose the overhead of an additional postMessage hop for users that don't care about stronger isolation, though, so I hope we can make the iframe wrapper optional.

@lawnsea
Copy link
Contributor Author

lawnsea commented Sep 27, 2021

@steve8708, I agree that Atomics plus a sandbox iframe without the allow-same-origin permission would be simpler and faster. Unfortunately, it doesn't look like it's possible to use a service worker with such an iframe: w3c/ServiceWorker#1390.

One consequence of this situation to note is that the service worker implementation provides an opportunity for stronger isolation, as the SW controls network requests from the web worker. For that reason, I hope we can support the Atomics implementation with the cross-origin iframe setup as well.

@adamdbradley adamdbradley added enhancement New feature or request help wanted Extra attention is needed labels Jun 13, 2022
@emilisb
Copy link
Contributor

emilisb commented Dec 7, 2022

Hi @adamdbradley @steve8708 , I started working on this feature. I made a quick and hacky POC to get it work (both SW and Atomics). I moved the service worker and the web worker to a cross-origin iframe which communicates with the main thread via postMessage and then passes all messages to web worker/service worker. Now I am trying to use a MessageChannel instead, that way we would avoid an additional message hop and performance should stay more or less the same as it is now. Atomics also work with this approach as long as both the main document and the new iframe satisfy crossOriginIsolated requirements.

I can finish it and open a PR, but I have a few questions:

  1. Would you take a PR for this? Do you have any specific requirements for this feature, such as make this new communication mechanism opt-in?
  2. Why does SW sandbox currently run inside an iframe? I cannot understand how it improves the isolation.

We can discuss this on Discord if that is more convenient for you, I left a message in partytown-general channel.

Copy link
Contributor

Partytown moves to QwikDev organization, this is a new beginning for the project. So it's time to clarify the status and clean up the current state a bit. This issue was automatically marked as deprecated and closed because it was not detected recent activity for 8 months, date of latest version. If this issue is still relevant, feel free to comment below and the maintainers will reopen it. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 24, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed stale
Projects
None yet
Development

No branches or pull requests

4 participants