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

Introduce sandboxed iframe mode. #1042

Merged
merged 4 commits into from
Apr 2, 2021
Merged

Introduce sandboxed iframe mode. #1042

merged 4 commits into from
Apr 2, 2021

Conversation

samouri
Copy link
Member

@samouri samouri commented Mar 29, 2021

background
The primary use-case of worker-dom is to power <amp-script>. With the current mechanisms in place, it acts well for 1p (mostly trusted) scripts. amp-script now has a new use case to allow less-trusted 3p scripts to run on the page without a CSP, and for this we need a stronger security boundary.

The goal of this PR is to enable a "sandboxed" mode by creating the Worker within a sandboxed iframe instead of within the main frame.

design

When starting up, worker-dom will create an iframe instead of a WebWorker. This iframe needs to perform a handshaking process and then essentially act as a proxy between the subframe Worker and the main frame. To make implementation as simple as possible, I've created a class called IframeWorker that presents almost the exact same API surface as a standard Worker.

The iframe must follow a specific contract:

  1. When initialized, send out a message {type: 'iframe-ready'}
  2. When receiving a message with shape: {type: 'init-worker', code: string}, create a Worker with the specified code.
  3. Proxy all messages received via shape {type: 'postMessage', message: string} to the subframe Worker.
  4. Proxy all messages received from the Worker back to the parent. The shape of should be: {type: 'onmessage' | 'onerror', message: string }

In order to take advantage of this feature, consumers will need to use the new option in the upgradeElement API called sandboxed. The option may remain undefined to men off, or be an object with the key iframeUrl. The specified iframe must follow the contract described above. An example is provided in the demo folder.

hosting the iframe
Depending on whether we want to release this for non-AMP mode as well, we could also have a default value for iframeUrl that points to a Github Pages hosted iframe.

note: There is some drift in our src code alignment in prettier and I've extracted that to a small separate PR: #1041

@samouri
Copy link
Member Author

samouri commented Mar 29, 2021

As this is only for amp-script, it doesn't make sense to bloat the regular binaries with the code for IframeWorker as well.
Is there a way to conditionally import IframeWorker within worker.ts in only AMP modes? Would shave ~0.3kb.

@samouri samouri marked this pull request as ready for review March 29, 2021 15:22
@samouri samouri force-pushed the iframe branch 2 times, most recently from b3378fd to 0e0a7b1 Compare March 30, 2021 19:33
@kristoferbaxter
Copy link
Contributor

You can create a new constant for AMP only additions, and use replacement to filter them out during the compilation process.

Or, you can create a custom plugin for removing the logic (see how debug is removed from evaluators).

@samouri
Copy link
Member Author

samouri commented Mar 30, 2021

@kristoferbaxter: was just about to push when you commented that. I was worried that a const wouldn't be enough to DCE the entire import, but it looks like it worked!

before

./dist/main.js 5.33 kb  
./dist/main.mjs 4.36 kb

after

./main.js 4.67 kb  
./dist/main.mjs 3.83 kb 

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for ensuring this is specific to the AMP output too.

Added some comments and nits to consider.

src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Outdated Show resolved Hide resolved
src/main-thread/worker.ts Outdated Show resolved Hide resolved
const messageListeners: any = [];
env.window.addEventListener = (type: string, listener: any) => messageListeners.push(listener);

global.fetch = (blob: any) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Believe globals are cleaned up automatically, but can we verify?

Copy link
Member Author

@samouri samouri Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ava 3.x we are safe. may need to change things when we upgrade to 4.x.
I'll do the migration asap, with keeping the isolation intact

https://github.com/avajs/ava/blob/main/docs/01-writing-tests.md#test-isolation

demo/sandboxed/index.html Outdated Show resolved Hide resolved
demo/sandboxed/sandbox-iframe.html Outdated Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/iframe-worker.ts Outdated Show resolved Hide resolved
src/main-thread/iframe-worker.ts Outdated Show resolved Hide resolved
src/main-thread/iframe-worker.ts Show resolved Hide resolved
src/main-thread/worker.ts Outdated Show resolved Hide resolved
src/main-thread/worker.ts Outdated Show resolved Hide resolved
src/test/main-thread/iframe-worker.test.ts Show resolved Hide resolved
src/test/main-thread/iframe-worker.test.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants